Closed Bug 827150 Opened 11 years ago Closed 11 years ago

Add Valgrind and ASan annotations to the pres arena


(Core :: Layout, defect)

Not set



Tracking Status
firefox21 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected


(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)



(Keywords: sec-want, Whiteboard: [adv-main21-])


(1 file)

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
Attached patch fixSplinter Review
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
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?

Keywords: sec-want
Here's an OSX debug build with the patch:

(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]

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.
Attachment #698469 - Attachment description: wip → fix
Attachment #698469 - Flags: review?(choller)
Comment on attachment 698469 [details] [diff] [review]

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+
Attachment #698469 - Flags: review?(roc)
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.
Group: core-security
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]

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?
Flags: needinfo?(jruderman)
No.  So far, it hasn't caused any new crash signatures to appear.
Flags: needinfo?(jruderman)
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 840480
This helped find bug 840480 \o/
Whiteboard: [adv-main21-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.