- User Since
- Dec 6 2012, 2:31 AM (237 w, 4 d)
Thu, Jun 22
Evgenii, please take a look at actual pass changes.
I know, not the feedback you was looking for :)
Tue, Jun 13
May 25 2017
I don't have time to figure out how to support Mac. Filed https://github.com/google/sanitizers/issues/816 so that this is not lost.
LGTM with a nit
May 24 2017
looks good to me
I am not certain about the Windows impact of this change
FWIW I've solved a similar problem in tcmalloc by trimming blocks either on left or on right. Namely, if heap grows up we trim on right; if heap grows down we trim on left. But I don't see how it is better than this solution.
May 23 2017
I think that's because we use _shadow_ memory to store pointer to ThreadState. See cur_thread in tsan_platform_mac.cc. We probably need to skip that slot or something.
I don't completely understand this. allocator_fork_no_hang.cc is broken and deserves deadlocking. Why are we trying to fix this program only under sanitizers rather than detecting and reporting the bug? Sanitizers are meant to be less forgiving than production environment. Why are we trying to conceal the bug?
May 22 2017
Yes, please do this only for Darwin. These things are extremely fragile, it's nice to be able to rely at least on some things. If we change it to if, it will silently break on other platforms and we will not notice.
You can either move some code to platform* files, or merely provide a flag in platform* files and use it here.
May 10 2017
Looks fine to me. Aleksey, do you want to take a look?
May 8 2017
May 5 2017
May 3 2017
I think this now provides a solid foundation for future extensions.
May 2 2017
One more thing: now that D31592 is landed, would you mind if we do the same for Tsan?
Dmitry, with your permission I'm going to update and commit this patch as it seems to be useful regardless of which way we support sanitizers on MMU-less platforms.
May 1 2017
Apr 29 2017
One last thing. Otherwise LGTM.
Apr 27 2017
LGTM on my side, but wait for Aleksey if he has more comments
Apr 26 2017
otherwise looks good
Apr 25 2017
Apr 23 2017
Apr 22 2017
Apr 21 2017
which will be checked in due time.
We have 3 potential users for the external API (Go, Java, races on fd, potentially more in future).
I don't like that we sprinkle swift-specific bits throughout the code when the external API was meant to be general enough to cover all such cases.
We have the special tag for swift, so it really seems to me that it should be the only swift-specific bit here and the rest must be handled by the external API on general basis. We have 3 potential users for the external API (Go, Java, races on fd, potentially more in future). So I would be really interested in making the external API general and flexible enough to support all of them, rather than later sprinkle Go/Java/fd/etc-specific bits throughout the code again later.
I am not completely follow motivation for this. Is it meant for user? Or for any automated tools?
For automated tools we already have SUMMARY line. User must be proficient enough to understand what happens. And in the end this is not specific to libc, and the guilty frame is not necessary the one just above libc. Consider a race in std::vector, it's probably not std::vector code that is guilty. And this extends just to any library, consider you have race in libfoo, but it's actually your code that misuses libfoo. We don't have enough info to provide precise signal. What we provide here is a weak, potentially incorrect signal.
If we actually plan to use such configuration, does it make sense to check header for corruption when we reallocate a block (in allocate)? That will give us at least some windows for UAF detection.
Apr 20 2017
Apr 19 2017
Apr 18 2017
Ivan, do you have commit access? Or I need to land it?
I was on vacation/ooo, sorry for delays.
First of all, do you have any benchmarks? Single-threaded, multi-threaded with/without contention. How do profiles look like?
Android doesn't support thread_local
Apr 7 2017
Apr 2 2017
- s/ignore_reads_and_writes/ignore_reads_writes_and_reports/ does not look like a good idea to me, because these things are not necessary related. If we go this route we need separate flags.
- From the description it seems that your intention is to ignore all reports, but you are ignoring only deadlocks. There are many more types of reports.
Wait, the external API was meant specifically to be used from non-instrumented libraries. Quoting you in D28836:
It might not be feasible/easy to recompile the library with TSan instrumentation... The idea of this patch is to allow a non-instrumented library to call into TSan runtime.
With this change it all become pointless. We annotate non-instrumented libraries and then ignore that. What am I missing?
Mar 30 2017
When an interceptor comes from a non-instrumented library, we set thr->in_ignored_lib=true. This causes SCOPED_TSAN_INTERCEPTOR to call the real function an skip all tsan processing. Consequently mutex lock/unlock operations in non-instrumented libraries are not handled by tsan already. So it's not clear to me how you get deadlock reports there. What am I missing?
Mar 28 2017
I see that we already exclude the first page from app memory on x86_64:
static const uptr kLoAppMemBeg = 0x000000001000ull;
So I think a better check would be:
It will also check for any other bogus addresses, like random values, NULL+epsilon (&null_ptr->field) and also will not duplicate mapping logic.
Mar 27 2017
You can use -asan-instrumentation-with-call-threshold=0 for this without any changes to llvm.
Mar 26 2017
What was the bug? How did it manifest?
I somewhat afraid of breaking some existing code. Currently we consider memory around 0 to be "user memory". And in fact it's possible to mmap the first page at least on some linuxes, not sure about other OSes. And there is some effort to port sanitizers to mmu-less environment (https://reviews.llvm.org/D30583). Another possible use if you have N virtual entities and use N as address passed to acquire/release annotations (not perfect, but currently valid).
If we are excluding NULL from user memory, then I think we need to change IsAppMem to return false for NULL. That will make behavior consistent across all functions. Acquire/Release already check IsAppMem for the passed address (at least in debug build). It will also make simpler to undo this decision in some particular contexts.
Mar 24 2017
Mar 23 2017
Mar 22 2017
Tests now have full coverage of annotations.
added more tests