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

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

[Alexander Kurtz]

[-- Attachment #1.1: Type: text/plain, Size: 1026 bytes --]

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

[-- Attachment #1.2: quote-small.patch --]
[-- Type: text/x-patch, Size: 770 bytes --]

diff -Naur bazaar/util/grub-mkconfig_lib.in small/util/grub-mkconfig_lib.in
--- bazaar/util/grub-mkconfig_lib.in	2011-04-05 10:38:13.357139000 +0200
+++ small/util/grub-mkconfig_lib.in	2011-04-05 14:10:09.617962451 +0200
@@ -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
 

[-- Attachment #1.3: quote-medium.patch --]
[-- Type: text/x-patch, Size: 7070 bytes --]

diff -Naur bazaar/util/grub-mkconfig_lib.in medium/util/grub-mkconfig_lib.in
--- bazaar/util/grub-mkconfig_lib.in	2011-04-05 10:38:13.357139000 +0200
+++ medium/util/grub-mkconfig_lib.in	2011-04-05 14:10:21.136952907 +0200
@@ -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}`
+if test "x${grub_probe}" = "x"; then
+  grub_probe="${sbindir}/`echo grub-probe | sed "${transform}"`"
 fi
-if test "x$grub_mkrelpath" = x; then
-  grub_mkrelpath=${bindir}/`echo grub-mkrelpath | sed ${transform}`
+if test "x${grub_mkrelpath}" = "x"; then
+  grub_mkrelpath="${bindir}/`echo grub-mkrelpath | sed "${transform}"`"
 fi
 
 if $(which gettext >/dev/null 2>/dev/null) ; then
@@ -39,25 +39,25 @@
 
 grub_warn ()
 {
-  echo "Warning: $@" >&2
+  echo "Warning: ${@}" >&2
 }
 
 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,23 +121,23 @@
     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
 }
 
 grub_file_is_not_garbage ()
 {
-  if test -f "$1" ; then
-    case "$1" in
+  if test -f "${1}" ; then
+    case "${1}" in
       *.dpkg-*) return 1 ;; # debian dpkg
       README*)  return 1 ;; # documentation
     esac
@@ -149,21 +149,21 @@
 
 version_test_numeric ()
 {
-  local a=$1
-  local cmp=$2
-  local b=$3
-  if [ "$a" = "$b" ] ; then
-    case $cmp in
+  local a="${1}"
+  local cmp="${2}"
+  local b="${3}"
+  if [ "${a}" = "${b}" ] ; then
+    case "${cmp}" in
       ge|eq|le) return 0 ;;
       gt|lt) return 1 ;;
     esac
   fi
-  if [ "$cmp" = "lt" ] ; then
-    c=$a
-    a=$b
-    b=$c
+  if [ "${cmp}" = "lt" ] ; then
+    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,54 +172,54 @@
 
 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
+  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
-    if version_test_gt "$i" "$a" ; then
-      a="$i"
+  for i in "${@}" ; do
+    if version_test_gt "${i}" "${a}" ; then
+      a="${i}"
     fi
   done
-  echo "$a"
+  echo "${a}"
 }
 
 # One layer of quotation is eaten by "", the second by sed, and the third by
 # 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
 # remaining arguments to printf.  This is a useful abbreviation and tends to
 # be easier to type.
 gettext_printf () {
-  local format="$1"
+  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
+    if test "x${module}" = "x${2}"; then
       return 0
     fi
   done

[-- Attachment #1.4: quote-big.patch --]
[-- Type: text/x-patch, Size: 7084 bytes --]

diff -Naur bazaar/util/grub-mkconfig_lib.in big/util/grub-mkconfig_lib.in
--- bazaar/util/grub-mkconfig_lib.in	2011-04-05 10:38:13.357139000 +0200
+++ big/util/grub-mkconfig_lib.in	2011-04-05 14:10:31.668952910 +0200
@@ -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}`
+if test "x${grub_probe}" = "x"; then
+  grub_probe="${sbindir}/`echo grub-probe | sed "${transform}"`"
 fi
-if test "x$grub_mkrelpath" = x; then
-  grub_mkrelpath=${bindir}/`echo grub-mkrelpath | sed ${transform}`
+if test "x${grub_mkrelpath}" = "x"; then
+  grub_mkrelpath="${bindir}/`echo grub-mkrelpath | sed "${transform}"`"
 fi
 
 if $(which gettext >/dev/null 2>/dev/null) ; then
@@ -39,25 +39,25 @@
 
 grub_warn ()
 {
-  echo "Warning: $@" >&2
+  echo "Warning: ${@}" >&2
 }
 
 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,23 +121,23 @@
     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
 }
 
 grub_file_is_not_garbage ()
 {
-  if test -f "$1" ; then
-    case "$1" in
+  if test -f "${1}" ; then
+    case "${1}" in
       *.dpkg-*) return 1 ;; # debian dpkg
       README*)  return 1 ;; # documentation
     esac
