Build failures on Ubuntu due to gettext

Build failures on Ubuntu due to gettext

[Colin Watson]

Ubuntu's GCC enables -Wformat-security by default. This causes GCC to
(IMO rightly!) complain about constructs such as this:

  grub_printf (_("foo"));

... because it's all too easy for a translator to (usually accidentally)
insert % sequences which would cause printf to behave incorrectly. This
should instead be:

  grub_printf ("%s", _("foo"));

Patch follows. I can't help thinking that this would be easier with a
grub_puts, but perhaps that isn't worth it given the relatively small
number of occurrences here?

Also, should the line in notify_execution_failure instead be:

-  grub_printf (_("Failed to boot default entries.\n"));
+  grub_printf ("%s\n", _("Failed to boot default entries."));

... to get rid of the unsightly \n in this translated string?

2009-12-07  Colin Watson  <cjwatson@ubuntu.com>

	* normal/menu_entry.c (run): Don't pass the result of gettext as
	the first argument to grub_printf, appeasing -Wformat-security.
	(grub_menu_entry_run): Likewise.
	* normal/menu_text.c (grub_wait_after_message): Likewise.
	(print_message): Likewise.
	(notify_execution_failure): Likewise.

=== modified file 'normal/menu_entry.c'
--- normal/menu_entry.c	2009-12-05 11:25:07 +0000
+++ normal/menu_entry.c	2009-12-07 14:02:20 +0000
@@ -1000,7 +1000,7 @@ run (struct screen *screen)
 
   grub_cls ();
   grub_printf ("  ");
-  grub_printf (_("Booting a command list"));
+  grub_printf ("%s", _("Booting a command list"));
   grub_printf ("\n\n");
 
 
@@ -1182,6 +1182,6 @@ grub_menu_entry_run (grub_menu_entry_t e
   grub_print_error ();
   grub_errno = GRUB_ERR_NONE;
   grub_putchar ('\n');
-  grub_printf (_("Press any key to continue..."));
+  grub_printf ("%s", _("Press any key to continue..."));
   (void) grub_getkey ();
 }

=== modified file 'normal/menu_text.c'
--- normal/menu_text.c	2009-12-05 11:25:07 +0000
+++ normal/menu_text.c	2009-12-07 14:02:45 +0000
@@ -40,7 +40,7 @@ void
 grub_wait_after_message (void)
 {
   grub_putchar ('\n');
-  grub_printf (_("Press any key to continue..."));
+  grub_printf ("%s", _("Press any key to continue..."));
   (void) grub_getkey ();
   grub_putchar ('\n');
 }
@@ -206,7 +206,7 @@ entry is highlighted.");
       if (nested)
         {
           grub_printf ("\n        ");
-          grub_printf (_("ESC to return previous menu."));
+          grub_printf ("%s", _("ESC to return previous menu."));
         }
     }
 }
@@ -655,7 +655,7 @@ notify_execution_failure (void *userdata
       grub_errno = GRUB_ERR_NONE;
     }
   grub_printf ("\n  ");
-  grub_printf (_("Failed to boot default entries.\n"));
+  grub_printf ("%s", _("Failed to boot default entries.\n"));
   grub_wait_after_message ();
 }
 

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Build failures on Ubuntu due to gettext

[Carles Pina i Estany]

Hello,

On Dec/07/2009, Colin Watson wrote:
> Ubuntu's GCC enables -Wformat-security by default. This causes GCC to
> (IMO rightly!) complain about constructs such as this:
> 
>   grub_printf (_("foo"));

I see...
(actually some weeks ago I thought about gettext security implications,
and I thought that if someone can change the .mo files there is some
bigger problem for the user... but I understand the point)

> ... because it's all too easy for a translator to (usually
> accidentally) insert % sequences which would cause printf to behave

(side and non-relevant note: if the translator wants to do some damage on
purpose he doesn't need to play with %s and %d, it's as easy as changing
strings from "Press C to Cancel and D to delete" to something like (in another
language) "Press D to Cancel and C to delete")

> incorrectly. This should instead be:
> 
>   grub_printf ("%s", _("foo"));

I like the general idea but I'm not 100% convinced of the
implementation:

a) It's a bit of false security because it's not fixing the case of file
normal/menu_text.c, line 191 (search the string "Use the %C and %C keys
to") or menu_text.c line 374 (search for "The highlighted entry")
(I agree that it's better than before... but not very solid)

b) How would you translate and handle:
grub_printf (_("Hello %s"), name);

The translator really needs "%s" because in other languages can be "%s
hello" (not the best example but maybe you get the point)

c) I'm thinking to implement grub_printf_ (str). I will send a patch
later to see if everybody likes. Then the call for simple strings would
be:
grub_printf_ ("%s", N_("Hello"));

