Closed Bug 827150 Opened 10 years ago Closed 10 years ago
Add Valgrind and ASan annotations to the pres arena
Currently we don't detect read access of objects in the pres arena free lists, which allows bugs like bug 824641 and bug 824643 to go undetected unless you disable the arena with -DDEBUG_TRACEMALLOC_PRESARENA
When putting an object on a free list mark it as "deallocated". When recycling such an object in Allocate mark it as "allocated but uninitialized" (do the same before deallocating the whole arena to avoid making it look like a double free when PLArena free up the memory).
decoder, could you try the wip patch in your non-Linux ASan builds please? Fwiw, it seems to work for me on Linux64 using Clang 3.3 (trunk 169730).
Does this affect debug builds only, or also optimized builds?
(In reply to Christian Holler (:decoder) from comment #3) > Does this affect debug builds only, or also optimized builds? Both.
Here's an OSX debug build with the patch: https://tbpl.mozilla.org/?tree=Try&rev=1919a579d2f3 (Don't mind the red and busted, that's normal ;)) I also created Linux builds and they seem to work just fine. Thanks for writing this :)
Comment on attachment 698469 [details] [diff] [review] fix I don't know what to do about the asan_interface.h header file issue... I think declaring the two ASan functions we need locally is acceptable for now.
Comment on attachment 698469 [details] [diff] [review] fix Not sure about the header problem, but I think that the solution you picked here is reasonable for now and I guess it's unlikely that these signatures will rapidly change. I cannot review anything beyond that, because I'm not a developer for this code, but what I've seen looks ok to me :)
Attachment #698469 - Flags: review?(choller) → review+
I think we should definitely have some fuzz around this before landing. It could save us a lot of money in bug bounties. In fact, I think we should mark this bug security-sensitive for now. Doing so.
Jesse mentioned on IRC that all bugs uncovered by this should be equivalent to problems mitigated by frame poisoning. Is that correct/incorrect?
Comment on attachment 698469 [details] [diff] [review] fix Review of attachment 698469 [details] [diff] [review]: ----------------------------------------------------------------- Please do not land this until Jesse or someone else has had a chance to fuzz with it. I expect that many or most of the bugs we find with this will be mitigated by frame poisoning, but we shall see. ::: layout/base/nsPresArena.cpp @@ +302,5 @@ > PR_CallOnce(&ARENA_POISON_guard, ARENA_POISON_init); > } > > +#if defined(MOZ_ASAN) || defined(MOZ_VALGRIND) > + static PLDHashOperator UnPoisonFreeList(FreeList* aEntry, void*) Unpoison is a single word
Attachment #698469 - Flags: review?(roc) → review+
(In reply to Christian Holler (:decoder) from comment #11) > Jesse mentioned on IRC that all bugs uncovered by this should be equivalent > to problems mitigated by frame poisoning. Is that correct/incorrect? I don't know what he means by that. "Mitigated by frame poisoning" needs to be evaluated for each bug separately. This patch makes ASan/Valgrind catch all such errors, rather than relying on the mitigation which catches most but not all errors. For example, here's two bugs that would have been caught by this patch: bug 824641 and bug 824643.
I've applied this patch on the Mac Mini I use for ASan testing. But layout is a mess right now, so I expect to have trouble interpreting the results. The mess includes: * CSS transform bugs (bug 830192, bug 830299, and the likelihood of other regressions from bug 827577) * CSS columns bugs (bug 600100, bug 743364, and the likelihood of regressions and uncovered bugs from bug 588237)
Thanks Jesse. I think roc's fix in bug 830192 will fix the recent transform + position:fixed crashes. I have a fix for some columns related crashes which I'll attach soon in bug 812893. So things should look better in a few days I hope.
Jesse, did fuzzing this patch uncover any new bugs?
No. So far, it hasn't caused any new crash signatures to appear.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
This helped find bug 840480 \o/
You need to log in before you can comment on or make changes to this bug.