node-sha.js autopkgtest failures on ppc64el with nodejs 12.18.2

[Olivier Tilloy]

Since nodejs 12.18.1 entered groovy-proposed, autopkgtests that rely on node-sha.js to compute SHA-1 hashes started failing reliably on ppc64el. This affects the following packages:

  - node-sha.js
  - node-crypto-browserify
  - node-create-hash

Other architectures do not appear to be affected, and it's only the SHA-1 hashes that are incorrect, other hashing algorithm are not affected.

For reference, I'm attaching the full log for the last failed run of node-create-hash on ppc64el (see https://autopkgtest.ubuntu.com/packages/n/node-create-hash/groovy/ppc64el).

Upstream (sha.js) bug report: https://github.com/crypto-browserify/sha.js/issues/66
Upstream (Node.js) bug report: https://github.com/nodejs/node/issues/34666

[Olivier Tilloy (osomon)]

And testing in a ppc64el cloud instance, I reverted nodejs to 10.19.0~dfsg-3ubuntu1 and the tests reliably pass. This confirms that something in node-sha broke with the update to nodejs 12.18.1~dfsg-1ubuntu1.

[Olivier Tilloy (osomon)]

Added some logging to the Sha1._update() method which is called repeatedly, and the problem doesn't appear to be deterministic: it doesn't always start diverging at the same place (although it's always early, within the first 100 iterations).

[Olivier Tilloy (osomon)]

(the above comment applies to the last test in test/test.js with the label "call digest for more than MAX_UINT32 bits of data".

[Bryce Harrington (bryce)]

Could it have anything to do with the patch being applied in node-sha.js, 0002-Fix-FTBFS-in-32-bits-arch.patch?

   var hash = new Sha1()
- var bigData = Buffer.alloc(0x1ffffffff / 8)
+ var bigData = Buffer.alloc(0x1fffffff8 / 8)

I wonder if this patch were disabled for ppc64el, if the test would then pass? If so, then perhaps the patch needs to test if its uint32 or uint64 first. (There appears to be code doing a 32/64 support check in hash.js' Hash.prototype.digest that might be pertinent.)

[Olivier Tilloy (osomon)]

Unfortunately no, the patch doesn't affect the outcome of tests on ppc64el (I did try reverting it, and the problem has also been reported upstream, which doesn't carry the patch).

[Olivier Tilloy (osomon)]

Some notes while I continue investigating the problem:

 - while I used the last test in test/test.js to highlight the problem, it isn't the only one that is failing

 - both sha.js and sha1.js are affected (unsurprisingly, as they have very similar implementations), other algorithms (sha224, sha256, sha384 and sha512) behave correctly

 - I don't have a good understanding of what's going on yet, but successive calls to the update() method on the hash object start diverging values when calling ft(s, b, c, d), and the divergence happens only when s === 2. Inlining the function doesn't fix the problem.

[Olivier Tilloy (osomon)]

Some progress: running one of the failing tests through nodejs with the --no-opt flag (for no optimizations) makes it consistently pass.

It looks like V8 is optimizing too aggressively, and incorrectly.

I can observe that when values start diverging, ft() is being called with s===2, and despite that the "other" code path is run (return b ^ c ^ d).

[Olivier Tilloy (osomon)]

To further prove the point, adding `eval('')` to the body of ft() ensures that the function is not optimized by V8, and the tests reliably pass.

[Olivier Tilloy (osomon)]

(note that I'm not suggesting the above as a workaround, we need to dig deeper and understand where this incorrect optimization comes from)

[Olivier Tilloy (osomon)]

Just a wild guess that will need testing: https://chromium-review.googlesource.com/c/v8/v8/+/1649711 might be related, would reverting it help?

[Olivier Tilloy (osomon)]

Unfortunately, that was a red herring. I rebuilt nodejs for ppc64el with that commit reverted, but it didn't help.

[Olivier Tilloy (osomon)]

Reported as an upstream Node.js bug: https://github.com/nodejs/node/issues/34666

[Gianfranco Costamagna (costamagnagianfranco)]

I'm uploading the patch suggested on the upstream tracker...

[Gianfranco Costamagna (costamagnagianfranco)]

I'm talking about commits 3f071e3 and 1a9c676a141b32483b48884f8cc0330e64c8e17f

[Olivier Tilloy (osomon)]

Tests now pass with nodejs 12.18.2~dfsg-1ubuntu2 in groovy-proposed:

  https://autopkgtest.ubuntu.com/packages/n/node-sha.js/groovy/ppc64el
  https://autopkgtest.ubuntu.com/packages/n/node-crypto-browserify/groovy/ppc64el
  https://autopkgtest.ubuntu.com/packages/n/node-create-hash/groovy/ppc64el

I'm marking the bug fix committed.

[Launchpad Janitor (janitor)]

This bug was fixed in the package nodejs - 12.18.2~dfsg-1ubuntu2

---------------
nodejs (12.18.2~dfsg-1ubuntu2) groovy; urgency=medium

  * debian/patches/3f071e3.patch:
  * debian/patches/1a9c676a141b32483b48884f8cc0330e64c8e17f.patch:
    - cherry-pick two upstream changes in v8 to fix a testsuite failure on
      ppc64el for some sha1 calculation errors (LP: #1887144)

 -- Gianfranco Costamagna <email address hidden> Wed, 19 Aug 2020 20:47:23 +0200