Support MSan (MemorySanitizer) in the JS shell

RESOLVED FIXED in mozilla35

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: decoder, Assigned: decoder)

Tracking

(Blocks: 1 bug, {sec-want})

Trunk
mozilla35
x86_64
Linux
sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
The JS shell is a low hanging fruit for getting MSan (MemorySanitizer) to work, because it has only one external library (zlib) that we need to take care of. I managed to get that working even without recompiling zlib by marking the memory we pass to zlib as initialized (MSan won't see the initialization in zlib happening).

I'll attach a patch here soon (it will also need build peer review since we need to add generic MSan support to the build system).

Together with bug 1056233, this will allow for a JS shell build (with --disable-ion) that also passes at least the jit-tests, I haven't tried more.

Once we have our ARM64 simulator working, we can even test parts of the JIT then.
(Assignee)

Comment 1

3 years ago
Created attachment 8479951 [details] [diff] [review]
jsshell-msan-build.patch

Build system support for MSan.

This adds --enable-memory-sanitizer similar to --enable-address-sanitizer.

Furthermore, it adds the basic poison/unpoison functions to mfbt/MemoryChecking.h. I tried making the three MOZ_MAKE_MEM_* macros useful as well under MSan. Since MSan and ASan can never be used together, it does not make sense to provide a combined interface. With this interface, working with NOACCESS or UNDEFINED memory will cause MSan to report an error.

Ted, can you review this in one, or does the mfbt part need additional review?
Attachment #8479951 - Flags: review?(ted)
(Assignee)

Comment 2

3 years ago
Created attachment 8479953 [details] [diff] [review]
jsshell-msan-zlib.patch

This patch marks memory that we pass to zlib as initialized. With this patch, we don't need to build zlib with MSan, initial tests show that this works fine :)

The downside is of course that we won't see if zlib really initializes all our memory, but that's a fair trade-off for making the build/use easier I guess.
Attachment #8479953 - Flags: review?(jorendorff)

Comment 3

3 years ago
(In reply to Christian Holler (:decoder) from comment #2)
> Created attachment 8479953 [details] [diff] [review]
> jsshell-msan-zlib.patch
> 
> This patch marks memory that we pass to zlib as initialized. With this
> patch, we don't need to build zlib with MSan, initial tests show that this
> works fine :)

Note that this approach is not guaranteed to prevent spurious reports from a non-instrumented DSO. E.g. some code in zlib may allocate memory, write to it and then pass it to a libc call. MSan will not register the write (because zlib is not instrumented), but will intercept the libc call and bark at the "uninitialized" argument.
(Assignee)

Comment 4

3 years ago
Thanks for pointing that out. I didn't think about that. I think that for the particular functions we use in zlib, this won't happen though (I guess). We just compress/decompress our source code, that's all.
Comment on attachment 8479951 [details] [diff] [review]
jsshell-msan-build.patch

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

You need someone else to review the MFBT changes. The build changes are fine.
Attachment #8479951 - Flags: review?(ted) → review+
(Assignee)

Comment 6

3 years ago
Comment on attachment 8479951 [details] [diff] [review]
jsshell-msan-build.patch

Waldo, can you look at these easy MFBT additions for MSan? Thanks!
Attachment #8479951 - Flags: review?(jwalden+bmo)
Attachment #8479953 - Flags: review?(jorendorff) → review+
Attachment #8479951 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8479953 [details] [diff] [review]
jsshell-msan-zlib.patch

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

::: js/src/vm/Compression.cpp
@@ +106,5 @@
>  js::DecompressString(const unsigned char *inp, size_t inplen, unsigned char *out, size_t outlen)
>  {
>      JS_ASSERT(inplen <= UINT32_MAX);
> +
> +    // Mark the memory we pass to zlib as initialized for MSan

'.' at end of sentence, please :)
(Assignee)

Comment 8

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/12cc55488b9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4e8253d215d
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/12cc55488b9c
https://hg.mozilla.org/mozilla-central/rev/b4e8253d215d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.