- User Since
- Dec 6 2012, 2:31 AM (228 w, 5 d)
Sun, Apr 23
Sat, Apr 22
Fri, Apr 21
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.
Thu, Apr 20
Wed, Apr 19
Tue, Apr 18
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
Fri, Apr 7
Sun, Apr 2
- 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?
Thu, Mar 30
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?
Tue, Mar 28
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.
Mon, Mar 27
You can use -asan-instrumentation-with-call-threshold=0 for this without any changes to llvm.
Sun, Mar 26
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
Mar 21 2017
addressed review comments
Mar 17 2017
I still need to look at this with fresh head next week, but it is ready for first review pass.
Need to add more tests. It's not that I did not test, I've worked out interface and tested on private programs.
Mar 13 2017
Few suggestions if you want to upstream this:
- remove unchanged files, unnecessary ifdefs and todo's
- start with just asan (tsan is more complex)
- use the existing asan compiler instrumentation that inserts function calls, and do the rest in runtime (at least in the first version, this will greatly simplify the change)
Mar 8 2017
Feb 16 2017
Feb 9 2017
LGTM if the comment is moved
Kuba, this is a prerequisite for lsan on mac.
Do you have commit access? Or you want me to commit after the comment move?
Since we spent so much time discussion this, I guess you need to leave some traces in comments for future generations. Document what's CheckLocked contract and why it is that way.
Feb 8 2017
The contract for CheckLocked is: CheckLocked does nothing if the current thread holds the mutex, otherwise behavior is undefined.
And, yes, we are currently abusing the sloppy check in BlockingMutex on linux in CheckForLeaks.
Feb 7 2017
This adds 5 calls and 3 syscalls per rw mutex lock/unlock. What's the performance effect of this?
Spin mutex can behave very poorly on contention, and thread registry can be contended. I suspect blocking mutex is there on purpose.
Sharing a mutex access across threads like this is not supported by blocking mutex
Feb 1 2017
Now this also improves asan.
Jan 27 2017
Jan 25 2017
Jan 24 2017
I do already have a buy in from 2 major libraries on our side and we're in the talks with a 3rd library. The library owners did preliminary performance tests and they are fine with inserting those callbacks. Sorry for being vague here :)
We can't ship those libraries TSanified (performance, security) nor can we maintain interceptors for them (too many APIs, changing APIs, ...). At this point, it's not clear which libraries will adopt this
Are they meant to be called from instrumented code or from non-instrumented code?
Do you plan to this functionality? Where? How?
Did not notice this before somehow. Sorry.
Jan 12 2017
One way to make this work on Darwin is by using the __asm trick:
Jan 11 2017
Jan 10 2017
Jan 7 2017
Submitted in r291346.