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

[seishun]

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

[cjihrig]

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

[seishun]

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.

[addaleax]

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?

[seishun]

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.

[bnoordhuis]

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

[seishun]

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)

[cjihrig]

@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?

[addaleax]

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

[Fishrock123]

@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?

[seishun]

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.

[jasnell]

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

[jasnell]

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

[bnoordhuis]

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

[seishun]

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.

[addaleax]

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. ;)

[seishun]

Added the check.

[jasnell]

LGTM

[jasnell]

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

[jasnell]

Landed in 09f861f

[MylesBorins]

@seishun should this be backported?

[seishun]

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