New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
link

Issue 875322: Function Signature Mismatch Error When Using Dynamic Linking for WebAssembly

Reported by knightac...@gmail.com, Aug 17

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.84 Safari/537.36

Steps to reproduce the problem:
1. Unzip testcase.zip
2. Start a http server
3. Open application in Chrome e.g. http://localhost:8080
4. Observe function signature mismatch runtime error

What is the expected behavior?
There should be no error and the line "start-:test" should be printed out. This is not reproducible in Firefox.

What went wrong?
Not sure. 

Did this work before? No 

Does this work in other browsers? Yes

Chrome version: 68.0.3440.84  Channel: stable
OS Version: 10.0
Flash Version:
 

Comment 1 by dschuff@chromium.org, Aug 17

Cc: jgravelle@chromium.org sbc@chromium.org
Components: Blink>JavaScript>WebAssembly
+cc sbc,jgravelle in case it's a tool bug instead of an engine bug.

Comment 2 by knightac...@gmail.com, Aug 17

testcase.zip
1.8 MB Download

Comment 3 by knightac...@gmail.com, Aug 17

I have uploaded my test case again.

Comment 4 by susan.boorgula@chromium.org, Aug 19

Labels: Needs-Triage-M68

Comment 5 by titzer@chromium.org, Aug 20

Owner: titzer@chromium.org
Status: Assigned (was: Unconfirmed)
I can reproduce this in ToT Chrome.
It throws a function signature mismatch error which is visible in the devtools console.
Firefox nightly does not exhibit the problem.
Investigating.

Comment 6 by titzer@chromium.org, Aug 29

Made some progress debugging this, but haven't root caused it yet.

The application does have some kind of application-level error (which is fine). This manifests in the console message:

"(index):1237 __pad_and_output:__op is null!!!"

I have verified that v8's 3 execution tiers (interpreter, liftoff, and turbofan), as well as Firefox all execute the program up to the error exactly the same afaict (same sequences to calls of imports, same memory checksum).

When the application enters its own error handling code, divergance occurs. Firefox continues to the end of the program and prints a couple more lines, whereas V8's execution tiers run into errors and disagree (turbofan and liftoff throw the signature mismatch and the interpreter throws memory index out of bounds).

Given that the memory state does not diverge before the error handling code, I suspect it might be our implementation of (mutable) globals, so I am looking into that now.

Comment 7 by titzer@chromium.org, Aug 30

Cc: knightac...@gmail.com kevin.ch...@autodesk.com
Labels: Restrict-View-Google Security_Severity-High Security_Impact-Stable

Comment 8 by titzer@chromium.org, Aug 30

Cc: -sbc@chromium.org -jgravelle@chromium.org
Labels: -Via-Wizard-API -Needs-Triage-M68 Restrict-View-SecurityNotify

Comment 9 by bugdroid1@chromium.org, Aug 30

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/09a717dbb9666231bc09ed6740f2270fe79f45b0

commit 09a717dbb9666231bc09ed6740f2270fe79f45b0
Author: Ben L. Titzer <titzer@chromium.org>
Date: Thu Aug 30 15:54:14 2018

[wasm] Fix dispatch table instance update

This CL fixes a bug where the receiving instance was updated improperly
in the dispatch table(s) of an imported table.

BUG= chromium:875322 
R=mstarzinger@chromium.org

Change-Id: Ib5af238a0847bf332a12863523e897f59f137c1d
Reviewed-on: https://chromium-review.googlesource.com/1196886
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Ben Titzer <titzer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55534}
[modify] https://crrev.com/09a717dbb9666231bc09ed6740f2270fe79f45b0/src/wasm/module-compiler.cc
[add] https://crrev.com/09a717dbb9666231bc09ed6740f2270fe79f45b0/test/mjsunit/wasm/import-table.js

Comment 10 by titzer@chromium.org, Aug 30

Status: Fixed (was: Assigned)

