-
Notifications
You must be signed in to change notification settings - Fork 310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Interpreter backend: Fix asan and tsan in interpreter_optimizer.hh #309
Conversation
I get a lot of compilation errors like the one below now, but I don't think it's related to this commit.
|
No, it's not related. Get the same error messages in master-dev. |
Oh, it's that cmake thing again. |
Sorry, but I don't get the point of this PR. Blocks have at least 2 instructions (see line 1154 comment in interpreter_optimzer.hh. So cur, cur+1, cur+2 are always supposed to access something valid. Or I'm missing something? |
Blocks have at least 2 instructions in line 1154, but inside the do/while loop below line 1154, you increase "cur", and then we will read past allocated memory when initialing "inst2". I don't know if this commit is the best solution here though. Perhaps it would be better to test if there can only be 1 "inst" variable, and then have the "inst2" and "inst3" variables in the else-part of the if-test. But this commit keeps the original code structure. It's easy to reproduce. Just compile faust with "-fsanitize=address" and run the interpreter backend. Asan should report memory error almost immediately. Valgrind should report it too. |
"Blocks have at least 2 instructions in line 1154, but inside the do/while loop below line 1154, you increase "cur", and then we will read past allocated memory when initialing "inst2"." So it is a false positive, since the do/while loop always runs on blocks with at least 2 instructions, then inst1 and inst2 are correct. Or I am still missing something? |
"cur" is increased inside the loop, causing inst1 to be created from memory outside the allocated memory. Asan should never report false positives. |
Still I dont get it: cur = cur_block->fInstructions.begin(); Then: so inst1 should be correct ? FBCBasicInstruction* inst2 = *(cur + 1); and 'cur + 1' is still correct (since blocks has at least 2 instructions) So 'inst2' is correct? |
Yes, inst1 is always correct.
Yes, in the first iteration of the loop, 'inst2' is always correct. But in the next iteration, 'cur' have increased it's value by at least one, and then 'inst2' might not be correct. |
Well, it's correct if it's needed. But it's not always needed, and in that case we have read past the allocated memory. |
…mory, not only in Debug mode
|
It's possible that the code is buggy, I don't know the details of what happens. But you can probably rule out #4 since asan doesn't report false positives. |
Trying to improve the code right now. |
Hmm, googling for "asan false positive" I got this hit: https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow So if one file is compiled with -fsanitize=address and another file is not, you can get false postivies for vectors. To add asan, I added |
Could not activate "asan" test here for now. Possible fix here: 3ac5872 |
It's not a false positive due to mixing asan and not-asan. abort() is called below:
(since even tsan reported an error for this, this was not really a surprise) |
Maybe this type of fix might be better:
Wouldn't c++ magically do the right thing now? |
What does "asan" says with the latest code? |
It's getting further, but there's still hits in 'rewrite'.
|
But by changing the remaning 'inst2' and 'inst3' variables to references, I get no hits:
By the way, I always use asan while developing. It only lowers performance by around 50%. |
fixed in master-dev |
Firstmost a fix for asan, but even tsan compaints (see below).
But maybe both NDEBUG and !NDEBUG should use "INST2 (*(cur+1))"? It's not so pretty reading past allocated memory, although a combination of the c compiler optimizing it out and the memory after allocation is readable anyway, probably makes it safe.