escape strings in grub.cfg

escape strings in grub.cfg

[Carles Pina i Estany]

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


Hello,

Last weekend, the Asturian team found a bug in Grub. Thanks for the
debugging. The string that appears in grub.cfg for "Loading the initram"
in Asturian is "Cargando'l discu RAM inicial...". So grub.cfg looks
like:
        echo    Cargando'l discu RAM inicial...

This is syntactically invalid because the character "'".

Easy work around: generate the line like:
	echo 	"Cargando'l discu RAM inicial..."

New problem: in some language, this string could have a double quote
inside.

New solution: escape the double quotes strings when they are used in
gettext and always use these strings with double quotes.

See the attached patch.

I'm not convinced of the patch, does anybody has a cleaner idea/patch?

Cheers,

PS: I haven't double checked, but lines like initrd  /boot/initrd.img-2.6.32-18-generic will fail if the path has spaces. I'll address it in the future.

PS2: I haven't checked the patch yet, let's comment on the idea. Before
committing I would write the changelog and check it.

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

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

=== modified file 'util/grub-mkconfig_lib.in'
--- util/grub-mkconfig_lib.in	2010-02-25 13:30:50 +0000
+++ util/grub-mkconfig_lib.in	2010-04-10 20:47:18 +0000
@@ -188,3 +188,8 @@ version_find_latest ()
   done
   echo "$a"
 }
+
+gettext_escape_double_quotes ()
+{
+  echo -n $(gettext $@) | sed 's/\"/\\\"/'
+}

=== modified file 'util/grub.d/10_linux.in'
--- util/grub.d/10_linux.in	2010-04-08 09:54:44 +0000
+++ util/grub.d/10_linux.in	2010-04-10 20:49:06 +0000
@@ -56,9 +56,9 @@ linux_entry ()
   recovery="$3"
   args="$4"
   if ${recovery} ; then
-    title="$(gettext "%s, with Linux %s (recovery mode)")"
+    title="$(gettext_escape_double_quotes "%s, with Linux %s (recovery mode)")"
   else
-    title="$(gettext "%s, with Linux %s")"
+    title="$(gettext_escape_double_quotes "%s, with Linux %s")"
   fi
   printf "menuentry \"${title}\" ${CLASS} {\n" "${os}" "${version}"
   save_default_entry | sed -e "s/^/\t/"
@@ -83,12 +83,12 @@ EOF
   fi
   printf '%s\n' "${prepare_boot_cache}"
   cat << EOF
-	echo	$(printf "$(gettext "Loading Linux %s ...")" ${version})
+	echo	\"$(printf "$(gettext_escape_double_quotes "Loading Linux %s ...")" ${version})\"
 	linux	${rel_dirname}/${basename} root=${linux_root_device_thisversion} ro ${args}
 EOF
   if test -n "${initrd}" ; then
     cat << EOF
-	echo	$(gettext "Loading initial ramdisk ...")
+	echo	\"$(gettext_escape_double_quotes "Loading initial ramdisk ...")\"
 	initrd	${rel_dirname}/${initrd}
 EOF
   fi

Re: escape strings in grub.cfg

[Colin Watson]

On Sat, Apr 10, 2010 at 10:10:15PM +0100, Carles Pina i Estany wrote:
> Last weekend, the Asturian team found a bug in Grub. Thanks for the
> debugging. The string that appears in grub.cfg for "Loading the initram"
> in Asturian is "Cargando'l discu RAM inicial...". So grub.cfg looks
> like:
>         echo    Cargando'l discu RAM inicial...
> 
> This is syntactically invalid because the character "'".
> 
> Easy work around: generate the line like:
> 	echo 	"Cargando'l discu RAM inicial..."
> 
> New problem: in some language, this string could have a double quote
> inside.
> 
> New solution: escape the double quotes strings when they are used in
> gettext and always use these strings with double quotes.
> 
> See the attached patch.
> 
> I'm not convinced of the patch, does anybody has a cleaner idea/patch?

This doesn't cover everything - you've just moved the goalposts a bit.
Here's the relevant bit of the lexer:

<SQUOTE>{
  \'            {
                  yy_pop_state (yyscanner);
                  ARG (GRUB_SCRIPT_ARG_TYPE_SQSTR);
                }
  [^\']+        { COPY (yytext, yyleng); }
}

<DQUOTE>{
  \"            {
                  yy_pop_state (yyscanner);
                  ARG (GRUB_SCRIPT_ARG_TYPE_DQSTR);
                }
  \$            {
                  yy_push_state (VAR, yyscanner);
                  ARG (GRUB_SCRIPT_ARG_TYPE_DQSTR);
                }
  \\\\          { COPY ("\\", 1); }
  \\\"          { COPY ("\"", 1); }
  \\\n          { /* ignore */ }
  [^\"$\\\n]+   { COPY (yytext, yyleng); }
  (.|\n)        { COPY (yytext, yyleng); }
}

In other words, only ' is special (as a terminator) when reading a
single-quoted string, and the sequences " $ \\ \" \n are special inside
a double-quoted string.  (Incidentally, I think there's another bug
here; if $ is special, \$ should be special as well.)

Given this, wouldn't you be better off using single-quotes instead?
You'd then end up with:

          echo    'Cargando'\''l discu RAM inicial...'

... where the construction used to escape ' is "close quotes, literal ',
reopen quotes".  (Tested in grub-emu.)

> +gettext_escape_double_quotes ()
> +{
> +  echo -n $(gettext $@) | sed 's/\"/\\\"/'
> +}

This is underquoted, and in any case the 'echo -n' is redundant since
gettext itself already writes to standard output and $() substitution
strips trailing newlines.  $@ requires special treatment, and that sed
s/// command needs /g.  With my suggestion, I'd recommend:

  gettext_quoted () {
    gettext "$@" | sed "s/'/'\\\\''/g"
  }

... and then make sure it's enclosed in '' rather than "" wherever it's
used (or modify the sed to put single-quotes at the start and end, if
that works out neater).

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: escape strings in grub.cfg

[BVK Chaitanya]

On Mon, Apr 12, 2010 at 5:39 AM, Colin Watson <cjwatson@ubuntu.com> wrote:
>
> In other words, only ' is special (as a terminator) when reading a
> single-quoted string, and the sequences " $ \\ \" \n are special inside
> a double-quoted string.  (Incidentally, I think there's another bug
> here; if $ is special, \$ should be special as well.)
>


Nice catch!  There is indeed a bug.  We cannot escape $ in dquote
strings currently, this needs to be fixed.




-- 
bvk.chaitanya

Re: escape strings in grub.cfg

[richardvoigt]

On Sun, Apr 11, 2010 at 7:09 PM, Colin Watson <cjwatson@ubuntu.com> wrote:
> On Sat, Apr 10, 2010 at 10:10:15PM +0100, Carles Pina i Estany wrote:
>> Last weekend, the Asturian team found a bug in Grub. Thanks for the
>> debugging. The string that appears in grub.cfg for "Loading the initram"
>> in Asturian is "Cargando'l discu RAM inicial...". So grub.cfg looks
>> like:
>>         echo    Cargando'l discu RAM inicial...
>>
>> This is syntactically invalid because the character "'".
>>
>> Easy work around: generate the line like:
>>       echo    "Cargando'l discu RAM inicial..."
>>
>> New problem: in some language, this string could have a double quote
>> inside.
>>
>> New solution: escape the double quotes strings when they are used in
>> gettext and always use these strings with double quotes.
>>
>> See the attached patch.
>>
>> I'm not convinced of the patch, does anybody has a cleaner idea/patch?
>
> This doesn't cover everything - you've just moved the goalposts a bit.
> Here's the relevant bit of the lexer:
>
> <SQUOTE>{
>  \'            {
>                  yy_pop_state (yyscanner);
>                  ARG (GRUB_SCRIPT_ARG_TYPE_SQSTR);
>                }
>  [^\']+        { COPY (yytext, yyleng); }
> }
>
> <DQUOTE>{
>  \"            {
>                  yy_pop_state (yyscanner);
>                  ARG (GRUB_SCRIPT_ARG_TYPE_DQSTR);
>                }
>  \$            {
>                  yy_push_state (VAR, yyscanner);
>                  ARG (GRUB_SCRIPT_ARG_TYPE_DQSTR);
>                }
>  \\\\          { COPY ("\\", 1); }
>  \\\"          { COPY ("\"", 1); }
>  \\\n          { /* ignore */ }
>  [^\"$\\\n]+   { COPY (yytext, yyleng); }
>  (.|\n)        { COPY (yytext, yyleng); }
> }
>
> In other words, only ' is special (as a terminator) when reading a
> single-quoted string, and the sequences " $ \\ \" \n are special inside
> a double-quoted string.  (Incidentally, I think there's another bug
> here; if $ is special, \$ should be special as well.)
>
> Given this, wouldn't you be better off using single-quotes instead?
> You'd then end up with:
>
>          echo    'Cargando'\''l discu RAM inicial...'
>
> ... where the construction used to escape ' is "close quotes, literal ',
> reopen quotes".  (Tested in grub-emu.)
>
>> +gettext_escape_double_quotes ()
>> +{
>> +  echo -n $(gettext $@) | sed 's/\"/\\\"/'
>> +}
>
> This is underquoted, and in any case the 'echo -n' is redundant since
> gettext itself already writes to standard output and $() substitution
> strips trailing newlines.  $@ requires special treatment, and that sed
> s/// command needs /g.  With my suggestion, I'd recommend:
>
>  gettext_quoted () {
>    gettext "$@" | sed "s/'/'\\\\''/g"
>  }

