Skip to content

assert,util: harden comparison#24831

Closed
BridgeAR wants to merge 4 commits into
nodejs:masterfrom
BridgeAR:harden-comparison
Closed

assert,util: harden comparison#24831
BridgeAR wants to merge 4 commits into
nodejs:masterfrom
BridgeAR:harden-comparison

Conversation

@BridgeAR

@BridgeAR BridgeAR commented Dec 4, 2018

Copy link
Copy Markdown
Member

The former algorithm used checks which were unsafe. Most of these
have been replaced with alternatives that can not be manipulated or
fooled that easily.

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

The former algorithm used checks which were unsafe. Most of these
have been replaced with alternatives that can not be manipulated or
fooled that easily.
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Dec 4, 2018
@BridgeAR

BridgeAR commented Dec 5, 2018

Copy link
Copy Markdown
Member Author

@nodejs/assert @nodejs/util PTAL

This needs some LGs.

@BridgeAR

BridgeAR commented Dec 5, 2018

Copy link
Copy Markdown
Member Author

@Trott

Trott commented Dec 5, 2018

Copy link
Copy Markdown
Member

@BridgeAR

BridgeAR commented Dec 5, 2018

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR added the assert Issues and PRs related to the assert subsystem. label Dec 5, 2018
@Trott

Trott commented Dec 5, 2018

Copy link
Copy Markdown
Member

Comment thread lib/internal/util/comparisons.js Outdated
Comment thread test/parallel/test-assert-deep.js Outdated
addaleax and others added 2 commits December 6, 2018 19:24
Co-Authored-By: BridgeAR <ruben@bridgewater.de>
@BridgeAR

BridgeAR commented Dec 8, 2018

Copy link
Copy Markdown
Member Author

@Trott

Trott commented Dec 8, 2018

Copy link
Copy Markdown
Member

If it helps, https://ci.nodejs.org/job/node-test-pull-request/19324/ is green. ✔️

@ryzokuken ryzokuken left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks really good to be overall. Will take a deeper look tomorrow or on Monday.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 10, 2018
@Trott

Trott commented Dec 10, 2018

Copy link
Copy Markdown
Member

Landed in 9fb4fa8

@Trott Trott closed this Dec 10, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 10, 2018
The former algorithm used checks which were unsafe. Most of these
have been replaced with alternatives that can not be manipulated or
fooled that easily.

PR-URL: nodejs#24831
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 17, 2018
The former algorithm used checks which were unsafe. Most of these
have been replaced with alternatives that can not be manipulated or
fooled that easily.

PR-URL: #24831
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 18, 2018
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
The former algorithm used checks which were unsafe. Most of these
have been replaced with alternatives that can not be manipulated or
fooled that easily.

PR-URL: nodejs#24831
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@BridgeAR BridgeAR deleted the harden-comparison branch January 20, 2020 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

assert Issues and PRs related to the assert subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. util Issues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants