Skip to content

src: use custom fprintf alike to write errors to stderr - #31446

Closed
addaleax wants to merge 11 commits into
nodejs:masterfrom
addaleax:sprintf
Closed

src: use custom fprintf alike to write errors to stderr#31446
addaleax wants to merge 11 commits into
nodejs:masterfrom
addaleax:sprintf

Conversation

@addaleax

Copy link
Copy Markdown
Member
src: add C++-style sprintf utility

Add an utility that handles C++-style strings and objects well.

src: use custom fprintf alike to write errors to stderr

This allows printing errors that contain nul characters, for example.

Fixes: #28761
Fixes: #31218

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Add an utility that handles C++-style strings and objects well.
This allows printing errors that contain nul characters, for example.
Fixes: nodejs#28761Fixes: nodejs#31218
@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 21, 2020
Comment threadtest/cctest/test_util.cc
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@devsnekdevsnek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is pretty cool

@bnoordhuisbnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pretty cool. I do wonder if this won't bloat the binary if it's used in a lot of compilation units. Each unit will get its own, possibly unrolled copy.

Comment threadsrc/debug_utils-inl.h Outdated
addaleax added a commit to addaleax/node that referenced this pull request Jan 21, 2020
From the issue:
> Some servers deviate from HTTP spec enougth that Node.js can't
> communicate with them, but "work" when `--insecure-http-parser`
> is enabled globally. It would be useful to be able to use this
> mode, as a client, only when connecting to known bad servers.
This is largely equivalent to nodejs#31446
in terms of code changes.
Fixes: nodejs#31440
Refs: nodejs#31446
@addaleax

addaleax commented Jan 21, 2020

Copy link
Copy Markdown
MemberAuthor

Pretty cool. I do wonder if this won't bloat the binary if it's used in a lot of compilation units. Each unit will get its own, possibly unrolled copy.

The binary size definitely increases with this, yes. The functions do get deduplicated as they aren’t inlined, though, at least for me (with the __attribute__((noinline)) working).

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Comment threadsrc/node_errors.cc
Comment threadsrc/debug_utils-inl.h Outdated
Comment threadsrc/debug_utils.cc
@addaleaxaddaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 22, 2020
@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 22, 2020
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

addaleax added a commit that referenced this pull request Jan 23, 2020
Add an utility that handles C++-style strings and objects well.
PR-URL: #31446Fixes: #28761Fixes: #31218
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax added a commit that referenced this pull request Jan 23, 2020
This allows printing errors that contain nul characters, for example.
Fixes: #28761Fixes: #31218
PR-URL: #31446
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@addaleax

Copy link
Copy Markdown
MemberAuthor

Landed in f6c6236...32e7e81

@addaleax
addaleax deleted the sprintf branch January 23, 2020 21:43
addaleax added a commit that referenced this pull request Jan 24, 2020
From the issue:
> Some servers deviate from HTTP spec enougth that Node.js can't
> communicate with them, but "work" when `--insecure-http-parser`
> is enabled globally. It would be useful to be able to use this
> mode, as a client, only when connecting to known bad servers.
This is largely equivalent to #31446
in terms of code changes.
Fixes: #31440
Refs: #31446
PR-URL: #31448
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax added a commit to sam-github/node that referenced this pull request Jan 24, 2020
From the issue:
> Some servers deviate from HTTP spec enougth that Node.js can't
> communicate with them, but "work" when `--insecure-http-parser`
> is enabled globally. It would be useful to be able to use this
> mode, as a client, only when connecting to known bad servers.
This is largely equivalent to nodejs#31446
in terms of code changes.
Fixes: nodejs#31440
Refs: nodejs#31446
PR-URL: nodejs#31448
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Jan 24, 2020
From the issue:
> Some servers deviate from HTTP spec enougth that Node.js can't
> communicate with them, but "work" when `--insecure-http-parser`
> is enabled globally. It would be useful to be able to use this
> mode, as a client, only when connecting to known bad servers.
This is largely equivalent to nodejs#31446
in terms of code changes.
Fixes: nodejs#31440
Refs: nodejs#31446
PR-URL: nodejs#31448
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
zsw007 added a commit to ibmruntimes/node that referenced this pull request Feb 11, 2020
Backport 7fc5656
Original commit message:
From the issue:
> Some servers deviate from HTTP spec enougth that Node.js can't
> communicate with them, but "work" when `--insecure-http-parser`
> is enabled globally. It would be useful to be able to use this
> mode, as a client, only when connecting to known bad servers.
This is largely equivalent to
nodejs/node#31446 in terms of code changes.
Fixes: nodejs/node#31440
Refs: nodejs/node#31446
Backport-PR-URL: nodejs/node#31500
PR-URL: nodejs/node#31448
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
zsw007 added a commit to ibmruntimes/node that referenced this pull request Feb 12, 2020
Backport 7fc5656
Original commit message:
From the issue:
> Some servers deviate from HTTP spec enougth that Node.js can't
> communicate with them, but "work" when `--insecure-http-parser`
> is enabled globally. It would be useful to be able to use this
> mode, as a client, only when connecting to known bad servers.
This is largely equivalent to
nodejs/node#31446 in terms of code changes.
Fixes: nodejs/node#31440
Refs: nodejs/node#31446
Backport-PR-URL: nodejs/node#31500
PR-URL: nodejs/node#31448
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
zsw007 added a commit to ibmruntimes/node that referenced this pull request Feb 12, 2020
Backport 7fc5656
Original commit message:
From the issue:
> Some servers deviate from HTTP spec enougth that Node.js can't
> communicate with them, but "work" when `--insecure-http-parser`
> is enabled globally. It would be useful to be able to use this
> mode, as a client, only when connecting to known bad servers.
This is largely equivalent to
nodejs/node#31446 in terms of code changes.
Fixes: nodejs/node#31440
Refs: nodejs/node#31446
Backport-PR-URL: nodejs/node#31500
PR-URL: nodejs/node#31448
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
Add an utility that handles C++-style strings and objects well.
PR-URL: #31446Fixes: #28761Fixes: #31218
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
This allows printing errors that contain nul characters, for example.
Fixes: #28761Fixes: #31218
PR-URL: #31446
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebyterecodebytere mentioned this pull request Feb 17, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
Add an utility that handles C++-style strings and objects well.
PR-URL: #31446Fixes: #28761Fixes: #31218
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 15, 2020
This allows printing errors that contain nul characters, for example.
Fixes: #28761Fixes: #31218
PR-URL: #31446
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
Add an utility that handles C++-style strings and objects well.
PR-URL: #31446Fixes: #28761Fixes: #31218
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
This allows printing errors that contain nul characters, for example.
Fixes: #28761Fixes: #31218
PR-URL: #31446
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebyterecodebytere mentioned this pull request Mar 17, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs 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++.lib / srcIssues and PRs related to general changes in the lib or src directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Corrupted stack trace backtrace truncated if msg contains null character \0

8 participants

@addaleax@nodejs-github-bot@bnoordhuis@jasnell@Trott@cjihrig@devsnek@legendecas