[PATCH] [gdb/tdep] Fix use-after-free in arm_exidx_fill_cache

[PATCH] [gdb/tdep] Fix use-after-free in arm_exidx_fill_cache

[Tom de Vries]

On arm-linux the linaro CI occasionally reports:
...
 (gdb) up 10
 #4  0x0001b864 in pthread_join ()
 (gdb) FAIL: gdb.threads/staticthreads.exp: up 10
...
while this is expected:
...
 (gdb) up 10
 #3  0x00010568 in main (argc=1, argv=0xfffeede4) at staticthreads.c:76
 76          pthread_join (thread, NULL);
 (gdb) PASS: gdb.threads/staticthreads.exp: up 10
...

Thiago investigated the problem, and using valgrind found an invalid read in
arm_exidx_fill_cache.

The problem happens as follows:
- an objfile and corresponding per_bfd are allocated
- some memory is allocated in arm_exidx_new_objfile using
  objfile->objfile_obstack, for the "exception table entry cache".
- a symbol reread is triggered, and the objfile, including the
  objfile_obstack, is destroyed
- a new objfile is allocated, using the same per_bfd
- again arm_exidx_new_objfile is called, but since the same per_bfd is used,
  it doesn't allocate any new memory for the "exception table entry cache".
- the "exception table entry cache" is accessed by arm_exidx_fill_cache,
  and we have a use-after-free.

