src: don't include a null character in the WriteConsoleW call #7764
Conversation
|
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. |
| @@ -251,7 +251,7 @@ static void PrintErrorString(const char* format, ...) { | ||
| vsprintf(out.data(), format, ap); | ||
|
|
||
| // Get required wide buffer size | ||
| n = MultiByteToWideChar(CP_UTF8, 0, out.data(), -1, nullptr, 0); | ||
| n = MultiByteToWideChar(CP_UTF8, 0, out.data(), n, nullptr, 0); |
There was a problem hiding this comment.
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);
#endifThere was a problem hiding this comment.
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 |
|
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 |
Where exactly?
The other comments in this function aren't punctuated either. |
Just before |
|
Added the check. |
|
LGTM |
|
Landed in 09f861f |
|
@seishun should this be backported? |
|
@thealphanerd If it applies cleanly to a branch, then that branch is affected. |
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected 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