Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: fix regression on randomFillSync #35723

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Oct 20, 2020

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
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Oct 20, 2020
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 20, 2020
@panva
Copy link
Member

panva commented Oct 20, 2020

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

Copy link
Member

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@jasnell jasnell added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 20, 2020
@jasnell jasnell force-pushed the fix-randomfillsync-regression branch from f1a56dc to 32635ff Compare October 20, 2020 17:04
@jasnell
Copy link
Member Author

jasnell commented Oct 20, 2020

Please 👍 to fast-track.

@panva ... added the Fixes note

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

This comment has been minimized.

@richardlau
Copy link
Member

richardlau commented Oct 20, 2020

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
Copy link
Member Author

jasnell commented Oct 20, 2020

regression test added

@jasnell jasnell force-pushed the fix-randomfillsync-regression branch from 32635ff to 98cfdaf Compare October 20, 2020 17:25
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

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

const x = Buffer.from("hello");
const y = Buffer.from("world");
require('crypto').randomFillSync(y)

Perhaps something like that could be added to tests.

@jasnell
Copy link
Member Author

jasnell commented Oct 20, 2020

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
Copy link
Member Author

jasnell commented Oct 20, 2020

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

@codecov-io

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member Author

jasnell commented Oct 20, 2020

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
Copy link
Member Author

jasnell commented Oct 20, 2020

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 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
@jasnell
Copy link
Member Author

jasnell commented Oct 21, 2020

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

@rvagg
Copy link
Member

rvagg commented Oct 21, 2020

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

@rvagg rvagg added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 21, 2020
@nodejs-github-bot
Copy link
Collaborator

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

rvagg commented Oct 21, 2020

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
Copy link
Collaborator

@jasnell
Copy link
Member Author

jasnell commented Oct 21, 2020

Good ci run finally. Tagging for the Commit queue

@jasnell jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 21, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 21, 2020
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 21, 2020
gireeshpunathil pushed a commit that referenced this pull request Oct 21, 2020
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
Copy link
Member

landed in 4cbcfae

BethGriggs pushed a commit that referenced this pull request Oct 21, 2020
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>
@BethGriggs BethGriggs mentioned this pull request Oct 21, 2020
BethGriggs added a commit that referenced this pull request Oct 21, 2020
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
BethGriggs added a commit that referenced this pull request Oct 21, 2020
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
BethGriggs added a commit that referenced this pull request Oct 21, 2020
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
BethGriggs added a commit that referenced this pull request Oct 21, 2020
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
@ChALkeR ChALkeR added the security Issues and PRs related to security. label Oct 22, 2020
@targos targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem. fast-track PRs that do not need to wait for 48 hours to land. security Issues and PRs related to security.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet