Re: [PATCH] Fix heap-use-after-free in index-cached with --disable-threading

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

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.