Re: [Libunwind-devel] data races in libunwind

[Dave Watson]

On 04/24/18 01:33 PM, Milian Wolff wrote:
> 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?

Yea I guess these should technically be relaxed atomic ops, although
word writes are atomic in hardware on x86_64.

> 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...

Indeed, looks like a real issue to me also.

> 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?

Sure.

Thanks for the pull request, will add comments there.