@@ -149,21 +149,21 @@
 
 version_test_numeric ()
 {
-  local a=$1
-  local cmp=$2
-  local b=$3
-  if [ "$a" = "$b" ] ; then
-    case $cmp in
+  local a="${1}"
+  local cmp="${2}"
+  local b="${3}"
+  if [ "${a}" = "${b}" ] ; then
+    case "${cmp}" in
       ge|eq|le) return 0 ;;
       gt|lt) return 1 ;;
     esac
   fi
-  if [ "$cmp" = "lt" ] ; then
-    c=$a
-    a=$b
-    b=$c
+  if [ "${cmp}" = "lt" ] ; then
+    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,54 +172,54 @@
 
 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
+  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
-    if version_test_gt "$i" "$a" ; then
-      a="$i"
+  for i in "${@}" ; do
+    if version_test_gt "${i}" "${a}" ; then
+      a="${i}"
     fi
   done
-  echo "$a"
+  echo "${a}"
 }
 
 # One layer of quotation is eaten by "", the second by sed, and the third by
 # 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
 # remaining arguments to printf.  This is a useful abbreviation and tends to
 # be easier to type.
 gettext_printf () {
-  local format="$1"
+  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
+    if test "x${module}" = "x${2}"; then
       return 0
     fi
   done

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

[Nicolas de Pesloüan]

Le 05/04/2011 14:55, Alexander Kurtz a écrit :
> 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?

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.

>
> 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
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

[Colin Watson]

On Tue, Apr 05, 2011 at 09:16:13PM +0200, Nicolas de Pesloüan wrote:
> 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.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

[Alexander Kurtz]

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

Am Dienstag, den 05.04.2011, 21:16 +0200 schrieb Nicolas de Pesloüan:
> 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

[Colin Watson]

On Tue, Apr 05, 2011 at 09:32:40PM +0200, Alexander Kurtz wrote:
> Am Dienstag, den 05.04.2011, 21:16 +0200 schrieb Nicolas de Pesloüan:
> > 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.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

[Nicolas de Pesloüan]

Le 05/04/2011 21:32, Alexander Kurtz a écrit :
> Am Dienstag, den 05.04.2011, 21:16 +0200 schrieb Nicolas de Pesloüan:
>> 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

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.

Re: [PATCH] add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

[Colin Watson]

On Tue, Apr 05, 2011 at 10:07:46PM +0200, Nicolas de Pesloüan wrote:
> 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"

> 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.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

[Nicolas de Pesloüan]

Le 05/04/2011 22:53, Colin Watson a écrit :

> 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.

Re: [PATCH] add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

[Alexander Kurtz]

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

Am Dienstag, den 05.04.2011, 23:27 +0200 schrieb Nicolas de Pesloüan:
> Le 05/04/2011 22:53, Colin Watson a écrit :
> 
> > 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

[Nicolas de Pesloüan]

Le 06/04/2011 17:31, Alexander Kurtz a écrit :
> Am Dienstag, den 05.04.2011, 23:27 +0200 schrieb Nicolas de Pesloüan:
>> Le 05/04/2011 22:53, Colin Watson a écrit :
>>
>>> 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?

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.

Re: [PATCH] add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

[Colin Watson]

On Wed, Apr 06, 2011 at 08:14:24PM +0200, Nicolas de Pesloüan wrote:
> a="`echo "$b"`"
>          ^ Is this quote closing the first one or opening a nested quote.

It nests.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

[Nicolas de Pesloüan]

Le 06/04/2011 21:31, Colin Watson a écrit :
> On Wed, Apr 06, 2011 at 08:14:24PM +0200, Nicolas de Pesloüan wrote:
>> 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.

Re: [PATCH] add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 06.04.2011 17:31, Alexander Kurtz wrote:
> Am Dienstag, den 05.04.2011, 23:27 +0200 schrieb Nicolas de Pesloüan:
>> Le 05/04/2011 22:53, Colin Watson a écrit :
>>
>>> 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?
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.
> Best regards
>
> Alexander Kurtz
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


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



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

Re: [PATCH] add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

[Alexander Kurtz]

[-- Attachment #1.1: Type: text/plain, Size: 318 bytes --]

Am Freitag, den 08.04.2011, 15:44 +0200 schrieb Vladimir 'φ-coder/phcoder' Serbinenko:
> 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.

Better now?

Best regards

Alexander Kurtz

[-- Attachment #1.2: quote-big-adjusted.patch --]
[-- Type: text/x-patch, Size: 6205 bytes --]

diff -Naur bazaar/util/grub-mkconfig_lib.in big-adjusted/util/grub-mkconfig_lib.in
--- bazaar/util/grub-mkconfig_lib.in	2011-04-08 18:17:27.789166000 +0200
+++ big-adjusted/util/grub-mkconfig_lib.in	2011-04-09 15:23:57.974913718 +0200
@@ -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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

tag 612417 fixed-upstream
thanks
On 09.04.2011 16:32, Alexander Kurtz wrote:
> Am Freitag, den 08.04.2011, 15:44 +0200 schrieb Vladimir 'φ-coder/phcoder' Serbinenko:
>> 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.
> Better now?
>
Committed, thanks.
> Best regards
>
> Alexander Kurtz
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


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



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