crypto: fix regression on randomFillSync by jasnell · Pull Request #35723 · nodejs/node

jasnell · 2020-10-20T17:01:25Z

Node.js 15.0.0 included a regression on crypto.randomFillSync that needs a quick patch. We should do a quick 15.0.1 once this lands. This should be fast-tracked.

Signed-off-by: James M Snell jasnell@gmail.com

@panva @nodejs/releasers

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2020-10-20T17:01:30Z

Review requested:

@nodejs/crypto

panva · 2020-10-20T17:03:24Z

@jasnell a note about it fixing #35722 would be great for completeness

MylesBorins

RSLGTM

jasnell · 2020-10-20T17:05:01Z

Please 👍 to fast-track.

@panva ... added the Fixes note

richardlau · 2020-10-20T17:11:00Z

Regression test? (I'm stepping away for dinner so don't block on it, but if you could add a regression test that would be great because obviously the regression wasn't covered by our existing tests).

Signed-off-by: James M Snell <jasnell@gmail.com> Fixes: nodejs#35722

jasnell · 2020-10-20T17:25:49Z

regression test added

nodejs-github-bot · 2020-10-20T17:26:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33763/

ChALkeR

LGTM.

That said, it's strange that this wasn't caught.
That also affects e.g.

constx=Buffer.from("hello");consty=Buffer.from("world");require('crypto').randomFillSync(y)

Perhaps something like that could be added to tests.

jasnell · 2020-10-20T17:43:48Z

It's concerning that none of the existing tests caught this particular variation. The regression test should cover that case @ChALkeR but we can extend it after this lands.

jasnell · 2020-10-20T17:44:37Z

Some unrelated flaky failures so far in CI. Once the full CI run completes I'll resume it.

nodejs-github-bot · 2020-10-20T18:34:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33764/

jasnell · 2020-10-20T18:35:04Z

The ARM job in CI appeared to hang up.. not sure what's up there... restarting CI to hopefully get a green run on the flaky failures

jasnell · 2020-10-20T19:13:07Z

Sigh. Another unrelated flaky failure and ARM is still hanging behind a list of pending CI runs. Assuming those clear out today and we can get by the flaky failures, this is ready to land any time

jasnell · 2020-10-21T00:30:10Z

Still waiting on the arm CI to get unjammed before we can move forward with this.... (@nodejs/build)

rvagg · 2020-10-21T01:11:47Z

removed arm-fanned from the roster, I had to cancel a few people's builds, you should dismiss them for now

nodejs-github-bot · 2020-10-21T01:15:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33765/

rvagg · 2020-10-21T02:24:20Z

New test run complete, you've got a very flaky parallel/test-http2-respond-file-error-pipe-offset on linux for both runs, this one on alpine and centos, the last run on the other alpine. But I don't suppose that has anything to do with this change.

nodejs-github-bot · 2020-10-21T03:18:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33766/

jasnell · 2020-10-21T03:43:41Z

Good ci run finally. Tagging for the Commit queue

github-actions · 2020-10-21T03:46:46Z

Commit Queue failed

- Loading data for nodejs/node/pull/35723
✔ Done loading data for nodejs/node/pull/35723
----------------------------------- PR info ------------------------------------
Title crypto: fix regression on randomFillSync (#35723)
Author James M Snell (@jasnell)
Branch jasnell:fix-randomfillsync-regression -> nodejs:master
Labels author ready, crypto, fast-track
Commits 1
- crypto: fix regression on randomFillSync
Committers 1
- James M Snell PR-URL: https://github.com/nodejs/node/pull/35723
Reviewed-By: Myles Borins Reviewed-By: Beth Griggs Reviewed-By: Сковорода Никита Андреевич Reviewed-By: Evan Lucas ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35723
Reviewed-By: Myles Borins Reviewed-By: Beth Griggs Reviewed-By: Сковорода Никита Андреевич Reviewed-By: Evan Lucas --------------------------------------------------------------------------------
✖ Last GitHub CI failed
ℹ Last Full PR CI on 2020-10-21T03:18:37Z: https://ci.nodejs.org/job/node-test-pull-request/33766/
- Querying data for job/node-test-pull-request/33766/
✔ Build data downloaded
✔ Last Jenkins CI successful
ℹ This PR was created on Tue, 20 Oct 2020 17:01:25 GMT
✔ Approvals: 4
✔ - Myles Borins (@MylesBorins) (TSC): https://github.com/nodejs/node/pull/35723#pullrequestreview-512950520
✔ - Beth Griggs (@BethGriggs) (TSC): https://github.com/nodejs/node/pull/35723#pullrequestreview-512951409
✔ - Сковорода Никита Андреевич (@ChALkeR) (TSC): https://github.com/nodejs/node/pull/35723#pullrequestreview-512977935
✔ - Evan Lucas (@evanlucas): https://github.com/nodejs/node/pull/35723#pullrequestreview-513223234
ℹ This PR is being fast-tracked
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

Commit Queue action: https://github.com/nodejs/node/actions/runs/319044303

Signed-off-by: James M Snell <jasnell@gmail.com> Fixes: #35722 PR-URL: #35723 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Beth Griggs <bgriggs@redhat.com>

gireeshpunathil · 2020-10-21T09:44:20Z

landed in 4cbcfae

Signed-off-by: James M Snell <jasnell@gmail.com> Fixes: #35722 PR-URL: #35723 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Beth Griggs <bgriggs@redhat.com>

Notable changes: - **crypto**: fix regression on randomFillSync (James M Snell) (#35723) * This fixes issue #35722. - **doc**: add release key for Danielle Adams (Danielle Adams) (#35545) PR-URL: #35736

Notable changes: - **crypto**: fix regression on randomFillSync (James M Snell) (#35723) - This fixes issue #35722. - **deps**: upgrade npm to 7.0.3 (Ruy Adorno) (#35724) - **doc**: add release key for Danielle Adams (Danielle Adams) (#35545) PR-URL: #35736

nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Oct 20, 2020

jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 20, 2020

MylesBorins approved these changes Oct 20, 2020
View reviewed changes

jasnell added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 20, 2020

BethGriggs approved these changes Oct 20, 2020
View reviewed changes

jasnell force-pushed the fix-randomfillsync-regression branch from f1a56dc to 32635ffCompare October 20, 2020 17:04

jasnell removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 20, 2020

This comment has been minimized.
Sign in to view

crypto: fix regression on randomFillSync
98cfdaf
Signed-off-by: James M Snell <jasnell@gmail.com> Fixes: nodejs#35722

jasnell force-pushed the fix-randomfillsync-regression branch from 32635ff to 98cfdafCompare October 20, 2020 17:25

panva approved these changes Oct 20, 2020
View reviewed changes

ChALkeR approved these changes Oct 20, 2020
View reviewed changes

This comment has been minimized.
Sign in to view

richardlau mentioned this pull request Oct 20, 2020
Buffer.allocUnsafe with randomFillSync is broken in Node.js 15.0.0 nodejs/help#3033
Closed

jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 20, 2020

evanlucas approved these changes Oct 21, 2020
View reviewed changes

rvagg added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 21, 2020

rvagg removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 21, 2020

jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 21, 2020

github-actionsBot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 21, 2020

github-actionsBot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 21, 2020

addaleax approved these changes Oct 21, 2020
View reviewed changes

gireeshpunathil approved these changes Oct 21, 2020
View reviewed changes

gireeshpunathil closed this Oct 21, 2020

BethGriggs mentioned this pull request Oct 21, 2020
v15.0.1 proposal #35736
Merged

ChALkeR added the security Issues and PRs related to security. label Oct 22, 2020

BethGriggs added the dont-land-on-v14.x label Dec 7, 2020

targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021

Uh oh!

Uh oh!

Conversation

jasnell commented Oct 20, 2020

Checklist

Uh oh!

nodejs-github-bot commented Oct 20, 2020

Uh oh!

panva commented Oct 20, 2020

Uh oh!

MylesBorins left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 20, 2020

Uh oh!

This comment has been minimized.

richardlau commented Oct 20, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Oct 20, 2020

Uh oh!

nodejs-github-bot commented Oct 20, 2020

Uh oh!

ChALkeR left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 20, 2020

Uh oh!

jasnell commented Oct 20, 2020

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Oct 20, 2020

Uh oh!

jasnell commented Oct 20, 2020

Uh oh!

jasnell commented Oct 20, 2020

Uh oh!

jasnell commented Oct 21, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rvagg commented Oct 21, 2020

Uh oh!

nodejs-github-bot commented Oct 21, 2020

Uh oh!

rvagg commented Oct 21, 2020

Uh oh!

nodejs-github-bot commented Oct 21, 2020

Uh oh!

jasnell commented Oct 21, 2020

Uh oh!

github-actionsBot commented Oct 21, 2020

Uh oh!

gireeshpunathil commented Oct 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

richardlau commented Oct 20, 2020•
edited
Loading

jasnell commented Oct 21, 2020•
edited
Loading