Closed Bug 1058500 Opened 5 years ago Closed 5 years ago

Support MSan (MemorySanitizer) in the JS shell


(Core :: JavaScript Engine, defect)

Not set





(Reporter: decoder, Assigned: decoder)


(Blocks 1 open bug, )


(Keywords: sec-want)


(2 files)

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

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+
Comment on attachment 8479951 [details] [diff] [review]

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]

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 :)
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.