Re: [PATCH] [gdb/contrib] Make gdb-add-index.sh shellcheck-clean

[Keith Seitz <keiths@redhat.com>]

Hi,

Thank you for doing this.

On 9/15/26 10:14 AM, Tom de Vries wrote:
> Make gdb-add-index.sh shellcheck-clean:
> - drop x in "xa" != "xb" comparison
> - eliminate tmp_files and use the args array instead
> - drop dwz_file=$(echo $dwz_file) and strip leading spaces using
>    sed instead
> ---
>   gdb/contrib/gdb-add-index.sh | 19 ++++++++++++-------
>   gdb/contrib/shellcheck.sh    |  4 ----
>   2 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/gdb/contrib/gdb-add-index.sh b/gdb/contrib/gdb-add-index.sh
> index 4a30c5c5536..e1a37f2cd4c 100755
> --- a/gdb/contrib/gdb-add-index.sh
> +++ b/gdb/contrib/gdb-add-index.sh
> @@ -82,7 +82,7 @@ case ${opt} in
>   	;;
>   esac
>   # Break from loop if the first character of OPT is not '-'.
> -[ "x$(printf %.1s "$opt")" != "x-" ]
> +[ "$(printf %.1s "$opt")" != "-" ]
>   do
>       shift
>   done
> @@ -93,6 +93,7 @@ if test $# != 1; then
>   fi
>   
>   file="$1"
> +set --
>   

This unsets the positional parameters, clearing $1, but this is
subsequently used if readlink fails:

>   if test -L "$file"; then
>       if ! command -v readlink >/dev/null 2>&1; then
         echo "$myname: 'readlink' missing.  Failed to follow symlink 
$1." 1>&2
         exit 1
     fi

Does this need to change to "$file"?

> @@ -135,8 +136,7 @@ if $READELF -S "$file" | grep -q " \.gnu_debugaltlink "; then
>       dwz_file=$($READELF --string-dump=.gnu_debugaltlink "$file" \
>   		   | grep -A1  "'\.gnu_debugaltlink':" \
>   		   | tail -n +2 \
> -		   | sed 's/.*]//')
> -    dwz_file=$(echo $dwz_file)
> +		   | sed 's/.*] *//')
>       if $READELF -S "$dwz_file" | grep -E -q " \.(gdb_index|debug_names) "; then
>   	# Already has an index, skip it.
>   	dwz_file=""
> @@ -154,19 +154,24 @@ set_files ()
>       debugstrerr="${fpath}.debug_str.err"
>   }
>   
> -tmp_files=
>   for f in "$file" "$dwz_file"; do
>       if [ "$f" = "" ]; then
>   	continue
>       fi
>       set_files "$f"
> -    tmp_files="$tmp_files $index4 $index5 $debugstr $debugstrmerge $debugstrerr"
> +    set -- \
> +	"$@" \
> +	"$index4" \
> +	"$index5" \
> +	"$debugstr" \
> +	"$debugstrmerge" \
> +	"$debugstrerr"
>   done
>   
> -rm -f $tmp_files
> +rm -f "$@"
>   
>   # Ensure intermediate index file is removed when we exit.
> -trap "rm -f $tmp_files" 0
> +trap 'rm -f "$@"' 0

I'm not sure this is safe.

The "$@" in the trap is expanded when the trap runs, using whatever
positional parameters are active in that context. If that happens when
exit is called from a function (as in handle_file on objcopy failure),
bash substitutes the function's arguments instead of the temp-file list,
erasing the real binary and leaving the temps behind. [On Fedora,
/bin/sh --> /bin/bash.]

Demonstration:
$ cat > trap-test.sh << 'EOF'
#! /bin/sh
# These are our temporary files...
set -- /tmp/index.tmp /tmp/debugstr.tmp
touch "$@"

trap 'echo "trap removes: $*"; rm -f "$@"' EXIT

handle_file () {
     echo "in handle_file, \$1=$1"
     exit 1   # like gdb-add-index.sh on objcopy failure
}

handle_file /tmp/important-binary
EOF
$ touch /tmp/important-binary; # for clarity
$ bash trap-test.sh
in handle_file, $1=/tmp/important-binary
trap removes: /tmp/important-binary

Keith