Add Valgrind and ASan annotations to the pres arena

RESOLVED FIXED in Firefox 21



5 years ago
4 years ago


(Reporter: mats, Assigned: mats)




Firefox Tracking Flags

(firefox21 fixed, firefox-esr17 unaffected, b2g18 unaffected)


(Whiteboard: [adv-main21-])


(1 attachment)



5 years ago
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

Comment 1

5 years ago
Created attachment 698469 [details] [diff] [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

Comment 2

5 years ago
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?

Comment 4

5 years ago
(In reply to Christian Holler (:decoder) from comment #3)
> Does this affect debug builds only, or also optimized builds?



5 years ago
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 7

5 years ago
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+


5 years ago
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+

Comment 13

5 years ago
(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.

Comment 14

5 years ago
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)

Comment 15

5 years ago
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.

Comment 16

5 years ago
Jesse, did fuzzing this patch uncover any new bugs?
Flags: needinfo?(jruderman)

Comment 17

5 years ago
No.  So far, it hasn't caused any new crash signatures to appear.
Flags: needinfo?(jruderman)
status-firefox21: --- → fixed
Target Milestone: --- → mozilla21


5 years ago
Last Resolved: 5 years ago
Resolution: --- → FIXED
status-b2g18: --- → unaffected
status-firefox-esr17: --- → unaffected


5 years ago
Depends on: 840480

Comment 20

5 years ago
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.