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

debug: Fix crash handling null strings #31523

Closed
wants to merge 2 commits into from
Closed

debug: Fix crash handling null strings #31523

wants to merge 2 commits into from

Conversation

rustyconover
Copy link
Contributor

When internal debug is enabled output null strings as
"(null)" rather than crashing matching glibc's behavior.

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows

@rustyconover
debug: Fix crash handling null strings
218aead 
When internal debug is enabled output null strings as
"(null)" rather than crashing matching glibc's behavior.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 26, 2020
@rustyconover rustyconover requested a review from addaleax January 26, 2020 15:55
@rustyconover rustyconover mentioned this pull request Jan 26, 2020
Closed
2 tasks
@cjihrig
Copy link
Contributor

cjihrig commented Jan 26, 2020

It's probably worth adding a test for this.

lundibundi
lundibundi approved these changes Jan 26, 2020
View reviewed changes
Copy link
Member

@lundibundi lundibundi left a comment

Choose a reason for hiding this comment

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

LGTM.

Could you please add a test case for this one. I think something like
EXPECT_EQ(SPrintF("%s", nullptr), "(null)")); in the SPrintF case of test/cctest/test_util.cc will be enough.

src/debug_utils-inl.h Outdated Show resolved Hide resolved
rustyconover
rustyconover commented Jan 26, 2020
View reviewed changes
Copy link
Contributor Author

@rustyconover rustyconover left a comment

Choose a reason for hiding this comment

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

Added test

@cjihrig
Copy link
Contributor

cjihrig commented Jan 26, 2020

Still LGTM. Thanks.

lundibundi
lundibundi approved these changes Jan 26, 2020
View reviewed changes
Copy link
Member

@lundibundi lundibundi left a comment

Choose a reason for hiding this comment

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

👍

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@addaleax addaleax mentioned this pull request Jan 27, 2020
Closed
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 27, 2020 •
edited by Trott
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/28718/ ✔️

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 27, 2020
Trott pushed a commit that referenced this pull request Jan 29, 2020
@rustyconover @Trott
src: fix debug crash handling null strings
32f63fc 
When internal debug is enabled, output null strings as
"(null)" rather than crashing, matching glibc's behavior.

PR-URL: #31523
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Jan 29, 2020

Landed in 32f63fc.

Thanks for the contribution! 🎉

@Trott Trott closed this Jan 29, 2020
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@rustyconover @codebytere
src: fix debug crash handling null strings
f04576e 
When internal debug is enabled, output null strings as
"(null)" rather than crashing, matching glibc's behavior.

PR-URL: #31523
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Feb 17, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 15, 2020
@rustyconover @codebytere
src: fix debug crash handling null strings
c8d71ca 
When internal debug is enabled, output null strings as
"(null)" rather than crashing, matching glibc's behavior.

PR-URL: #31523
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
@rustyconover @codebytere
src: fix debug crash handling null strings
96add18 
When internal debug is enabled, output null strings as
"(null)" rather than crashing, matching glibc's behavior.

PR-URL: #31523
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 30, 2020
@rustyconover @codebytere
src: fix debug crash handling null strings
91dd101 
When internal debug is enabled, output null strings as
"(null)" rather than crashing, matching glibc's behavior.

PR-URL: #31523
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lundibundi lundibundi lundibundi approved these changes

@richardlau richardlau richardlau approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig Awaiting requested review from cjihrig

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@rustyconover @cjihrig @nodejs-github-bot @Trott @jasnell @addaleax @richardlau @lundibundi