From 3b7c7e61065875707fb22b70553d30f39cb1c498 Mon Sep 17 00:00:00 2001
From: "Yann E. MORIN" <yann.morin.1998@free.fr>
Date: Mon, 7 Aug 2023 23:04:10 +0200
Subject: [PATCH] support/scripts: fix fix-rpath

Commit 134900401f08 (support/scripts/fix-rpath: parallelize patching
files) broke the rpath fixup, because it improperly quoted or expanded
variables:

  - $@ was expanded in the main() context, rather than in the sub-bash
    as expected, propagating incorrect parameters to patch_file();

  - an array was passed without array expansion, so only the first item
    was passed; that was in turn assigned to a string, anyway loosign
    the array. Liuckily, we only ever put a single item in that array,
    so that worked by chance.

We fix that by inverting the parameters to patch_elf(), where the extra
args are passed last, so we can put as many we want in the future. We
also pass every variables as positional parameters outside the bash -c
command, which allows us proper quoting of all variables, specifically
of the extra args array which now comes last.

The ultralong line was split, too, in a hopefully easier-to-read form.

Fixing all that also required fixing the many shellcheck issues at the
same time (wome were pre-existing before 134900401f08).

While at it, expand two TABs into spaces like the rest of the script.

Note: shellcheck does not seem to warn when a variable expansion will be
used as the command to run, i.e. ${PATCHELF} does not trigger the
quoting error. Still, for consistency, we also double-quote it (we know
it is a single word, as it is already double-quoted once in the script).

Fixes: 134900401f08

Cc: Victor Dumas <dumasv.dev@gmail.com>
Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
---
 .checkpackageignore       |  1 -
 support/scripts/fix-rpath | 40 +++++++++++++++++++++++----------------
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/.checkpackageignore b/.checkpackageignore
index d7fab118a9..b229a4e015 100644
--- a/.checkpackageignore
+++ b/.checkpackageignore
@@ -1663,7 +1663,6 @@ support/scripts/check-bin-arch Shellcheck
 support/scripts/check-host-rpath Shellcheck
 support/scripts/expunge-gconv-modules Shellcheck
 support/scripts/fix-configure-powerpc64.sh EmptyLastLine
-support/scripts/fix-rpath Shellcheck
 support/scripts/generate-gitlab-ci-yml Shellcheck
 support/scripts/mkmakefile ConsecutiveEmptyLines Shellcheck
 support/scripts/mkusers Shellcheck
diff --git a/support/scripts/fix-rpath b/support/scripts/fix-rpath
index a9b348e189..1e58646cea 100755
--- a/support/scripts/fix-rpath
+++ b/support/scripts/fix-rpath
@@ -53,7 +53,7 @@ Returns:         0 if success or 1 in case of error
 EOF
 }
 
-: ${PATCHELF:=${HOST_DIR}/bin/patchelf}
+: "${PATCHELF:=${HOST_DIR}/bin/patchelf}"
 
 # ELF files should not be in these sub-directories
 HOST_EXCLUDEPATHS="/share/terminfo"
@@ -61,19 +61,23 @@ STAGING_EXCLUDEPATHS="/usr/include /usr/share/terminfo"
 TARGET_EXCLUDEPATHS="/lib/firmware"
 
 patch_file() {
+    local PATCHELF rootdir file
+    local -a sanitize_extra_args
+
     PATCHELF="${1}"
     rootdir="${2}"
-    sanitize_extra_args="${3}"
-    file="${4}"
+    file="${3}"
+    shift 3
+    sanitize_extra_args=("${@}")
 
     # check if it's an ELF file
-    rpath=$(${PATCHELF} --print-rpath "${file}" 2>&1)
+    rpath="$("${PATCHELF}" --print-rpath "${file}" 2>&1)"
     if test $? -ne 0 ; then
         return 0
     fi
 
     # make files writable if necessary
-    changed=$(chmod -c u+w "${file}")
+    changed="$(chmod -c u+w "${file}")"
 
     # With per-package directory support, most RPATH of host
     # binaries will point to per-package directories. This won't
@@ -81,26 +85,27 @@ patch_file() {
     # the per-package host directory is not within ${rootdir}. So,
     # we rewrite all RPATHs pointing to per-package directories so
     # that they point to the global host directry.
-    changed_rpath=$(echo ${rpath} | sed "s@${PER_PACKAGE_DIR}/[^/]\+/host@${HOST_DIR}@")
+    # shellcheck disable=SC2001 # ${var//search/replace} hard when search or replace have / in them
+    changed_rpath="$(echo "${rpath}" | sed "s@${PER_PACKAGE_DIR}/[^/]\+/host@${HOST_DIR}@")"
     if test "${rpath}" != "${changed_rpath}" ; then
-        ${PATCHELF} --set-rpath ${changed_rpath} "${file}"
+        "${PATCHELF}" --set-rpath "${changed_rpath}" "${file}"
     fi
 
     # call patchelf to sanitize the rpath
-    ${PATCHELF} --make-rpath-relative "${rootdir}" ${sanitize_extra_args[@]} "${file}"
+    "${PATCHELF}" --make-rpath-relative "${rootdir}" "${sanitize_extra_args[@]}" "${file}"
     # restore the original permission
     test "${changed}" != "" && chmod u-w "${file}"
 }
 
 main() {
-    local rootdir
-    local tree="${1}"
-    local find_args=( )
-    local sanitize_extra_args=( )
+    local rootdir tree
+    local -a find_args sanitize_extra_args
+
+    tree="${1}"
 
     if ! "${PATCHELF}" --version > /dev/null 2>&1; then
-	echo "Error: can't execute patchelf utility '${PATCHELF}'"
-	exit 1
+        echo "Error: can't execute patchelf utility '${PATCHELF}'"
+        exit 1
     fi
 
     case "${tree}" in
@@ -161,7 +166,10 @@ main() {
 
     export -f patch_file
     # Limit the number of cores used
-    find "${rootdir}" ${find_args[@]} | xargs -0 -r -P ${PARALLEL_JOBS} -I {} bash -c "patch_file '${PATCHELF}' '${rootdir}' '${sanitize_extra_args}' $@" _ {}
+    # shellcheck disable=SC2016  # ${@} has to be expanded in the sub-shell.
+    find "${rootdir}" "${find_args[@]}" \
+    | xargs -0 -r -P "${PARALLEL_JOBS:-1}" -I {} \
+            bash -c 'patch_file "${@}"' _ "${PATCHELF}" "${rootdir}" {} "${sanitize_extra_args[@]}"
 
     # Restore patched patchelf utility
     test "${tree}" = "host" && mv "${PATCHELF}.__to_be_patched" "${PATCHELF}"
@@ -170,4 +178,4 @@ main() {
     return 0
 }
 
-main ${@}
+main "${@}"