From: Bruno H. <br...@cl...> - 2017-08-24 21:11:42
|
Hi Sam, > Use (f)puts instead of (f)printf when possible. I guess this was motivated by Jerry's remark: ! Fedora builds with -Werror=format-security, so I had to change ! one line in the aarch64 patch. That patch modifies spvw_allocate.d ! and in one place issues a warning. I had to change this line: ! ! fprintf(stderr, GETTEXTL("Warning: ")); ! ! to: ! ! fputs(GETTEXTL("Warning: "),stderr); ! ! since the string is non-constant, and therefore triggers the format ! security error. Two things about this: 1) There is an (undocumented) way to avoid this security warning. Test case: =============================================================================== #include <stdio.h> extern const char * transform1 (const char * s); extern const char * transform2 (const char * s) __attribute__ ((__format_arg__ (1))); void foo1 () { fprintf(stderr, transform1("Hello")); } void foo2 () { fprintf(stderr, transform2("Hello")); } =============================================================================== $ gcc -S -Wall -Wformat-security foo.c foo.c: In function 'foo1': foo.c:4:1: warning: format not a string literal and no format arguments [-Wformat-security] void foo1 () { fprintf(stderr, transform1("Hello")); } ^~~~ As you can see, this __attribute__ ((__format_arg__ (1))) has the effect of avoiding the warning. But since it is undocumented, I agree to avoid fprintf with format-strings that take 0 arguments, and use literal strings instead. Even though it will likely cause work for the translators. 2) I dislike APIs that are hard to remember. * fputs, fputc, putc have the problem that it requires you do put the stream after the arguments, which is unlike fprintf, object_out, etc. * puts additionally emits an extra newline. Really, when writing a sequence of outputs to the same stream, these inconsistencies lead to mistakes: extra or missing newlines at the end of an output. If we were using <stdio.h> only in 1 or 2 places, this would be acceptable. But we're using it all over the place. Additionally, there is no need to optimize fputs("x",stdout) to fputc('x',stdout) because the compiler (gcc) does this optimization already for years. Therefore I've introduced two macros /* Use fprintf and printf only for format strings that take at least 1 argument. For literal strings, use print and fprint. Avoid using fputs, puts, fputc, putc, putchar directly, because these APIs are hard to memorize (fputs, fputc, putc don't take the stream first; puts outputs an extra newline) or redundant (fputc, putc, putchar are special cases of fputs that GCC recognizes anyway). */ #define fprint(fp,string) fputs(string,fp) #define print(string) fputs(string,stdout) The code looks now more consistent, since there are now exactly 4 primitives: fprintf printf fprint print and they have the signature that you expect. Bruno |
From: Pascal B. <pj...@in...> - 2017-08-24 22:37:45
|
> On 24 Aug 2017, at 23:11, Bruno Haible <br...@cl...> wrote: > > Hi Sam, > >> Use (f)puts instead of (f)printf when possible. > > I guess this was motivated by Jerry's remark: > > ! Fedora builds with -Werror=format-security, so I had to change > ! one line in the aarch64 patch. That patch modifies spvw_allocate.d > ! and in one place issues a warning. I had to change this line: > ! > ! fprintf(stderr, GETTEXTL("Warning: “)); > ! > ! to: > ! > ! fputs(GETTEXTL("Warning: "),stderr); > ! > ! since the string is non-constant, and therefore triggers the format > ! security error. > The standard way to avoid it is to use a literal format string! fprintf(stderr,”%s”,GETTEXTL(“Warning: “)); > Therefore I've introduced two macros > > /* Use fprintf and printf only for format strings that take at least 1 argument. > For literal strings, use print and fprint. > Avoid using fputs, puts, fputc, putc, putchar directly, because these APIs > are hard to memorize (fputs, fputc, putc don't take the stream first; puts > outputs an extra newline) or redundant (fputc, putc, putchar are special > cases of fputs that GCC recognizes anyway). */ > #define fprint(fp,string) fputs(string,fp) > #define print(string) fputs(string,stdout) At first, I thought that fprint would take a format string (why fprint(fp,“3 %%”); prints two percent characters?). At second, it feels like a typo! I would prefer fputs... -- __Pascal J. Bourguignon__ |
From: Bruno H. <br...@cl...> - 2017-08-25 06:50:45
|
Pascal Bourguignon wrote: > The standard way to avoid it is to use a literal format string! > > fprintf(stderr,”%s”,GETTEXTL(“Warning: “)); Indeed gcc recognizes "%s" (and "%c") as special cases of format strings and optimizes these cases. But I still find such code harder to read than fprint(stderr,GETTEXTL(“Warning: “)); Bruno |
From: <Joe...@t-...> - 2017-08-25 12:19:57
|
Hi, Bruno wrote: ----- ! fprintf(stderr, GETTEXTL("Warning: ")); ! fputs(GETTEXTL("Warning: "),stderr); 1) There is an (undocumented) way to avoid this security warning. Test case: =============================================================================== #include <stdio.h> extern const char * transform1 (const char * s); extern const char * transform2 (const char * s) __attribute__ ((__format_arg__ (1))); void foo1 () { fprintf(stderr, transform1("Hello")); } void foo2 () { fprintf(stderr, transform2("Hello")); } =============================================================================== $ gcc -S -Wall -Wformat-security foo.c foo.c: In function 'foo1': foo.c:4:1: warning: format not a string literal and no format arguments [-Wformat-security] void foo1 () { fprintf(stderr, transform1("Hello")); } ^~~~ As you can see, this __attribute__ ((__format_arg__ (1))) has the effect of avoiding the warning. ----- What is undocumented? https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html https://gcc.gnu.org/onlinedocs/gcc-3.1/gcc/Function-Attributes.html ... mentions __attribute__ ((format_arg...)). Is the side-effect undocumented? But that's specifically the intention of using such an attribute: Have the compiler know & check where format strings are, or let the programmer declaim that the function is safe -- here I find that declaration completely unsafe, because it depends on foreign PO/MO files' contents!! Funny, I remember attribute((format)), but format_arg was new to me today. Summary: either - use %s with printf, or - avoid %s and printf altogether and instead use fputs. Regards, Jörg |
From: Bruno H. <br...@cl...> - 2017-09-10 14:13:22
|
Jörg wrote on 2017-08-25: > 1) There is an (undocumented) way to avoid this security warning. > Test case: > =============================================================================== > #include <stdio.h> > extern const char * transform1 (const char * s); > extern const char * transform2 (const char * s) __attribute__ ((__format_arg__ (1))); > void foo1 () { fprintf(stderr, transform1("Hello")); } > void foo2 () { fprintf(stderr, transform2("Hello")); } > =============================================================================== > $ gcc -S -Wall -Wformat-security foo.c > foo.c: In function 'foo1': > foo.c:4:1: warning: format not a string literal and no format arguments [-Wformat-security] > void foo1 () { fprintf(stderr, transform1("Hello")); } > ^~~~ > As you can see, this __attribute__ ((__format_arg__ (1))) has the effect of > avoiding the warning. > ----- > > What is undocumented? > https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html > https://gcc.gnu.org/onlinedocs/gcc-3.1/gcc/Function-Attributes.html > ... mentions __attribute__ ((format_arg...)). It is not documented that this attribute will silence a -Wformat-security warning. > here I find that declaration completely unsafe, because it depends on foreign PO/MO files' contents!! GCC itself declares the gettext function like this. builtins.def [1] contains this: DEF_EXT_LIB_BUILTIN (BUILT_IN_GETTEXT, "gettext", BT_FN_STRING_CONST_STRING, ATTR_FORMAT_ARG_1) The ATTR_FORMAT_ARG_1 is __attribute__ ((__format_arg__ (1))). clisp does the same with gettext, clgettext, clgettext1 (which are merely variants of gettext). So, apparently the GCC developers think it's not worth for GCC to complain about every use of a translated format string. The reason is that the gettext tools ('msgfmt' in particular) contain the appropriate checking. Bruno [1] https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/builtins.def?revision=249685&view=co |
From: Sam S. <sd...@gn...> - 2017-08-25 13:54:43
|
Hi Bruno, > * Bruno Haible <oe...@py...t> [2017-08-24 23:11:32 +0200]: > > #define fprint(fp,string) fputs(string,fp) > #define print(string) fputs(string,stdout) I don't like this for 2 reasons: 1. I am opposed to gratuitous deviations from the standard coding practices because they raise the barrier to entry for new people. 2. I prefer names that are not substrings of other names for the sake of grepping convenience. E.g., I would use `cprint` instead of `fprint' and `cputs` instead of `print` ("c" stands for CLISP &c) Thus I am with Pascal and Joerg on this. However, I don't think this is important enough to warrant further discussion. Thanks. -- Sam Steingold (http://sds.podval.org/) on darwin Ns 10.3.1504 http://steingoldpsychology.com http://www.childpsy.net http://honestreporting.com https://jihadwatch.org http://think-israel.org http://americancensorship.org If You Want Breakfast In Bed, Sleep In the Kitchen. |
From: Pascal B. <pj...@in...> - 2017-08-25 18:07:43
|
> On 25 Aug 2017, at 08:50, Bruno Haible <br...@cl...> wrote: > > Pascal Bourguignon wrote: >> The standard way to avoid it is to use a literal format string! >> >> fprintf(stderr,”%s”,GETTEXTL(“Warning: “)); > > Indeed gcc recognizes "%s" (and "%c") as special cases of format strings > and optimizes these cases. But I still find such code harder to read than > > fprint(stderr,GETTEXTL(“Warning: “)); The point is that this is some external data, obtained at run-time from files that are not necessarily under the control of the program (eg. some distributions package the localisable file separately). Therefore if you insist on avoiding “%s” you should, to keep the program correct and secure, do the following: fprint(stderr,escape_print_directives(GETTEXTL(“Warning: “))); and suddenly, using “%s” looks way simpler. -- __Pascal J. Bourguignon__ |
From: Sam S. <sd...@gn...> - 2017-08-25 18:22:14
|
> * Pascal Bourguignon <cwo@vasbezngvzntb.pbz> [2017-08-25 20:07:30 +0200]: > >> On 25 Aug 2017, at 08:50, Bruno Haible <br...@cl...> wrote: >> >> Pascal Bourguignon wrote: >>> The standard way to avoid it is to use a literal format string! >>> >>> fprintf(stderr,”%s”,GETTEXTL(“Warning: “)); >> >> Indeed gcc recognizes "%s" (and "%c") as special cases of format strings >> and optimizes these cases. But I still find such code harder to read than >> >> fprint(stderr,GETTEXTL(“Warning: “)); > > > The point is that this is some external data, obtained at run-time from > files that are not necessarily under the control of the program > (eg. some distributions package the localisable file separately). > > Therefore if you insist on avoiding “%s” you should, to keep the program > correct and secure, do the following: > > fprint(stderr,escape_print_directives(GETTEXTL(“Warning: “))); > > and suddenly, using “%s” looks way simpler. This only avoids a minor problem. What would you do with fprintf(stderr,GETTEXTL("OS Error: %s %d"),a,b) In fact there is no real problem here: GETTEXTL _is_ under our control, it takes the data from our translation files. Thanks -- Sam Steingold (http://sds.podval.org/) on darwin Ns 10.3.1504 http://steingoldpsychology.com http://www.childpsy.net http://iris.org.il http://think-israel.org http://islamexposedonline.com http://jij.org Democracy vs Despotism = Theft vs Robbery |
From: Bruno H. <br...@cl...> - 2017-09-10 13:58:52
|
Pascal Bourguignon wrote on 2017-08-25: > > fprint(stderr,GETTEXTL(“Warning: “)); > > The point is that this is some external data, obtained at run-time from files that are not necessarily under the control of the program (eg. some distributions package the localisable file separately). 'fprint' does not do format string processing (unlike fprintf). Therefore if some malicious translation is present in a .mo file, the program will output it, but it will not crash from it. (Although if it contains ANSI control codes, it may make your terminal go into strange states or crash. But that's a bug in the terminal emulator.) The security risks for setuid programs are handled through glibc/intl/ or libintl (on non-glibc systems). Bruno |
From: Bruno H. <br...@cl...> - 2017-09-10 14:20:34
|
Hi Sam, > > #define fprint(fp,string) fputs(string,fp) > > #define print(string) fputs(string,stdout) > > I don't like this for 2 reasons: > > 1. I am opposed to gratuitous deviations from the standard coding > practices because they raise the barrier to entry for new people. I understand the "barrier to entry for new people" argument. But in this case I found it worth it. > 2. I prefer names that are not substrings of other names for the sake of > grepping convenience. E.g., I would use `cprint` instead of `fprint' and > `cputs` instead of `print` ("c" stands for CLISP &c) You can use the grep option '-w' to search for 'print' only: $ grep -w print *.d $ grep -w 'print *(' *.d Bruno |
From: Sam S. <sd...@gn...> - 2017-09-11 14:34:11
|
Hi Bruno, > * Bruno Haible <oe...@py...t> [2017-09-10 16:20:26 +0200]: > >> > #define fprint(fp,string) fputs(string,fp) >> > #define print(string) fputs(string,stdout) >> >> I don't like this for 2 reasons: >> >> 2. I prefer names that are not substrings of other names for the sake of >> grepping convenience. E.g., I would use `cprint` instead of `fprint' and >> `cputs` instead of `print` ("c" stands for CLISP &c) > > You can use the grep option '-w' to search for 'print' only: > $ grep -w print *.d > $ grep -w 'print *(' *.d Yes, I can also use regexps and tag-search in emacs. This is not the point. The point is not "impossible". The point is "I need to make an extra effort". "Write" (code) is done once. "Read" is done many times. Replacing "print" with, e.g., "cprint" is done _once_ by _one_ person. The benefits are reaped _every_ time _anyone_ is reading the code. Thanks. -- Sam Steingold (http://sds.podval.org/) on darwin Ns 10.3.1504 http://steingoldpsychology.com http://www.childpsy.net http://camera.org http://americancensorship.org http://www.memritv.org http://www.dhimmitude.org Nostalgia isn't what it used to be. |
From: Sam S. <sd...@gn...> - 2017-08-25 13:59:33
|
> * Bruno Haible <oe...@py...t> [2017-08-24 23:11:32 +0200]: > > 2) I dislike APIs that are hard to remember. No one likes them. However, as long as they are commonly known and accepted, deviation from them is counterproductive. There are many such idiosyncrasies. E.g., omitting multiplication signs and parentheses in math. The optimal solution is a complete switch from infix to prefix notation. Do you think this will ever happen? Do you think using the infix notation in CLISP C files and adding a pre-processor converting the prefix notation to infix is a good idea? Thanks. -- Sam Steingold (http://sds.podval.org/) on darwin Ns 10.3.1504 http://steingoldpsychology.com http://www.childpsy.net https://jihadwatch.org http://mideasttruth.com http://www.memritv.org http://camera.org Sniper's job is to reduce his target audience. |