Comment 11 by titzer@chromium.org, Aug 30

Labels: Merge-Request-70 Merge-Request-69
Requesting backmerge to 69 and 70.

Comment 12 by sheriffbot@chromium.org, Aug 30

Project Member
Labels: -Merge-Request-70 Merge-Review-70 Hotlist-Merge-Review
This bug requires manual review: We don't branch M70 until 2018-08-30.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 13 by sheriffbot@chromium.org, Aug 30

Project Member
Labels: -Merge-Request-69 Merge-Review-69
This bug requires manual review: We are only 4 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 14 by gov...@chromium.org, Aug 30

Cc: awhalley@chromium.org mbarbe...@chromium.org infe...@chromium.org
Cl listed at #9 landed 3 hrs back, not in canary yet. We're cutting M69 stable RC today.

+inferno@ for M69 merge review as awhalley@ is OOO.

Comment 15 by infe...@chromium.org, Aug 30

Bug needs to get backed on canary first, lets wait on m69 respin for this.

Comment 16 by gov...@chromium.org, Aug 30

Labels: -Type-Bug Type-Bug-Security
Sure, thank you inferno@.

Comment 17 by sheriffbot@chromium.org, Aug 31

Project Member
Labels: M-68 Target-68

Comment 18 by sheriffbot@chromium.org, Aug 31

Project Member
Labels: -Pri-2 Pri-1

Comment 19 by abdulsyed@google.com, Sep 4

how do results look in canary?

Comment 20 by knightac...@gmail.com, Sep 5

I just tried dynamic linking and its working on Version 71.0.3542.0 (Official Build) canary (64-bit). Great job!

When can we get this fix onto Chrome production?

Comment 21 by titzer@chromium.org, Sep 5

I had a look at crash reports, and everything looks normal with yesterday's canary.

Comment 22 by abdulsyed@google.com, Sep 5

Labels: -Merge-Review-70 Merge-Approved-70
approved - branch:3538

Comment 23 by awhalley@chromium.org, Sep 5

Labels: reward-topanel

Comment 24 by titzer@chromium.org, Sep 6

This needs to be merged to 69 as well.

Comment 25 by bugdroid1@chromium.org, Sep 6

Project Member
Labels: merge-merged-7.0
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/d67885e6da3d65846a6553e6897a5aa9b30c1150

commit d67885e6da3d65846a6553e6897a5aa9b30c1150
Author: Ben L. Titzer <titzer@chromium.org>
Date: Thu Sep 06 07:39:20 2018

Merged: [wasm] Fix dispatch table instance update

This CL fixes a bug where the receiving instance was updated improperly
in the dispatch table(s) of an imported table.

BUG= chromium:875322 
R=​mstarzinger@chromium.org
CC=titzer@chromium.org

