V8 in Node.js 12.18.2 optimizes incorrectly a function on ppc64el

[oSoMoN]

• Version: v12.18.2
• Platform: Linux groovy-ppc64el 5.8.0-12-generic Failed assert in AsyncWrap::MakeCallback(uint32_t, int, Handle) from test-timers-unref.js on ARM #13-Ubuntu SMP Wed Jul 29 22:18:31 UTC 2020 ppc64le ppc64le ppc64le GNU/Linux
• Subsystem: deps/v8

This report summarizes my findings from https://bugs.launchpad.net/ubuntu/+source/nodejs/+bug/1887144.

When upgrading from Node.js 10.19 to 12.18 in Ubuntu, some tests in node-sha.js and related packages started failing consistently on ppc64el. All other architectures are fine. This was reported separately in node-sha.js: browserify/sha.js#66.

It turns out V8 optimizes a function in the sha0 and sha1 implementations in node-sha.js incorrectly, leading to unexpected results. The function that is being incorrectly optimized is ft(…) (https://github.com/crypto-browserify/sha.js/blob/master/sha1.js#L51):

function ft (s, b, c, d) {
  if (s === 0) return (b & c) | ((~b) & d)
  if (s === 2) return (b & c) | (b & d) | (c & d)
  return b ^ c ^ d
}

That function is called multiple times when incrementally updating a hash, and I am seeing it eventually start using the other code path (s ≠ 0 and s ≠ 2) when s === 2.

I confirmed that the problem comes from incorrect optimizations by two different methods:

• running nodejs with the --no-opt flag makes the tests consistently pass
• similarly, adding an eval('') in the body of ft(…) to prevent optimization makes the tests consistently pass

What steps will reproduce the bug?

git clone https://github.com/crypto-browserify/sha.js.git
cd sha.js
node test/test.js

How often does it reproduce? Is there a required condition?

100% reproducible on ppc64el, doesn't occur on other architectures (x86, x86_64, armhf, arm64, s390x).

What is the expected behavior?

That test should reliably pass.

What do you see instead?

The test reliably fails:

# call digest for more than MAX_UINT32 bits of data
not ok 29 should be equal
  ---
    operator: equal
    expected: '8cd9f41b675a8eb11748681b3b7fa0bb43573740'
    actual:   '470d946dd5b9e09147207be5b5dfbfe9e0f07ac1'
    at: Test.<anonymous> (/home/ubuntu/plic/sha.js/test/test.js:98:5)
    stack: |-
      Error: should be equal
          at Test.assert [as _assert] (/usr/lib/nodejs/tape/lib/test.js:235:54)
          at Test.bound [as _assert] (/usr/lib/nodejs/tape/lib/test.js:87:32)
          at Test.equal (/usr/lib/nodejs/tape/lib/test.js:395:10)
          at Test.bound [as equal] (/usr/lib/nodejs/tape/lib/test.js:87:32)
          at Test.<anonymous> (/home/ubuntu/plic/sha.js/test/test.js:98:5)
          at Test.bound [as _cb] (/usr/lib/nodejs/tape/lib/test.js:87:32)
          at Test.run (/usr/lib/nodejs/tape/lib/test.js:106:10)
          at Test.bound [as run] (/usr/lib/nodejs/tape/lib/test.js:87:32)
          at Immediate.next [as _onImmediate] (/usr/lib/nodejs/tape/lib/results.js:71:15)
          at processImmediate (internal/timers.js:456:21)
  ...

[richardlau]

@miladfarca can you take a look at this please?

[miladfarca]

Seems like the issue was introduced after updating to V8 6.9:
12ed7c9...dbf7203

will need to check the details.

[miladfarca]

This patch should fix the problem, will need to backport it to node 12:
https://chromium-review.googlesource.com/c/v8/v8/+/2349468

[miladfarca]

What are the branches we like to backport this to, master, v13.x-staging, v12.x-staging ?

[richardlau]

@miladfarca Don't bother with v13.x-staging as Node.js 13 is End-of-Life. v12.x-staging, v14.x-staging and master would be the branches to target (sounds like the bug doesn't apply to 10.x).

FYI @nodejs/platform-ppc

[miladfarca]

PRs created:
master:
#35036

v14.x-staging:
#35039

v12.x-staging:
#35038

[targos]

the fixes landed