Fix heap-use-after-free in index-cached with --disable-threading

  If threads are disabled, either by --disable-threading explicitely, or by
missing std::thread support, you get the following ASAN error when
loading symbols:

==7310==ERROR: AddressSanitizer: heap-use-after-free on address 0x614000002128 at pc 0x00000098794a bp 0x7ffe37e6af70 sp 0x7ffe37e6af68
READ of size 1 at 0x614000002128 thread T0
    #0 0x987949 in index_cache_store_context::store() const ../../gdb/dwarf2/index-cache.c:163
    #1 0x943467 in cooked_index_worker::write_to_cache(cooked_index const*, deferred_warnings*) const ../../gdb/dwarf2/cooked-index.c:601
    #2 0x1705e39 in std::function<void ()>::operator()() const /lisec/gcc/9/include/c++/9.2.0/bits/std_function.h:690
    #3 0x1705e39 in gdb::task_group::impl::~impl() ../../gdbsupport/task-group.cc:38

0x614000002128 is located 232 bytes inside of 408-byte region [0x614000002040,0x6140000021d8)
freed by thread T0 here:
    #0 0x7fd75ccf8ea5 in operator delete(void*, unsigned long) ../../.././libsanitizer/asan/asan_new_delete.cc:177
    #1 0x9462e5 in cooked_index::index_for_writing() ../../gdb/dwarf2/cooked-index.h:689
    #2 0x9462e5 in operator() ../../gdb/dwarf2/cooked-index.c:657
    #3 0x9462e5 in _M_invoke /lisec/gcc/9/include/c++/9.2.0/bits/std_function.h:300

It's happening because cooked_index_worker::wait always returns true in
this case, which tells cooked_index::wait it can delete the m_state
cooked_index_worker member, but cooked_index_worker::write_to_cache tries
to access it immediately afterwards.

Fixed by making cooked_index_worker::wait only return true if desired_state
is CACHE_DONE, same as if threading was enabled, so m_state will not be
prematurely deleted.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31694
---
 gdb/dwarf2/cooked-index.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:

Hannes> Fixed by making cooked_index_worker::wait only return true if desired_state
Hannes> is CACHE_DONE, same as if threading was enabled, so m_state will not be
Hannes> prematurely deleted.

Hannes> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31694

Thank you.  This is ok.
Approved-By: Tom Tromey <tom@tromey.com>

Hannes>  #else
Hannes>    /* Without threads, all the work is done immediately on the main
Hannes>       thread, and there is never anything to wait for.  */
Hannes> -  done = true;
Hannes> +  done = desired_state == cooked_state::CACHE_DONE;
Hannes>  #endif /* CXX_STD_THREAD */
 
I wouldn't mind if this code were lowered out of the #if, like:

    #endif /* CXX_STD_THREAD */
      bool done = ...

However I think it's also fine as-is and can be tidied up later if
anyone cares to.

Tom
  

Am Samstag, 4. Mai 2024 um 17:45:06 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> >>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:
>
> Hannes>  #else
> Hannes>    /* Without threads, all the work is done immediately on the main
> Hannes>      thread, and there is never anything to wait for.  */
> Hannes> -  done = true;
> Hannes> +  done = desired_state == cooked_state::CACHE_DONE;
> Hannes>  #endif /* CXX_STD_THREAD */
>
> I wouldn't mind if this code were lowered out of the #if, like:
>
>
>     #endif /* CXX_STD_THREAD */
>
>       bool done = ...
>
> However I think it's also fine as-is and can be tidied up later if
> anyone cares to.

Note that there is a small difference between them:

#if CXX_STD_THREAD
  done = m_state == cooked_state::CACHE_DONE;
#else
  done = desired_state == cooked_state::CACHE_DONE;
#endif

Though I wonder if not both could check for desired_state.


Hannes
  

Am Samstag, 4. Mai 2024 um 17:45:06 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> >>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:
>
> Hannes> Fixed by making cooked_index_worker::wait only return true if desired_state
> Hannes> is CACHE_DONE, same as if threading was enabled, so m_state will not be
> Hannes> prematurely deleted.
>
> Hannes> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31694
>
> Thank you.  This is ok.
> Approved-By: Tom Tromey <tom@tromey.com>

