Skip to contentSkip to conversation

add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

PATCH In review Alexander Kurtz 4 participants 15 messages Apr 5, 2011 – Apr 10, 2011

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

Hi,

currently you can't use an image which has whitespace in its filename as GRUB background image because grub-mkconfig_lib lacks proper variable quoting (see [1] for more information). I've attached three patches which should fix this problem:

quote-big.patch:
This patch fixes the problem with the minimal set of changes.

quote-medium.patch:
This patch adds proper quoting wherever it is safe to do so.

quote-small.patch
This patch additionally adds quoting in cases like this:
${grub-probe} --foo --bar => "${grub-probe}" --foo --bar
This breaks things if ${grub-probe} contains additional parameters.

Please note that I already submitted similar patches a while ago[2], but these are a little outdated now. The attached patches have been refreshed and should apply cleanly to the current trunk.

What do you think?

Best regards

Alexander Kurtz

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=612417#5
[2] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=612417#10

util/grub-mkconfig_lib.in +4 -4
diff -Naur bazaar/util/grub-mkconfig_lib.in small/util/grub-mkconfig_lib.in
@@ -44,20 +44,20 @@
 
 make_system_path_relative_to_its_root ()
 {
-  ${grub_mkrelpath} $1
+  ${grub_mkrelpath} "${1}"
 }
 
 is_path_readable_by_grub ()
 {
-  path=$1
+  path="${1}"
 
   # abort if path doesn't exist
-  if test -e $path ; then : ;else
+  if test -e "${path}" ; then : ;else
     return 1
   fi
 
   # abort if file is in a filesystem we can't read
-  if ${grub_probe} -t fs $path > /dev/null 2>&1 ; then : ; else
+  if ${grub_probe} -t fs "${path}" > /dev/null 2>&1 ; then : ; else
     return 1
   fi
 
util/grub-mkconfig_lib.in +57 -57

238 lines not on this page · raw message

util/grub-mkconfig_lib.in +57 -57

238 lines not on this page · raw message

#1 lore raw
Quoted 18 lines · Alexander Kurtz

Hi,

currently you can't use an image which has whitespace in its filename as GRUB background image because grub-mkconfig_lib lacks proper variable quoting (see [1] for more information). I've attached three patches which should fix this problem:

quote-big.patch:
This patch fixes the problem with the minimal set of changes.

quote-medium.patch:
This patch adds proper quoting wherever it is safe to do so.

  quote-small.patch
   This patch additionally adds quoting in cases like this:
    ${grub-probe} --foo --bar =>  "${grub-probe}" --foo --bar
   This breaks things if ${grub-probe} contains additional parameters.
Quoted 5 lines

Please note that I already submitted similar patches a while ago[2], but these are a little outdated now. The attached patches have been refreshed and should apply cleanly to the current trunk.

What do you think?

Why do you use construct like "${x}" instead of "$x"?

${x} is useless, unless the character that follow $x might be part of the variable name:

"${x}y" is obviously different from "$xy", but "${x}" is identical to "$x".

Nicolas.

Quoted 14 lines

Best regards

Alexander Kurtz

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=612417#5
[2] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=612417#10

_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

#2 lore raw
Quoted 1 line · Nicolas de Pesloüan

Why do you use construct like "${x}" instead of "$x"?

There's a fair bit of this in existing GRUB code. I agree it's fairly pointless, but in general it's best for patches to follow local style.

Signature · 1 line
Colin Watson                                       [[email protected]]
#3 lore raw
In reply to Nicolas de Pesloüan

Am Dienstag, den 05.04.2011, 21:16 +0200 schrieb Nicolas de Pesloüan:

Quoted 1 line

Why do you use construct like "${x}" instead of "$x"?

Because code isn't written once and then stays untouched forever. It changes over time and may be used in situations you did not anticipate. Writing solid code (and in shell scripts that definitely includes quoting your variables) avoids unnecessary bugs like this one.

I've just seen too many poorly written shell scripts with hidden (sometimes even security-relevant) bugs to not do things properly.

And it looks cleaner ;-)

Best regards

Alexander Kurtz

#4 lore raw
Quoted 7 lines · Alexander Kurtz

