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.

Sign up for GitHub

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

Jump to bottom

assert: fix infinite loop #18611

Closed
wants to merge 3 commits into from
Closed

assert: fix infinite loop #18611

wants to merge 3 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 7, 2018

If an object has a custom inspect function set and it returns the same output for different objects and those objects are compared using strict assert, an infinite loop is triggered because a loop is not well defined.

Commit 1 fixes that. The second commit changes the output for strict assert to deactivate custom inspect functions. Showing identical output would be very confusing for users. This fixes that edge case.

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
Affected core subsystem(s)

assert

@BridgeAR
assert: fix infinite loop
155d932 
In rare cirumstances it is possible to get a identical error diff.
In such a case the advances diffing runs into a infinite loop.
This fixes it by properly checking for extra entries.
@BridgeAR
assert: show proper differences
6d0eb21 
Right now it is possible to get an AssertionError from input that has
the customInspect function set to always return the same value.

That way the error message is actually misleading because the output
is going to look the same. This fixes it by deactivating the custom
inspect function.
@nodejs-github-bot nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Feb 7, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 7, 2018

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

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 7, 2018

Since assert.strict has not yet landed in 9.x this is no breaking change.

mcollina
mcollina approved these changes Feb 7, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 7, 2018
addaleax
addaleax approved these changes Feb 8, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I’d have a mild preference for using the custom inspect if and only if it differs, but this LGTM either way.

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 8, 2018

@addaleax I see your point that this might make sense. I am somewhat in-between your suggestion to only deactivate custom inspect if both show the same result and the other end: to everywhere deactivate custom inspect to make sure the errors will always show what is really in those objects.

The latter would for example prevent the possible case that someone tries to sneak in some malicious data with the premise that it will not be detected in errors because it uses a custom inspect function.

Right now I have a minor preference for the latter but I would like to hear others as well on it.

@BridgeAR
Copy link
Member Author

@mcollina @joyeecheung any opinions on the comment?

@mcollina
Copy link
Member

@BridgeAR I prefer keeping it disabled.

@BridgeAR
Copy link
Member Author

I added a commit to also disable it in another case that is not semver-major. I am going to open another PR that changes the behavior to always disable custom inspect as a default but that is of course semver-major.

@BridgeAR
Copy link
Member Author

CI https://ci.nodejs.org/job/node-test-commit-light/250/

mcollina
mcollina approved these changes Feb 12, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 12, 2018
@BridgeAR
assert: fix infinite loop
656a5d0 
In rare cirumstances it is possible to get a identical error diff.
In such a case the advances diffing runs into a infinite loop.
This fixes it by properly checking for extra entries.

PR-URL: nodejs#18611
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 12, 2018
@BridgeAR
assert: show proper differences
0cdc877 
Right now it is possible to get an AssertionError from input that has
the customInspect function set to always return the same value.

That way the error message is actually misleading because the output
is going to look the same. This fixes it by deactivating the custom
inspect function.

PR-URL: nodejs#18611
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@BridgeAR
Copy link
Member Author

Landed in 656a5d0, 0cdc877.

@BridgeAR BridgeAR closed this Feb 12, 2018
@MylesBorins
Copy link
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018
@BridgeAR
assert: fix infinite loop
ca8a591 
In rare cirumstances it is possible to get a identical error diff.
In such a case the advances diffing runs into a infinite loop.
This fixes it by properly checking for extra entries.

PR-URL: nodejs#18611
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018
@BridgeAR
assert: show proper differences
254f888 
Right now it is possible to get an AssertionError from input that has
the customInspect function set to always return the same value.

That way the error message is actually misleading because the output
is going to look the same. This fixes it by deactivating the custom
inspect function.

PR-URL: nodejs#18611
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Mar 8, 2018
Closed
4 tasks
@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 8, 2018

Backport opened in #19230

MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
@BridgeAR @MylesBorins
assert: fix infinite loop
4ca0fc4 
In rare cirumstances it is possible to get a identical error diff.
In such a case the advances diffing runs into a infinite loop.
This fixes it by properly checking for extra entries.

Backport-PR-URL: #19230
PR-URL: #18611
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
@BridgeAR @MylesBorins
assert: show proper differences
61baaba 
Right now it is possible to get an AssertionError from input that has
the customInspect function set to always return the same value.

That way the error message is actually misleading because the output
is going to look the same. This fixes it by deactivating the custom
inspect function.

Backport-PR-URL: #19230
PR-URL: #18611
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@targos targos mentioned this pull request Mar 18, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@BridgeAR @MylesBorins
assert: fix infinite loop
9abbb6b 
In rare cirumstances it is possible to get a identical error diff.
In such a case the advances diffing runs into a infinite loop.
This fixes it by properly checking for extra entries.

Backport-PR-URL: #19230
PR-URL: #18611
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@BridgeAR @MylesBorins
assert: show proper differences
5e6b42e 
Right now it is possible to get an AssertionError from input that has
the customInspect function set to always return the same value.

That way the error message is actually misleading because the output
is going to look the same. This fixes it by deactivating the custom
inspect function.

Backport-PR-URL: #19230
PR-URL: #18611
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@BridgeAR @MayaLekova
assert: fix infinite loop
9b6817c 
In rare cirumstances it is possible to get a identical error diff.
In such a case the advances diffing runs into a infinite loop.
This fixes it by properly checking for extra entries.

PR-URL: nodejs#18611
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@BridgeAR @MayaLekova
assert: show proper differences
1729885 
Right now it is possible to get an AssertionError from input that has
the customInspect function set to always return the same value.

That way the error message is actually misleading because the output
is going to look the same. This fixes it by deactivating the custom
inspect function.

PR-URL: nodejs#18611
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins
Copy link
Contributor

Requested backport to 8.x in #19230

@BridgeAR BridgeAR deleted the fix-infinite-loop branch April 1, 2019 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@addaleax addaleax addaleax approved these changes

@mcollina mcollina mcollina approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@BridgeAR @mcollina @MylesBorins @addaleax @joyeecheung @nodejs-github-bot