1401939 - Unaligned read for XDRAtom in HashKnownLength and PodEqual

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[André Bargull [:anba]]

Test case (reduced from js/src/jit-test/tests/latin1/assorted.js):
---
var code = cacheEntry("'\u1234';");
var ctx = {
  fileName: "",
  global: newGlobal({ cloneSingletons: true }),
};
evaluate(code, Object.create(ctx, {saveBytecode: { value: true } }));
evaluate(code, Object.create(ctx, {loadBytecode: { value: true } }));
---


UBSan reports:
---
/home/andre/hg/mozilla-inbound/js/src/vm/String.h:1195:33: runtime error: load of misaligned address 0x7f634efa4d8d for type 'const char16_t', which requires 2 byte alignment
0x7f634efa4d8d: note: pointer points here
 02 00 00 00 34 12 0b  00 00 00 ff ff ff ff 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00
             ^ 
/home/andre/hg/mozilla-inbound/js/src/build-debug-ub-opt-obj/dist/include/mozilla/HashFunctions.h:226:32: runtime error: load of misaligned address 0x7f634efa4d8d for type 'const char16_t', which requires 2 byte alignment
0x7f634efa4d8d: note: pointer points here
 02 00 00 00 34 12 0b  00 00 00 ff ff ff ff 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00
             ^ 
/home/andre/hg/mozilla-inbound/js/src/build-debug-ub-opt-obj/dist/include/mozilla/PodOperations.h:173:7: runtime error: load of misaligned address 0x7f634efa4d8d for type 'const char16_t', which requires 2 byte alignment
0x7f634efa4d8d: note: pointer points here
 02 00 00 00 34 12 0b  00 00 00 ff ff ff ff 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00
---


When adding a break point in HashKnownLength (`br mozilla::detail::HashKnownLength<char16_t> if (((unsigned)aStr)%2!=0)`), we got the following stack trace:
---
#0  0x0000000000597692 in mozilla::detail::HashKnownLength<char16_t>(char16_t const*, unsigned long) (aStr=0x7ffff69a4e8d u"ሴ\v", aLength=1)
    at /home/andre/git/mozilla-central/js/src/build-debug-master/dist/include/mozilla/HashFunctions.h:224
#1  0x00000000005957ff in mozilla::HashString(char16_t const*, unsigned long) (aStr=0x7ffff69a4e8d u"ሴ\v", aLength=1)
    at /home/andre/git/mozilla-central/js/src/build-debug-master/dist/include/mozilla/HashFunctions.h:267
#2  0x0000000000595ad5 in js::AtomHasher::Lookup::Lookup(char16_t const*, unsigned long) (this=0x7fffffffbac0, chars=0x7ffff69a4e8d u"ሴ\v", length=1)
    at /home/andre/git/mozilla-central/js/src/jsatom.h:88
#3  0x0000000000592b11 in AtomizeAndCopyChars<char16_t>(JSContext*, char16_t const*, size_t, js::PinningBehavior, mozilla::Maybe<unsigned int> const&) (cx=0x7ffff6976000, tbchars=0x7ffff69a4e8d u"ሴ\v", length=1, pin=js::DoNotPinAtom, indexValue=...) at /home/andre/git/mozilla-central/js/src/jsatom.cpp:324
#4  0x00000000005971b3 in js::AtomizeChars<char16_t>(JSContext*, char16_t const*, unsigned long, js::PinningBehavior) (cx=0x7ffff6976000, chars=0x7ffff69a4e8d u"ሴ\v", length=1, pin=js::DoNotPinAtom)
    at /home/andre/git/mozilla-central/js/src/jsatom.cpp:499
