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

net: check and throw on error for getsockname #12871

Closed
wants to merge 4 commits into
base: master
from

Conversation

@danbev
Member

danbev commented May 6, 2017

This commit attempts fix a TODO in net.js:
TODO(bnoordhuis) Check err and throw?

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

lib

@danbev

This comment has been minimized.

Show comment
Hide comment
@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 6, 2017

Member

Does this have semver significance? It was a "safe" method, now it might throw?

Member

refack commented May 6, 2017

Does this have semver significance? It was a "safe" method, now it might throw?

// TODO(bnoordhuis) Check err and throw?
var err = this._handle.getsockname(out);
if (err) {
throw errnoException(err, 'address');

This comment has been minimized.

@refack

refack May 6, 2017

Member

maybe

if (err) {
  self.emit('error', errnoException(err, 'address'));
  return;
}
@refack

refack May 6, 2017

Member

maybe

if (err) {
  self.emit('error', errnoException(err, 'address'));
  return;
}

This comment has been minimized.

@refack

refack May 6, 2017

Member

@addaleax won't throwing make this semver-major?
Ohh just saw you marked it.

@refack

refack May 6, 2017

Member

@addaleax won't throwing make this semver-major?
Ohh just saw you marked it.

This comment has been minimized.

@addaleax

addaleax May 6, 2017

Member

Yeah, that’s why I’ve labelled it as a major. ;) But emitting an error event a) would be semver-major too and b) doesn’t really make sense here, it’s a fully synchronous call.

@addaleax

addaleax May 6, 2017

Member

Yeah, that’s why I’ve labelled it as a major. ;) But emitting an error event a) would be semver-major too and b) doesn’t really make sense here, it’s a fully synchronous call.

This comment has been minimized.

@refack

refack May 6, 2017

Member

Ok. I agree.

@refack

refack May 6, 2017

Member

Ok. I agree.

@refack

Anyway need to add a test for this.

@danbev

This comment has been minimized.

Show comment
Hide comment
@addaleax

Looks good, but I am afraid you might need to tweak the error regexp in the test a bit more to work cross-platform ;)

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev May 6, 2017

Member

Looks good, but I am afraid you might need to tweak the error regexp in the test a bit more to work cross-platform ;)

I guess I'll find out shortly about that 😄

Member

danbev commented May 6, 2017

Looks good, but I am afraid you might need to tweak the error regexp in the test a bit more to work cross-platform ;)

I guess I'll find out shortly about that 😄

@refack

refack approved these changes May 6, 2017

@refack

This comment has been minimized.

Show comment
Hide comment
Member

refack commented May 6, 2017

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 6, 2017

Member

@danbev Windows error is Error: address Unknown system error -1 so you need /^Error: address ([\w \-\d]+)$/
or do an if (common.isWindows) {

Member

refack commented May 6, 2017

@danbev Windows error is Error: address Unknown system error -1 so you need /^Error: address ([\w \-\d]+)$/
or do an if (common.isWindows) {

@evanlucas

I'm not sure the benefits outweigh the cons on this one. I feel like this is a subtle change that could take down processes. I'm leaning towards -1 on this.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 6, 2017

Member

@evanlucas Going through the errors that getsockname(2) can return according to my manpage copy:

EBADF The argument sockfd is not a valid file descriptor.
ENOTSOCK The file descriptor sockfd does not refer to a socket.

These would point to a bug in Node, I say throwing an error is okay.

EFAULT The addr argument points to memory not in a valid part of the process address space.
EINVAL addrlen is invalid (e.g., is negative).

These won’t happen/also would point to a serious bug in Node. Should definitely make some noise.

ENOBUFS Insufficient resources were available in the system to perform the operation.

Again, this probably never happens. Also, throwing seems better than returning invalid data here.

→ I think it’s safe to do this.

Member

addaleax commented May 6, 2017

@evanlucas Going through the errors that getsockname(2) can return according to my manpage copy:

EBADF The argument sockfd is not a valid file descriptor.
ENOTSOCK The file descriptor sockfd does not refer to a socket.

These would point to a bug in Node, I say throwing an error is okay.

EFAULT The addr argument points to memory not in a valid part of the process address space.
EINVAL addrlen is invalid (e.g., is negative).

These won’t happen/also would point to a serious bug in Node. Should definitely make some noise.

ENOBUFS Insufficient resources were available in the system to perform the operation.

Again, this probably never happens. Also, throwing seems better than returning invalid data here.

→ I think it’s safe to do this.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex May 6, 2017

Contributor

The subsystem in the commit message should be net: instead of lib:

Contributor

mscdex commented May 6, 2017

The subsystem in the commit message should be net: instead of lib:

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev May 7, 2017

Member

@danbev Windows error is Error: address Unknown system error -1 so you need /^Error: address ([\w -\d]+)$/
or do an if (common.isWindows) {

Thanks, updated now and I'll give CI another go.

Member

danbev commented May 7, 2017

@danbev Windows error is Error: address Unknown system error -1 so you need /^Error: address ([\w -\d]+)$/
or do an if (common.isWindows) {

Thanks, updated now and I'll give CI another go.

danbev added some commits May 6, 2017

net: check and throw on error for getsockname
This commit attempts fix a TODO in net.js:
TODO(bnoordhuis) Check err and throw?
@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev May 7, 2017

Member

The subsystem in the commit message should be net: instead of lib:

Ah, let me fix that. thanks

Member

danbev commented May 7, 2017

The subsystem in the commit message should be net: instead of lib:

Ah, let me fix that. thanks

@danbev danbev changed the title from lib: check and throw on error for getsockname to net: check and throw on error for getsockname May 7, 2017

@danbev

This comment has been minimized.

Show comment
Hide comment
Member

danbev commented May 7, 2017

@jasnell

jasnell approved these changes May 7, 2017

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev May 8, 2017

Member

@evanlucas Just wanted to ask if you are still leaning towards a -1 on this?

Member

danbev commented May 8, 2017

@evanlucas Just wanted to ask if you are still leaning towards a -1 on this?

@cjihrig

cjihrig approved these changes May 8, 2017

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas May 8, 2017

Member

@addaleax thanks for the info. No objections from me :] Thanks!

@danbev nope

Member

evanlucas commented May 8, 2017

@addaleax thanks for the info. No objections from me :] Thanks!

@danbev nope

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev May 9, 2017

Member

@evanlucas Great, thanks!

Member

danbev commented May 9, 2017

@evanlucas Great, thanks!

danbev added a commit to danbev/node that referenced this pull request May 9, 2017

net: check and throw on error for getsockname
This commit attempts fix a TODO in net.js:
TODO(bnoordhuis) Check err and throw?

PR-URL: nodejs#12871
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev May 9, 2017

Member

Landed in cf980b0

Member

danbev commented May 9, 2017

Landed in cf980b0

@danbev danbev closed this May 9, 2017

@danbev danbev deleted the danbev:handle-error-net.js branch May 9, 2017

anchnk added a commit to anchnk/node that referenced this pull request May 19, 2017

net: check and throw on error for getsockname
This commit attempts fix a TODO in net.js:
TODO(bnoordhuis) Check err and throw?

PR-URL: nodejs#12871
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment