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

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

[kkourt]

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.

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

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

[Arnaldo Carvalho de Melo]

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

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

[Kornilios Kourtis]

Hi Arnaldo,

Apologies for the delayed reply.

On Sat, Mar 05, 2022 at 03:49:55PM -0300, Arnaldo Carvalho de Melo wrote:
> 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.

Yap, makes perfect sense. I will reply to this email with the two patches
momentarily.

-- 
Kornilios Kourtis.

[PATCH 1/2] pahole: avoid segfault when parsing bogus file

[kkourt]

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
returned DWARF_CB_ABORT.)

[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 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/dwarf_loader.c b/dwarf_loader.c
index 151bc83..c87378b 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -3268,7 +3268,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
-- 
2.25.1

RE: [PATCH 1/2] pahole: avoid segfault when parsing bogus file

[John Fastabend]

kkourt@ wrote:
> 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
> returned DWARF_CB_ABORT.)
> 

[...]
 
> [2] https://sourceware.org/git/?p=elfutils.git;a=blob;f=libdwfl/libdwfl.h;h=f98f1d525d94bc7bcfc7c816890de5907ee4bd6d;hb=HEAD#l200

Thanks for the reference and fix.

Acked-by: John Fastabend <john.fastabend@gmail.com>

[PATCH 2/2] dwarves: cus__load_files: set errno if load fails

[kkourt]

From: Kornilios Kourtis <kornilios@isovalent.com>

This patch improves the error seen by the user by setting errno in
cus__load_files(). Otherwise, we get a "No such file or directory" error
which might be confusing.

Before the patch, using a bogus file:
$ ./pahole -J ./vmlinux-5.3.18-24.102-default.debug
pahole: ./vmlinux-5.3.18-24.102-default.debug: No such file or directory
$ ls ./vmlinux-5.3.18-24.102-default.debug
/home/kkourt/src/hubble-fgs/vmlinux-5.3.18-24.102-default.debug

After the patch:
$ ./pahole -J ./vmlinux-5.3.18-24.102-default.debug
pahole: ./vmlinux-5.3.18-24.102-default.debug: Unknown error -22

Which is not very helpful, but less confusing.

Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
---
 dwarves.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/dwarves.c b/dwarves.c
index 89b58ef..5d0b420 100644
--- a/dwarves.c
+++ b/dwarves.c
@@ -2399,8 +2399,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

Re: [PATCH 2/2] dwarves: cus__load_files: set errno if load fails

[Arnaldo Carvalho de Melo]

Em Wed, Mar 16, 2022 at 02:23:54PM +0100, kkourt@kkourt.io escreveu:
> From: Kornilios Kourtis <kornilios@isovalent.com>
> 
> This patch improves the error seen by the user by setting errno in
> cus__load_files(). Otherwise, we get a "No such file or directory" error
> which might be confusing.
> 
> Before the patch, using a bogus file:
> $ ./pahole -J ./vmlinux-5.3.18-24.102-default.debug
> pahole: ./vmlinux-5.3.18-24.102-default.debug: No such file or directory
> $ ls ./vmlinux-5.3.18-24.102-default.debug
> /home/kkourt/src/hubble-fgs/vmlinux-5.3.18-24.102-default.debug
> 
> After the patch:
> $ ./pahole -J ./vmlinux-5.3.18-24.102-default.debug
> pahole: ./vmlinux-5.3.18-24.102-default.debug: Unknown error -22
> 
> Which is not very helpful, but less confusing.

Humm, because you should've set errno to -err back in cus__load_files(),
with this on top of your two patches we should get the:

#define EINVAL          22      /* Invalid argument */


"Invalid argument" or so from getting.

diff --git a/dwarves.c b/dwarves.c
index 5d0b420f0110452e..89609e96c46747ce 100644
--- a/dwarves.c
+++ b/dwarves.c
@@ -2401,7 +2401,7 @@ int cus__load_files(struct cus *cus, struct conf_load *conf,
 	while (filenames[i] != NULL) {
 		int err = cus__load_file(cus, conf, filenames[i]);
 		if (err) {
-			errno = err;
+			errno = -err;
 			return -++i;
 		}
 		++i;



Agreed? I'll fix it up here and apply if so.

- Arnaldo
 
> Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
> ---
>  dwarves.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/dwarves.c b/dwarves.c
> index 89b58ef..5d0b420 100644
> --- a/dwarves.c
> +++ b/dwarves.c
> @@ -2399,8 +2399,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

Re: [PATCH 2/2] dwarves: cus__load_files: set errno if load fails

[Kornilios Kourtis]

On Wed, Mar 16, 2022 at 05:51:03PM -0300, Arnaldo Carvalho de Melo wrote:
> Agreed? I'll fix it up here and apply if so.

Agreed, thanks!

cheers,
Kornilios.

RE: [PATCH 2/2] dwarves: cus__load_files: set errno if load fails

[John Fastabend]

kkourt@ wrote:
> From: Kornilios Kourtis <kornilios@isovalent.com>
> 
> This patch improves the error seen by the user by setting errno in
> cus__load_files(). Otherwise, we get a "No such file or directory" error
> which might be confusing.
> 
> Before the patch, using a bogus file:
> $ ./pahole -J ./vmlinux-5.3.18-24.102-default.debug
> pahole: ./vmlinux-5.3.18-24.102-default.debug: No such file or directory
> $ ls ./vmlinux-5.3.18-24.102-default.debug
> /home/kkourt/src/hubble-fgs/vmlinux-5.3.18-24.102-default.debug
> 
> After the patch:
> $ ./pahole -J ./vmlinux-5.3.18-24.102-default.debug
> pahole: ./vmlinux-5.3.18-24.102-default.debug: Unknown error -22
> 
> Which is not very helpful, but less confusing.
> 
> Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
> ---

With the err to -err fix Arnaldo proposed.

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH 2/2] dwarves: cus__load_files: set errno if load fails

[Arnaldo Carvalho de Melo]

Em Wed, Mar 16, 2022 at 10:00:57PM -0700, John Fastabend escreveu:
> kkourt@ wrote:
> > From: Kornilios Kourtis <kornilios@isovalent.com>
> > 
> > This patch improves the error seen by the user by setting errno in
> > cus__load_files(). Otherwise, we get a "No such file or directory" error
> > which might be confusing.
> > 
> > Before the patch, using a bogus file:
> > $ ./pahole -J ./vmlinux-5.3.18-24.102-default.debug
> > pahole: ./vmlinux-5.3.18-24.102-default.debug: No such file or directory
> > $ ls ./vmlinux-5.3.18-24.102-default.debug
> > /home/kkourt/src/hubble-fgs/vmlinux-5.3.18-24.102-default.debug
> > 
> > After the patch:
> > $ ./pahole -J ./vmlinux-5.3.18-24.102-default.debug
> > pahole: ./vmlinux-5.3.18-24.102-default.debug: Unknown error -22
> > 
> > Which is not very helpful, but less confusing.
> > 
> > Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
> > ---
> 
> With the err to -err fix Arnaldo proposed.
> 
> Acked-by: John Fastabend <john.fastabend@gmail.com>

Thanks, did the ammendment and collected your Acked-by,

- Arnaldo