add proper variable quoting to grub-mkconfig_lib (Debian bug #612417)
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
@@ -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
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
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]]
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
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]]
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.
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]]
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.
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
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.
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]]
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.
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
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" arestrictly identical.
Better now?
Best regards
Alexander Kurtz
util/grub-mkconfig_lib.in +44 -44
@@ -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
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" arestrictly 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