Change-Id: I2c1ca840147b1d315abe6d9237822511a1cd826b
Originally-reviewed-on: https://chromium-review.googlesource.com/1196886
No-Try: true
No-Presubmit: true
No-Treechecks: true
Reviewed-on: https://chromium-review.googlesource.com/1208573
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/branch-heads/7.0@{#7}
Cr-Branched-From: 6e2adae6f7f8e891cfd01f3280482b20590427a6-refs/heads/7.0.276@{#1}
Cr-Branched-From: bc08a8624cbbea7a2d30071472bc73ad9544eadf-refs/heads/master@{#55424}
[modify] https://crrev.com/d67885e6da3d65846a6553e6897a5aa9b30c1150/src/wasm/module-compiler.cc
[add] https://crrev.com/d67885e6da3d65846a6553e6897a5aa9b30c1150/test/mjsunit/wasm/import-table.js

Comment 26 by clemensh@chromium.org, Sep 6

Labels: -Merge-Approved-70

Comment 27 by clemensh@chromium.org, Sep 6

Labels: ReleaseBlock-Stable
This is a security bug found outside of Google. Making this a release blocker. We should ensure that it's included in next week's M-69 respin.

Comment 28 by awhalley@chromium.org, Sep 6

This hasn't had the level of bake time that a stable merge would usually require, but fix would indeed be good to take and we've got confirmation from two senior v8 engineers that they'd like the fix merged, so I'm happy with taking it for 69.

Comment 29 by awhalley@chromium.org, Sep 6

Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac

Comment 30 by gov...@chromium.org, Sep 6

Cc: hablich@chromium.org benmason@chromium.org
Labels: -Merge-Review-69 Merge-Approved-69
Approving merge to M69 branch 3497 based on comment #27 and #28.

Comment 31 by bugdroid1@chromium.org, Sep 7

Project Member
Labels: merge-merged-6.9
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/442977e1ae21e3df4e7cc5cc880f728dcc0c2711

commit 442977e1ae21e3df4e7cc5cc880f728dcc0c2711
Author: Ben L. Titzer <titzer@chromium.org>
Date: Fri Sep 07 08:18:18 2018

Merged: [wasm] Fix dispatch table instance update

This CL fixes a bug where the receiving instance was updated improperly
in the dispatch table(s) of an imported table.

BUG= chromium:875322 
R=​mstarzinger@chromium.org
CC=titzer@chromium.org

Change-Id: Iff24953a1fb6a8ab794e12a7a976d544b56fc3c2
Originally-reviewed-on: https://chromium-review.googlesource.com/1196886
No-Try: true
No-Presubmit: true
No-Treechecks: true
Reviewed-on: https://chromium-review.googlesource.com/1212922
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.9@{#45}
Cr-Branched-From: d7b61abe7b48928aed739f02bf7695732d359e7e-refs/heads/6.9.427@{#1}
Cr-Branched-From: b7e108d6016bf6b7de3a34e6d61cb522f5193460-refs/heads/master@{#54504}
[modify] https://crrev.com/442977e1ae21e3df4e7cc5cc880f728dcc0c2711/src/wasm/module-compiler.cc
[add] https://crrev.com/442977e1ae21e3df4e7cc5cc880f728dcc0c2711/test/mjsunit/wasm/import-table.js

Comment 32 by clemensh@chromium.org, Sep 7

Labels: -Merge-Approved-69

Comment 33 by awhalley@chromium.org, Sep 11

Labels: -reward-topanel reward-unpaid reward-3000
*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************

Comment 34 by awhalley@google.com, Sep 11

Hi knightacesg@ - thanks for the report! The VRP panel has decided to award $3,000 for this bug. A member of our finance team will be in touch to arrange payment. Cheers!

Comment 35 by awhalley@google.com, Sep 11

Labels: Release-1-M69

Comment 36 by awhalley@chromium.org, Sep 11

Labels: -reward-unpaid reward-inprocess

Comment 37 by awhalley@chromium.org, Sep 25

Labels: CVE-2018-17458
Pardon the delay. A CVE has has been allocated for this bug, and https://chromereleases.googleblog.com/2018/09/stable-channel-update-for-desktop_11.html updated accordingly. Cheers!

Comment 38 by awhalley@chromium.org, Sep 25

Labels: CVE_description-missing

Comment 39 by sheriffbot@chromium.org, Dec 7

Project Member
Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 40 by ofrobots@google.com, Dec 28

Labels: NodeJS-Backport-Done
Node.js v10.x PR: https://github.com/nodejs/node/pull/25242

Comment 41 by awhalley@chromium.org, Jan 4

Labels: -CVE_description-missing CVE_description-submitted

Comment 42 by awhalley@google.com, Jan 8

titzer@ - is there any reason this still needs to be marked as Restrict-View-Google

Comment 43 by titzer@chromium.org, Jan 8

Labels: -Restrict-View-Google
Nope, its fixed and was merged all the way back to 6.9.

Sign in to add a comment