* [PATCH] Fix heap-use-after-free in index-cached with --disable-threading [not found] <20240504110942.922-1-ssbssa.ref@yahoo.de> @ 2024-05-04 11:09 ` Hannes Domani 2024-05-04 15:45 ` Tom Tromey 2024-05-10 19:16 ` Pedro Alves 0 siblings, 2 replies; 10+ messages in thread From: Hannes Domani @ 2024-05-04 11:09 UTC (permalink / raw) To: gdb-patches 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(-) diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c index 3b95c075a55..767f119e04f 100644 --- 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; #endif /* CXX_STD_THREAD */ /* Only the main thread is allowed to report complaints and the -- 2.35.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix heap-use-after-free in index-cached with --disable-threading 2024-05-04 11:09 ` [PATCH] Fix heap-use-after-free in index-cached with --disable-threading Hannes Domani @ 2024-05-04 15:45 ` Tom Tromey 2024-05-04 16:28 ` Hannes Domani 2024-05-04 16:56 ` Hannes Domani 2024-05-10 19:16 ` Pedro Alves 1 sibling, 2 replies; 10+ messages in thread From: Tom Tromey @ 2024-05-04 15:45 UTC (permalink / raw) To: Hannes Domani; +Cc: gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix heap-use-after-free in index-cached with --disable-threading 2024-05-04 15:45 ` Tom Tromey @ 2024-05-04 16:28 ` Hannes Domani 2024-05-04 16:56 ` Hannes Domani 1 sibling, 0 replies; 10+ messages in thread From: Hannes Domani @ 2024-05-04 16:28 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix heap-use-after-free in index-cached with --disable-threading 2024-05-04 15:45 ` Tom Tromey 2024-05-04 16:28 ` Hannes Domani @ 2024-05-04 16:56 ` Hannes Domani 2024-05-10 5:59 ` Bernd Edlinger 1 sibling, 1 reply; 10+ messages in thread From: Hannes Domani @ 2024-05-04 16:56 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix heap-use-after-free in index-cached with --disable-threading 2024-05-04 16:56 ` Hannes Domani @ 2024-05-10 5:59 ` Bernd Edlinger 2024-05-10 13:50 ` Hannes Domani 2024-05-10 18:03 ` Tom Tromey 0 siblings, 2 replies; 10+ messages in thread From: Bernd Edlinger @ 2024-05-10 5:59 UTC (permalink / raw) To: Hannes Domani, Tom Tromey; +Cc: gdb-patches 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix heap-use-after-free in index-cached with --disable-threading 2024-05-10 5:59 ` Bernd Edlinger @ 2024-05-10 13:50 ` Hannes Domani 2024-05-10 18:03 ` Tom Tromey 1 sibling, 0 replies; 10+ messages in thread From: Hannes Domani @ 2024-05-10 13:50 UTC (permalink / raw) To: Tom Tromey, Bernd Edlinger; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix heap-use-after-free in index-cached with --disable-threading 2024-05-10 5:59 ` Bernd Edlinger 2024-05-10 13:50 ` Hannes Domani @ 2024-05-10 18:03 ` Tom Tromey 2024-05-11 6:44 ` Bernd Edlinger 1 sibling, 1 reply; 10+ messages in thread From: Tom Tromey @ 2024-05-10 18:03 UTC (permalink / raw) To: Bernd Edlinger; +Cc: Hannes Domani, Tom Tromey, gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix heap-use-after-free in index-cached with --disable-threading 2024-05-10 18:03 ` Tom Tromey @ 2024-05-11 6:44 ` Bernd Edlinger 0 siblings, 0 replies; 10+ messages in thread From: Bernd Edlinger @ 2024-05-11 6:44 UTC (permalink / raw) To: Tom Tromey; +Cc: Hannes Domani, gdb-patches 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix heap-use-after-free in index-cached with --disable-threading 2024-05-04 11:09 ` [PATCH] Fix heap-use-after-free in index-cached with --disable-threading Hannes Domani 2024-05-04 15:45 ` Tom Tromey @ 2024-05-10 19:16 ` Pedro Alves 2024-05-11 10:47 ` Hannes Domani 1 sibling, 1 reply; 10+ messages in thread From: Pedro Alves @ 2024-05-10 19:16 UTC (permalink / raw) To: Hannes Domani, gdb-patches 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". ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix heap-use-after-free in index-cached with --disable-threading 2024-05-10 19:16 ` Pedro Alves @ 2024-05-11 10:47 ` Hannes Domani 0 siblings, 0 replies; 10+ messages in thread From: Hannes Domani @ 2024-05-11 10:47 UTC (permalink / raw) To: gdb-patches, Pedro Alves 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-05-11 10:47 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20240504110942.922-1-ssbssa.ref@yahoo.de> 2024-05-04 11:09 ` [PATCH] Fix heap-use-after-free in index-cached with --disable-threading Hannes Domani 2024-05-04 15:45 ` Tom Tromey 2024-05-04 16:28 ` Hannes Domani 2024-05-04 16:56 ` Hannes Domani 2024-05-10 5:59 ` Bernd Edlinger 2024-05-10 13:50 ` Hannes Domani 2024-05-10 18:03 ` Tom Tromey 2024-05-11 6:44 ` Bernd Edlinger 2024-05-10 19:16 ` Pedro Alves 2024-05-11 10:47 ` Hannes Domani
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).