src: don't include a null character in the WriteConsoleW call by seishun · Pull Request #7764 · nodejs/node · GitHub

Checklist

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

Affected core subsystem(s)

Description of change

It seems if WriteConsoleW is called with a string that has a null character at the end, the console turns it into a space.

Fixes: #7755

Could you add a regression test for this based on #7755?

Not sure how. I'm unable to reproduce this issue when stderr is piped. It seems to only occur when it's written directly to a console.

It might be nice to know why node is trying to print an error message in the first place, maybe that also answers why it doesn’t occur with piped stderr?

No, it does still print an error message with piped stderr. It just doesn't have anything after the trailing newline in that case.

The "error message" is "Debugger listening on [::]:5858" for example.

I don't think this works, it's going to fail when n == 0. (Also, passing n here but -1 three lines below doesn't look Obviously Correct to me.)

I think the patch should look something like this, using explicit sizes everywhere.

diff --git a/src/node.cc b/src/node.cc
index 3998659..1f0ba27 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -246,16 +246,19 @@ static void PrintErrorString(const char* format, ...) {
   }

   // Fill in any placeholders
-  int n = _vscprintf(format, ap);
-  std::vector<char> out(n + 1);
-  vsprintf(out.data(), format, ap);
+  const int numbytes = _vscprintf(format, ap);
+  if (numbytes <= 0) return;
+  std::vector<char> bytes(numbytes + 1);
+  vsprintf(bytes.data(), format, ap);

   // Get required wide buffer size
-  n = MultiByteToWideChar(CP_UTF8, 0, out.data(), -1, nullptr, 0);
-
-  std::vector<wchar_t> wbuf(n);
-  MultiByteToWideChar(CP_UTF8, 0, out.data(), -1, wbuf.data(), n);
-  WriteConsoleW(stderr_handle, wbuf.data(), n, nullptr, nullptr);
+  const int numchars =
+      MultiByteToWideChar(CP_UTF8, 0, bytes.data(), numbytes, nullptr, 0);
+  std::vector<wchar_t> chars(numchars);
+  MultiByteToWideChar(CP_UTF8, 0,
+                      bytes.data(), numbytes,
+                      chars.data(), numchars);
+  WriteConsoleW(stderr_handle, chars.data(), numchars, nullptr, nullptr);
 #else
   vfprintf(stderr, format, ap);
 #endif

OK, agreed, I'll make it simpler. (I wanted to avoid allocating for the unused wide null character, but it's not worth it if it makes the code more complicated)

@seishun regarding the regression test, @Fishrock123 recently added better support for TTY testing (see /test/pseudo-tty/). Did you happen to try adding a TTY test?

I think these tests are POSIX-only right now, unfortunately.

@cjihrig Pseudo-terminals from python are not available on windows.

We could start using a shim like https://www.npmjs.com/package/pty.js uses. Maybe we should wrap it in a Node shim that uses that module? Or maybe someone with better python knowledge could duplicate the pty.js shim for Windows?

Using a terminal emulator wouldn't really make sense for testing this, since the issue is caused by a peculiarity of the Windows console. For instance, this issue doesn't happen in the MSYS2 shell.

Perhaps we could run the test through winpty. If we could somehow capture its output, we could test whether there are any unnecessary spaces on the second line.

LGTM.. but it is unfortunate that there does not appear to be a reliable regression test for this.

@nodejs/ctc @nodejs/platform-windows ... ping... any further thoughts on this one?

There should probably be a CHECK_GT(n, 0) and the comment should be punctuated, otherwise LGTM.

Fixes: #7755

There should probably be a CHECK_GT(n, 0)

Where exactly? n can't be 0 here.

the comment should be punctuated, otherwise LGTM

The other comments in this function aren't punctuated either.

There should probably be a CHECK_GT(n, 0)

Where exactly? n can't be 0 here.

Just before WriteConsoleW. And yes, that CHECK_GT is pretty much supposed to make sure that you’re always right here. ;)

Added the check.

LGTM

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

Fixes: #7755 PR-URL: #7764 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Landed in 09f861f

Fixes: #7755 PR-URL: #7764 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@seishun should this be backported?

@thealphanerd If it applies cleanly to a branch, then that branch is affected.

Fixes: #7755 PR-URL: #7764 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 16, 2016

addaleax added the windows Issues and PRs related to the Windows platform. label Jul 16, 2016

bnoordhuis reviewed Jul 17, 2016
View reviewed changes

seishun added 2 commits August 21, 2016 23:32

src: don't include a null character in the WriteConsoleW call

5dc5e74

Fixes: #7755

different approach

d325fbe

add a check

8e246c5

jasnell pushed a commit that referenced this pull request Aug 23, 2016

src: don't include a null character in the WriteConsoleW call

09f861f

Fixes: #7755 PR-URL: #7764 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell closed this Aug 23, 2016

evanlucas pushed a commit that referenced this pull request Aug 24, 2016

src: don't include a null character in the WriteConsoleW call

4118598

Fixes: #7755 PR-URL: #7764 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

evanlucas mentioned this pull request Aug 24, 2016

2016-08-26, Version 6.5.0 (Current) #8253

Merged

MylesBorins added the lts-watch-v4.x label Sep 30, 2016

MylesBorins pushed a commit that referenced this pull request Sep 30, 2016

src: don't include a null character in the WriteConsoleW call

87a2ece

Fixes: #7755 PR-URL: #7764 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Sep 30, 2016

MylesBorins pushed a commit that referenced this pull request Sep 30, 2016

src: don't include a null character in the WriteConsoleW call

0834c75

Fixes: #7755 PR-URL: #7764 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins pushed a commit that referenced this pull request Oct 10, 2016

src: don't include a null character in the WriteConsoleW call

ccc48f4

Fixes: #7755 PR-URL: #7764 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

rvagg pushed a commit that referenced this pull request Oct 18, 2016

src: don't include a null character in the WriteConsoleW call

5d80a67

Fixes: #7755 PR-URL: #7764 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins pushed a commit that referenced this pull request Oct 26, 2016

src: don't include a null character in the WriteConsoleW call

606deec

Fixes: #7755 PR-URL: #7764 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins mentioned this pull request Oct 26, 2016

V4.6.2 proposal #9298

Closed

seishun deleted the debug branch June 10, 2017 18:49

src: don't include a null character in the WriteConsoleW call by seishun · Pull Request #7764 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: don't include a null character in the WriteConsoleW call - #7764

src: don't include a null character in the WriteConsoleW call#7764seishun wants to merge 3 commits intonodejs:masternodejs/node:masterfrom seishun:debugCopy head branch name to clipboard

Conversation

seishun commented Jul 16, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

cjihrig commented Jul 16, 2016

Uh oh!

seishun commented Jul 16, 2016

Uh oh!

addaleax commented Jul 16, 2016

Uh oh!

seishun commented Jul 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis Jul 17, 2016

Choose a reason for hiding this comment

Uh oh!

seishun Jul 17, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Jul 18, 2016

Uh oh!

addaleax commented Jul 18, 2016

Uh oh!

Fishrock123 commented Jul 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seishun commented Jul 18, 2016

Uh oh!

jasnell commented Aug 8, 2016

Uh oh!

jasnell commented Aug 18, 2016

Uh oh!

bnoordhuis commented Aug 18, 2016

Uh oh!

seishun commented Aug 21, 2016

Uh oh!

addaleax commented Aug 22, 2016

Uh oh!

seishun commented Aug 23, 2016

Uh oh!

jasnell commented Aug 23, 2016

Uh oh!

jasnell commented Aug 23, 2016

Uh oh!

jasnell commented Aug 23, 2016

Uh oh!

MylesBorins commented Sep 30, 2016

Uh oh!

seishun commented Sep 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

src: don't include a null character in the WriteConsoleW call#7764
seishun wants to merge 3 commits into
nodejs:masterfrom
seishun:debug

seishun commented Jul 16, 2016 •
edited
Loading

Fishrock123 commented Jul 18, 2016 •
edited
Loading