827150 - Add Valgrind and ASan annotations to the pres arena

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

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

[Mats Palmgren (inactive)]

Attachment: fix

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).

Comment 2

[Mats Palmgren (inactive)]

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).

Comment 3

[Christian Holler (:decoder)]

Does this affect debug builds only, or also optimized builds?

Comment 4

[Mats Palmgren (inactive)]

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

Both.

Comment 6

[Christian Holler (:decoder)]

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 7

[Mats Palmgren (inactive)]

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 9

[Christian Holler (:decoder)]

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 :)

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 11

[Christian Holler (:decoder)]

Jesse mentioned on IRC that all bugs uncovered by this should be equivalent to problems mitigated by frame poisoning. Is that correct/incorrect?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 13

[Mats Palmgren (inactive)]

(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

[Jesse Ruderman]

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

[Mats Palmgren (inactive)]

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

[Mats Palmgren (inactive)]

Jesse, did fuzzing this patch uncover any new bugs?

Comment 17

[Jesse Ruderman]

No.  So far, it hasn't caused any new crash signatures to appear.

Comment 18

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2f1089d6cd4

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b2f1089d6cd4

Comment 20

[Jesse Ruderman]

This helped find bug 840480 \o/