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: use proper errors as coming from StringBytes #14579

Closed
wants to merge 1 commit into from
Closed

src: use proper errors as coming from StringBytes #14579

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Aug 1, 2017

The previous errors were incorrect here, as the code
only failed in situations where strings exceeded size limits or
an OOM situation was encountered, not for invalid encodings
(which aren’t even detected explicitly).

Unfortunately, these situations are hard to test for.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@addaleax addaleax added fs Issues and PRs related to the fs subsystem / file system. lib / src Issues and PRs related to general changes in the lib or src directory. os Issues and PRs related to the os subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 1, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. os Issues and PRs related to the os subsystem. labels Aug 1, 2017
bnoordhuis
bnoordhuis requested changes Aug 2, 2017
View reviewed changes
src/node_file.cc
"readlink",
"Invalid character encoding for link",
*path);
env->isolate()->ThrowException(error);
Copy link
Member

Choose a reason for hiding this comment

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

return env->isolate()->ThrowException(error);? The call to rc.ToLocalChecked() two lines down will fail if you fall through.

Same issue appears a few more times further down in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

src/node_os.cc Outdated
@@ -354,7 +354,8 @@ static void GetUserInfo(const FunctionCallbackInfo<Value>& args) {
const int err = uv_os_get_passwd(&pwd);

if (err) {
return env->ThrowUVException(err, "uv_os_get_passwd");
env->ThrowUVException(err, "uv_os_get_passwd");
return;
Copy link
Member

Choose a reason for hiding this comment

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

Stylistic change.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed up, this was not intentional

src/node_os.cc Outdated
uv_os_free_passwd(&pwd);
env->isolate()->ThrowException(error);
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: move the .IsEmpty() checks to after the StringBytes::Encode() calls, then you don't have to do the goto jump-around thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

error is cleared by StringBytes::Encode() if the call succeeded… I’ve thought about doing that, would you prefer to change that behaviour? I can see the advantage that would have.

Copy link
Member

Choose a reason for hiding this comment

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

Good idea. I was thinking of guarding the Encode() calls with an error.IsEmpty() check but that's a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bnoordhuis Done, PTAL!

@addaleax addaleax force-pushed the src-error-usage branch 3 times, most recently from 4951e75 to 84a16b0 Compare August 7, 2017 12:48
@addaleax
Copy link
Member Author

@bnoordhuis Mind taking another look?

bnoordhuis
bnoordhuis approved these changes Aug 19, 2017
View reviewed changes
src/node_os.cc Outdated
@@ -365,6 +365,7 @@ static void GetUserInfo(const FunctionCallbackInfo<Value>& args) {
pwd.username,
encoding,
&error);

Copy link
Member

Choose a reason for hiding this comment

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

Mildly unrelated change.

@addaleax
src: use proper errors as coming from StringBytes
116447a 
The previous errors were incorrect here, as the code
only failed in situations where strings exceeded size limits or
an OOM situation was encountered, not for invalid encodings
(which aren’t even detected explicitly).

Unfortunately, these situations are hard to test for.

PR-URL: nodejs#14579
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@addaleax
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/11985/

This should be ready.

@jasnell
Copy link
Member

jasnell commented Aug 23, 2017

@addaleax ... fyi https://ci.nodejs.org/job/node-test-commit-aix/8108/nodes=aix61-ppc64/console

Can you check to see if this failure is related to the change?

@addaleax
Copy link
Member Author

Ugh … a lot of the Linux failures are the same, so, probably? test-commit-linux on master for comparison: https://ci.nodejs.org/job/node-test-commit-linux/12023/

@addaleax
Copy link
Member Author

Sigh. Yes, also a problem on master, so I’ll take a look.

@jasnell
Copy link
Member

jasnell commented Aug 24, 2017 •
edited
Loading

I'm digging in also. :-) This wasn't a problem earlier today so it has to be one of the PRs landed today.... and I can't get to Jenkins... sigh. Will keep trying

@addaleax
Copy link
Member Author

@jasnell I think it’s only 32-bit systems (?) My guess would be

node/src/node_perf.cc

Lines 318 to 327 in c6da5c8

#define SET_STATE_TYPEDARRAY(name, type, field) \
target->Set(context, \
FIXED_ONE_BYTE_STRING(isolate, (name)), \
type::New(state_ab, \
offsetof(performance_state, field), \
arraysize(state->field))) \
.FromJust()
SET_STATE_TYPEDARRAY("observerCounts", v8::Uint32Array, observers);
SET_STATE_TYPEDARRAY("milestones", v8::Float64Array, milestones);
#undef SET_STATE_TYPEDARRAY

I realize that’s kind of my code because it mirrors the HTTP2 one, so … uuh. 😄

@addaleax
Copy link
Member Author

Heh, okay, I think I got this.

@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

Um.. that's confusing then :-/ ... the CI was good for that PR even on 32 bit system.

What do you have?

@addaleax
Copy link
Member Author

Landed in 784c6d4

(for the record, the above conversation was finished in #14996)

@addaleax addaleax closed this Aug 24, 2017
@addaleax addaleax deleted the src-error-usage branch August 24, 2017 19:09
addaleax added a commit that referenced this pull request Aug 24, 2017
@addaleax
src: use proper errors as coming from StringBytes
784c6d4 
The previous errors were incorrect here, as the code
only failed in situations where strings exceeded size limits or
an OOM situation was encountered, not for invalid encodings
(which aren’t even detected explicitly).

Unfortunately, these situations are hard to test for.

PR-URL: #14579
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax added a commit to addaleax/ayo that referenced this pull request Aug 25, 2017
@addaleax
src: use proper errors as coming from StringBytes
a526ccb 
The previous errors were incorrect here, as the code
only failed in situations where strings exceeded size limits or
an OOM situation was encountered, not for invalid encodings
(which aren’t even detected explicitly).

Unfortunately, these situations are hard to test for.

PR-URL: nodejs/node#14579
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax added a commit to ayojs/ayo that referenced this pull request Aug 28, 2017
@addaleax
src: use proper errors as coming from StringBytes
dd3a0ca 
The previous errors were incorrect here, as the code
only failed in situations where strings exceeded size limits or
an OOM situation was encountered, not for invalid encodings
(which aren’t even detected explicitly).

Unfortunately, these situations are hard to test for.

PR-URL: nodejs/node#14579
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. lib / src Issues and PRs related to general changes in the lib or src directory. os Issues and PRs related to the os subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@addaleax @jasnell @bnoordhuis @nodejs-github-bot