1352449 - JSErrorReport::initBorrowedLinebuf should be called with aligned pointer for char16_t

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Petr Sumbera]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170323105023

Steps to reproduce:

On sparc I was getting many 'check-jit-test' failures. With closer looks the tests were core dumping:

slitheen 04:34 /builds2/psumbera/userland-ff-52-tests/components/desktop/firefox/build/sparcv7/js/src: LD_LIBRARY_PATH=/builds2/psumbera/userland-ff-52/components/desktop/firefox/build/sparcv7/dist/bin /builds2/psumbera/userland-ff-52-tests/components/desktop/firefox/build/sparcv7/dist/bin/js -f /builds2/psumbera/userland-ff-52-tests/components/desktop/firefox/firefox-52.0/js/src/jit-test/lib/prologue.js --js-cache /builds2/psumbera/userland-ff-52-tests/components/desktop/firefox/firefox-52.0/js/src/jit-test/.js-cache -e 'const platform='"'"'sunos5'"'"'; const libdir='"'"'/builds2/psumbera/userland-ff-52-tests/components/desktop/firefox/firefox-52.0/js/src/jit-test/lib/'"'"'; const scriptdir='"'"'/builds2/psumbera/userland-ff-52-tests/components/desktop/firefox/firefox-52.0/js/src/jit-test/tests/parser/'"'"'' -f /builds2/psumbera/userland-ff-52-tests/components/desktop/firefox/firefox-52.0/js/src/jit-test/tests/parser/bug-896126.js
/builds2/psumbera/userland-ff-52-tests/components/desktop/firefox/firefox-52.0/js/src/jit-test/tests/parser/bug-896126.js:10:18 SyntaxError: missing ) in parenthetical:
/builds2/psumbera/userland-ff-52-tests/components/desktop/firefox/firefox-52.0/js/src/jit-test/tests/parser/bug-896126.js:10:18         return (1 for
Bus Error (core dumped)
slitheen 04:34 /builds2/psumbera/userland-ff-52-tests/components/desktop/firefox/build/sparcv7/js/src:

With this stack:

t@1 (l@1) terminated by signal BUS (invalid address alignment)
Current function is js::PrintError
  387   {
(dbx) where
current thread: t@1
=>[1] js::PrintError(cx = <bad address 0xfd05eff4>, file = <bad address 0xfd05f020>, toStringResult = CLASS, report = <bad address 0xfd05f089>, reportWarnings = <bad address 0xfd05f0c0>), line 387 in "jscntxt.cpp"
  [2] js::shell::AutoReportException::~AutoReportException(this = <bad address 0xfcc3fc0d>), line 6439 in "js.cpp"
  [3] main(argc = <bad address 0xfcc54cea>, argv = <bad address 0xfcc54d3a>, envp = <bad address 0xfcc54d7f>), line 7630 in "js.cpp"
(dbx) 

====

The issue was that:

js::PrintError() was getting odd adress below (which turned into sigbus error):

const char16_t* linebuf = report->linebuf()

After some investigation it seems that JSErrorReport::initBorrowedLinebuf should be called with aligned pointer for char16_t.

Following change works for Firefox 52 and makes all test pass on sparc:

--- firefox-52.0/js/src/jsexn.cpp.orig  2017-03-31 05:34:08.202506586 +0000
+++ firefox-52.0/js/src/jsexn.cpp       2017-03-31 06:59:30.734728725 +0000
@@ -174,8 +174,10 @@
     /*
      * The mallocSize can not overflow since it represents the sum of the
      * sizes of already allocated objects.
+     * Plus one is to allow skip one byte when the messageSize is odd because
+     * some platforms need to have char16_t* even.
      */
-    size_t mallocSize = sizeof(JSErrorReport) + messageSize + linebufSize + filenameSize;
+    size_t mallocSize = sizeof(JSErrorReport) + messageSize + linebufSize + filenameSize + 1;
     uint8_t* cursor = cx->pod_calloc<uint8_t>(mallocSize);
     if (!cursor)
         return nullptr;
@@ -190,6 +192,8 @@
     }

     if (report->linebuf()) {
+        if ((size_t)cursor % 2)
+            cursor++;
         const char16_t* linebufCopy = (const char16_t*)cursor;
         js_memcpy(cursor, report->linebuf(), linebufSize);
         cursor += linebufSize;

Comment 1

[Petr Sumbera]

Attachment: Possible patch for current tip.

The code was changed against 52 but the issue is probably still there. I expect that there must be better way. So this is to start the discussion.

Comment 2

[Tooru Fujisawa [:arai]]

Comment on attachment 8853441 [details] [diff] [review]
Possible patch for current tip.

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

good catch :D

ExtraMallocSize for JSErrorReport* calculates the size of linebuf, so we can add 1 there for alignment:
(the existing +1 is for null terminator)

> size_t
> ExtraMallocSize(JSErrorReport* report)
> {
>     if (report->linebuf())
>         return (report->linebufLength() + 1) * sizeof(char16_t);

::: js/src/jsexn.cpp
@@ +224,5 @@
>  CopyExtraData(JSContext* cx, uint8_t** cursor, JSErrorReport* copy, JSErrorReport* report)
>  {
>      if (report->linebuf()) {
> +        /* Make sure cursor is properly aligned for char16_t for platforms which need it. */
> +        if ((size_t)*cursor % 2)

size_t(*cursor) % 2

@@ +225,5 @@
>  {
>      if (report->linebuf()) {
> +        /* Make sure cursor is properly aligned for char16_t for platforms which need it. */
> +        if ((size_t)*cursor % 2)
> +            *cursor++;

(*cursor)++;

Comment 3

[Petr Sumbera]

Attachment: Updated patch based on given feedback.

Comment 4

[Petr Sumbera]

Attachment: Fixed typo in previous version...

Comment 5

[Tooru Fujisawa [:arai]]

Comment on attachment 8853890 [details] [diff] [review]
Fixed typo in previous version...

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

thanks again :)

::: js/src/jsexn.cpp
@@ +209,4 @@
>  ExtraMallocSize(JSErrorReport* report)
>  {
>      if (report->linebuf())
> +        /* Count with null terminator and alignment. */

it would be nice to say "See CopyExtraData for the details about alignment" or something like that.
(if you know better wording, feel free to change)

@@ +209,5 @@
>  ExtraMallocSize(JSErrorReport* report)
>  {
>      if (report->linebuf())
> +        /* Count with null terminator and alignment. */
> +        return (report->linebufLength() + 1) * sizeof(char16_t) + 1;

now the then-branch has 2 lines, so please add braces around the branch, like
if (...) {
  /* ... */
  return ...;
}

Comment 6

[Petr Sumbera]

Attachment: Bug1352449.patch

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8853910 [details] [diff] [review]
Bug1352449.patch

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

::: js/src/jsexn.cpp
@@ +208,5 @@
>  size_t
>  ExtraMallocSize(JSErrorReport* report)
>  {
> +    if (report->linebuf()) {
> +        /* See CopyExtraData for the details about alignment. */

Sorry, my review comment was unclear.
I meant adding "See CopyExtraData..." comment in addition to the previous comment (so that it gets clear that one +1 is for null-terminator and another +1 for alignment).

Comment 8

[Petr Sumbera]

Attachment: Bug1352449.patch

Newer mind. We are getting close. Thank you!

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8853915 [details] [diff] [review]
Bug1352449.patch

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

Thanks!

I overlooked the line length of the 2nd part of the change.

::: js/src/jsexn.cpp
@@ +229,4 @@
>  CopyExtraData(JSContext* cx, uint8_t** cursor, JSErrorReport* copy, JSErrorReport* report)
>  {
>      if (report->linebuf()) {
> +        /* Make sure cursor is properly aligned for char16_t for platforms which need it. */

please wrap this to make it fit into 80-column, like the comment in the above change.

Comment 10

[Petr Sumbera]

Attachment: Bug1352449.patch

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 8853996 [details] [diff] [review]
Bug1352449.patch

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

thank you!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c30eae661333
JSErrorReport::initBorrowedLinebuf should be called with aligned pointer for char16_t. r=arai

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Sorry, this had to be backed out for failing non-unified spidermonkey build:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8efcd39c326ad1137c4e39303fa115d26bd62e4b

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c30eae661333f5dc89809592616456faabeb4556&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=90554669&repo=mozilla-inbound

[task 2017-04-11T18:06:24.554940Z] TEST-PASS | testObjectEmulatingUndefined_truthy | ok
[task 2017-04-11T18:06:24.554955Z] testNewContext
[task 2017-04-11T18:06:27.215758Z] Test new context: startAssertion failure: cursor == (uint8_t*)copy + mallocSize, at /home/worker/workspace/build/src/js/src/jsexn.cpp:326
[task 2017-04-11T18:06:27.335264Z] PROCESS-CRASH | jsapi-tests | application crashed
[task 2017-04-11T18:06:27.335303Z] Return code: -11

Please fix the issue and submit an updated patch. Thank you.

Comment 14

[Petr Sumbera]

I'm sorry. Can you please guide me how to reproduce the failure? So far I have tried to build js and then ran 'make check-jit-test'.

I don't know anything about spidermonkey. Any link to documentation?

Comment 15

[Tooru Fujisawa [:arai]]

you need to build "debug build" of SpiderMonkey shell, that enables assertions (MOZ_ASSERT).
for SpiderMonkey, you can build it by passing "--enable-debug" to configure.

then, the failure happens while running jit-test, like js/src/jit-test/tests/arguments/defaults-invalid-syntax.js

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c30eae661333f5dc89809592616456faabeb4556&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=90560004

you can run jit-test by executing the following command in js/src/

  ./jit-test/jit_test.py ${BUILDDIR}/dist/bin/js arguments/defaults-invalid-syntax.js

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation#Test


then, the assertion that fails is here:
https://dxr.mozilla.org/mozilla-central/rev/a374c35469935a874fefe64d3e07003fc5bc8884/js/src/jsexn.cpp#314

so, it expects the cursor to be at the end of the buffer,
but now we may leave single byte at the end of buffer, when the alignment is valid without incrementing it in CopyExtraData.

So, if we don't increment *cursor at the top of CopyExtraData, we should increment *cursor at the end of CopyExtraData,
to make it match to ExtraMallocSize.

Comment 16

[Petr Sumbera]

Attachment: Bug1352449.patch

Comment 17

[Tooru Fujisawa [:arai]]

Comment on attachment 8860357 [details] [diff] [review]
Bug1352449.patch

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

Looks good.
thank you for addressing the issue!

here's try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aeae8785d9082400c52245f90e3fbc13418647cb

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6f60edd4d0
JSErrorReport::initBorrowedLinebuf should be called with aligned pointer for char16_t. r=arai

Comment 19

[Iris Hsiao [:ihsiao]]

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