[Libunwind-devel] data races in libunwind

[Milian Wolff]

Hey all,

trying to figure out a strange bug in heaptrack, I decided to use TSAN to 
validate the thread safety. Turns out that there are some issues in libunwind 
itself:

a) last_good_addr / lga_victim in x86_64/Ginit.c validate_mem

This cache isn't thread safe but accessed from multiple threads in parallel. 
The reads and writes should probably be made atomic?

WARNING: ThreadSanitizer: data race (pid=17824)
  Read of size 8 at 0x7f4dac484140 by thread T3:
    #0 validate_mem ../../src/x86_64/Ginit.c:201 (libunwind.so.
8+0x00000001f836)
    #1 access_mem ../../src/x86_64/Ginit.c:242 (libunwind.so.8+0x00000001fe06)
    #2 dwarf_get ../../include/tdep-x86_64/libunwind_i.h:168 (libunwind.so.
8+0x0000000221cf)
    #3 _ULx86_64_access_reg ../../src/x86_64/Gregs.c:130 (libunwind.so.
8+0x000000022e99)
    #4 _ULx86_64_get_reg ../../src/mi/Gget_reg.c:40 (libunwind.so.
8+0x00000001e5bc)
    #5 apply_reg_state ../../src/dwarf/Gparser.c:796 (libunwind.so.
8+0x000000038209)
    #6 _ULx86_64_dwarf_step ../../src/dwarf/Gparser.c:961 (libunwind.so.
8+0x000000039dbc)
    #7 _ULx86_64_step ../../src/x86_64/Gstep.c:71 (libunwind.so.
8+0x00000002481f)
    #8 trace_init_addr ../../src/x86_64/Gtrace.c:248 (libunwind.so.
8+0x000000026f0e)
    #9 trace_lookup ../../src/x86_64/Gtrace.c:330 (libunwind.so.
8+0x000000027429)
    #10 _ULx86_64_tdep_trace ../../src/x86_64/Gtrace.c:447 (libunwind.so.
8+0x00000002789a)
    #11 unw_backtrace ../../src/mi/backtrace.c:69 (libunwind.so.
8+0x00000001cb07)

  Previous write of size 8 at 0x7f4dac484140 by thread T2:
    #0 validate_mem ../../src/x86_64/Ginit.c:220 (libunwind.so.
8+0x00000001fc54)
    #1 access_mem ../../src/x86_64/Ginit.c:242 (libunwind.so.8+0x00000001fe06)
    #2 dwarf_get ../../include/tdep-x86_64/libunwind_i.h:168 (libunwind.so.
8+0x0000000221cf)
    #3 _ULx86_64_access_reg ../../src/x86_64/Gregs.c:130 (libunwind.so.
8+0x000000022e99)
    #4 _ULx86_64_get_reg ../../src/mi/Gget_reg.c:40 (libunwind.so.
8+0x00000001e5bc)
    #5 apply_reg_state ../../src/dwarf/Gparser.c:796 (libunwind.so.
8+0x000000038209)
    #6 _ULx86_64_dwarf_step ../../src/dwarf/Gparser.c:961 (libunwind.so.
8+0x000000039dbc)
    #7 _ULx86_64_step ../../src/x86_64/Gstep.c:71 (libunwind.so.
8+0x00000002481f)
    #8 trace_init_addr ../../src/x86_64/Gtrace.c:248 (libunwind.so.
8+0x000000026f0e)
    #9 trace_lookup ../../src/x86_64/Gtrace.c:330 (libunwind.so.
8+0x000000027429)
    #10 _ULx86_64_tdep_trace ../../src/x86_64/Gtrace.c:447 (libunwind.so.
8+0x00000002789a)
    #11 unw_backtrace ../../src/mi/backtrace.c:69 (libunwind.so.
8+0x00000001cb07)

  Location is global 'last_good_addr' of size 32 at 0x7f4dac484140 
(libunwind.so.8+0x000000273140)

b) cache hints in dwarf/Gparser.c find_reg_state:

This can be reproduced when trying to use libunwind with UNW_CACHE_GLOBAL from 
multiple threads, e.g. when libunwind wasn't build with --enable-per-thread-
cache. Here, I believe the issue comes from the put_rs_cache in 
find_reg_state, which needs to be moved after the last usage of cache, i.e. 
below the `if (rs)` branch? Can someone confirm this? It seems to fix the 
error for me...

WARNING: ThreadSanitizer: data race (pid=16551)
  Read of size 2 at 0x7f7dda69ed7a by thread T2:
    #0 find_reg_state ../../src/dwarf/Gparser.c:939 (libunwind.so.
8+0x000000037254)
    #1 _ULx86_64_dwarf_step ../../src/dwarf/Gparser.c:958 (libunwind.so.
8+0x000000037543)
    #2 _ULx86_64_step ../../src/x86_64/Gstep.c:71 (libunwind.so.
8+0x000000021fdd)
    #3 trace_init_addr ../../src/x86_64/Gtrace.c:248 (libunwind.so.
8+0x0000000246cc)
    #4 trace_lookup ../../src/x86_64/Gtrace.c:330 (libunwind.so.
8+0x000000024be7)
    #5 _ULx86_64_tdep_trace ../../src/x86_64/Gtrace.c:447 (libunwind.so.
8+0x000000025058)
    #6 unw_backtrace ../../src/mi/backtrace.c:69 (libunwind.so.
8+0x00000001a6e7)

  Previous write of size 2 at 0x7f7dda69ed7a by thread T3:
    #0 find_reg_state ../../src/dwarf/Gparser.c:940 (libunwind.so.
8+0x000000037383)
    #1 _ULx86_64_dwarf_step ../../src/dwarf/Gparser.c:958 (libunwind.so.
8+0x000000037543)
    #2 _ULx86_64_step ../../src/x86_64/Gstep.c:71 (libunwind.so.
8+0x000000021fdd)
    #3 trace_init_addr ../../src/x86_64/Gtrace.c:248 (libunwind.so.
8+0x0000000246cc)
    #4 trace_lookup ../../src/x86_64/Gtrace.c:330 (libunwind.so.
8+0x000000024be7)
    #5 _ULx86_64_tdep_trace ../../src/x86_64/Gtrace.c:447 (libunwind.so.
8+0x000000025058)
    #6 unw_backtrace ../../src/mi/backtrace.c:69 (libunwind.so.
8+0x00000001a6e7)

  Location is global 'local_addr_space' of size 26296 at 0x7f7dda698c60 
(libunwind.so.8+0x000000268d7a)

c) Not a race, but with valgrind's memcheck I also stumbled upon  x86_64's 
Ginit.c open_pipe:

  /* ignore errors for closing invalid fd's */
  close (mem_validate_pipe[0]);
  close (mem_validate_pipe[1]);

On the first call, both fds will be -1 and valgrind will complain. Adding the 
check is easy, can we add that?
-- 
Milian Wolff
address@hidden
http://milianw.de