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

fd leak in send #22

Merged
merged 1 commit into from Jul 3, 2013
Merged

fd leak in send #22

merged 1 commit into from Jul 3, 2013

Conversation

yuvalkariv
Copy link

If request was disconnected before the stat() callback was called, we
will never get the 'close' or 'error' event and the file descriptor will
stay open forever
Note that this fix:
nodejs/node-v0.x-archive#5770
expressjs/express#1554

If request was disconnected before the stat() callback was called, we
will never get the 'close' or 'error' event and the file descriptor will
stay open forever
tj added a commit that referenced this pull request Jul 3, 2013
@tj tj merged commit 593ee25 into pillarjs:master Jul 3, 2013
@tj
Copy link
Contributor

tj commented Jul 3, 2013

thanks for investigating this

@mhart
Copy link

mhart commented Jul 5, 2013

Hmmmm, not a fan... 593ee25#commitcomment-3570462

@mhart
Copy link

mhart commented Jul 5, 2013

I really don't think emitting an error is the correct solution. At least not if the consuming code isn't aware of it (which express isn't).

Is there anything wrong with just returning and not emitting anything? Or handling it in some other fashion?

Re expressjs/express#1554 (comment) - I'd argue it's just as bad a denial of service attack, if not worse, if you can crash the process consistently by crafting the same requests as you're using to demonstrate there.

@tj
Copy link
Contributor

tj commented Jul 5, 2013

every app should have a socket error handler but yeah certainly not ideal if most people don't have that... hmm..

@tj
Copy link
Contributor

tj commented Jul 5, 2013

oh sorry i thought that was emitted on the socket, we do have file.on('error in express, it shouldn't be crashing the app

@tjfontaine
Copy link

see also nodejs/node-v0.x-archive#7065

@pillarjs pillarjs locked and limited conversation to collaborators Aug 23, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants