Re: [PATCH] pahole: avoid segfault when parsing a problematic file

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Fri, Mar 04, 2022 at 12:38:21PM +0100, kkourt@kkourt.io escreveu:
> From: Kornilios Kourtis <kornilios@isovalent.com>
> 
> When trying to use btf encoding for an apparently problematic kernel file,
> pahole segfaults. As can be seen below [1], the problem is that we are trying to
> dereference a NULL decoder.
> 
> Fix this by checking the return value of dwfl_getmodules which [2] whill return
> -1 on errors or an offset if one of the modules did not return DWARF_CB_OK. (In
> this specific case, it was __cus__load_debug_types that returnd
> DWARF_CB_ABORT.)
> 
> Also, ensure that we get a reasonable error by setting errno in
> cus__load_files(). Otherwise, we get a "No such file or directory" error which
> might be confusing.

Can you break this into two patches, one for checking dwfl_getmodules()
failure and the second setting errno?

We should try to avoid these patches that do multiple fixes, as
sometimes one of the fixes isn't really correct and we end up not being
able to use 'git revert' which should be the case when we figure out
that some previous fix wasn't correct.

Thanks for working on this, lemme know if you're busy in which case I
can do this myself.

Best regards,

- Arnaldo
 
> After tha patch:
> $ ./pahole -J vmlinux-5.3.18-24.102-default.debug
> pahole: vmlinux-5.3.18-24.102-default.debug: Unknown error -22
> 
> [1]:
> $ gdb -q --args ./pahole -J vmlinux-5.3.18-24.102-default.debug
> Reading symbols from ./pahole...
> (gdb) r
> Starting program: /tmp/pahole/build/pahole -J vmlinux-5.3.18-24.102-default.debug
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x00007ffff7f4000e in gobuffer__size (gb=0x18) at /tmp/pahole/gobuffer.h:39
> 39              return gb->index;
> (gdb) bt
> (gdb) frame 1
> 1042            if (gobuffer__size(&encoder->percpu_secinfo) != 0)
> (gdb) list
> 1037
> 1038    int btf_encoder__encode(struct btf_encoder *encoder)
> 1039    {
> 1040            int err;
> 1041
> 1042            if (gobuffer__size(&encoder->percpu_secinfo) != 0)
> 1043                    btf_encoder__add_datasec(encoder, PERCPU_SECTION);
> 1044
> 1045            /* Empty file, nothing to do, so... done! */
> 1046            if (btf__get_nr_types(encoder->btf) == 0)
> (gdb) print encoder
> $1 = (struct btf_encoder *) 0x0
> 
> [2] https://sourceware.org/git/?p=elfutils.git;a=blob;f=libdwfl/libdwfl.h;h=f98f1d525d94bc7bcfc7c816890de5907ee4bd6d;hb=HEAD#l200
> 
> Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
> ---
>  dwarf_loader.c | 5 ++++-
>  dwarves.c      | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/dwarf_loader.c b/dwarf_loader.c
> index e30b03c..fecf711 100644
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -3235,7 +3235,10 @@ static int cus__process_file(struct cus *cus, struct conf_load *conf, int fd,
>  	};
>  
>  	/* Process the one or more modules gleaned from this file. */
> -	dwfl_getmodules(dwfl, cus__process_dwflmod, &parms, 0);
> +	int err = dwfl_getmodules(dwfl, cus__process_dwflmod, &parms, 0);
> +	if (err) {
> +		return -1;
> +	}
>  
>  	// We can't call dwfl_end(dwfl) here, as we keep pointers to strings
>  	// allocated by libdw that will be freed at dwfl_end(), so leave this for
> diff --git a/dwarves.c b/dwarves.c
> index 81fa47b..c5935ec 100644
> --- a/dwarves.c
> +++ b/dwarves.c
> @@ -2391,8 +2391,11 @@ int cus__load_files(struct cus *cus, struct conf_load *conf,
>  	int i = 0;
>  
>  	while (filenames[i] != NULL) {
> -		if (cus__load_file(cus, conf, filenames[i]))
> +		int err = cus__load_file(cus, conf, filenames[i]);
> +		if (err) {
> +			errno = err;
>  			return -++i;
> +		}
>  		++i;
>  	}
>  
> -- 
> 2.25.1

-- 

- Arnaldo