[PATCH] Improve gcore shell quoting and portability

[PATCH] Improve gcore shell quoting and portability

[Georg Sauthoff]

The gcore shell script (gdb/gcore.in) doesn't quote its variables
enough.

For example, trying to write a core file with - say - a space
ungraciously fails like this:

    $ gcore -o 'foo bar' 6270
    /usr/bin/gcore: line 92: [: foo: binary operator expected
    gcore: failed to create foo bar.6270

Similarly, one can inject meta characters like * (by accident)
that may yield unexpected results, e.g. as in:

    $ gcore -o foobar '*'

This change fixes these issues in several places.

Aso, since the script uses array syntax, the patch changes the
the shell in the first line from `/bin/sh` to /bin/bash`.

POSIX doesn't specify the array syntax for shell, thus, the
script doesn't work on systems where /bin/sh is linked to - say -
dash.

Since the source gcore.in already is processed by a pre-processor
one could even auto-detect the path to bash and thus dynamically
generate the first line. For systems where bash isn't available
via /bin/bash. But I think this would be overkill and /bin/bash
is good enough as most systems probably have it.

gdb/ChangeLog:

        PR gdb/22888
        * gcore.in: quote variables and switch interpreter to bash
---
 gdb/gcore.in | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/gcore.in b/gdb/gcore.in
index b7f57cd3..816c5629 100644
--- a/gdb/gcore.in
+++ b/gdb/gcore.in
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
 
 #   Copyright (C) 2003-2018 Free Software Foundation, Inc.
 
@@ -28,9 +28,9 @@ name=core
 dump_all_cmds=()
 
 while getopts :ao: opt; do
-    case $opt in
+    case "$opt" in
         a)
-            case $OSTYPE in
+            case "$OSTYPE" in
                 linux*)
                     dump_all_cmds=("-ex" "set use-coredump-filter off")
                     dump_all_cmds+=("-ex" "set dump-excluded-mappings on")
@@ -93,16 +93,16 @@ fi
 rc=0
 
 # Loop through pids
-for pid in $*
+for pid in "$@"
 do
 	# `</dev/null' to avoid touching interactive terminal if it is
 	# available but not accessible as GDB would get stopped on SIGTTIN.
-	$binary_path/@GDB_TRANSFORM_NAME@ </dev/null --nx --batch \
+	"$binary_path"/@GDB_TRANSFORM_NAME@ </dev/null --nx --batch \
 	    -ex "set pagination off" -ex "set height 0" -ex "set width 0" \
 	    "${dump_all_cmds[@]}" \
 	    -ex "attach $pid" -ex "gcore $name.$pid" -ex detach -ex quit
 
-	if [ -r $name.$pid ] ; then 
+	if [ -r "$name.$pid" ] ; then
 	    rc=0
 	else
 	    echo "@GCORE_TRANSFORM_NAME@: failed to create $name.$pid"
-- 
2.13.6

Re: [PATCH] Improve gcore shell quoting and portability

[Simon Marchi]

Hi Georg,

Thanks for the patch.

I checked the resulting script (with your patch applied) with
https://www.shellcheck.net, and it doesn't give any important
warning/errors anymore (only suggests replacing `...` with $(...)).

On 2018-02-25 03:46 PM, Georg Sauthoff wrote:
> The gcore shell script (gdb/gcore.in) doesn't quote its variables
> enough.
> 
> For example, trying to write a core file with - say - a space
> ungraciously fails like this:
> 
>     $ gcore -o 'foo bar' 6270
>     /usr/bin/gcore: line 92: [: foo: binary operator expected
>     gcore: failed to create foo bar.6270
> 
> Similarly, one can inject meta characters like * (by accident)
> that may yield unexpected results, e.g. as in:
> 
>     $ gcore -o foobar '*'
> 
> This change fixes these issues in several places.
> 
> Aso, since the script uses array syntax, the patch changes the
> the shell in the first line from `/bin/sh` to /bin/bash`.
> 
> POSIX doesn't specify the array syntax for shell, thus, the
> script doesn't work on systems where /bin/sh is linked to - say -
> dash.
> 
> Since the source gcore.in already is processed by a pre-processor
> one could even auto-detect the path to bash and thus dynamically
> generate the first line. For systems where bash isn't available
> via /bin/bash. But I think this would be overkill and /bin/bash
> is good enough as most systems probably have it.

The script will not necessarily run on the same machine than the one
where it is preprocessed, so it's not possible to determine the path
to bash in advance.  I think the usual way to do it is

  #!/usr/bin/env bash

> gdb/ChangeLog:
> 
>         PR gdb/22888
>         * gcore.in: quote variables and switch interpreter to bash

Missing period at the end.

> ---
>  gdb/gcore.in | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/gcore.in b/gdb/gcore.in
> index b7f57cd3..816c5629 100644
> --- a/gdb/gcore.in
> +++ b/gdb/gcore.in
> @@ -1,4 +1,4 @@
> -#!/bin/sh
> +#!/bin/bash
>  
>  #   Copyright (C) 2003-2018 Free Software Foundation, Inc.
>  
> @@ -28,9 +28,9 @@ name=core
>  dump_all_cmds=()
>  
>  while getopts :ao: opt; do
> -    case $opt in
> +    case "$opt" in
>          a)
> -            case $OSTYPE in
> +            case "$OSTYPE" in
>                  linux*)
>                      dump_all_cmds=("-ex" "set use-coredump-filter off")
>                      dump_all_cmds+=("-ex" "set dump-excluded-mappings on")
> @@ -93,16 +93,16 @@ fi
>  rc=0
>  
>  # Loop through pids
> -for pid in $*
> +for pid in "$@"
>  do
>  	# `</dev/null' to avoid touching interactive terminal if it is
>  	# available but not accessible as GDB would get stopped on SIGTTIN.
> -	$binary_path/@GDB_TRANSFORM_NAME@ </dev/null --nx --batch \
> +	"$binary_path"/@GDB_TRANSFORM_NAME@ </dev/null --nx --batch \

To be pedantic, I guess we would have to quote the whole path, since
GDB_TRANSFORM_NAME could contain some spaces.  It's not very likely, I agree,
but it's also easy to change.

If you are fine with the suggestions above, I could fix them and push the resulting
patch, is that ok with you?

Simon

Re: [PATCH] Improve gcore shell quoting and portability

[Georg Sauthoff]

On Sun, Feb 25, 2018 at 05:41:40PM -0500, Simon Marchi wrote:

Hello,

[..]
> >  	# `</dev/null' to avoid touching interactive terminal if it is
> >  	# available but not accessible as GDB would get stopped on SIGTTIN.
> > -	$binary_path/@GDB_TRANSFORM_NAME@ </dev/null --nx --batch \
> > +	"$binary_path"/@GDB_TRANSFORM_NAME@ </dev/null --nx --batch \

> To be pedantic, I guess we would have to quote the whole path, since
> GDB_TRANSFORM_NAME could contain some spaces.  It's not very likely, I agree,
> but it's also easy to change.
 
> If you are fine with the suggestions above, I could fix them and push the resulting
> patch, is that ok with you?

yes, I'm fine with all your suggestion.

I'm also a fan of the `#!/usr/bin/env someinterpreter` construct.

Thus, please go ahead.

Best regards
Georg

Re: [PATCH] Improve gcore shell quoting and portability

[Simon Marchi]

On 2018-03-01 02:48 PM, Georg Sauthoff wrote:
> yes, I'm fine with all your suggestion.
> 
> I'm also a fan of the `#!/usr/bin/env someinterpreter` construct.
> 
> Thus, please go ahead.

Thanks, here's what I pushed:


From e1e6f073a9f5d7c3183cb8096fb24a42c28ba36b Mon Sep 17 00:00:00 2001
From: Georg Sauthoff <mail@georg.so>
Date: Thu, 1 Mar 2018 17:23:31 -0500
Subject: [PATCH] Improve gcore shell quoting and portability

The gcore shell script (gdb/gcore.in) doesn't quote its variables
enough.

For example, trying to write a core file with - say - a space
ungraciously fails like this:

    $ gcore -o 'foo bar' 6270
    /usr/bin/gcore: line 92: [: foo: binary operator expected
    gcore: failed to create foo bar.6270

Similarly, one can inject meta characters like * (by accident)
that may yield unexpected results, e.g. as in:

    $ gcore -o foobar '*'

This change fixes these issues in several places.

Aso, since the script uses array syntax, the patch changes the
the shell in the first line from `/bin/sh` to /bin/bash`.

POSIX doesn't specify the array syntax for shell, thus, the
script doesn't work on systems where /bin/sh is linked to - say -
dash.

Since the source gcore.in already is processed by a pre-processor
one could even auto-detect the path to bash and thus dynamically
generate the first line. For systems where bash isn't available
via /bin/bash. But I think this would be overkill and /bin/bash
is good enough as most systems probably have it.

gdb/ChangeLog:

	PR gdb/22888
	* gcore.in: Quote variables and switch interpreter to bash.
---
 gdb/ChangeLog |  5 +++++
 gdb/gcore.in  | 14 +++++++-------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5b911af..5bfbe9b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2018-03-01  Georg Sauthoff  <mail@georg.so>
+
+	PR gdb/22888
+	* gcore.in: Quote variables and switch interpreter to bash.
+
 2018-03-01  Tom Tromey  <tom@tromey.com>

 	* dwarf2read.c (alloc_discriminant_info): Fix default_index
diff --git a/gdb/gcore.in b/gdb/gcore.in
index b7f57cd..233c00d 100644
--- a/gdb/gcore.in
+++ b/gdb/gcore.in
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/usr/bin/env bash

 #   Copyright (C) 2003-2018 Free Software Foundation, Inc.

@@ -28,9 +28,9 @@ name=core
 dump_all_cmds=()

 while getopts :ao: opt; do
-    case $opt in
+    case "$opt" in
         a)
-            case $OSTYPE in
+            case "$OSTYPE" in
                 linux*)
                     dump_all_cmds=("-ex" "set use-coredump-filter off")
                     dump_all_cmds+=("-ex" "set dump-excluded-mappings on")
@@ -84,7 +84,7 @@ fi

 # Check if the GDB binary is in the expected path.  If not, just
 # quit with a message.
-if [ ! -f "$binary_path"/@GDB_TRANSFORM_NAME@ ]; then
+if [ ! -f "$binary_path/@GDB_TRANSFORM_NAME@" ]; then
   echo "gcore: GDB binary (${binary_path}/@GDB_TRANSFORM_NAME@) not found"
   exit 1
 fi
@@ -93,16 +93,16 @@ fi
 rc=0

 # Loop through pids
-for pid in $*
+for pid in "$@"
 do
 	# `</dev/null' to avoid touching interactive terminal if it is
 	# available but not accessible as GDB would get stopped on SIGTTIN.
-	$binary_path/@GDB_TRANSFORM_NAME@ </dev/null --nx --batch \
+	"$binary_path/@GDB_TRANSFORM_NAME@" </dev/null --nx --batch \
 	    -ex "set pagination off" -ex "set height 0" -ex "set width 0" \
 	    "${dump_all_cmds[@]}" \
 	    -ex "attach $pid" -ex "gcore $name.$pid" -ex detach -ex quit

-	if [ -r $name.$pid ] ; then
+	if [ -r "$name.$pid" ] ; then
 	    rc=0
 	else
 	    echo "@GCORE_TRANSFORM_NAME@: failed to create $name.$pid"
-- 
2.7.4