#5  0x00000000005975eb in js::XDRAtom<(js::XDRMode)1>(js::XDRState<(js::XDRMode)1>*, JS::MutableHandle<JSAtom*>) (xdr=0x7fffffffbfb0, atomp=0x0) at /home/andre/git/mozilla-central/js/src/jsatom.cpp:656
#6  0x0000000000cdc661 in js::XDRScript<(js::XDRMode)1>(js::XDRState<(js::XDRMode)1>*, JS::Handle<js::Scope*>, JS::Handle<js::ScriptSourceObject*>, JS::Handle<JSFunction*>, JS::MutableHandle<JSScript*>) (xdr=0x7fffffffbfb0, scriptEnclosingScope=0x0, sourceObjectArg=0x0, fun=0x0, scriptp=0x7ffff44c21c0) at /home/andre/git/mozilla-central/js/src/jsscript.cpp:687
#7  0x000000000109c5fa in js::XDRState<(js::XDRMode)1>::codeScript(JS::MutableHandle<JSScript*>) (this=0x7fffffffbfb0, scriptp=0x7ffff44c21c0) at /home/andre/git/mozilla-central/js/src/vm/Xdr.cpp:177
#8  0x0000000000b988ca in JS::DecodeScript(JSContext*, mozilla::Vector<unsigned char, 0ul, mozilla::MallocAllocPolicy>&, JS::MutableHandle<JSScript*>, unsigned long) (cx=0x7ffff6976000, buffer=..., scriptp=0x7ffff44c21c0, cursorIndex=0) at /home/andre/git/mozilla-central/js/src/jsapi.cpp:7568
#9  0x0000000000447605 in Evaluate(JSContext*, unsigned int, JS::Value*) (cx=0x7ffff6976000, argc=2, vp=0x7ffff4093090) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:1666
...
---


It still needs to be investigated if this unaligned read also occurs on platforms which don't support unaligned reads (for 16-bit values).

Comment 1

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

I will look at it in a few days.
If I recall correctly this was also a crash reason on Beta 55.

Comment 2

[Kris Maglione [:kmag]]

I'm not sure whether this is the cause of the crashes, but it's definitely possible.

We currently don't add any alignment padding to the preloader cache file after the header block or after the individual script XDR data on the assumption that the XDR code doesn't have any particular alignment constraints. We could enforce 4 or 8 byte alignment at the start of each XDR block, but I'd be surprised if the XDR data isn't already internally misaligned in some cases.

Comment 3

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

I suspect the alignment could be a problem on old CPU if the string copy is implemented with a memncpy, and with large enough strings to justify the use of xmm registers.

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Naveed, Nicolas, is this something we'll get to soon? We are halfway through the beta57 cycle and if we have any speculative fixes that we'd like to uplift to see if the fix works, now is the time to do it. Thanks!

Comment 5

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

I will look at it later this week.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

It looks like this might fix bug 1373934. So it might be good to have on 57.

Comment 7

[u279076]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #6)
> It looks like this might fix bug 1373934. So it might be good to have on 57.

Is this a nice-to-have for Firefox 57? If so I'd consider this firefox57:wontfix at this point in the cycle.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Yes, definitely wontfix for 57 at this point a few days before the release. 
It looks like 58 may also be affected, and we could still take a patch for 58.

Comment 9

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Attachment: Align XDR content to avoid undefined behaviours.

This patch mostly adds one function, named codeAlign, which are used as a way to
add some padding in the encoded buffers and to eat the padding in decoded
buffers.

The padding is used to enforce the alignment of the XDR content within a
TranscodeBuffer / TranscodeRange, such as we can avoid copies of long strings as
much as possible when decoding Atoms. Currently the required alignment is 2
bytes, and is defined by the XDRAlignment type.

Unfortunately, the alignment was not present, nor in the ScriptPreloader, nor in
the ScriptLoader. The ScriptPreloader append every buffers after a header which
has a odd length, which causes everything to be miss-aligned after it. The
ScriptLoader pre-fill the TranscodeBufffer with the SRI information.

In the case of the ScriptPreloader, as the initial XDR buffers are encoded
within aligned allocations, the alignment has to part of the copying script
which create a single buffer with all pre-loaded scripts. Therefore, the
XDRAlign type is used in a similar way as the XDRAlignment type within the JS
engine. The written length is being tracked to provide the proper padding.

