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.

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

src: don't print garbage errors #4112

Closed
wants to merge 1 commit into from
Closed

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Dec 2, 2015

If JS throws an object whose toString() method throws, then Node attempts to print an empty message, but actually prints garbage. This commit checks for this case, and prints a newline instead.

Closes #4079

@mscdex mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 2, 2015
@chrisdickinson
Copy link
Contributor

LGTM.

if (message.length() == 0)
PrintErrorString("\n");
else
PrintErrorString("%s\n", *message);
Copy link
Member

Choose a reason for hiding this comment

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

You could simplify this to PrintErrorString("%s\n", *message ? *message : "");

I would print a message like "<toString() threw exception>" instead of a blank line.

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 would print a message like "<toString() threw exception>" instead of a blank line.

I thought about that, but the current check is insufficient, since empty string is a valid toString() response. Would you be OK with me using a TryCatch to actually detect if an error was thrown?

Copy link
Member

Choose a reason for hiding this comment

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

I mean when *message == nullptr, that means .toString() failed somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't seem to be the case though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnoordhuis is *message == nullptr for you? If so, it might be platform specific based on #4079 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I missed that it's using node::Utf8Value (which indeed never returns a nullptr because it returns a pointer to its embedded char array.)

If you use String::Utf8Value, you should see *message == nullptr after a .toString() exception.

@jasnell
Copy link
Member

jasnell commented Dec 3, 2015

LGTM once @bnoordhuis is happy with it

@bnoordhuis
Copy link
Member

LGTM

@JungMinu
Copy link
Member

JungMinu commented Dec 4, 2015

@JungMinu
Copy link
Member

JungMinu commented Dec 4, 2015

LGTM

@JungMinu
Copy link
Member

JungMinu commented Dec 4, 2015

Would you mind if I land this PR? 😄

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 4, 2015

@JungMinu Don't land it yet, as the test fails on Windows.

I'd also like @bnoordhuis's input. message alone, at least on OS X, doesn't seem to provide enough information to differentiate between a toString() error, and a valid empty string result. Do you think it's worth using a TryCatch to differentiate between the two? I've implemented it, and it seems very straightforward.

@JungMinu
Copy link
Member

JungMinu commented Dec 5, 2015

@cjihrig Sorry to bug you, It slipped my mind. 😢

@JungMinu
Copy link
Member

JungMinu commented Dec 5, 2015

IMHO, I think that using a TryCatch to differentiate between the two is fine :)
(I guess that I saw some similar cases in code base before)

If JS throws an object whose toString() method throws, then Node
attempts to print an empty message, but actually prints garbage.
This commit checks for this case, and prints a message instead.
@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 5, 2015

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

Made the change to String::Utf8Value. @bnoordhuis still LGTY?

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 5, 2015

CI failures are unrelated.

@bnoordhuis
Copy link
Member

LGTM

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 5, 2015

Thanks, landed in 1ec09b0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants