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

http: destroy the socket on parse error #24757

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/_http_server.js
Expand Up @@ -512,7 +512,8 @@ function socketOnError(e) {

if (!this.server.emit('clientError', e, this)) {
if (this.writable) {
this.end(badRequestResponse);
this.write(badRequestResponse);
this.destroy(e);
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to wait for the write() callback to be called?

Copy link
Member Author

Choose a reason for hiding this comment

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

@addaleax @mcollina the assumption is that badRequestResponse is the first chunk that is written to the socket (it initiates the HTTP response). As such it is written synchronously so I think there is no need to wait for the write callback.

Copy link
Member

Choose a reason for hiding this comment

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

We should give a chance to the event loop to write this. How about we put it into a setImmediate?

Copy link
Member

Choose a reason for hiding this comment

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

Using setImmediate() would make things even more race-y. I think it’s either using the write callback or not delaying it at all (which is still race-y, but probably more consistent?).

return;
}
this.destroy(e);
Expand Down
45 changes: 45 additions & 0 deletions test/parallel/test-http-server-destroy-socket-on-client-error.js
@@ -0,0 +1,45 @@
'use strict';

const { expectsError, mustCall } = require('../common');

// Test that the request socket is destroyed if the `'clientError'` event is
// emitted and there is no listener for it.

const assert = require('assert');
const { createServer } = require('http');
const { createConnection } = require('net');

const server = createServer();

server.on('connection', mustCall((socket) => {
socket.on('error', expectsError({
type: Error,
message: 'Parse Error',
code: 'HPE_INVALID_METHOD',
bytesParsed: 0,
rawPacket: Buffer.from('FOO /\r\n')
}));
}));

server.listen(0, () => {
const chunks = [];
const socket = createConnection({
allowHalfOpen: true,
port: server.address().port
});

socket.on('connect', mustCall(() => {
socket.write('FOO /\r\n');
}));

socket.on('data', (chunk) => {
chunks.push(chunk);
});

socket.on('end', mustCall(() => {
const expected = Buffer.from('HTTP/1.1 400 Bad Request\r\n\r\n');
assert(Buffer.concat(chunks).equals(expected));

server.close();
}));
});