Not much different from:
grub_printf_ (N_("Hello"));

But it's getting hard to print a string :-)

d) (important, even if it's the last one):
How it prevents mistakes from the current msgfmt checks?
For example, and using the option -c in msgfmt:
#: normal/misc.c:67
#, c-format
msgid "test %s t"
msgstr "test %d test2"

/usr/bin/msgfmt -c --statistics -o po/ca.mo po/ca.po
po/ca.po:1183: format specifications in 'msgid' and 'msgstr' for argument 1 are
not the same
/usr/bin/msgfmt: found 1 fatal error
26 translated messages, 213 untranslated messages.

Using any different number of %X from msgid and msgstr ishalting msgfmt (so, if
msgid contains 1 %s and 2 %d, msgstr has to contain the same)

We are talking from _(" "), I see that -Wformat-security is for "string came
from untrusted input and contains" when .mo are trusted. Are they?

How are other projectes implementing it? Specially the dynamic strings.

> Patch follows. I can't help thinking that this would be easier with a
> grub_puts, but perhaps that isn't worth it given the relatively small
> number of occurrences here?

We could replace grub_puts with grub_printf... or some other idea.

> Also, should the line in notify_execution_failure instead be:
> 
> -  grub_printf (_("Failed to boot default entries.\n"));
> +  grub_printf ("%s\n", _("Failed to boot default entries."));
> 
> ... to get rid of the unsightly \n in this translated string?

I really like that this fix the \n discussion that we had :-)

I like the idea and I understand that it's easy to make mistakes
translating strings. Actually I'm surprised that msgfmt is not giving
any warning if the 

-- 
Carles Pina i Estany
	http://pinux.info

Re: Build failures on Ubuntu due to gettext

[Colin Watson]

