[committed][gdb/symtab] Fix data race in ~charset_vector

[committed][gdb/symtab] Fix data race in ~charset_vector

[Tom de Vries]

Hi,

When doing:
...
$ gdb ./outputs/gdb.ada/char_enum_unicode/foo -batch -ex "break foo.adb:26"
...
with a gdb build with -fsanitize=thread I run into a data race:
...
WARNING: ThreadSanitizer: data race (pid=30917)
  Write of size 8 at 0x7b0400004070 by main thread:
    #0 free <null> (libtsan.so.2+0x4c5e2)
    #1 xfree<char> gdbsupport/gdb-xfree.h:37 (gdb+0x650f17)
    #2 charset_vector::clear() gdb/charset.c:703 (gdb+0x651354)
    #3 charset_vector::~charset_vector() gdb/charset.c:697 (gdb+0x6512d3)
    #4 <null> <null> (libtsan.so.2+0x32643)
    #5 captured_main_1 gdb/main.c:1310 (gdb+0xa3975a)
...

The problem is that we're freeing the charset_vector elements in the destructor,
which may still be used by a worker thread.

Fix this by not freeing the charset_vector elements in the destructor.

Tested on x86_64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29311

Committed to trunk.

Thanks,
- Tom

[gdb/symtab] Fix data race in ~charset_vector

---
 gdb/charset.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/charset.c b/gdb/charset.c
index 74f742e0aa7..a6261fc505c 100644
--- a/gdb/charset.c
+++ b/gdb/charset.c
@@ -694,7 +694,13 @@ struct charset_vector
 {
   ~charset_vector ()
   {
-    clear ();
+    /* Note that we do not call charset_vector::clear, which would also xfree
+       the elements.  This destructor is only called after exit, at which point
+       those will be freed anyway on process exit, so not freeing them now is
+       not classified as a memory leak.  OTOH, freeing them now might be
+       classified as a data race, because some worker thread might still be
+       accessing them.  */
+    charsets.clear ();
   }
 
   void clear ()

Re: [committed][gdb/symtab] Fix data race in ~charset_vector

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> The problem is that we're freeing the charset_vector elements in the destructor,
Tom> which may still be used by a worker thread.

Could you say how this happens?
What is the worker thread doing?

Normally I think gdb should be waiting for background tasks to finish
before proceeding with this sort of destruction.  If this is coming from
the parallel DWARF reader, maybe we need some higher level API to
cleanly shut down those threads first.

Tom

Re: [committed][gdb/symtab] Fix data race in ~charset_vector

[Tom de Vries]

On 7/14/22 19:31, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> The problem is that we're freeing the charset_vector elements in the destructor,
> Tom> which may still be used by a worker thread.
> 
> Could you say how this happens?
> What is the worker thread doing?
> 

As I understand it, the worker thread is executing do_finalize which 
uses a string (host_charset_name or some such) contained in the 
charset_vector, which is being destroyed in the main thread.

But that's a guess, thread sanitizer didn't report what the worker 
thread was doing: "[failed to restore the stack]".

> Normally I think gdb should be waiting for background tasks to finish
> before proceeding with this sort of destruction.  If this is coming from
> the parallel DWARF reader, maybe we need some higher level API to
> cleanly shut down those threads first.

Well, the worker threads are detached, so they run until process exit. 
We can not detach them, and do a join, but that postpones process exit.

We could try to do pthread_cancel, but still there might be some waiting 
involved.

This was sofar the only case I found of this type of race, so I thought 
it reasonable to fix like this.  If there are more cases, that might not 
be practical.

Thanks,
- Tom

Re: [committed][gdb/symtab] Fix data race in ~charset_vector

[Tom Tromey]

Tom> Well, the worker threads are detached, so they run until process
Tom> exit. We can not detach them, and do a join, but that postpones
Tom> process exit.

Yeah, we probably don't want to do that.

Tom> This was sofar the only case I found of this type of race, so I
Tom> thought it reasonable to fix like this.  If there are more cases, that
Tom> might not be practical.

Makes sense to me.

Tom