This is a regression since commit a2726d4ff80 ("[ARM] Store exception handling
information per-bfd instead of per-objfile"), which changed the "exception
table entry cache" from per-objfile to per-bfd, but failed to update the
obstack_alloc.

Fix this by using objfile->per_bfd->storage_obstack instead of
objfile->objfile_obstack.

I couldn't reproduce the FAIL myself, but Thiago confirmed that the patch
fixes it.

Tested on arm-linux.

PR tdep/31254
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31254
---
 gdb/arm-tdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 0d0431e0d1c..861d50a6a3b 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -2701,7 +2701,7 @@ arm_exidx_new_objfile (struct objfile *objfile)
       if (n_bytes || n_words)
 	{
 	  gdb_byte *p = entry
-	    = (gdb_byte *) obstack_alloc (&objfile->objfile_obstack,
+	    = (gdb_byte *) obstack_alloc (&objfile->per_bfd->storage_obstack,
 					  n_bytes + n_words * 4 + 1);
 
 	  while (n_bytes--)

base-commit: 029e52bac7f3a6dd8b39f7f3d298b73174da806b
-- 
2.35.3

Re: [PATCH] [gdb/tdep] Fix use-after-free in arm_exidx_fill_cache

[Luis Machado]

On 2/5/24 05:54, Tom de Vries wrote:
> On arm-linux the linaro CI occasionally reports:
> ...
>  (gdb) up 10
>  #4  0x0001b864 in pthread_join ()
>  (gdb) FAIL: gdb.threads/staticthreads.exp: up 10
> ...
> while this is expected:
> ...
>  (gdb) up 10
>  #3  0x00010568 in main (argc=1, argv=0xfffeede4) at staticthreads.c:76
>  76          pthread_join (thread, NULL);
>  (gdb) PASS: gdb.threads/staticthreads.exp: up 10
> ...
> 
> Thiago investigated the problem, and using valgrind found an invalid read in
> arm_exidx_fill_cache.
> 
> The problem happens as follows:
> - an objfile and corresponding per_bfd are allocated
> - some memory is allocated in arm_exidx_new_objfile using
>   objfile->objfile_obstack, for the "exception table entry cache".
> - a symbol reread is triggered, and the objfile, including the
>   objfile_obstack, is destroyed
> - a new objfile is allocated, using the same per_bfd
> - again arm_exidx_new_objfile is called, but since the same per_bfd is used,
>   it doesn't allocate any new memory for the "exception table entry cache".
> - the "exception table entry cache" is accessed by arm_exidx_fill_cache,
>   and we have a use-after-free.
> 
> This is a regression since commit a2726d4ff80 ("[ARM] Store exception handling
> information per-bfd instead of per-objfile"), which changed the "exception
> table entry cache" from per-objfile to per-bfd, but failed to update the
> obstack_alloc.
> 
> Fix this by using objfile->per_bfd->storage_obstack instead of
> objfile->objfile_obstack.
> 
> I couldn't reproduce the FAIL myself, but Thiago confirmed that the patch
> fixes it.
> 
> Tested on arm-linux.
> 
> PR tdep/31254
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31254
> ---
>  gdb/arm-tdep.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 0d0431e0d1c..861d50a6a3b 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -2701,7 +2701,7 @@ arm_exidx_new_objfile (struct objfile *objfile)
>        if (n_bytes || n_words)
>  	{
>  	  gdb_byte *p = entry
> -	    = (gdb_byte *) obstack_alloc (&objfile->objfile_obstack,
> +	    = (gdb_byte *) obstack_alloc (&objfile->per_bfd->storage_obstack,
>  					  n_bytes + n_words * 4 + 1);
>  
>  	  while (n_bytes--)
> 
> base-commit: 029e52bac7f3a6dd8b39f7f3d298b73174da806b

Looks like I missed a spot in the per-bfd conversion there.

Thanks Thiago and Tom for the investigation and fix. This is OK.

Approved-By: Luis Machado <luis.machado@arm.com>

Re: [PATCH] [gdb/tdep] Fix use-after-free in arm_exidx_fill_cache

[Thiago Jung Bauermann]

Luis Machado <luis.machado@arm.com> writes:

> On 2/5/24 05:54, Tom de Vries wrote:
>> On arm-linux the linaro CI occasionally reports:
>> ...
>>  (gdb) up 10
>>  #4  0x0001b864 in pthread_join ()
>>  (gdb) FAIL: gdb.threads/staticthreads.exp: up 10
>> ...
>> while this is expected:
>> ...
>>  (gdb) up 10
>>  #3  0x00010568 in main (argc=1, argv=0xfffeede4) at staticthreads.c:76
>>  76          pthread_join (thread, NULL);
>>  (gdb) PASS: gdb.threads/staticthreads.exp: up 10
>> ...
>> 
>> Thiago investigated the problem, and using valgrind found an invalid read in
>> arm_exidx_fill_cache.
>> 
>> The problem happens as follows:
>> - an objfile and corresponding per_bfd are allocated
>> - some memory is allocated in arm_exidx_new_objfile using
>>   objfile->objfile_obstack, for the "exception table entry cache".
>> - a symbol reread is triggered, and the objfile, including the
>>   objfile_obstack, is destroyed
>> - a new objfile is allocated, using the same per_bfd
>> - again arm_exidx_new_objfile is called, but since the same per_bfd is used,
>>   it doesn't allocate any new memory for the "exception table entry cache".
>> - the "exception table entry cache" is accessed by arm_exidx_fill_cache,
>>   and we have a use-after-free.
>> 
>> This is a regression since commit a2726d4ff80 ("[ARM] Store exception handling
>> information per-bfd instead of per-objfile"), which changed the "exception
>> table entry cache" from per-objfile to per-bfd, but failed to update the
>> obstack_alloc.
>> 
>> Fix this by using objfile->per_bfd->storage_obstack instead of
>> objfile->objfile_obstack.
>> 
>> I couldn't reproduce the FAIL myself, but Thiago confirmed that the patch
>> fixes it.
>> 
>> Tested on arm-linux.
>> 
>> PR tdep/31254
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31254
>> ---
>>  gdb/arm-tdep.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>> index 0d0431e0d1c..861d50a6a3b 100644
>> --- a/gdb/arm-tdep.c
>> +++ b/gdb/arm-tdep.c
>> @@ -2701,7 +2701,7 @@ arm_exidx_new_objfile (struct objfile *objfile)
>>        if (n_bytes || n_words)
>>  	{
>>  	  gdb_byte *p = entry
>> -	    = (gdb_byte *) obstack_alloc (&objfile->objfile_obstack,
>> +	    = (gdb_byte *) obstack_alloc (&objfile->per_bfd->storage_obstack,
>>  					  n_bytes + n_words * 4 + 1);
>>  
>>  	  while (n_bytes--)
>> 
>> base-commit: 029e52bac7f3a6dd8b39f7f3d298b73174da806b
>
> Looks like I missed a spot in the per-bfd conversion there.
>
> Thanks Thiago and Tom for the investigation and fix. This is OK.
>
> Approved-By: Luis Machado <luis.machado@arm.com>

Could this patch also be committed to the GDB 14 branch?

-- 
Thiago

Re: [PATCH] [gdb/tdep] Fix use-after-free in arm_exidx_fill_cache

[Tom Tromey]

>>>>> "Thiago" == Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:

>> Looks like I missed a spot in the per-bfd conversion there.
>> 
>> Thanks Thiago and Tom for the investigation and fix. This is OK.
>> 
>> Approved-By: Luis Machado <luis.machado@arm.com>

Thiago> Could this patch also be committed to the GDB 14 branch?

I think it's fine.

Tom

Re: [PATCH] [gdb/tdep] Fix use-after-free in arm_exidx_fill_cache

[Thiago Jung Bauermann]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Thiago" == Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
>
>>> Looks like I missed a spot in the per-bfd conversion there.
>>> 
>>> Thanks Thiago and Tom for the investigation and fix. This is OK.
>>> 
>>> Approved-By: Luis Machado <luis.machado@arm.com>
>
> Thiago> Could this patch also be committed to the GDB 14 branch?
>
> I think it's fine.

Thanks! I just pushed it to the branch as commit 7c709b26ba13.

-- 
Thiago