src: fix exception message encoding by mscdex · Pull Request #3288 · nodejs/node

mscdex · 2015-10-08T23:28:43Z

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 · 2015-10-08T23:53:59Z

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

evanlucas · 2015-10-09T13:09:28Z

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

bnoordhuis · 2015-10-09T13:47:52Z

Style issue: const char*.

bnoordhuis · 2015-10-09T13:56:47Z

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

mscdex · 2015-10-09T15:47:13Z

@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 · 2015-10-09T17:47:03Z

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

silverwind · 2015-10-12T20:44:13Z

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

bnoordhuis · 2015-10-13T20:48:22Z

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

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

bnoordhuis · 2015-10-13T20:49:36Z

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

piscisaureus · 2015-10-13T21:06:42Z

Lgtm

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 · 2015-10-13T22:02:22Z

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.

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: #3288Fixes: #3284 Reviewed-By: Bert Belder <bertbelder@gmail.com>

mscdex · 2015-10-13T22:17:39Z

Landed in 2d35607.

bergus · 2015-10-13T22:39:09Z

Thanks for the quick fix!

MylesBorins · 2015-10-23T18:37:38Z

@jasnell should this be LTS?

jasnell · 2015-10-24T03:23:06Z

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

jasnell · 2015-10-26T17:03:37Z

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

rvagg · 2015-10-28T04:29:54Z

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

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: #3288Fixes: #3284 Reviewed-By: Bert Belder <bertbelder@gmail.com>

jasnell · 2015-10-28T15:14:37Z

Landed in v4.x-staging in 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: #3288Fixes: #3284 Reviewed-By: Bert Belder <bertbelder@gmail.com>

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

bnoordhuis reviewed Oct 9, 2015
View reviewed changes

Comment threadsrc/node.cc Outdated
bnoordhuisOct 9, 2015
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.
Style issue: const char*.

mscdex force-pushed the fix-exception-encoding branch 2 times, most recently from 3bdc6b3 to 5d7e67aCompare October 9, 2015 17:02

bnoordhuis reviewed Oct 13, 2015
View reviewed changes

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 force-pushed the fix-exception-encoding branch from 5d7e67a to 238d1aeCompare October 13, 2015 21:18

mscdex closed this Oct 13, 2015

rvagg mentioned this pull request Oct 21, 2015
Release proposal: v5.0.0 #3466
Merged

jasnell added the lts-watch-v4.x label Oct 24, 2015

rvagg mentioned this pull request Oct 28, 2015
Preparing v4.2.2 LTS Maintenance nodejs/Release#50
Closed

srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Oct 28, 2015

jasnell added land-on-v4.x and removed lts-watch-v4.x labels Oct 28, 2015

jasnell mentioned this pull request Oct 29, 2015
Release Proposal: v4.2.2 "Argon" (LTS) #3588
Closed

jasnell removed the land-on-v4.x label Nov 3, 2015

mscdex deleted the fix-exception-encoding branch December 17, 2015 18:22

Uh oh!

Uh oh!

Conversation

mscdex commented Oct 8, 2015

Uh oh!

mscdex commented Oct 8, 2015

Uh oh!

evanlucas commented Oct 9, 2015

Uh oh!

bnoordhuisOct 9, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Oct 9, 2015

Uh oh!

mscdex commented Oct 9, 2015

Uh oh!

mscdex commented Oct 9, 2015

Uh oh!

silverwind commented Oct 12, 2015

Uh oh!

bnoordhuisOct 13, 2015

Choose a reason for hiding this comment

Uh oh!

mscdexOct 13, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Oct 13, 2015

Uh oh!

piscisaureus commented Oct 13, 2015

Uh oh!

mscdex commented Oct 13, 2015

Uh oh!

mscdex commented Oct 13, 2015

Uh oh!

bergus commented Oct 13, 2015

Uh oh!

MylesBorins commented Oct 23, 2015

Uh oh!

jasnell commented Oct 24, 2015

Uh oh!

jasnell commented Oct 26, 2015

Uh oh!

rvagg commented Oct 28, 2015

Uh oh!

jasnell commented Oct 28, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants