gdb: Fix use-after-free when an objfile has no symbols to load

  The recent commit <HASH> moved an initialization of an objfile_holder in
syms_from_objfile_1 much earlier in the function, to better deal with
when GDB is unable to read the objfile format.

However, there is an early exit from syms_from_objfile_1 when the
objfile can be understood, but has no symbols. That was not releasing
the objfile_holder, so the objfile was being unlinked from the program
space, but the process of reading the objfile was being continued,
leading to use-after-frees flagged by the Address Sanitizer.

This commit fixes that UAF by making the objfile_holder release the
objfile right before the early exit.

This commit also changes the test gdb.base/dump.exp since that was the
original test that flagged the UAF, but at the end of the test the
generated files were being deleted, meaning we couldn't redo the test
manually after teh fact. That final deletion was removed

Reported-by: Simon Marchi <simark@simark.ca>
---
 gdb/symfile.c                   | 4 ++++
 gdb/testsuite/gdb.base/dump.exp | 4 ----
 2 files changed, 4 insertions(+), 4 deletions(-)
  

On 12/5/24 10:19 AM, Guinevere Larsen wrote:
> The recent commit <HASH> moved an initialization of an objfile_holder in
Oops, I meant to go back and add the hash of the commit there.... it is 
commit 32e3f1a0aa0
> syms_from_objfile_1 much earlier in the function, to better deal with
> when GDB is unable to read the objfile format.
>
> However, there is an early exit from syms_from_objfile_1 when the
> objfile can be understood, but has no symbols. That was not releasing
> the objfile_holder, so the objfile was being unlinked from the program
> space, but the process of reading the objfile was being continued,
> leading to use-after-frees flagged by the Address Sanitizer.
>
> This commit fixes that UAF by making the objfile_holder release the
> objfile right before the early exit.
>
> This commit also changes the test gdb.base/dump.exp since that was the
> original test that flagged the UAF, but at the end of the test the
> generated files were being deleted, meaning we couldn't redo the test
> manually after teh fact. That final deletion was removed
>
> Reported-by: Simon Marchi <simark@simark.ca>
> ---
>   gdb/symfile.c                   | 4 ++++
>   gdb/testsuite/gdb.base/dump.exp | 4 ----
>   2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 3fd6c8d73a2..28c0d46ab54 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -901,6 +901,10 @@ syms_from_objfile_1 (struct objfile *objfile,
>         int num_sections = gdb_bfd_count_sections (objfile->obfd.get ());
>   
>         objfile->section_offsets.assign (num_sections, 0);
> +
> +      /* Release the objfile unique pointer, since nothing went wrong
> +	 in reading it.  */
> +      objfile_holder.release ();
>         return;
>       }
>   
> diff --git a/gdb/testsuite/gdb.base/dump.exp b/gdb/testsuite/gdb.base/dump.exp
> index 3c7bee5ff30..58fedb1d36b 100644
> --- a/gdb/testsuite/gdb.base/dump.exp
> +++ b/gdb/testsuite/gdb.base/dump.exp
> @@ -564,7 +564,3 @@ if {![string compare $is64bitonly "no"]} {
>         "reload struct as memory, tekhex" \
>   	$struct_val "\*$struct_ptr_type"
>   }
> -
> -# clean up files
> -
> -remote_exec host "rm -f $filenames"
  

On 2024-12-05 08:19, Guinevere Larsen wrote:
> The recent commit <HASH> moved an initialization of an objfile_holder in
> syms_from_objfile_1 much earlier in the function, to better deal with
> when GDB is unable to read the objfile format.
> 
> However, there is an early exit from syms_from_objfile_1 when the
> objfile can be understood, but has no symbols. That was not releasing
> the objfile_holder, so the objfile was being unlinked from the program
> space, but the process of reading the objfile was being continued,
> leading to use-after-frees flagged by the Address Sanitizer.
> 
> This commit fixes that UAF by making the objfile_holder release the
> objfile right before the early exit.
> 
> This commit also changes the test gdb.base/dump.exp since that was the
> original test that flagged the UAF, but at the end of the test the
> generated files were being deleted, meaning we couldn't redo the test
> manually after teh fact. That final deletion was removed

teh -> the

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Thanks,

Simon
  

On 12/6/24 2:35 PM, Simon Marchi wrote:
>
> On 2024-12-05 08:19, Guinevere Larsen wrote:
>> The recent commit <HASH> moved an initialization of an objfile_holder in
>> syms_from_objfile_1 much earlier in the function, to better deal with
>> when GDB is unable to read the objfile format.
>>
>> However, there is an early exit from syms_from_objfile_1 when the
>> objfile can be understood, but has no symbols. That was not releasing
>> the objfile_holder, so the objfile was being unlinked from the program
>> space, but the process of reading the objfile was being continued,
>> leading to use-after-frees flagged by the Address Sanitizer.
>>
>> This commit fixes that UAF by making the objfile_holder release the
>> objfile right before the early exit.
>>
>> This commit also changes the test gdb.base/dump.exp since that was the
>> original test that flagged the UAF, but at the end of the test the
>> generated files were being deleted, meaning we couldn't redo the test
>> manually after teh fact. That final deletion was removed
> teh -> the
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
>
> Thanks,
>
> Simon
>
Thanks for the review, pushed!