Am Dienstag, den 05.04.2011, 21:16 +0200 schrieb Nicolas de Pesloüan:

Quoted 1 line

Why do you use construct like "${x}" instead of "$x"?

Because code isn't written once and then stays untouched forever. It changes over time and may be used in situations you did not anticipate. Writing solid code (and in shell scripts that definitely includes quoting your variables) avoids unnecessary bugs like this one.

While I agree with the general sentiments here, I would like to point out that "${x}" is rigorously identical to "$x" in shell. "Quoting your variables" refers to the "", not to the {}. It appears to be a common misconception that {} has some quote-like semantics; it does not. As Nicolas says, it exists purely to separate variable names from subsequent text that would otherwise be part of the name.

Signature · 1 line
Colin Watson                                       [[email protected]]
#5 lore raw
In reply to Alexander Kurtz
Quoted 8 lines · Alexander Kurtz

Am Dienstag, den 05.04.2011, 21:16 +0200 schrieb Nicolas de Pesloüan:

Quoted 1 line

Why do you use construct like "${x}" instead of "$x"?

Because code isn't written once and then stays untouched forever. It changes over time and may be used in situations you did not anticipate. Writing solid code (and in shell scripts that definitely includes quoting your variables) avoids unnecessary bugs like this one.

Quoted 8 lines

I've just seen too many poorly written shell scripts with hidden (sometimes even security-relevant) bugs to not do things properly.

And it looks cleaner ;-)

Best regards

Alexander Kurtz

As Colin says, the {} construct is mostly useless and at least does not contribute to quoting.

On the same kind of things, the construct if test "x$foo" = "x" is pointless. It is a very strange heritage from DOS, where IF x%A = x was a common construct, because DOS (command.com) lack quoting. (Is wasn't possible to write IF "%a" = ""). In shell, "" is an empty string, but a real argument to commands. The following construct is the good one : if test "$foo" = "". The test command will receive three arguments: the value of $foo, the = sign and an empty argument and will return true if $foo happens to be empty.

Writing solid code imply - in particular - knowing the exact behaviors of the programming language :-)

I agree of course that this is definitely cosmetic, and this doesn't means that your work to remove unquoted variable is not good and necessary.

Nicolas.

#6 lore raw
Quoted 8 lines · Nicolas de Pesloüan

On the same kind of things, the construct if test "x$foo" = "x" is pointless. It is a very strange heritage from DOS, where IF x%A = x was a common construct, because DOS (command.com) lack quoting. (Is wasn't possible to write IF "%a" = ""). In shell, "" is an empty string, but a real argument to commands. The following construct is the good one : if test "$foo" = "". The test command will receive three arguments: the value of $foo, the = sign and an empty argument and will return true if $foo happens to be empty.

Actually, it's more relevantly a heritage from pre-POSIX shells, which became habit for many GNU programmers via Autoconf. Here's what the Autoconf documentation has to say on the subject:

`test' (strings)
Posix says that test "STRING"' succeeds if STRING is not null, but this usage is not portable to traditional platforms like Solaris 10 /bin/sh', which mishandle strings like !' and -n'.

Posix also says that test ! "STRING"', test -n "STRING"' and `test -z "STRING"' work with any string, but many shells (such as Solaris, AIX 3.2, UNICOS 10.0.0.6, Digital Unix 4, etc.) get confused if STRING looks like an operator:

$ test -n =
test: argument expected
$ test ! -n
test: argument expected

Similarly, Posix says that both test "STRING1" = "STRING2"' and test "STRING1" != "STRING2"' work for any pairs of strings, but in practice this is not true for troublesome strings that look like operators or parentheses, or that begin with `-'.

It is best to protect such strings with a leading X', e.g., test "XSTRING" != X' rather than test -n "STRING"' or test ! "STRING"'.

Solaris 10 is not that old, and this has long been a problem for users of /bin/sh on Solaris in particular. Note that GRUB works on Solaris. I would be interested to know if OpenSolaris, or even Illumos, still uses a pre-POSIX /bin/sh.

In most cases, of course, this doesn't matter since it only affects certain strings. But on packages that run on Solaris I am always extremely reticent to change existing code to assume POSIX shell, even if I might write new code that way.

When I can assume POSIX shell, your version:

if test "$foo" = ""

is needlessly verbose. The standard stipulates that this works just fine:

if test "$foo"

Quoted 2 lines

Writing solid code imply - in particular - knowing the exact behaviors of the programming language :-)

On Unix-like systems, though, and particularly when it comes to shell, this often involves knowing about historical behaviour as well as current behaviour.

Signature · 1 line
Colin Watson                                       [[email protected]]
#7 lore raw
Quoted 3 lines · Colin Watson

On Unix-like systems, though, and particularly when it comes to shell, this often involves knowing about historical behaviour as well as current behaviour.

Thanks Colin, for clarifying this!!

Very appreciated.

Nicolas.

#8 lore raw

Am Dienstag, den 05.04.2011, 23:27 +0200 schrieb Nicolas de Pesloüan:

Quoted 9 lines · Colin Watson
Quoted 4 lines

On Unix-like systems, though, and particularly when it comes to shell, this often involves knowing about historical behaviour as well as current behaviour.

Thanks Colin, for clarifying this!!

Very appreciated.

Now that we've all learned something new again (at least I have ;-) ), let's go back to the main topic:

What do you think of the patch itself?

Best regards

Alexander Kurtz

#9 lore raw
Quoted 7 lines · Alexander Kurtz

Am Dienstag, den 05.04.2011, 23:27 +0200 schrieb Nicolas de Pesloüan:

Quoted 6 lines · Colin Watson
Quoted 4 lines

On Unix-like systems, though, and particularly when it comes to shell, this often involves knowing about historical behaviour as well as current behaviour.

Quoted 8 lines
Quoted 3 lines

Thanks Colin, for clarifying this!!

Very appreciated.

Now that we've all learned something new again (at least I have ;-) ), let's go back to the main topic:

What do you think of the patch itself?

It sounds good to me.

I'm just in doubt with the following construct :

a="echo "$b""
^ Is this quote closing the first one or opening a nested quote.

Nicolas.

#10 lore raw
Quoted 2 lines · Nicolas de Pesloüan

a="echo "$b""
^ Is this quote closing the first one or opening a nested quote.

It nests.

Signature · 1 line
Colin Watson                                       [[email protected]]
#11 lore raw
Quoted 6 lines · Colin Watson
Quoted 2 lines · Nicolas de Pesloüan

a="echo "$b""
^ Is this quote closing the first one or opening a nested quote.

It nests.

Ok, thanks. I failed to find a clear answer in the man.

So the whole patch is good for me.

Nicolas.

#12 lore raw
In reply to Alexander Kurtz
Quoted 6 lines · Alexander Kurtz

Am Dienstag, den 05.04.2011, 23:27 +0200 schrieb Nicolas de Pesloüan:

Quoted 5 lines · Colin Watson
Quoted 4 lines

On Unix-like systems, though, and particularly when it comes to shell, this often involves knowing about historical behaviour as well as current behaviour.

Quoted 7 lines
Quoted 3 lines

Thanks Colin, for clarifying this!!

Very appreciated.

Now that we've all learned something new again (at least I have ;-) ), let's go back to the main topic:

What do you think of the patch itself?

The (big) patch is mostly fine except:
-Please don't add brances where not needed
-Please don't add quoting to constants like x since x or "x" are strictly identical.

Quoted 9 lines

Best regards

Alexander Kurtz

_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Signature · 2 lines
Regards
Vladimir 'φ-coder/phcoder' Serbinenko
#13 lore raw

Am Freitag, den 08.04.2011, 15:44 +0200 schrieb Vladimir 'φ-coder/phcoder' Serbinenko:

Quoted 4 lines

The (big) patch is mostly fine except:

(quoted diff) +0 -2
-Please don't add brances where not needed
-Please don't add quoting to constants like x since x or "x" are

strictly identical.

Better now?

Best regards

Alexander Kurtz

util/grub-mkconfig_lib.in +44 -44
diff -Naur bazaar/util/grub-mkconfig_lib.in big-adjusted/util/grub-mkconfig_lib.in
@@ -16,19 +16,19 @@
 
 transform="@program_transform_name@"
 
-prefix=@prefix@
-exec_prefix=@exec_prefix@
-datarootdir=@datarootdir@
-datadir=@datadir@
-bindir=@bindir@
-sbindir=@sbindir@
-pkgdatadir=${datadir}/`echo @PACKAGE_TARNAME@ | sed "${transform}"`
+prefix="@prefix@"
+exec_prefix="@exec_prefix@"
+datarootdir="@datarootdir@"
+datadir="@datadir@"
+bindir="@bindir@"
+sbindir="@sbindir@"
+pkgdatadir="${datadir}/`echo "@PACKAGE_TARNAME@" | sed "${transform}"`"
 
 if test "x$grub_probe" = x; then
-  grub_probe=${sbindir}/`echo grub-probe | sed ${transform}`
+  grub_probe="${sbindir}/`echo grub-probe | sed "${transform}"`"
 fi
 if test "x$grub_mkrelpath" = x; then
-  grub_mkrelpath=${bindir}/`echo grub-mkrelpath | sed ${transform}`
+  grub_mkrelpath="${bindir}/`echo grub-mkrelpath | sed "${transform}"`"
 fi
 
 if $(which gettext >/dev/null 2>/dev/null) ; then
@@ -44,20 +44,20 @@
 
 make_system_path_relative_to_its_root ()
 {
-  ${grub_mkrelpath} $1
+  "${grub_mkrelpath}" "$1"
 }
 
 is_path_readable_by_grub ()
 {
-  path=$1
+  path="$1"
 
   # abort if path doesn't exist
-  if test -e $path ; then : ;else
+  if test -e "$path" ; then : ;else
     return 1
   fi
 
   # abort if file is in a filesystem we can't read
-  if ${grub_probe} -t fs $path > /dev/null 2>&1 ; then : ; else
+  if "${grub_probe}" -t fs "$path" > /dev/null 2>&1 ; then : ; else
     return 1
   fi
 
@@ -72,24 +72,24 @@
 
 convert_system_path_to_grub_path ()
 {
-  path=$1
+  path="$1"
 
   grub_warn "convert_system_path_to_grub_path() is deprecated.  Use prepare_grub_to_access_device() instead."
 
   # abort if GRUB can't access the path
-  if is_path_readable_by_grub ${path} ; then : ; else
+  if is_path_readable_by_grub "${path}" ; then : ; else
     return 1
   fi
 
-  if drive=`${grub_probe} -t drive $path` ; then : ; else
+  if drive="`"${grub_probe}" -t drive "$path"`" ; then : ; else
     return 1
   fi
 
-  if relative_path=`make_system_path_relative_to_its_root $path` ; then : ; else
+  if relative_path="`make_system_path_relative_to_its_root "$path"`" ; then : ; else
     return 1
   fi
 
-  echo ${drive}${relative_path}
+  echo "${drive}${relative_path}"
 }
 
 save_default_entry ()
@@ -103,15 +103,15 @@
 
 prepare_grub_to_access_device ()
 {
-  device=$1
+  device="$1"
 
   # Abstraction modules aren't auto-loaded.
-  abstraction="`${grub_probe} --device ${device} --target=abstraction`"
+  abstraction="`"${grub_probe}" --device "${device}" --target=abstraction`"
   for module in ${abstraction} ; do
     echo "insmod ${module}"
   done
 
-  partmap="`${grub_probe} --device ${device} --target=partmap`"
+  partmap="`"${grub_probe}" --device "${device}" --target=partmap`"
   for module in ${partmap} ; do
     case "${module}" in
       netbsd | openbsd)
@@ -121,15 +121,15 @@
     esac
   done
 
-  fs="`${grub_probe} --device ${device} --target=fs`"
+  fs="`"${grub_probe}" --device "${device}" --target=fs`"
   for module in ${fs} ; do
     echo "insmod ${module}"
   done
 
   # If there's a filesystem UUID that GRUB is capable of identifying, use it;
   # otherwise set root as per value in device.map.
-  echo "set root='`${grub_probe} --device ${device} --target=drive`'"
-  if fs_uuid="`${grub_probe} --device ${device} --target=fs_uuid 2> /dev/null`" ; then
+  echo "set root='`"${grub_probe}" --device "${device}" --target=drive`'"
+  if fs_uuid="`"${grub_probe}" --device "${device}" --target=fs_uuid 2> /dev/null`" ; then
     echo "search --no-floppy --fs-uuid --set=root ${fs_uuid}"
   fi
 }
@@ -149,21 +149,21 @@
 
 version_test_numeric ()
 {
-  local a=$1
-  local cmp=$2
-  local b=$3
+  local a="$1"
+  local cmp="$2"
+  local b="$3"
   if [ "$a" = "$b" ] ; then
-    case $cmp in
+    case "$cmp" in
       ge|eq|le) return 0 ;;
       gt|lt) return 1 ;;
     esac
   fi
   if [ "$cmp" = "lt" ] ; then
-    c=$a
-    a=$b
-    b=$c
+    c="$a"
+    a="$b"
+    b="$c"
   fi
-  if (echo $a ; echo $b) | sort -n | head -n 1 | grep -qx $b ; then
+  if (echo "$a" ; echo "$b") | sort -n | head -n 1 | grep -qx "$b" ; then
     return 0
   else
     return 1
@@ -172,25 +172,25 @@
 
 version_test_gt ()
 {
-  local a=`echo $1 | sed -e "s/[^-]*-//"`
-  local b=`echo $2 | sed -e "s/[^-]*-//"`
+  local a="`echo "$1" | sed -e "s/[^-]*-//"`"
+  local b="`echo "$2" | sed -e "s/[^-]*-//"`"
   local cmp=gt
   if [ "x$b" = "x" ] ; then
     return 0
   fi
-  case $a:$b in
+  case "$a:$b" in
     *.old:*.old) ;;
-    *.old:*) a=`echo -n $a | sed -e s/\.old$//` ; cmp=gt ;;
-    *:*.old) b=`echo -n $b | sed -e s/\.old$//` ; cmp=ge ;;
+    *.old:*) a="`echo -n "$a" | sed -e 's/\.old$//'`" ; cmp=gt ;;
+    *:*.old) b="`echo -n "$b" | sed -e 's/\.old$//'`" ; cmp=ge ;;
   esac
