kbuild: pahole-version: avoid errors if executing fails

Commit Message

Miguel Ojeda July 28, 2024, 12:55 p.m. UTC

Like patch "rust: suppress error messages from
CONFIG_{RUSTC,BINDGEN}_VERSION_TEXT" [1], do not assume the file existing
and being executable implies executing it will succeed. Instead, bail
out if executing it fails for any reason.

For instance, `pahole` may be built for another architecture, may be a
program we do not expect or may be completely broken:

    $ echo 'bad' > bad-pahole
    $ chmod u+x bad-pahole
    $ make PAHOLE=./bad-pahole defconfig
    ...
    ./bad-pahole: 1: bad: not found
    init/Kconfig:112: syntax error
    init/Kconfig:112: invalid statement

Link: https://lore.kernel.org/rust-for-linux/[e-mail]/ [1]
Signed-off-by: Miguel Ojeda <[e-mail]>
---
 scripts/pahole-version.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


base-commit: 256abd8e550ce977b728be79a74e1729438b4948

Comments

Nicolas Schier Aug. 2, 2024, 11:34 p.m. UTC | #1

On Sun, Jul 28, 2024 at 02:55:27PM +0200, Miguel Ojeda wrote:
> Like patch "rust: suppress error messages from
> CONFIG_{RUSTC,BINDGEN}_VERSION_TEXT" [1], do not assume the file existing
> and being executable implies executing it will succeed. Instead, bail
> out if executing it fails for any reason.
> 
> For instance, `pahole` may be built for another architecture, may be a
> program we do not expect or may be completely broken:
> 
>     $ echo 'bad' > bad-pahole
>     $ chmod u+x bad-pahole
>     $ make PAHOLE=./bad-pahole defconfig
>     ...
>     ./bad-pahole: 1: bad: not found
>     init/Kconfig:112: syntax error
>     init/Kconfig:112: invalid statement
> 
> Link: https://lore.kernel.org/rust-for-linux/[e-mail]/ [1]
> Signed-off-by: Miguel Ojeda <[e-mail]>
> ---
>  scripts/pahole-version.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/pahole-version.sh b/scripts/pahole-version.sh
> index f8a32ab93ad1..a35b557f1901 100755
> --- a/scripts/pahole-version.sh
> +++ b/scripts/pahole-version.sh
> @@ -5,9 +5,9 @@
>  #
>  # Prints pahole's version in a 3-digit form, such as 119 for v1.19.
>  
> -if [ ! -x "$(command -v "$@")" ]; then
> +if output=$("$@" --version 2>/dev/null); then
> +	echo "$output" | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'
> +else
>  	echo 0
>  	exit 1
>  fi
> -
> -"$@" --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'
> 
> base-commit: 256abd8e550ce977b728be79a74e1729438b4948
> -- 
> 2.45.2
> 

thanks, looks good to me.

Reviewed-by: Nicolas Schier <[e-mail]>

Masahiro Yamada Aug. 22, 2024, 5:28 p.m. UTC | #2

On Sun, Jul 28, 2024 at 9:55 PM Miguel Ojeda <[e-mail]> wrote:
>
> Like patch "rust: suppress error messages from
> CONFIG_{RUSTC,BINDGEN}_VERSION_TEXT" [1], do not assume the file existing
> and being executable implies executing it will succeed. Instead, bail
> out if executing it fails for any reason.
>
> For instance, `pahole` may be built for another architecture, may be a
> program we do not expect or may be completely broken:
>
>     $ echo 'bad' > bad-pahole
>     $ chmod u+x bad-pahole
>     $ make PAHOLE=./bad-pahole defconfig
>     ...
>     ./bad-pahole: 1: bad: not found
>     init/Kconfig:112: syntax error
>     init/Kconfig:112: invalid statement



Even with this patch applied, a syntax error can happen.

$ git log --oneline -1
dd1c54d77f11 kbuild: pahole-version: avoid errors if executing fails
$ echo 'echo' > bad-pahole
$ chmod u+x bad-pahole
$ make PAHOLE=./bad-pahole defconfig
*** Default configuration is based on 'x86_64_defconfig'
init/Kconfig:114: syntax error
init/Kconfig:114: invalid statement
make[2]: *** [scripts/kconfig/Makefile:95: defconfig] Error 1
make[1]: *** [/home/masahiro/workspace/linux-kbuild/Makefile:680:
defconfig] Error 2
make: *** [Makefile:224: __sub-make] Error 2















> Link: https://lore.kernel.org/rust-for-linux/[e-mail]/ [1]
> Signed-off-by: Miguel Ojeda <[e-mail]>
> ---
>  scripts/pahole-version.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/pahole-version.sh b/scripts/pahole-version.sh
> index f8a32ab93ad1..a35b557f1901 100755
> --- a/scripts/pahole-version.sh
> +++ b/scripts/pahole-version.sh
> @@ -5,9 +5,9 @@
>  #
>  # Prints pahole's version in a 3-digit form, such as 119 for v1.19.
>
> -if [ ! -x "$(command -v "$@")" ]; then
> +if output=$("$@" --version 2>/dev/null); then
> +       echo "$output" | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'
> +else
>         echo 0
>         exit 1
>  fi
> -
> -"$@" --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'
>
> base-commit: 256abd8e550ce977b728be79a74e1729438b4948
> --
> 2.45.2
>


--
Best Regards
Masahiro Yamada

Nicolas Schier Aug. 23, 2024, 1:59 p.m. UTC | #3

On Fri 23 Aug 2024 02:28:28 GMT, Masahiro Yamada wrote:
> Date: Fri, 23 Aug 2024 02:28:28 +0900
> From: Masahiro Yamada <[e-mail]>
> To: Miguel Ojeda <[e-mail]>
> Cc: Alexei Starovoitov <[e-mail]>, Daniel Borkmann
>  <[e-mail]>, Andrii Nakryiko <[e-mail]>, Martin KaFai
>  Lau <[e-mail]>, Eduard Zingerman <[e-mail]>, Song Liu
>  <[e-mail]>, Yonghong Song <[e-mail]>, John Fastabend
>  <[e-mail]>, KP Singh <[e-mail]>, Stanislav
>  Fomichev <[e-mail]>, Hao Luo <[e-mail]>, Jiri Olsa
>  <[e-mail]>, [e-mail], Nathan Chancellor
>  <[e-mail]>, Nicolas Schier <[e-mail]>,
>  [e-mail], [e-mail],
>  [e-mail]
> Subject: Re: [PATCH] kbuild: pahole-version: avoid errors if executing fails
> X-Mailing-List: [e-mail]
> Message-ID: <CAK7LNARhR=[e-mail]>
> 
> On Sun, Jul 28, 2024 at 9:55 PM Miguel Ojeda <[e-mail]> wrote:
> >
> > Like patch "rust: suppress error messages from
> > CONFIG_{RUSTC,BINDGEN}_VERSION_TEXT" [1], do not assume the file existing
> > and being executable implies executing it will succeed. Instead, bail
> > out if executing it fails for any reason.
> >
> > For instance, `pahole` may be built for another architecture, may be a
> > program we do not expect or may be completely broken:
> >
> >     $ echo 'bad' > bad-pahole
> >     $ chmod u+x bad-pahole
> >     $ make PAHOLE=./bad-pahole defconfig
> >     ...
> >     ./bad-pahole: 1: bad: not found
> >     init/Kconfig:112: syntax error
> >     init/Kconfig:112: invalid statement
> 
> 
> 
> Even with this patch applied, a syntax error can happen.
> 
> $ git log --oneline -1
> dd1c54d77f11 kbuild: pahole-version: avoid errors if executing fails
> $ echo 'echo' > bad-pahole
> $ chmod u+x bad-pahole
> $ make PAHOLE=./bad-pahole defconfig
> *** Default configuration is based on 'x86_64_defconfig'
> init/Kconfig:114: syntax error
> init/Kconfig:114: invalid statement
> make[2]: *** [scripts/kconfig/Makefile:95: defconfig] Error 1
> make[1]: *** [/home/masahiro/workspace/linux-kbuild/Makefile:680:
> defconfig] Error 2
> make: *** [Makefile:224: __sub-make] Error 2
> 

Do we have to catch all possibilities?  Then, what about this:


#!/bin/sh
trap "echo 0; exit 1" EXIT
set -e

output=$("$@" --version 2>/dev/null)

output=$(echo "${output}" |  sed -nE 's/^v([0-9]+)\.([0-9][0-9])$/\1\2/p')
if [ -z "${output}" ]; then
	echo "warning: pahole binary '$1' outputs incompatible version number, pahole will not be used." >&2
	exit 1
fi

echo "${output}"
trap EXIT



Kind regards,
Nicolas

Miguel Ojeda Aug. 23, 2024, 6:47 p.m. UTC | #4

On Fri, Aug 23, 2024 at 4:00 PM Nicolas Schier <[e-mail]> wrote:
>
> Do we have to catch all possibilities?  Then, what about this:

Something like that sounds good to me too -- we do something similar
in `rust_is_available.sh`. We also have a `1` in the beginning of
(most of) the `sed` commands there to check only the first line.

I guess it depends on whether Masahiro thinks the extra
checks/complexity is worth it. Here I was aiming to catch the case he
reported, i.e. non-successful programs.

Cheers,
Miguel

Masahiro Yamada Sept. 2, 2024, 2:15 a.m. UTC | #5

On Sat, Aug 24, 2024 at 3:48 AM Miguel Ojeda
<[e-mail]> wrote:
>
> On Fri, Aug 23, 2024 at 4:00 PM Nicolas Schier <[e-mail]> wrote:
> >
> > Do we have to catch all possibilities?  Then, what about this:
>
> Something like that sounds good to me too -- we do something similar
> in `rust_is_available.sh`. We also have a `1` in the beginning of
> (most of) the `sed` commands there to check only the first line.
>
> I guess it depends on whether Masahiro thinks the extra
> checks/complexity is worth it. Here I was aiming to catch the case he
> reported, i.e. non-successful programs.


My previous report was slightly different.

CONFIG_{RUSTC,BINDGEN}_VERSION_TEXT are string type symbols.
The shell command is allowed to return any string, including an empty string,
as long as the value is enclosed by double quotes.

In this case, CONFIG_PAHOLE_VERSION is an int type symbol,
hence the shell command must not return an empty string.

Ensuring this should be easy.
Why don't we fix it properly while we are here?




>
> Cheers,
> Miguel
>


--
Best Regards
Masahiro Yamada

Miguel Ojeda Sept. 2, 2024, 4:11 p.m. UTC | #6

On Mon, Sep 2, 2024 at 4:15 AM Masahiro Yamada <[e-mail]> wrote:
>
> Ensuring this should be easy.
> Why don't we fix it properly while we are here?

That is great, I would prefer that.

Sent v2: https://lore.kernel.org/all/[e-mail]/

Cheers,
Miguel

Patch

diff --git a/scripts/pahole-version.sh b/scripts/pahole-version.sh
index f8a32ab93ad1..a35b557f1901 100755
--- a/scripts/pahole-version.sh
+++ b/scripts/pahole-version.sh
@@ -5,9 +5,9 @@ 
 #
 # Prints pahole's version in a 3-digit form, such as 119 for v1.19.
 
-if [ ! -x "$(command -v "$@")" ]; then
+if output=$("$@" --version 2>/dev/null); then
+	echo "$output" | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'
+else
 	echo 0
 	exit 1
 fi
-
-"$@" --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'