1562187 - Some asan checks are disabled when nursery poisoning is disabled.

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P3)

Description

[Paul Bone [:pbone]]

After the nursery is evicted we poison the nursery

https://searchfox.org/mozilla-central/source/js/src/gc/Nursery.cpp#87

But only in zeal, debug and nightly builds:

https://searchfox.org/mozilla-central/source/js/src/gc/Nursery.cpp#1125

That poisoning call does:

https://searchfox.org/mozilla-central/source/js/src/jsutil.h#314-321

Which sets the memory to NOACCESS for asan and valgrind, but that's not
executed if poisoning is disabled at runtime or this code is not compiled
in. So it looks as if we're missing that on SM(asan) builds (from what you
said above) but it runs in debug builds of the browser.

---

Info from Sfink:

The SM(asan) build is an address sanitizer build (heh, the American
spelling has infected the command-line option) which is non-debug,
optimized. And it runs on beta and release, so I think that means it
runs without full poisoning.

There are also full-browser asan builds, both debug and opt varieties
(which should really be called debug and non-debug; I think both have
optimization enabled). They also run on all trees, and the release tree
at least doesn't do most poisoning for opt builds.

Comment 1

[Jan de Mooij [:jandem]]

We could add a SetMemCheckKind call to a new #elif branch in Poison and DebugPoison.

I added the ASan/Valgrind instrumentation to Poison, mostly for fuzzing and that's done almost exclusively on Nightly. I don't know how important it is to separate these two, but it probably won't hurt anyway.

Comment 2

[Paul Bone [:pbone]]

(In reply to Jan de Mooij [:jandem] from comment #1)

We could add a SetMemCheckKind call to a new #elif branch in Poison and DebugPoison.

I added the ASan/Valgrind instrumentation to Poison, mostly for fuzzing and that's done almost exclusively on Nightly. I don't know how important it is to separate these two, but it probably won't hurt anyway.

Mmm, I thought that would be the case. It seems that it is a valid way to build the software which is why I looked at it.

I have a patch that adds that #elif case to the poisoning code in Nursery.cpp, but I think your suggestion of putting it in Poison and DebugPoison would be good. Although I found at least one case where the calls to these are #ifdef'd out on non-debug non-nightly. So we'd need to check all the uses.

Thanks.

Comment 3

[Paul Bone [:pbone]]

Attachment: Bug 1562187 - (part 1) All paths through Poison code make asan/valgrind calls r=jandem

Comment 4

[Paul Bone [:pbone]]

Attachment: Bug 1562187 - (part 2) Use correct ifdef r=jandem

Depends on D37359

Comment 5

[Paul Bone [:pbone]]

Attachment: Bug 1562187 - (part 3) Remove ifdefs from around Poison calls in Nursery.cpp r=jandem

Depends on D37360

Comment 6

[Paul Bone [:pbone]]

Attachment: Bug 1562187 - (part 4) Move an assertion to it makes more sense r=jandem

Move this assertion so it's nearer to where its assuption is used.

Depends on D37361

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ced2071dc865
(part 1) All paths through Poison code make asan/valgrind calls r=jandem
https://hg.mozilla.org/integration/autoland/rev/00c4d4a19d85
(part 2) Use correct ifdef r=jandem
https://hg.mozilla.org/integration/autoland/rev/8eb98866eb23
(part 3) Remove ifdefs from around Poison calls in Nursery.cpp r=jandem
https://hg.mozilla.org/integration/autoland/rev/41dcaf3a1818
(part 4) Move an assertion to it makes more sense r=jandem

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/ced2071dc865
https://hg.mozilla.org/mozilla-central/rev/00c4d4a19d85
https://hg.mozilla.org/mozilla-central/rev/8eb98866eb23
https://hg.mozilla.org/mozilla-central/rev/41dcaf3a1818