1450790 - ASAN use-after-poison violation in WeakCollection_finalize()

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

When starting FF in a debug ASAN build, I hit use-after-poison in WeakCollection_finalize() (it likely doesn't matter, but I'm recovering from a crash and it puts up the sessionrestore query, then crashes).

Per jan, this is because we attempt to poison the memory there after calling Poison(this, JS_FREED_HEAP_PTR_PATTERN, sizeof(*this), MemCheckKind::MakeNoAccess); in ~GCPtr()

Fallout from bug 1448589.  We don't test Debug ASAN builds on Treeherder.... :-(

Comment 1

[Jan de Mooij [:jandem]]

I do want to understand why we don't see this on shell tests or in fuzzing.

Comment 2

[Jan de Mooij [:jandem]]

FWIW we don't see this on ASan builds on Try even if I enable ~GCPtr in all builds.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> FWIW we don't see this on ASan builds on Try even if I enable ~GCPtr in all
> builds.

Actually for this to work I also have to make the #ifdef DEBUG code in WeakCollection_finalize work in opt builds. Will try that too...

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

WeakCollection_finalize implements its own poisoning in DEBUG builds, but that seems unnecessary: the fop->free_ after that will call into jemalloc which does its own poisoning.

Comment 5

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8964553 [details] [diff] [review]
Patch

Review of attachment 8964553 [details] [diff] [review]:
-----------------------------------------------------------------

The poisoning dates from bug 641025, incremental GC. jemalloc has always poisoned, I thought?

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

> The poisoning dates from bug 641025, incremental GC. jemalloc has always
> poisoned, I thought?

We don't/didn't always use jemalloc

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Steve Fink [:sfink] [:s:] from comment #5)
> The poisoning dates from bug 641025, incremental GC. jemalloc has always
> poisoned, I thought?

IGC was in 2012. We didn't enable jemalloc poisoning until bug 860254, which landed in 2014.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4329ee32328
Remove redundant poisoning in DEBUG builds in WeakCollection_finalize. r=sfink

Comment 9

[Cosmin Sabou [:CosminS]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Doesn't sound like this needs backporting, but feel free to nominate it if that's wrong.