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

src: fix exception message encoding #3288

Closed
wants to merge 1 commit into from
Closed

src: fix exception message encoding #3288

wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Oct 8, 2015

The printf family of functions do not properly display UTF8 strings well on Windows. Use the appropriate wide character API instead if stderr is a tty.

Fixes: #3284

@mscdex mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. windows Issues and PRs related to the Windows platform. labels Oct 8, 2015
@mscdex
Copy link
Contributor Author

mscdex commented Oct 8, 2015

CI: https://ci.nodejs.org/view/iojs/job/node-test-pull-request/461/

@evanlucas
Copy link
Contributor

Is it not necessary to add on https://github.com/nodejs/node/blob/master/src/node.cc#L3210?

bnoordhuis
bnoordhuis reviewed Oct 9, 2015
View reviewed changes
src/node.cc
@@ -156,6 +156,42 @@ static Isolate* node_isolate = nullptr;
static v8::Platform* default_platform;


static void PrintErrorString(const char *format, ...) {
Copy link
Member

Choose a reason for hiding this comment

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

Style issue: const char*.

@bnoordhuis
Copy link
Member

This isn't complete yet, I think? There are a lot of (f)printf statements strewn throughout the code base.

@mscdex
Copy link
Contributor Author

mscdex commented Oct 9, 2015

@evanlucas That has to do with program arguments, but the string values are not from user input such that they could contain utf8.

@bnoordhuis Well, of the other printf()/fprintf() instances I could find, the string variables did not seem likely to contain utf8. If you had some specifically in mind, let me know.

@mscdex mscdex force-pushed the fix-exception-encoding branch 2 times, most recently from 3bdc6b3 to 5d7e67a Compare October 9, 2015 17:02
@mscdex
Copy link
Contributor Author

mscdex commented Oct 9, 2015

CI again: https://ci.nodejs.org/view/iojs/job/node-test-pull-request/465/

@silverwind
Copy link
Contributor

CI is fine except the random ARM failures. I can confirm it fixes #3284.

bnoordhuis
bnoordhuis reviewed Oct 13, 2015
View reviewed changes
src/node.cc
}

// Fill in any placeholders
size_t n = vsnprintf(nullptr, 0, format, ap);
Copy link
Member

Choose a reason for hiding this comment

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

I'm kind of surprised this works, I thought vsnprintf() on Windows returned -1 when the output buffer is too small.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested it on Windows 7 with VS2013 at least. I suppose _vscprintf() could be used instead...

@bnoordhuis
Copy link
Member

I'm going to defer to @nodejs/platform-windows.

@piscisaureus
Copy link
Contributor

Lgtm

@mscdex
src: fix exception message encoding
238d1ae 
The printf family of functions do not properly display UTF8
strings well on Windows. Use the appropriate wide character
API instead if stderr is a tty.

Fixes: nodejs#3284
@mscdex
Copy link
Contributor Author

mscdex commented Oct 13, 2015

Final CI run after changing vsnprintf()->_vscprintf(): https://ci.nodejs.org/job/node-test-commit/830/

Only failures are an unrelated failure on Win10 and a recurring unrelated failure on FreeBSD 10.1 64-bit.

mscdex added a commit that referenced this pull request Oct 13, 2015
@mscdex
src: fix exception message encoding on Windows
2d35607 
The printf family of functions do not properly display UTF8
strings well on Windows. Use the appropriate wide character
API instead if stderr is a tty.

PR-URL: #3288
Fixes: #3284
Reviewed-By: Bert Belder <bertbelder@gmail.com>
@mscdex
Copy link
Contributor Author

mscdex commented Oct 13, 2015

Landed in 2d35607.

@mscdex mscdex closed this Oct 13, 2015
@bergus
Copy link

bergus commented Oct 13, 2015

Thanks for the quick fix!

@rvagg rvagg mentioned this pull request Oct 21, 2015
Merged
@MylesBorins
Copy link
Member

@jasnell should this be LTS?

@jasnell
Copy link
Member

jasnell commented Oct 24, 2015

hmmm... we may need an LTS WG discussion on this one.

@jasnell
Copy link
Member

jasnell commented Oct 26, 2015

@nodejs/lts ... should we land this one in v4.x?

@rvagg rvagg mentioned this pull request Oct 28, 2015
Closed
@rvagg
Copy link
Member

rvagg commented Oct 28, 2015

after looking at #3284 I would vote yes for this one, if this were just a "make it proper" fix then I'd be borderline but given this is fixing an actual user-reported bug it's much easier to justify because it's not academic

@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Oct 28, 2015
mscdex added a commit that referenced this pull request Oct 28, 2015
@mscdex @jasnell
src: fix exception message encoding on Windows
9bffceb 
The printf family of functions do not properly display UTF8
strings well on Windows. Use the appropriate wide character
API instead if stderr is a tty.

PR-URL: #3288
Fixes: #3284
Reviewed-By: Bert Belder <bertbelder@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 28, 2015

Landed in v4.x-staging in 9bffceb

mscdex added a commit that referenced this pull request Oct 29, 2015
@mscdex @jasnell
src: fix exception message encoding on Windows
fe01d0d 
The printf family of functions do not properly display UTF8
strings well on Windows. Use the appropriate wide character
API instead if stderr is a tty.

PR-URL: #3288
Fixes: #3284
Reviewed-By: Bert Belder <bertbelder@gmail.com>
@jasnell jasnell mentioned this pull request Oct 29, 2015
Closed
@mscdex mscdex deleted the fix-exception-encoding branch December 17, 2015 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@mscdex @evanlucas @bnoordhuis @silverwind @piscisaureus @bergus @MylesBorins @jasnell @rvagg @srl295