User Details
- User Since
- Feb 25 2014, 6:35 AM (435 w, 1 d)
Feb 7 2020
Thanks for the reviews! I do not have commit access, so please merge this whenever you can.
Jan 31 2020
Thank you for the answers.
Jan 30 2020
Dec 18 2019
Indeed, I'm closing this then, I haven't tested your patch yet but the fix seems to be the exact same. You seem to have a better idea of what you are doing and your patch reflects that ^^
Dec 16 2019
Nov 26 2016
No, D23765 seems to fix this issue.
Nov 25 2016
I got what "shadows" means, this patch can handle a UsingDecl with multiple shadows.
Nov 24 2016
I'm new to this and I'm not sure at all this is the correct way to fix it. Any input is welcome :)
Sep 16 2016
Sep 15 2016
Seems fine to me, but I think you forgot to update the tests :)
Jun 21 2016
I don't have a windows machine at hand, I didn't test this commit.
I also don't understand why it is needed for these new functions and not for some other functions like __sanitizer_print_memory_profile.
I guess I should add INTERFACE_FUNCTION(__sanitizer_*_switch_fiber) to that file. Should I open a new review on phabricator?
Thank you for your time reviewing this! :)
I added some documentation in common_interface_defs.h.
No, in my library, when a fiber returns, it is completely destroyed. And we never kill a fiber from outside before it is finished.
This use-case came to my mind but I thought it wasn't a real world use-case, I was wrong ^^
Jun 20 2016
Correct. But it's not that bad anyway I think. If the fake stack is there to find use-after-return errors, when the fiber entry function returns, it means the fiber is dead, so there is no real need to track those anymore. It's like a thread dying, and in both cases (thread or fiber) the fake stack is destroyed and it won't detect use-after-returns as such.
I think it is fine. The guideline for manual annotations should then recommend to introduce an additional function that merely finishes the switch and calls real non-inlinable fiber body. Worth noting in __sanitizer_start_switch_fiber comment, and, yes, it needs some comment.
Yes, I was planning on writing a few lines of documentation when we all agree on the interface.
I don't see any new test files. Did you forget to add a file?
I didn't add it. The test proposed by @filcab is based on printfs inside the asan library to show when fake frames are collected. We couldn't write an automatic test for that.
Jun 19 2016
I think I got how fake stacks work. I implemented your idea @filcab, and with your test code, the FakeFrame is not collected anymore.
Jun 14 2016
Hm... shouldn't I fix @filcab's issue with FakeStack before?
Wow, thanks for taking the time to explain all this ^^
I have a few questions about it.
Jun 13 2016
Here's what's new:
- replaced warnings by asserts when calling start/finish_switch_fiber in wrong order
- simplified code with only one atomic as suggested by @dvyukov
- clang-formatted the two test files
- removed the useless x/s argument from the test, the signal now always triggers
- replaced ThrowAndCatch by a call to a noreturn function that will longjmp back
- replaced some exit(0) by exit(1) in the test
- loop 30 times instead of doing a single run
setjmp/longjmp seems to work, the test is now correctly passing. The only thing is that I loop only 30 times and it already incurs a 1s total overhead (4 compilations at all optimization levels) on my machine, quite far from the 10000 you wished for. 100 iterations gives 5s overhead, ~60s for 1000 iterations.
@dvyukov I wrote that loop to run the tests longer while the signal triggers, and I have found... a deadlock :(
The test is in this diff, the file named swapcontext_use_after_return.cc. I didn't write any implementation as you can see and the test passes on my machine. I don't know how the fakestack mechanism works, so I'm not sure what problem I should be trying to solve by adding the void** argument to __asan_start_switch_fiber().
Yes, I didn't add non-portable code to the library itself. I said that because it forces users to write non-portable code to annotate their code, but it's acceptable.
Jun 12 2016
I changed the interface to what @filcab suggested. Another downside is that getting the current thread's stack bounds is done through a non-portable pthread API, but it does simplify asan code.
Jun 11 2016
Jun 10 2016
My implementation does not know about the thread's stack bounds. It could ask them through pthread and store them in some thread_local storage to avoid asking them each time.
Jun 9 2016
Oh, that what they are used for! I think what you said could work, I must give it more thought.
My understanding is that we're calling __sanitizer_*_enter_fiber on an anything->fiber transition (be it the first one into that fiber or any other), and calling __sanitizer_*_exit_fiber on the *last* (for a given fiber) fiber->anything transition.
Let me know if that's not the case. And if it's not, why it's not generalized like that.
No, the second part is not exact (or I'm misunderstanding you). __sanitizer_*_exit_fiber are used when returning from the fiber to the thread's stack. The fiber may be re-entered later.
To be clear:
- you call enter_fiber when you are about to switch to a fiber
- you call exit_fiber when you are about to switch back to the thread's stack
These are not related to the fiber's lifetime.
As for the stack bounds: Would it be a problem to figure them out dinamycally in __sanitizer_finish_enter_fiber? Possibly with a callback from the fiber library, since we need to know if a fiber is running (Our system's fiber library has a function that queries TLS and tells you if you're running in a fiber. I have no idea about other implementations).
I'm not sure I understand. You want to avoid giving the stack bounds as arguments to __sanitizer_start_enter_fiber? We actually need to know the stack bounds we are switching to beforehand, in case a signal handler runs on the fiber, before __sanitizer_finish_enter_fiber is called.
Jun 7 2016
I found one of these tests you are talking about in tsan/signal_sync.cc. It seems to trigger a signal every 10 microseconds, but even with that, isn't the probability that the signal triggers right in between __sanitizer_enter() and swapcontext() very low? I think it would be useful to add such a test, but not replace the current one by it, as the current one is deterministic and tests all tricky cases (except the signals).
Because then I will not be able to write something like
Jun 6 2016
I added support for switching from one fiber to another and added that to the unit test. I also added a few ThrowAndCatch calls there to try to trigger the warning that this patch fixes in __asan_handle_no_return.
Jun 5 2016
Ok, I think I did everything you asked for, except for the interception of the swapcontext calls, I will come back at it later.
Jun 3 2016
You generally can do that by taking an address of any local variable.
If we are looking for the stack of the current thread, yes. But there are functions that need to know the stack of other threads, like GetThreadRangesLocked. It is the only such function I have found but I don't know if there are others.
If we express the annotations as start_switch/fnish_switch, then we have more flexibility to handle it. First, we can handle signal masks internally. Second, we can remember both old and new stacks for the duration of the switch, and then check both in __asan_handle_no_return. This will eliminate the need to mess with signal masks.
Oh, I see!
I think this is easy to support, we can remove the warning about entering a fiber twice, then, for example, you could go main -> enter_fiber -> fiber 1 -> enter_fiber -> fiber 2 -> exit_fiber -> main.
I wonder if it can interfere with signals (without altstack). Namely, a signal arrives when asan has setup fiber stack, but the actual switch did not happen (SP is still pointing to the old stack). Then signal handler executes something that causes __asan_handle_no_return. Seems that we can get the same crash. Need tests.
I didn't think about that. I think you are right, it will cause the warning in __asan_handle_no_return and the false positives afterwards. However, I don't know how we could avoid that... The user doing the annotation could take care of that by deferring signals between the enter_fiber and the switch to the new stack. Do you have any other idea?
I use boost::context as you seem to have guessed
We should intercept and annotate at least ucontext API, so that applications using ucontext work out of the box. It will also simplify testing and make it more realistic, as we will just need to write a real program using ucontext.
So you mean redefining makecontext and swapcontext in compiler-rt, annotate them and call libc's implementation after that? Is there an example of such a code for another api?
Unfortunately, boost::context uses own assembly to implement coroutines, so we still need to expose annotations. We can talk to boost maintainers to add annotations directly to boost::context.
I can try to make them a patch for that when I'm done with this. For the moment I added the annotations around boost::context calls.
Jun 2 2016
Aug 27 2015
Thank you!
Oh I see! Indeed, I don't have commit rights, so I would help me if you could do it :)
Sorry, I'm not sure I got what is the workflow on this platform. This patch is now "accepted", but do I need to do something else here? The only relevant action I can do seems to be "close revision", should I do that?
Jul 14 2015
May 9 2015
Do not include pthread.h and sched.h when threads are disabled