I haven't tried it myself, but you just said the newline is stripped
by $() and then took out the $(), seems like the newline would be
present again.


>
> ... and then make sure it's enclosed in '' rather than "" wherever it's
> used (or modify the sed to put single-quotes at the start and end, if
> that works out neater).
>
> --
> Colin Watson                                       [cjwatson@ubuntu.com]
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>

Re: escape strings in grub.cfg

[Colin Watson]

On Mon, Apr 12, 2010 at 12:23:01PM -0500, richardvoigt@gmail.com wrote:
> On Sun, Apr 11, 2010 at 7:09 PM, Colin Watson <cjwatson@ubuntu.com> wrote:
> > On Sat, Apr 10, 2010 at 10:10:15PM +0100, Carles Pina i Estany wrote:
> >> +gettext_escape_double_quotes ()
> >> +{
> >> +  echo -n $(gettext $@) | sed 's/\"/\\\"/'
> >> +}
> >
> > This is underquoted, and in any case the 'echo -n' is redundant since
> > gettext itself already writes to standard output and $() substitution
> > strips trailing newlines.  $@ requires special treatment, and that sed
> > s/// command needs /g.  With my suggestion, I'd recommend:
> >
> >  gettext_quoted () {
> >    gettext "$@" | sed "s/'/'\\\\''/g"
> >  }
> 
> I haven't tried it myself, but you just said the newline is stripped
> by $() and then took out the $(), seems like the newline would be
> present again.

In context, gettext_quoted would always be called from inside $(), so
this isn't a problem.

(In general I find that it's best to write shell functions such that
they emit trailing newlines on their output, and let the caller strip it
if necessary; this usually comes out quite naturally.  Of course there
are exceptions.)

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: escape strings in grub.cfg

[Colin Watson]

On Mon, Apr 12, 2010 at 01:09:06AM +0100, Colin Watson wrote:
> With my suggestion, I'd recommend:
> 
>   gettext_quoted () {
>     gettext "$@" | sed "s/'/'\\\\''/g"
>   }
> 
> ... and then make sure it's enclosed in '' rather than "" wherever it's
> used (or modify the sed to put single-quotes at the start and end, if
> that works out neater).

I've gone ahead and committed a patch based on this to trunk, and I'll
backport it to Ubuntu Lucid shortly as well.  Testing welcome!

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: escape strings in grub.cfg

[Carles Pina i Estany]

Hi,

On Apr/13/2010, Colin Watson wrote:
> On Mon, Apr 12, 2010 at 01:09:06AM +0100, Colin Watson wrote:
> > With my suggestion, I'd recommend:
> > 
> >   gettext_quoted () {
> >     gettext "$@" | sed "s/'/'\\\\''/g"
> >   }
> > 
> > ... and then make sure it's enclosed in '' rather than "" wherever it's
> > used (or modify the sed to put single-quotes at the start and end, if
> > that works out neater).
> 
> I've gone ahead and committed a patch based on this to trunk, and I'll
> backport it to Ubuntu Lucid shortly as well.  Testing welcome!

thanks Colin, I'll do some tests as well.

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

Re: escape strings in grub.cfg

[BVK Chaitanya]

On Mon, Apr 12, 2010 at 10:19 PM, BVK Chaitanya <bvk.groups@gmail.com> wrote:
> On Mon, Apr 12, 2010 at 5:39 AM, Colin Watson <cjwatson@ubuntu.com> wrote:
>>
>> In other words, only ' is special (as a terminator) when reading a
>> single-quoted string, and the sequences " $ \\ \" \n are special inside
>> a double-quoted string.  (Incidentally, I think there's another bug
>> here; if $ is special, \$ should be special as well.)
>>
>
>
> Nice catch!  There is indeed a bug.  We cannot escape $ in dquote
> strings currently, this needs to be fixed.
>


FYI, this bug is fixed in mainline.



-- 
bvk.chaitanya