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

net: add fd protection, fix #3604 #3605

Closed
wants to merge 1 commit into from

Conversation

pmq20
Copy link
Contributor

@pmq20 pmq20 commented Oct 30, 2015

This is one possible way to fix #3604

@Fishrock123 Fishrock123 added the net Issues and PRs related to the net subsystem. label Oct 30, 2015
stream.on('error', function() {});
stream.write('might crash');
} catch(e) {}
}); // Should not crash.
Copy link
Contributor

Choose a reason for hiding this comment

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

This test isn't durable enough; see #3604 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead wrap this in assert.throws() to check that it both doesn't abort and that it throws properly. CI should be able to give us a better idea whether the test will work on all platforms.

@Fishrock123
Copy link
Contributor

Maybe someone with more knowledge can help come up with a better test?

bool ret = false;

if (fd >= 0 &&
(unsigned)fd < loop->nwatchers &&
Copy link
Contributor

Choose a reason for hiding this comment

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

no C-style casts.

@trevnorris
Copy link
Contributor

left few comments.

@pmq20
Copy link
Contributor Author

pmq20 commented Nov 3, 2015

@trevnorris Thanks for reviewing! I have revised the commit according to your advice.

bool ret = false;

if (static_cast<unsigned>(fd) < loop->nwatchers &&
loop->watchers[fd] != NULL)
Copy link
Member

Choose a reason for hiding this comment

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

loop->watchers and loop->nwatchers are UNIX-specific. This won't compile on Windows.

Copy link
Member

Choose a reason for hiding this comment

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

And considered "private", so code should access them really...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case this job had better be handled by libuv

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 22, 2016
@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

Any reason to keep this open?

@saghul
Copy link
Member

saghul commented Mar 22, 2016

Nope, I'd move to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

a simple script to crash Node.js with an assertion failure: (loop->watchers[w->fd] == w)
7 participants