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

util: inspect: do not crash on an Error stack that contains a Symbol #56573

Closed
wants to merge 1 commit into from

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Jan 12, 2025

See #56570 - this avoids a crash when an Error's stack property contains a Symbol inside an array

@ljharb ljharb added the util Issues and PRs related to the built-in util module. label Jan 12, 2025
@ljharb ljharb requested a review from BridgeAR January 12, 2025 21:06
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 12, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 12, 2025

Copy link

codecov bot commented Jan 12, 2025

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.14%. Comparing base (bf59539) to head (3523bf2).

Files with missing lines Patch % Lines
lib/internal/util/inspect.js 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56573      +/-   ##
==========================================
- Coverage   89.21%   89.14%   -0.07%     
==========================================
  Files         662      662              
  Lines      191934   191940       +6     
  Branches    36945    36889      -56     
==========================================
- Hits       171227   171114     -113     
- Misses      13543    13672     +129     
+ Partials     7164     7154      -10     
Files with missing lines Coverage Δ
lib/internal/util/inspect.js 95.21% <81.81%> (-4.71%) ⬇️

... and 41 files with indirect coverage changes

@jasnell jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Jan 13, 2025
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

We could try to still log something more helpful than just the array with the symbol in it. Something like:

Error
    [Symbol(foo)]

We just have to detect it's deviating from regular errors.

@ljharb ljharb removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2025
@ljharb ljharb force-pushed the inspect-symbol-error-stack branch from 0a8293c to 8ed3e8b Compare January 15, 2025 00:41
@ljharb ljharb requested a review from BridgeAR January 15, 2025 00:41
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Landing this as is is an improvement over the current situation (since it does not error anymore), while I believe we should push for a tad more and show the error name and message as well.

test/parallel/test-util-inspect.js Show resolved Hide resolved
@ljharb ljharb marked this pull request as draft January 20, 2025 21:55
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

After giving it another thought, I think it is best to land this as is and improve the error output later.
Error output for these cases is broken one way or the other and this is an improvement, even without the other part (which is tricky, since stack traces can have an arbitrary shape).

lib/internal/util/inspect.js Show resolved Hide resolved
@ljharb ljharb marked this pull request as ready for review January 22, 2025 06:56
@ljharb ljharb force-pushed the inspect-symbol-error-stack branch 2 times, most recently from 2e0f183 to 3523bf2 Compare January 22, 2025 07:01
@ljharb ljharb marked this pull request as draft January 22, 2025 08:31
@ljharb ljharb force-pushed the inspect-symbol-error-stack branch from 3523bf2 to ce97b94 Compare January 22, 2025 18:21
@ljharb ljharb marked this pull request as ready for review January 22, 2025 18:21
@ljharb ljharb added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 23, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 23, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@ljharb ljharb force-pushed the inspect-symbol-error-stack branch from ce97b94 to dd5d34c Compare January 23, 2025 22:32
@ljharb ljharb added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 24, 2025
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 24, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/56573
✔  Done loading data for nodejs/node/pull/56573
----------------------------------- PR info ------------------------------------
Title      util: inspect: do not crash on an Error stack that contains a Symbol (#56573)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     ljharb:inspect-symbol-error-stack -> nodejs:main
Labels     util, author ready
Commits    1
 - util: inspect: do not crash on an Error stack that contains a Symbol
Committers 1
 - Jordan Harband <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/56573
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jason Zhang <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/56573
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jason Zhang <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
--------------------------------------------------------------------------------
   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ℹ  This PR was created on Sun, 12 Jan 2025 21:06:20 GMT
   ✔  Approvals: 4
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/56573#pullrequestreview-2545519208
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/56573#pullrequestreview-2546408229
   ✔  - Jason Zhang (@jazelly): https://github.com/nodejs/node/pull/56573#pullrequestreview-2548382788
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/56573#pullrequestreview-2565164187
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-01-23T22:28:33Z: https://ci.nodejs.org/job/node-test-pull-request/64674/
- Querying data for job/node-test-pull-request/64674/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/12952622843

@ljharb ljharb removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jan 24, 2025
@ljharb
Copy link
Member Author

ljharb commented Jan 24, 2025

looks like i need one more stamp :-)

jasnell pushed a commit that referenced this pull request Jan 24, 2025
See #56570

PR-URL: #56573
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jason Zhang <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jan 24, 2025

Landed in 19fabc0

@jasnell jasnell closed this Jan 24, 2025
@ljharb ljharb deleted the inspect-symbol-error-stack branch January 24, 2025 16:08
aduh95 pushed a commit that referenced this pull request Jan 27, 2025
See #56570

PR-URL: #56573
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jason Zhang <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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. 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