This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 2/2] Delete thread_info is refcount is zero


On 03/28/2017 09:24 AM, Yao Qi wrote:
> I build GDB with asan, and run test case hook-stop.exp, and threadapply.exp,
> I got the following asan error,
> 
> =================================================================^M
> ^[[1m^[[31m==2291==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160000999c4 at pc 0x000000826022 bp 0x7ffd28a8ff70 sp 0x7ffd28a8ff60^M
> ^[[1m^[[0m^[[1m^[[34mREAD of size 4 at 0x6160000999c4 thread T0^[[1m^[[0m^M
>     #0 0x826021 in release_stop_context_cleanup ../../binutils-gdb/gdb/infrun.c:8203^M
>     #1 0x72798a in do_my_cleanups ../../binutils-gdb/gdb/common/cleanups.c:154^M
>     #2 0x727a32 in do_cleanups(cleanup*) ../../binutils-gdb/gdb/common/cleanups.c:176^M
>     #3 0x826895 in normal_stop() ../../binutils-gdb/gdb/infrun.c:8381^M
>     #4 0x815208 in fetch_inferior_event(void*) ../../binutils-gdb/gdb/infrun.c:4011^M
>     #5 0x868aca in inferior_event_handler(inferior_event_type, void*) ../../binutils-gdb/gdb/inf-loop.c:44^M
> ....
> ^[[1m^[[32m0x6160000999c4 is located 68 bytes inside of 568-byte region [0x616000099980,0x616000099bb8)^M
> ^[[1m^[[0m^[[1m^[[35mfreed by thread T0 here:^[[1m^[[0m^M
>     #0 0x7fb0bc1312ca in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x982ca)^M
>     #1 0xb8c62f in xfree(void*) ../../binutils-gdb/gdb/common/common-utils.c:100^M
>     #2 0x83df67 in free_thread ../../binutils-gdb/gdb/thread.c:207^M
>     #3 0x83dfd2 in init_thread_list() ../../binutils-gdb/gdb/thread.c:223^M
>     #4 0x805494 in kill_command ../../binutils-gdb/gdb/infcmd.c:2595^M
> ....
> 
> Detaching from program: /home/yao.qi/SourceCode/gnu/build-with-asan/gdb/testsuite/outputs/gdb.threads/threadapply/threadapply, process 2399^M
> =================================================================^M
> ^[[1m^[[31m==2387==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160000a98c0 at pc 0x00000083fd28 bp 0x7ffd401c3110 sp 0x7ffd401c3100^M
> ^[[1m^[[0m^[[1m^[[34mREAD of size 4 at 0x6160000a98c0 thread T0^[[1m^[[0m^M
>     #0 0x83fd27 in thread_alive ../../binutils-gdb/gdb/thread.c:741^M
>     #1 0x844277 in thread_apply_all_command ../../binutils-gdb/gdb/thread.c:1804^M
> ....
> ^M
> ^[[1m^[[32m0x6160000a98c0 is located 64 bytes inside of 568-byte region [0x6160000a9880,0x6160000a9ab8)^M
> ^[[1m^[[0m^[[1m^[[35mfreed by thread T0 here:^[[1m^[[0m^M
>     #0 0x7f59a7e322ca in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x982ca)^M
>     #1 0xb8c62f in xfree(void*) ../../binutils-gdb/gdb/common/common-utils.c:100^M
>     #2 0x83df67 in free_thread ../../binutils-gdb/gdb/thread.c:207^M
>     #3 0x83dfd2 in init_thread_list() ../../binutils-gdb/gdb/thread.c:223^M
> 
> This patch fixes the issue by always checking refcount before decreasing it.
> If it is zero already, delete the thread_info.

So this init_thread_list is just blasting away all threads in the
the thread list, even if something is holding a strong reference
to some thread.  That does look wrong.

> 
> gdb:
> 
> 2017-03-28  Yao Qi  <yao.qi@linaro.org>
> 
> 	PR gdb/19942
> 	* infrun.c (release_stop_context_cleanup): If refcount is zero
> 	delete thread, otherwise decrease the refcount.
> 	* thread.c (init_thread_list): Likewise.
> 	(restore_current_thread_cleanup_dtor): Likewise.
> 	(set_thread_refcount): Likewise.
> ---
>  gdb/infrun.c |  7 ++++++-
>  gdb/thread.c | 19 ++++++++++++++++---
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index c8c2d6e..f8eeddb 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -8182,7 +8182,12 @@ release_stop_context_cleanup (void *arg)
>    struct stop_context *sc = (struct stop_context *) arg;
>  
>    if (sc->thread != NULL)
> -    sc->thread->refcount--;
> +    {
> +      if (sc->thread->refcount == 0)
> +	delete sc->thread;
> +      else
> +	sc->thread->refcount--;

This pattern now appears in multiple places.  How about
moving it into a function?  Or a method, and use the
"delete this;" idiom.  But, see below first.

> +    }
>    xfree (sc);
>  }
>  
> diff --git a/gdb/thread.c b/gdb/thread.c
> index 28907c5..b38c5bd 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -205,8 +205,11 @@ init_thread_list (void)
>    for (tp = thread_list; tp; tp = tpnext)
>      {
>        tpnext = tp->next;
> -      delete tp;
>  
> +      if (tp->refcount == 0)
> +	delete tp;
> +      else
> +	tp->refcount--;

I'm not convinced this is correct.  Only something that incremented
a refcount should decrement it back.  This looks like stealing a
refcount from something else, which is a recipe for bugs.

For example, say a thread has refcount == 2 when you get here.
init_thread_list decrements the count, and then clear the 
thread_list head pointer.  I.e., thread list is now empty, even if
there are still thread objects around.  Later on, if one of
the refcounts was owned by a make_cleanup_restore_current_thread call,
then essentially, the thread leaks, because
restore_current_thread_cleanup_dtor won't be able to find it anymore
in the thread list.

Also, if we're disposing of all threads (via init_thread_list),
but end up leaving some thread behind, I'd think we should mark
the thread as THREAD_EXITED, and maybe call clear_thread_inferior_resources
on it.

Essentially, it looks to me like we need to factor out, or copy
parts of what delete_thread_1 does and use those bits in
init_thread_list.

On thread refcounts and lifetime:

Thread start with refcount 0.  Putting a thread on the
thread list does not increment a thread's ref count.  It's an implicit
strong reference instead.  So removing a thread from the thread list should
not decrement the thread's refcount either.

Selecting a thread by making inferior_ptid point at it is another
way to create a "strong reference" to a thread.  That does not increment
the thread's refcount either, as it seems impossible to do while we
have code that switches inferior_ptid directly (and at places
inferior_ptid may not even point to a valid thread).

>      }
>  
>    thread_list = NULL;
> @@ -1636,7 +1639,12 @@ restore_current_thread_cleanup_dtor (void *arg)
>  
>    tp = find_thread_ptid (old->inferior_ptid);
>    if (tp)
> -    tp->refcount--;
> +    {
> +      if (tp->refcount == 0)

This looks really incorrect to me.  If we found the thread on the list,
then it should still have the refcount that the cleanup originally
added.  Anything else just leads to potential refcounting
mismatching bugs.

For example, say a new thread with the same ptid was added
to the thread list, between make_cleanup_restore_current_thread and the
cleanup running (the code above).  E.g., because of a command like
"thread apply all my_reattach", where "my_reattach" detaches from a process,
and reattaches to the same process (or kill+run, and thread reuse,
or disconnect/reconnect.  etc.).

The code above will incorrectly delete the _new_ thread, which has
refcount == 0, because it happened to have the same ptid as the
old one, and so find_thread_ptid finds it.

So all in all, it seems to me that init_thread_list should skip deleting
a thread if the thread still has a reference (like delete_thread_1),
and, make_cleanup_restore_current_thread should be changed to
hold a pointer to a thread instead of a ptid.  And then
do_restore_current_thread_cleanup / restore_current_thread_cleanup_dtor
should use the pointer directly instead of doing ptid look ups.
And with that, the refcounting should sort itself out.

Thanks,
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]