In the case of the ScriptLoader, the encoding is made incrementally by the
linearize function, which encode all scripts within an aligned buffer. When the
linearize function copies the bytes to the TranscodeBuffer, it ensures that the
content has the proper padding, as-if it got added by codeAlign. Note,
codeScript will eat this as if it got encoded in a non-aligned buffer, as it
would appear to be when decoded.

Comment 10

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(In reply to Nicolas B. Pierron [:nbp] {backlog: ~38} from comment #9)
> Created attachment 8954066 [details] [diff] [review]
> Align XDR content to avoid undefined behaviours.

I forgot to mention that AutoXDTTree checks that we have the required alignment because AutoXDRTree is used for grafting XDR'd scripts in-place of the XDR'd lazy script.  Thus we need to ensure that the content which is being replaced has the same alignment constraints as the one which replaces it.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8954066 [details] [diff] [review]
Align XDR content to avoid undefined behaviours.

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

::: js/xpconnect/loader/ScriptPreloader.cpp
@@ +52,5 @@
>  
>  ProcessType ScriptPreloader::sProcessType;
>  
> +// This type correspond to js::vm::XDRAlignment type, which is used as a size
> +// reference for alignment of XDR buffers.

Please add a comment that the magic number version needs to be bumped any time this changes (and also bump it).

@@ +491,5 @@
>  
>          InputBuffer buf(header);
>  
> +        size_t len = data.get() - start;
> +        size_t alignLen = (sizeof(XDRAlign) - len % sizeof(XDRAlign)) % sizeof(XDRAlign);

Maybe add a helper like:

  static inline size_t
  AlignOffset(size_t len, size_t align)
  {
    return (align - len % align) % align;
  }

@@ +685,5 @@
>          MOZ_TRY(Write(fd, buf.Get(), buf.cursor()));
> +        len += buf.cursor();
> +        size_t alignLen = (sizeof(XDRAlign) - len % sizeof(XDRAlign)) % sizeof(XDRAlign);
> +        if (alignLen) {
> +            const char* alignPadding = "\0\0\0\0";

Please make this something like:

  static const char alignPadding[] = "\0\0\0";
  static_assert(sizeof(alignPadding) >= sizeof(XDRAlign));

Or just:

  static const char alignPadding[sizeof(XDRAlign)];

@@ +693,2 @@
>          for (auto script : scripts) {
> +            MOZ_ASSERT(len % sizeof(XDRAlign) == 0);

This is redundant with the offset checks above.

Comment 12

[Ted Campbell [:tcampbell]]

Comment on attachment 8954066 [details] [diff] [review]
Align XDR content to avoid undefined behaviours.

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

Largely pre-existing, but XDRAtom was already pretty magic. Can we push stuff around a little to make it more boring and like all the other XDR? Suggestings in-line.

::: js/src/vm/JSAtom.cpp
@@ +713,5 @@
>      if (mode == XDR_ENCODE) {
>          static_assert(JSString::MAX_LENGTH <= INT32_MAX, "String length must fit in 31 bits");
>          uint32_t length = atomp->length();
>          uint32_t lengthAndEncoding = (length << 1) | uint32_t(atomp->hasLatin1Chars());
>          if (!xdr->codeUint32(&lengthAndEncoding))

Pre-existing, but can we make the length encoding look like all the other XDR code.

>    bool latin1;
>    uint32_t length;
>    uint32_t lengthAndEncoding;
>
>    if (mode == XDR_ENCODE) {
>        static_assert(JSString::MAX_LENGTH <= INT32_MAX, "String length must fit in 31 bits");
>        latin1 = atomp->hasLatin1Chars();
>        length = atomp->length();
>        lengthAndEncoding = (length << 1) | uint32_t(latin1);
>    }
>
>    if (!xdr->codeUint32(&lengthAndEncoding))
>        return false;
>
>    if (mode == XDR_DECODE) {
>        length = lengthAndEncoding >> 1;
>        latin1 = lengthAndEncoding & 0x1;
>    }

@@ +720,5 @@
>          JS::AutoCheckCannotGC nogc;
> +        if (atomp->hasLatin1Chars())
> +            return xdr->codeChars(atomp->latin1Chars(nogc), length);
> +
> +        if (!xdr->codeAlign(sizeof(char16_t)))

Can we cut down to one of these calls? If we pull out the length stuff as I suggest above it should be fine. With or without the |if (!latin1)| check.

::: js/src/vm/Xdr.cpp
@@ +169,5 @@
>      TraceLoggerTextId event =
>          mode == XDR_DECODE ? TraceLogger_DecodeScript : TraceLogger_EncodeScript;
>      AutoTraceLog tl(logger, event);
>  
> +    // This should be a no-op unless the buffer the content of the script got

some extra or missing words here

Comment 13

[Ted Campbell [:tcampbell]]

Comment on attachment 8954066 [details] [diff] [review]
Align XDR content to avoid undefined behaviours.

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

Added some comments for the XdrTree stuff. I like all this stuff to deal with alignment for subtrees. The setAligned stuff is a good idea.

I gave a bunch of nit-picky comments about align masking/padding code. Be more boring :)

::: js/src/vm/Xdr.cpp
@@ +215,5 @@
>      parent_(this),
>      xdr_(xdr)
>  {
> +    // Expect sub-tree to start with the maximum alignment required.
> +    MOZ_ASSERT(xdr->isAligned(sizeof(js::XDRAlignment)));

Having the ability to maintain sub-tree alignment is nice anyways :)

::: js/src/vm/Xdr.h
@@ +257,5 @@
> +    // Alignment is required when doing memcpy of data which contains element
> +    // largers than 1 byte.
> +    bool isAligned(size_t n) override {
> +        MOZ_ASSERT(mozilla::IsPowerOfTwo(n));
> +        // If there is a failure, always assume that we are aligned.

Why are we still running code if there are errors already signaled?

@@ +267,5 @@
> +        return offset == 0 && buf.isAligned();
> +    }
> +    bool codeAlign(size_t n) {
> +        MOZ_ASSERT(mozilla::IsPowerOfTwo(n));
> +        MOZ_ASSERT_IF(mode == XDR_ENCODE, (buf.uptr() & (n - 1)) == (buf.cursor() & (n - 1)));

Factor out (n - 1) as a variable. |mask| or something.

@@ +268,5 @@
> +    }
> +    bool codeAlign(size_t n) {
> +        MOZ_ASSERT(mozilla::IsPowerOfTwo(n));
> +        MOZ_ASSERT_IF(mode == XDR_ENCODE, (buf.uptr() & (n - 1)) == (buf.cursor() & (n - 1)));
> +        size_t offset = buf.uptr() & (n - 1);

Can we compute |pad| instead of n - offset inside the later code.

@@ +274,5 @@
> +            if (mode == XDR_ENCODE) {
> +                uint8_t* ptr = buf.write(n - offset);
> +                if (!ptr)
> +                    return fail(JS::TranscodeResult_Throw);
> +                while (offset++ < n)

memset / memchr maybe? Otherwise a boring |for (size_t i = 0; i < pad; i++)|

Comment 14

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(In reply to Ted Campbell [:tcampbell] from comment #13)
> ::: js/src/vm/Xdr.h
> @@ +257,5 @@
> > +    // Alignment is required when doing memcpy of data which contains element
> > +    // largers than 1 byte.
> > +    bool isAligned(size_t n) override {
> > +        MOZ_ASSERT(mozilla::IsPowerOfTwo(n));
> > +        // If there is a failure, always assume that we are aligned.
> 
> Why are we still running code if there are errors already signaled?

The reason is that if we do not do that, then we would have to make resultCode() virtual, in order to execute it within AutoXDRTree assertions.  So I preferred to move the check in here instead.

Comment 15

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Attachment: Align XDR content to avoid undefined behaviours.

Delta:
 - Use ComputeByteAlignment in js/xpconnect/loader/ScriptPreloader.cpp, which is currently in js/src/jsutil.h and should later be moved to mfbt.
 - Add named variables for computing the alignment offset in XDR.h.
 - Use memcmp and memcpy as a unified way to implement codeAlign function.

Comment 16

[Ted Campbell [:tcampbell]]

Comment on attachment 8958111 [details] [diff] [review]
Align XDR content to avoid undefined behaviours.

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

Thanks for doing the cleanups.

::: js/src/vm/Xdr.cpp
@@ +357,5 @@
> +    // This alignment difference should be consumed by the codeAlign function
> +    // call of codeScript when decoded.
> +    size_t alignLen = sizeof(js::XDRAlignment);
> +    if (buffer.length() % alignLen) {
> +        alignLen = (alignLen - buffer.length() % alignLen) % alignLen;

ComputeByteAlignment?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/294a422d49b0
Align XDR content to avoid undefined behaviours. r=kmag,tcampbell

Comment 18

[Cristian Brindusan [:cbrindusan]]

Backed out for spidermonkey sm-arm64 test failures in /builds/worker/workspace/build/src/js/src/vm/Xdr.cpp:228:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd0be57361f30175ae02d33f900fa39ca63fa0d5
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cd0be57361f30175ae02d33f900fa39ca63fa0d5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=294a422d49b01b0b79992b527bf96b416789b9d6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=167468107&repo=mozilla-inbound&lineNumber=18101

[task 2018-03-12T18:36:54.639Z] {"action": "test_end", "extra": {"jitflags": ""}, "message": "Success", "pid": 5721, "source": "jittests", "status": "PASS", "test": "xdr/lazy.js", "thread": "main", "time": 1520879814.639187}
[task 2018-03-12T18:36:55.095Z] Assertion failure: xdr_->isAligned(sizeof(js::XDRAlignment)), at /builds/worker/workspace/build/src/js/src/vm/Xdr.cpp:228
[task 2018-03-12T18:36:55.095Z] Exit code: -11
[task 2018-03-12T18:36:55.095Z] FAIL - xdr/bug1390856.js
[task 2018-03-12T18:36:55.095Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/xdr/bug1390856.js | Assertion failure: xdr_->isAligned(sizeof(js::XDRAlignment)), at /builds/worker/workspace/build/src/js/src/vm/Xdr.cpp:228 (code -11, args "") [1.0 s]
[task 2018-03-12T18:36:55.095Z] {"action": "test_start", "pid": 5721, "source": "jittests", "test": "xdr/bug1390856.js", "thread": "main", "time": 1520879814.069023}
[task 2018-03-12T18:36:55.095Z] {"action": "test_end", "extra": {"jitflags": ""}, "message": "Assertion failure: xdr_->isAligned(sizeof(js::XDRAlignment)), at /builds/worker/workspace/build/src/js/src/vm/Xdr.cpp:228", "pid": 5721, "source": "jittests", "status": "FAIL", "test": "xdr/bug1390856.js", "thread": "main", "time": 1520879815.095135}
[task 2018-03-12T18:36:55.095Z] INFO exit-status     : -11
[task 2018-03-12T18:36:55.095Z] INFO timed-out       : False
[task 2018-03-12T18:36:55.095Z] INFO stderr         2> Assertion failure: xdr_->isAligned(sizeof(js::XDRAlignment)), at /builds/worker/workspace/build/src/js/src/vm/Xdr.cpp:228

Comment 19

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

The problem seen here is that we still need to convert cx exception into a TranscodeResult_Throw at the end, and thus we do not catch them properly when checking the assertion under AutoXDRTree destructor.

We can either remove the assertion from AutoXDRTree destructor, or to convert everything to returning a TranscodeResult_Throw.  The second option would take longer to implement.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b18239e3cd32
Align XDR content to avoid undefined behaviours. r=kmag,tcampbell

Comment 21

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(In reply to Pulsebot from comment #20)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b18239e3cd32
> Align XDR content to avoid undefined behaviours. r=kmag,tcampbell

Note, this is exactly the same patch as before, except that this time it landed with Bug 1419094's patch which should remove the reason which caused the test case failure previously (and potentially many more).

Comment 22

[Raul Gurzau (:RaulG)]

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