-  version_test_numeric $a $cmp $b
-  return $?
+  version_test_numeric "$a" "$cmp" "$b"
+  return "$?"
 }
 
 version_find_latest ()
 {
   local a=""
-  for i in $@ ; do
+  for i in "$@" ; do
     if version_test_gt "$i" "$a" ; then
       a="$i"
     fi
@@ -202,7 +202,7 @@
 # printf; so this turns ' into \'.  Note that you must use the output of
 # this function in a printf format string.
 gettext_quoted () {
-  $gettext "$@" | sed "s/'/'\\\\\\\\''/g"
+  "$gettext" "$@" | sed "s/'/'\\\\\\\\''/g"
 }
 
 # Run the first argument through gettext_quoted, and then pass that and all
@@ -211,13 +211,13 @@
 gettext_printf () {
   local format="$1"
   shift
-  printf "$(gettext_quoted "$format")" "$@"
+  printf "`gettext_quoted "$format"`" "$@"
 }
 
 uses_abstraction () {
-  device=$1
+  device="$1"
 
-  abstraction="`${grub_probe} --device ${device} --target=abstraction`"
+  abstraction="`"${grub_probe}" --device "${device}" --target=abstraction`"
   for module in ${abstraction}; do
     if test "x${module}" = "x$2"; then
       return 0
#14 lore raw

tag 612417 fixed-upstream
thanks

Quoted 7 lines · Alexander Kurtz

Am Freitag, den 08.04.2011, 15:44 +0200 schrieb Vladimir 'φ-coder/phcoder' Serbinenko:

Quoted 4 lines

The (big) patch is mostly fine except:

(quoted diff) +0 -2
-Please don't add brances where not needed
-Please don't add quoting to constants like x since x or "x" are

strictly identical.

Better now?

Committed, thanks.

Quoted 9 lines

Best regards

Alexander Kurtz

_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Signature · 2 lines
Regards
Vladimir 'φ-coder/phcoder' Serbinenko