On Mon, Dec 07, 2009 at 08:14:08PM +0000, Carles Pina i Estany wrote:
> On Dec/07/2009, Colin Watson wrote:
> > Ubuntu's GCC enables -Wformat-security by default. This causes GCC to
> > (IMO rightly!) complain about constructs such as this:
> > 
> >   grub_printf (_("foo"));
> 
> I see...
> (actually some weeks ago I thought about gettext security implications,
> and I thought that if someone can change the .mo files there is some
> bigger problem for the user... but I understand the point)
> 
> > ... because it's all too easy for a translator to (usually
> > accidentally) insert % sequences which would cause printf to behave
> 
> (side and non-relevant note: if the translator wants to do some damage on
> purpose he doesn't need to play with %s and %d, it's as easy as changing
> strings from "Press C to Cancel and D to delete" to something like (in another
> language) "Press D to Cancel and C to delete")
> 
> > incorrectly. This should instead be:
> > 
> >   grub_printf ("%s", _("foo"));
> 
> I like the general idea but I'm not 100% convinced of the
> implementation:
> 
> a) It's a bit of false security because it's not fixing the case of file
> normal/menu_text.c, line 191 (search the string "Use the %C and %C keys
> to") or menu_text.c line 374 (search for "The highlighted entry")
> (I agree that it's better than before... but not very solid)

Forget security, in this case; let's just think of it as a way to avoid
translators accidentally shooting themselves (and their users) in the
foot. I have seen this happen and it's best to prevent it by static
checking.

> b) How would you translate and handle:
> grub_printf (_("Hello %s"), name);

-Wformat-security doesn't interfere with that. As its documentation
says:

  At present, this warns about calls to `printf' and `scanf' functions
  where the format string is not a string literal and there are no
  format arguments, as in `printf (foo);'.

> c) I'm thinking to implement grub_printf_ (str). I will send a patch
> later to see if everybody likes. Then the call for simple strings would
> be:
> grub_printf_ ("%s", N_("Hello"));
> 
> Not much different from:
> grub_printf_ (N_("Hello"));
> 
> But it's getting hard to print a string :-)

grub_puts would be nice and would match the POSIX API (or maybe
grub_fputs or grub_putstr or something, since POSIX puts is kind of odd
in writing a trailing newline).

I don't really like significant trailing underscores in function names
myself, although I see why you chose it here.

> d) (important, even if it's the last one):
> How it prevents mistakes from the current msgfmt checks?
> For example, and using the option -c in msgfmt:
> #: normal/misc.c:67
> #, c-format
> msgid "test %s t"
> msgstr "test %d test2"
> 
> /usr/bin/msgfmt -c --statistics -o po/ca.mo po/ca.po
> po/ca.po:1183: format specifications in 'msgid' and 'msgstr' for argument 1 are
> not the same
> /usr/bin/msgfmt: found 1 fatal error
> 26 translated messages, 213 untranslated messages.
> 
> Using any different number of %X from msgid and msgstr ishalting msgfmt (so, if
> msgid contains 1 %s and 2 %d, msgstr has to contain the same)

That only works for strings with the "c-format" attribute, which
xgettext only applies (and should only apply) to msgids that contain
"%". It helps for the string you mentioned earlier with %C in the msgid,
but it's no use for _("literal string with no percent characters").

> We are talking from _(" "), I see that -Wformat-security is for "string came
> from untrusted input and contains" when .mo are trusted. Are they?

I would recommend against considering .mo as entirely trusted, unless
you really think that developers will read every submission
line-by-line. I realise that as you say there are plenty of more subtle
avenues of attack; as I said above, I really think of this as
foot-shooting prevention more than security.

> How are other projectes implementing it? Specially the dynamic strings.

Every competent project I've seen that uses gettext in C is in line with
the patch I sent, although some use an equivalent of printf("%s", ...)
while some use an equivalent of fputs(..., stdout).

> I like the idea and I understand that it's easy to make mistakes
> translating strings. Actually I'm surprised that msgfmt is not giving
> any warning if the 

In general it can only do this for appropriately tagged strings. For an
arbitrary literal string it often isn't actually wrong to translate it
with text containing a "%" sign - it's just that it crashes the program.
Thus, it's the program's responsibility to avoid crashing.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Build failures on Ubuntu due to gettext

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3280 bytes --]

Colin Watson wrote:
> Ubuntu's GCC enables -Wformat-security by default. This causes GCC to
> (IMO rightly!) complain about constructs such as this:
>
>   grub_printf (_("foo"));
>
> ... because it's all too easy for a translator to (usually accidentally)
> insert % sequences which would cause printf to behave incorrectly. This
> should instead be:
>
>   grub_printf ("%s", _("foo"));
>
> Patch follows. I can't help thinking that this would be easier with a
> grub_puts, but perhaps that isn't worth it given the relatively small
> number of occurrences here?
>
> Also, should the line in notify_execution_failure instead be:
>
> -  grub_printf (_("Failed to boot default entries.\n"));
> +  grub_printf ("%s\n", _("Failed to boot default entries."));
>
> ... to get rid of the unsightly \n in this translated string?
>   
This warning is simply wrong in this context. And silencing it is
against gettext manual. Read
http://www.gnu.org/software/hello/manual/gettext/Preparing-Strings.html
http://www.gnu.org/software/hello/manual/gettext/c_002dformat-Flag.html#c_002dformat-Flag
> 2009-12-07  Colin Watson  <cjwatson@ubuntu.com>
>
> 	* normal/menu_entry.c (run): Don't pass the result of gettext as
> 	the first argument to grub_printf, appeasing -Wformat-security.
> 	(grub_menu_entry_run): Likewise.
> 	* normal/menu_text.c (grub_wait_after_message): Likewise.
> 	(print_message): Likewise.
> 	(notify_execution_failure): Likewise.
>
> === modified file 'normal/menu_entry.c'
> --- normal/menu_entry.c	2009-12-05 11:25:07 +0000
> +++ normal/menu_entry.c	2009-12-07 14:02:20 +0000
> @@ -1000,7 +1000,7 @@ run (struct screen *screen)
>  
>    grub_cls ();
>    grub_printf ("  ");
> -  grub_printf (_("Booting a command list"));
> +  grub_printf ("%s", _("Booting a command list"));
>    grub_printf ("\n\n");
>  
>  
> @@ -1182,6 +1182,6 @@ grub_menu_entry_run (grub_menu_entry_t e
>    grub_print_error ();
>    grub_errno = GRUB_ERR_NONE;
>    grub_putchar ('\n');
> -  grub_printf (_("Press any key to continue..."));
> +  grub_printf ("%s", _("Press any key to continue..."));
>    (void) grub_getkey ();
>  }
>
> === modified file 'normal/menu_text.c'
> --- normal/menu_text.c	2009-12-05 11:25:07 +0000
> +++ normal/menu_text.c	2009-12-07 14:02:45 +0000
> @@ -40,7 +40,7 @@ void
>  grub_wait_after_message (void)
>  {
>    grub_putchar ('\n');
> -  grub_printf (_("Press any key to continue..."));
> +  grub_printf ("%s", _("Press any key to continue..."));
>    (void) grub_getkey ();
>    grub_putchar ('\n');
>  }
> @@ -206,7 +206,7 @@ entry is highlighted.");
>        if (nested)
>          {
>            grub_printf ("\n        ");
> -          grub_printf (_("ESC to return previous menu."));
> +          grub_printf ("%s", _("ESC to return previous menu."));
>          }
>      }
>  }
> @@ -655,7 +655,7 @@ notify_execution_failure (void *userdata
>        grub_errno = GRUB_ERR_NONE;
>      }
>    grub_printf ("\n  ");
> -  grub_printf (_("Failed to boot default entries.\n"));
> +  grub_printf ("%s", _("Failed to boot default entries.\n"));
>    grub_wait_after_message ();
>  }
>  
>
> Thanks,
>
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 293 bytes --]

Re: Build failures on Ubuntu due to gettext

[Carles Pina i Estany]

Hi,

On Dec/07/2009, Colin Watson wrote:
> On Mon, Dec 07, 2009 at 08:14:08PM +0000, Carles Pina i Estany wrote:

> > a) It's a bit of false security because it's not fixing the case of file
> > normal/menu_text.c, line 191 (search the string "Use the %C and %C keys
> > to") or menu_text.c line 374 (search for "The highlighted entry")
> > (I agree that it's better than before... but not very solid)
> 
> Forget security, in this case; let's just think of it as a way to avoid

Ok, I'll forget about security (even when the name is not helping to
forget about security :-) )

> > b) How would you translate and handle:
> > grub_printf (_("Hello %s"), name);
> 
> -Wformat-security doesn't interfere with that. As its documentation
> says:
> 
>   At present, this warns about calls to `printf' and `scanf' functions
>   where the format string is not a string literal and there are no
>   format arguments, as in `printf (foo);'.

I see now (not before)

> > Using any different number of %X from msgid and msgstr ishalting
> > msgfmt (so, if msgid contains 1 %s and 2 %d, msgstr 
> > has to contain the same)
> 
> That only works for strings with the "c-format" attribute, which
> xgettext only applies (and should only apply) to msgids that contain
> "%". It helps for the string you mentioned earlier with %C in the msgid,

right. Actually I was expecting that all strings in a C file would
has c-format, but it's not the case:
----
#: normal/menu_entry.c:840
#, c-format
msgid "Possible %s are:"

#: normal/menu_entry.c:1003
msgid "Booting a command list"
----

And confirmed: if the translator adds %C in the second string
("Booting a command list") msgfmt -c is not complaining.

So I would apply your patch, after understanding that it's only for
strings that doesn't have any %C (and in my opinion xgettext could
add the c-format if it's in a C file, but we will not discuss it here
now :-) , I guess that in some cases would not be correct)

Your patch conflicts with my one (Subject: gettext: grub_printf_ and N_)
but it's not a big problem. If you commit before I would adapt
my one, else I would adapt your one. 

-- 
Carles Pina i Estany
	http://pinux.info

Re: Build failures on Ubuntu due to gettext

[Colin Watson]

On Mon, Dec 07, 2009 at 11:04:52PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> Colin Watson wrote:
> > Ubuntu's GCC enables -Wformat-security by default. This causes GCC to
> > (IMO rightly!) complain about constructs such as this:
> >
> >   grub_printf (_("foo"));
> >
> > ... because it's all too easy for a translator to (usually accidentally)
> > insert % sequences which would cause printf to behave incorrectly. This
> > should instead be:
> >
> >   grub_printf ("%s", _("foo"));
> >
> > Patch follows. I can't help thinking that this would be easier with a
> > grub_puts, but perhaps that isn't worth it given the relatively small
> > number of occurrences here?
> >
> > Also, should the line in notify_execution_failure instead be:
> >
> > -  grub_printf (_("Failed to boot default entries.\n"));
> > +  grub_printf ("%s\n", _("Failed to boot default entries."));
> >
> > ... to get rid of the unsightly \n in this translated string?
> 
> This warning is simply wrong in this context.

I disagree. I have seen many practical problems caused by this coding
style.

> And silencing it is against gettext manual. Read
> http://www.gnu.org/software/hello/manual/gettext/Preparing-Strings.html

I see nothing relevant in that link. (I am familiar with gettext
already, having been using it for many years in other free software
projects.)

> http://www.gnu.org/software/hello/manual/gettext/c_002dformat-Flag.html#c_002dformat-Flag

That suggests one available option for tweaking the way gettext handles
such strings, but it does not prescribe any particular solution to the
problem. Indeed, it says "Of course one would normally use fputs", which
is just as valid a solution as adding the c-format flag, and quite
possibly more valid since it's a heck of a lot easier to maintain. We
don't have fputs in GRUB right now, so grub_printf ("%s", _(...)) is
perfectly valid too, in just the same way that fputs is.

IME, forcing no-c-format is much more useful than forcing c-format.

In short, it is not true that "silencing it is against gettext manual",
but thank you for providing me with links that if anything support my
position. ;-)

Vladimir, please can you be a little less terse in your objections in
future? For example, it is often helpful to quote specific text rather
than vague URLs. If you would like arbitration on whether my approach is
"correct" as per the gettext maintainers, I'm happy to bring it up with
them.

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Build failures on Ubuntu due to gettext

[Carles Pina i Estany]

Hi,

On Dec/07/2009, Vladimir '??-coder/phcoder' Serbinenko wrote:
> Colin Watson wrote:
> > Ubuntu's GCC enables -Wformat-security by default. This causes GCC to
> > (IMO rightly!) complain about constructs such as this:
> >
> >   grub_printf (_("foo"));
> >
> > ... because it's all too easy for a translator to (usually accidentally)
> > insert % sequences which would cause printf to behave incorrectly. This
> > should instead be:
> >
> >   grub_printf ("%s", _("foo"));
> >
> > Patch follows. I can't help thinking that this would be easier with a
> > grub_puts, but perhaps that isn't worth it given the relatively small
> > number of occurrences here?
> >
> > Also, should the line in notify_execution_failure instead be:
> >
> > -  grub_printf (_("Failed to boot default entries.\n"));
> > +  grub_printf ("%s\n", _("Failed to boot default entries."));
> >
> > ... to get rid of the unsightly \n in this translated string?
> >   
> This warning is simply wrong in this context. And silencing it is
> against gettext manual. Read
> http://www.gnu.org/software/hello/manual/gettext/Preparing-Strings.html
> http://www.gnu.org/software/hello/manual/gettext/c_002dformat-Flag.html#c_002dformat-Flag

other solution that I like even more (but I have a problem to
implement): use --flag=_:1:pass-c-format . So all strings will be
c-format and msgfmt will check the number of parameters (even if the
string doesn't have %C, will be c-format and msgfmt should complain if
msgstr has a new %C)
-- 
Carles Pina i Estany
	http://pinux.info

Re: Build failures on Ubuntu due to gettext

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 809 bytes --]

Carles Pina i Estany wrote:
> And confirmed: if the translator adds %C in the second string
> ("Booting a command list") msgfmt -c is not complaining.
>
> So I would apply your patch, after understanding that it's only for
> strings that doesn't have any %C (and in my opinion xgettext could
> add the c-format if it's in a C file, but we will not discuss it here
> now :-) , I guess that in some cases would not be correct)
>
> Your patch conflicts with my one (Subject: gettext: grub_printf_ and N_)
> but it's not a big problem. If you commit before I would adapt
> my one, else I would adapt your one. 
>
>   
Your N_ patch superseeds Colin's patch. Feel free to use printf_ where
necessary. It solves format-security problems


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 293 bytes --]

Re: Build failures on Ubuntu due to gettext

[Carles Pina i Estany]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]


Hi,

On Dec/07/2009, Carles Pina i Estany wrote:

> So I would apply your patch, after understanding that it's only for

one more thing Colin. What do you think to not apply your patch and
apply the attached patch?

It force that the argument 1 of _ and N_ is a c-format (in this way
appears in grub.pot). This is, IMO, correct approach. And then msgfmt -c
checks the number of arguments.

If other projects are not using this approach they could have some
reason, like _("File") is a menu label and not a C format string. In
this cases, if this happens in Grub it's not making things worse.

Using it we should have all checks about the number of arguments and we
are not programming warning oriented.

If it's a linguism and wide accepted to do in the other way I personally
don't have any strong objection.

-- 
Carles Pina i Estany
	http://pinux.info

[-- Attachment #2: always_c_format.patch --]
[-- Type: text/x-diff, Size: 648 bytes --]

=== modified file 'Makefile.in'
--- Makefile.in	2009-11-30 01:25:57 +0000
+++ Makefile.in	2009-12-07 23:28:49 +0000
@@ -477,7 +477,7 @@ genkernsyms.sh: genkernsyms.sh.in config
 	$(SHELL) ./config.status
 
 $(srcdir)/po/$(PACKAGE).pot: po/POTFILES po/POTFILES-shell
-	cd $(srcdir) && $(XGETTEXT) --from-code=utf-8 -o $@ -f $< --keyword=_ --keyword=N_
+	cd $(srcdir) && $(XGETTEXT) --from-code=utf-8 -o $@ -f $< --keyword=_ --keyword=N_ --flag=N_:1:c-format --flag=_:1:c-format
 	cd $(srcdir) && $(XGETTEXT) --from-code=utf-8 -o $@ -f po/POTFILES-shell -j --language=Shell
 
 $(foreach lang, $(LINGUAS), $(srcdir)/po/$(lang).po): po/$(PACKAGE).pot

Re: Build failures on Ubuntu due to gettext

[Colin Watson]

Furthermore, the style I suggested is used in many other GNU projects.
Here are just a few examples:

  coreutils/src/du.c:994:       error (0, 0, "%s", _("invalid zero-length file name"));
  diffutils/src/sdiff.c:853:  fprintf (stderr, "%s", _("\
  diffutils/src/sdiff.c-854-ed:\tEdit then use both versions, each decorated with a header.\n\
  glibc/inet/rcmd.c:178:                              __fxprintf(NULL, "%s", _("\
  glibc/inet/rcmd.c-179-rcmd: socket: All ports in use\n"));
  gnulib/lib/xalloc-die.c:34:  error (exit_failure, 0, "%s", _("memory exhausted"));

Indeed, here's a commit from the gettext author using this style:

  http://git.savannah.gnu.org/cgit/gettext.git/commit/?id=d5d1ae3b3c3ae2f837740e5f5d65326197ccdb98

(Look for close_stdout_status in lib/closeout.c.)

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Build failures on Ubuntu due to gettext

[Colin Watson]

On Tue, Dec 08, 2009 at 12:25:01AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> Your N_ patch superseeds Colin's patch. Feel free to use printf_ where
> necessary. It solves format-security problems

It only works around the compiler warning if functions have not been
given the correct function attributes. It certainly doesn't solve the
underlying issue.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Build failures on Ubuntu due to gettext

[Colin Watson]

On Mon, Dec 07, 2009 at 11:38:04PM +0000, Carles Pina i Estany wrote:
> On Dec/07/2009, Carles Pina i Estany wrote:
> > So I would apply your patch, after understanding that it's only for
> 
> one more thing Colin. What do you think to not apply your patch and
> apply the attached patch?
> 
> It force that the argument 1 of _ and N_ is a c-format (in this way
> appears in grub.pot). This is, IMO, correct approach. And then msgfmt -c
> checks the number of arguments.

I really think that this is incorrect, I'm afraid. I've certainly never
seen any other project do this. There's no reason to restrict _ and N_
only to be used by printf-a-likes.

Cheers,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Build failures on Ubuntu due to gettext

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1374 bytes --]

Colin Watson wrote:
> Furthermore, the style I suggested is used in many other GNU projects.
> Here are just a few examples:
>
>   coreutils/src/du.c:994:       error (0, 0, "%s", _("invalid zero-length file name"));
>   diffutils/src/sdiff.c:853:  fprintf (stderr, "%s", _("\
>   diffutils/src/sdiff.c-854-ed:\tEdit then use both versions, each decorated with a header.\n\
>   glibc/inet/rcmd.c:178:                              __fxprintf(NULL, "%s", _("\
>   glibc/inet/rcmd.c-179-rcmd: socket: All ports in use\n"));
>   gnulib/lib/xalloc-die.c:34:  error (exit_failure, 0, "%s", _("memory exhausted"));
>
>   
I have nothing against defining grub_putl_ (str); equivalent to
grub_printf ("%s\n", str); (defined as function if it's used extensively
for space reasons).
Just from your previous mails it seemed that you proposed to transform
strings like
grub_printf (_("Moving file %s to %s."), f1, f2); to grub_printf ("%s %s
%s %s.", _("Moving file"), f1, _("to"), f2);
to avoid translating format-strings which would be completely
untranslatable.
> Indeed, here's a commit from the gettext author using this style:
>
>   http://git.savannah.gnu.org/cgit/gettext.git/commit/?id=d5d1ae3b3c3ae2f837740e5f5d65326197ccdb98
>
> (Look for close_stdout_status in lib/closeout.c.)
>
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 293 bytes --]

Re: Build failures on Ubuntu due to gettext

[Colin Watson]

On Mon, Dec 07, 2009 at 10:46:30PM +0000, Carles Pina i Estany wrote:
> Your patch conflicts with my one (Subject: gettext: grub_printf_ and N_)
> but it's not a big problem. If you commit before I would adapt
> my one, else I would adapt your one. 

Please go ahead with your patch after due discussion, and I'll adjust
mine afterwards. It's too confusing to try to discuss both at the same
time.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Build failures on Ubuntu due to gettext

[Colin Watson]

On Tue, Dec 08, 2009 at 12:59:28AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> Colin Watson wrote:
> > Furthermore, the style I suggested is used in many other GNU projects.
> > Here are just a few examples:
> >
> >   coreutils/src/du.c:994:       error (0, 0, "%s", _("invalid zero-length file name"));
> >   diffutils/src/sdiff.c:853:  fprintf (stderr, "%s", _("\
> >   diffutils/src/sdiff.c-854-ed:\tEdit then use both versions, each decorated with a header.\n\
> >   glibc/inet/rcmd.c:178:                              __fxprintf(NULL, "%s", _("\
> >   glibc/inet/rcmd.c-179-rcmd: socket: All ports in use\n"));
> >   gnulib/lib/xalloc-die.c:34:  error (exit_failure, 0, "%s", _("memory exhausted"));
> 
> I have nothing against defining grub_putl_ (str); equivalent to
> grub_printf ("%s\n", str); (defined as function if it's used extensively
> for space reasons).
> Just from your previous mails it seemed that you proposed to transform
> strings like
> grub_printf (_("Moving file %s to %s."), f1, f2); to grub_printf ("%s %s
> %s %s.", _("Moving file"), f1, _("to"), f2);
> to avoid translating format-strings which would be completely
> untranslatable.

What on earth?! I said nothing of the kind! That would obviously be
completely insane. How did you arrive at that impression?

My patch made the following transformation:

-  grub_printf (_("literal string"));
+  grub_printf ("%s", _("literal string"));

This was only necessary in five places, so I doubt that a function is
worth it.

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Build failures on Ubuntu due to gettext

[Carles Pina i Estany]

Hi,

On Dec/08/2009, Colin Watson wrote:
> On Mon, Dec 07, 2009 at 10:46:30PM +0000, Carles Pina i Estany wrote:
> > Your patch conflicts with my one (Subject: gettext: grub_printf_ and N_)
> > but it's not a big problem. If you commit before I would adapt
> > my one, else I would adapt your one. 
> 
> Please go ahead with your patch after due discussion, and I'll adjust
> mine afterwards. It's too confusing to try to discuss both at the same
> time.

I just pushed my one since I understand that it's all right (using
grub_vprintf). And it's complementary with your one.

If you push your one (for which one I agree) I will adjust my pending
patches to follow the coding sytle.

Thanks,

-- 
Carles Pina i Estany
	http://pinux.info

Re: Build failures on Ubuntu due to gettext

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

Colin Watson wrote:
> What on earth?! I said nothing of the kind! That would obviously be
> completely insane.
Sorry for having misunderstood you.
>  How did you arrive at that impression?
>   
Extrapolation of proposed changes since a lot of patches in gettext
threads were "samples".
> My patch made the following transformation:
>
> -  grub_printf (_("literal string"));
> +  grub_printf ("%s", _("literal string"));
>
> This was only necessary in five places, so I doubt that a function is
> worth it.
>   
Then it's not worth it. But again we haven't gettext'ized whole grub2
yet to know for sure. Perhaps a macro so we can change it later if
necessary?
> Thanks,
>
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 293 bytes --]

Re: Build failures on Ubuntu due to gettext

[Carles Pina i Estany]

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]


Hi Vladimir, Colin,

On Dec/08/2009, Vladimir '??-coder/phcoder' Serbinenko wrote:

> > My patch made the following transformation:
> >
> > -  grub_printf (_("literal string"));
> > +  grub_printf ("%s", _("literal string"));
> >
> > This was only necessary in five places, so I doubt that a function is
> > worth it.
> >   
> Then it's not worth it. But again we haven't gettext'ized whole grub2
> yet to know for sure. Perhaps a macro so we can change it later if
> necessary?

Vladimir, Colin: see the attached patch with the Colin changes but
macrofied.

Should compile fine in Ubuntu, but I have not tried.

Are we happy with grub_put_ function name?

Colin: could you apply this patch or similar?

I don't want to push more gettext strings before setting up the basic
infrastructure to avoid working twice.

Thanks,

-- 
Carles Pina i Estany
	http://pinux.info

[-- Attachment #2: grub_put_.patch --]
[-- Type: text/x-diff, Size: 2099 bytes --]

=== modified file 'include/grub/misc.h'
--- include/grub/misc.h	2009-12-08 00:08:52 +0000
+++ include/grub/misc.h	2009-12-09 23:51:07 +0000
@@ -34,6 +34,8 @@
 /* XXX: If grub_memmove is too slow, we must implement grub_memcpy.  */
 #define grub_memcpy(d,s,n)	grub_memmove ((d), (s), (n))
 
+#define grub_put_(str)		grub_printf("%s", (str))
+
 void *EXPORT_FUNC(grub_memmove) (void *dest, const void *src, grub_size_t n);
 char *EXPORT_FUNC(grub_strcpy) (char *dest, const char *src);
 char *EXPORT_FUNC(grub_strncpy) (char *dest, const char *src, int c);

=== modified file 'normal/menu_entry.c'
--- normal/menu_entry.c	2009-12-08 00:08:52 +0000
+++ normal/menu_entry.c	2009-12-09 23:46:36 +0000
@@ -1000,7 +1000,7 @@ run (struct screen *screen)
 
   grub_cls ();
   grub_printf ("  ");
-  grub_printf_ (N_("Booting a command list"));
+  grub_put_ (N_("Booting a command list"));
   grub_printf ("\n\n");
 
 
@@ -1182,6 +1182,6 @@ grub_menu_entry_run (grub_menu_entry_t e
   grub_print_error ();
   grub_errno = GRUB_ERR_NONE;
   grub_putchar ('\n');
-  grub_printf_ (N_("Press any key to continue..."));
+  grub_put_ (N_("Press any key to continue..."));
   (void) grub_getkey ();
 }

=== modified file 'normal/menu_text.c'
--- normal/menu_text.c	2009-12-08 00:08:52 +0000
+++ normal/menu_text.c	2009-12-09 23:47:04 +0000
@@ -40,7 +40,7 @@ void
 grub_wait_after_message (void)
 {
   grub_putchar ('\n');
-  grub_printf_ (N_("Press any key to continue..."));
+  grub_put_ (N_("Press any key to continue..."));
   (void) grub_getkey ();
   grub_putchar ('\n');
 }
@@ -206,7 +206,7 @@ entry is highlighted.");
       if (nested)
         {
           grub_printf ("\n        ");
-          grub_printf_ (N_("ESC to return previous menu."));
+          grub_put_ (N_("ESC to return previous menu."));
         }
     }
 }
@@ -655,7 +655,7 @@ notify_execution_failure (void *userdata
       grub_errno = GRUB_ERR_NONE;
     }
   grub_printf ("\n  ");
-  grub_printf_ (N_("Failed to boot default entries.\n"));
+  grub_put_ (N_("Failed to boot default entries.\n"));
   grub_wait_after_message ();
 }
 

Re: Build failures on Ubuntu due to gettext

[Carles Pina i Estany]

Hi,

As Vladimir spotted:

On Dec/09/2009, Carles Pina i Estany wrote:

This...:
> +#define grub_put_(str)		grub_printf("%s", (str))

should be:
#define grub_put_(str)          grub_printf(N_ ("%s"), (str))

If you Colin commit it don't propagate my mistake.

-- 
Carles Pina i Estany
	http://pinux.info

Re: Build failures on Ubuntu due to gettext

[Carles Pina i Estany]

Hi,

On Dec/10/2009, Carles Pina i Estany wrote:

As Richard commented (thanks):

> #define grub_put_(str)          grub_printf(N_ ("%s"), (str))

#define grub_put_(str)                  grub_printf("%s", N (str))

I should not be sending patches when too tired / without properly
testing with the .po that I've just deleted doing some other tests :-)

Now should be fine and after it's committed next gettext patches will
use it when needed.

Cheers,

-- 
Carles Pina i Estany
	http://pinux.info

Re: Build failures on Ubuntu due to gettext

[Robert Millan]

On Tue, Dec 08, 2009 at 12:10:34AM +0000, Colin Watson wrote:
> 
> My patch made the following transformation:
> 
> -  grub_printf (_("literal string"));
> +  grub_printf ("%s", _("literal string"));
> 
> This was only necessary in five places, so I doubt that a function is
> worth it.

I think we all agree that it's safer to avoid gettextizing the template;
furthemore I agree with Colin that this seems to be a correct approach,
but our case is unusual in that we're very pressed for size.

Even if we're only discussing a change in five places, I assume we're
going to find this situation in lots of calls throurough GRUB code.  If
this is so, it really calls for a solution that doesn't make GCC generate
two memory references and pass two arguments.

So first of all, how many times are we going to find similar calls that
need to be adjusted in some way?  If we're going to find many of them (as
I expect), I think this warrants adding a new function (which, btw, will
also make GRUB slightly faster).

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: Build failures on Ubuntu due to gettext

[Carles Pina i Estany]

Hi,

On Dec/10/2009, Robert Millan wrote:
> On Tue, Dec 08, 2009 at 12:10:34AM +0000, Colin Watson wrote:

> Even if we're only discussing a change in five places, I assume we're
> going to find this situation in lots of calls throurough GRUB code.
> If this is so, it really calls for a solution that doesn't make GCC
> generate two memory references and pass two arguments.
> 
> So first of all, how many times are we going to find similar calls
> that

very rush numbers, based on my non-commited pending to improve patch to
change commands/*:
carles@pinux:~/grub2/grub/po$ grep -A 3 "\#\: commands\/" grub.pot |
grep ^msgid | grep -c -v %
86

> need to be adjusted in some way?  If we're going to find many of them
> (as I expect), I think this warrants adding a new function (which,
> btw, will also make GRUB slightly faster).

Plus about 10 in normal/* (aprox, 5 that Colin did and some more in
another pending patch I guess)

I haven't looked in other code yet.

I think that Vladimir proposed to do it in a macro now and then properly
count and implement as a function or not later.

From: Vladimir
Date: Tue, 08 Dec 2009 01:15:30 +0100
> Then it's not worth it. But again we haven't gettext'ized whole grub2
> yet to know for sure. Perhaps a macro so we can change it later if
> necessary?

So, for about 86 in commands/* and probably some more in normal/*:
function now? Now a macro and then we review?

Colin: for me you can commit your patch some way or another so you could
compile in Ubuntu and I can commit soon more things.

Thanks,

-- 
Carles Pina i Estany
	http://pinux.info