Pushed, thanks.


Hannes
  

On 5/4/24 18:56, Hannes Domani wrote:
>  Am Samstag, 4. Mai 2024 um 17:45:06 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:
> 
>>>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:
>>
>> Hannes> Fixed by making cooked_index_worker::wait only return true if desired_state
>> Hannes> is CACHE_DONE, same as if threading was enabled, so m_state will not be
>> Hannes> prematurely deleted.
>>
>> Hannes> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31694
>>
>> Thank you.  This is ok.
>> Approved-By: Tom Tromey <tom@tromey.com>
> 
> Pushed, thanks.
> 
> 
> Hannes
> 
Hi,

due to this incident you fixed here, I did some testing with tsan,
and found a couple issues that I think are important, but I have no
good idea how to solve them.
https://sourceware.org/bugzilla/show_bug.cgi?id=31713
https://sourceware.org/bugzilla/show_bug.cgi?id=31715
https://sourceware.org/bugzilla/show_bug.cgi?id=31716

I have found an issue (bug#31715) with the function
cooked_index_worker::wait that was changed here.
In one of the tsan reports I see something interesting here:
https://sourceware.org/bugzilla/attachment.cgi?id=15506
The cooked_index_worker::wait apparently proceeds and reads
the "canonical" using cooked_index_entry::full_name
without lock, and later a worker thread changes this value
also without lock.
Do you have any idea what is going on here?


Thanks
Bernd.
  

Am Freitag, 10. Mai 2024 um 07:57:58 MESZ hat Bernd Edlinger <bernd.edlinger@hotmail.de> Folgendes geschrieben:

> On 5/4/24 18:56, Hannes Domani wrote:
>
> >  Am Samstag, 4. Mai 2024 um 17:45:06 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:
> >
> >>>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:
> >>
> >> Hannes> Fixed by making cooked_index_worker::wait only return true if desired_state
> >> Hannes> is CACHE_DONE, same as if threading was enabled, so m_state will not be
> >> Hannes> prematurely deleted.
> >>
> >> Hannes> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31694
> >>
> >> Thank you.  This is ok.
> >> Approved-By: Tom Tromey <tom@tromey.com>
> >
> > Pushed, thanks.
> >
> >
> > Hannes
>
> >
> Hi,
>
> due to this incident you fixed here, I did some testing with tsan,
> and found a couple issues that I think are important, but I have no
> good idea how to solve them.
> https://sourceware.org/bugzilla/show_bug.cgi?id=31713
> https://sourceware.org/bugzilla/show_bug.cgi?id=31715
> https://sourceware.org/bugzilla/show_bug.cgi?id=31716
>
> I have found an issue (bug#31715) with the function
> cooked_index_worker::wait that was changed here.
> In one of the tsan reports I see something interesting here:
> https://sourceware.org/bugzilla/attachment.cgi?id=15506
> The cooked_index_worker::wait apparently proceeds and reads
> the "canonical" using cooked_index_entry::full_name
> without lock, and later a worker thread changes this value
> also without lock.
> Do you have any idea what is going on here?

Looks to me they are because while the background DWARF reading is happening,
gdb is processing some command (break/load/set), and both are accessing the
same memory.


Hannes
  

>>>>> "Bernd" == Bernd Edlinger <bernd.edlinger@hotmail.de> writes:

Bernd> due to this incident you fixed here, I did some testing with tsan,
Bernd> and found a couple issues that I think are important, but I have no
Bernd> good idea how to solve them.
Bernd> https://sourceware.org/bugzilla/show_bug.cgi?id=31713
Bernd> https://sourceware.org/bugzilla/show_bug.cgi?id=31715
Bernd> https://sourceware.org/bugzilla/show_bug.cgi?id=31716

One option is to disable background reading, by having the DWARF reader
wait for the indexer to finish its work before returning.

This is easy to implement, but unfortunate to have to do.  Still, maybe
the best approach for GDB 15.

I'll try to look into these bugs soon.

Tom
  

On 2024-05-04 12:09, Hannes Domani wrote:

> --- a/gdb/dwarf2/cooked-index.c
> +++ b/gdb/dwarf2/cooked-index.c
> @@ -513,7 +513,7 @@ cooked_index_worker::wait (cooked_state desired_state, bool allow_quit)
>  #else
>    /* Without threads, all the work is done immediately on the main
>       thread, and there is never anything to wait for.  */
> -  done = true;
> +  done = desired_state == cooked_state::CACHE_DONE;

I know nothing about this code, but I wondered if the "never" above in the comment
should say something else.  It matched the old code that just assigned to true, but
now it's conditional, which doesn't read like "never".
  

On 5/10/24 20:03, Tom Tromey wrote:
>>>>>> "Bernd" == Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> 
> Bernd> due to this incident you fixed here, I did some testing with tsan,
> Bernd> and found a couple issues that I think are important, but I have no
> Bernd> good idea how to solve them.
> Bernd> https://sourceware.org/bugzilla/show_bug.cgi?id=31713
> Bernd> https://sourceware.org/bugzilla/show_bug.cgi?id=31715
> Bernd> https://sourceware.org/bugzilla/show_bug.cgi?id=31716
> 
> One option is to disable background reading, by having the DWARF reader
> wait for the indexer to finish its work before returning.
> 
> This is easy to implement, but unfortunate to have to do.  Still, maybe
> the best approach for GDB 15.
> 
> I'll try to look into these bugs soon.
> 

Thanks Tom,

I think the call stack from the lambda function is probably a bit misleading.
It seems to be that the state MAIN_AVAILABLE is set too early, because
one or all of the Finalize functions need to be run first.

This could solve most of the issues:

--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -644,8 +644,6 @@ cooked_index::set_contents (vec_type &&vec, deferred_warnings *warn,
   gdb_assert (m_vector.empty ());
   m_vector = std::move (vec);
 
-  m_state->set (cooked_state::MAIN_AVAILABLE);
-
   /* This is run after finalization is done -- but not before.  If
      this task were submitted earlier, it would have to wait for
      finalization.  However, that would take a slot in the global
@@ -653,6 +651,7 @@ cooked_index::set_contents (vec_type &&vec, deferred_warnings *warn,
      would cause a livelock.  */
   gdb::task_group finalizers ([=] ()
   {
+    m_state->set (cooked_state::MAIN_AVAILABLE);
     m_state->set (cooked_state::FINALIZED);
     m_state->write_to_cache (index_for_writing (), warn);
     m_state->set (cooked_state::CACHE_DONE);

but #31716 remains, and #31713 is now even more nasty.
I've uploaded new error reports to bugzilla with the details.

What I wonder, is how the life-cycle of these objects continue,
are they immutable after CACHE_DONE, or can they be deleted later?
Can a worker thread theoretically access an object that is about to be deleted?


Bernd.
  

Am Freitag, 10. Mai 2024 um 21:16:51 MESZ hat Pedro Alves <pedro@palves.net> Folgendes geschrieben:

> On 2024-05-04 12:09, Hannes Domani wrote:
>
>
> > --- a/gdb/dwarf2/cooked-index.c
> > +++ b/gdb/dwarf2/cooked-index.c
> > @@ -513,7 +513,7 @@ cooked_index_worker::wait (cooked_state desired_state, bool allow_quit)
> >  #else
> >    /* Without threads, all the work is done immediately on the main
> >      thread, and there is never anything to wait for.  */
> > -  done = true;
> > +  done = desired_state == cooked_state::CACHE_DONE;
>
>
> I know nothing about this code, but I wondered if the "never" above in the comment
> should say something else.  It matched the old code that just assigned to true, but
> now it's conditional, which doesn't read like "never".

The waiting is done in the #if CXX_STD_THREAD block above, and none of
it is done in this #else block, so the comment is still fine.


Hannes