Re: [PATCH] Improve gcore shell quoting and portability

[Simon Marchi <simark at simark dot ca>]

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