http: add rawPacket in err of `clientError` event #17672

XadillaX · 2017-12-14T04:46:39Z

The rawPacket is the current buffer that just parsed. Adding this
buffer to the error object of clientError event is to make it possible
that developers can log the broken packet.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

http

The `rawPacket` is the current buffer that just parsed. Adding this buffer to the error object of `clientError` event is to make it possible that developers can log the broken packet.

addaleax · 2017-12-14T21:38:43Z

lib/_http_server.js

@@ -476,6 +476,7 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
  resetSocketTimeout(server, socket, state);

  if (ret instanceof Error) {
+    ret.rawPacket = d || parser.getCurrentBuffer();


I'm not sure, but can you maybe move the

if (!d) d = parser.getCurrentBuffer();

block in front of the if (ret instanceof Error) part? Then you can always just use d inside of this function

Refs: #17672 (comment)

addaleax · 2017-12-14T21:39:17Z

doc/api/http.md

@@ -765,6 +765,11 @@ object, so any HTTP response sent, including response headers and payload,
 *must* be written directly to the `socket` object. Care must be taken to
 ensure the response is a properly formatted HTTP response message.

+> `err` is an instance of `Error` with two extra columns:


Why the > blockquote?

addaleax · 2017-12-14T21:39:46Z

doc/api/http.md

@@ -765,6 +765,11 @@ object, so any HTTP response sent, including response headers and payload,
 *must* be written directly to the `socket` object. Care must be taken to
 ensure the response is a properly formatted HTTP response message.

+> `err` is an instance of `Error` with two extra columns:
+>
+> + `bytesParsed`: the bytes count of request packet that Node.js may parse correctly;


s/parse correctly/have parsed incorrectly/?

It's the count of 'correctly', not 'incorrectly'. Means Node.js have parsed bytesParsed correctly and the left are incorrectly.

XadillaX · 2017-12-15T03:42:31Z

@addaleax I've updated the code

addaleax · 2017-12-15T03:45:31Z

doc/api/http.md

+`err` is an instance of `Error` with two extra columns:
+
+ `bytesParsed`: the bytes count of request packet that Node.js may have parsed correctly;
+ `rawPacket`: the raw packet of current request.


Sorry, one thing I forgot to mention: Can you list the addition of rawPacket in the changes: section of the YAML block for clientError?

bnoordhuis · 2017-12-15T03:53:07Z

lib/_http_server.js

@@ -475,7 +475,11 @@ function socketOnError(e) {
 function onParserExecuteCommon(server, socket, parser, state, ret, d) {
  resetSocketTimeout(server, socket, state);

+  if (!d)
+    d = parser.getCurrentBuffer();


Can you move this into the branches of the if statement? parser.getCurrrentBuffer() creates a copy. Calling it when the result is unused is wasteful and probably has a pretty big performance impact.

But if we don't get the buffer, we cannot pass it to the clientError event.

XadillaX · 2017-12-18T04:31:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12164/

XadillaX · 2017-12-18T05:42:16Z

/ping @addaleax @bnoordhuis

addaleax · 2017-12-18T05:43:26Z

doc/api/http.md

@@ -734,6 +734,11 @@ changes:
    description: The default action of calling `.destroy()` on the `socket`
                 will no longer take place if there are listeners attached
                 for `clientError`.
+  - version: VERSION


REPLACEME is a string that will get picked up by release tooling ):

addaleax · 2017-12-18T05:44:31Z

lib/_http_server.js

@@ -475,7 +475,12 @@ function socketOnError(e) {
 function onParserExecuteCommon(server, socket, parser, state, ret, d) {
  resetSocketTimeout(server, socket, state);

+  if (!d) {
+    d = parser.getCurrentBuffer();
+  }


I think @bnoordhuis’ comment was basically about undoing my earlier comment (which is probably better, yes). i.e. move this part back into the branches were d is actually used, not before the if

XadillaX · 2017-12-18T06:01:12Z

new CI: https://ci.nodejs.org/job/node-test-pull-request/12165/

XadillaX · 2017-12-20T02:04:30Z

/ping @addaleax @bnoordhuis

bnoordhuis · 2017-12-20T10:28:56Z

doc/api/http.md

@@ -765,6 +770,11 @@ object, so any HTTP response sent, including response headers and payload,
 *must* be written directly to the `socket` object. Care must be taken to
 ensure the response is a properly formatted HTTP response message.

+`err` is an instance of `Error` with two extra columns:
+
+ `bytesParsed`: the bytes count of request packet that Node.js may have parsed correctly;


XadillaX · 2017-12-27T02:09:56Z

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/48/

addaleax · 2017-12-27T18:54:05Z

Landed in 6c0da34

The `rawPacket` is the current buffer that just parsed. Adding this buffer to the error object of `clientError` event is to make it possible that developers can log the broken packet. PR-URL: #17672 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069

Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 - Refactoring and cleanup of Http2Session and Http2Stream destroy (James M Snell) #17406 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069

The `rawPacket` is the current buffer that just parsed. Adding this buffer to the error object of `clientError` event is to make it possible that developers can log the broken packet. PR-URL: nodejs#17672 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

The `rawPacket` is the current buffer that just parsed. Adding this buffer to the error object of `clientError` event is to make it possible that developers can log the broken packet. PR-URL: #17672 Backport-PR-URL: #18370 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [#16413](#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [#16413](#16413) * upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260) * re land npm 5.6.0 (Myles Borins) [#18625](#18625) * ICU 60 bump (Steven R. Loomis) [#16876](#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [#16130](#16130) * warn on invalid authentication tag length (Tobias Nießen) [#17566](#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [#18004](#18004) * use typed array stack as fast path (Anna Henningsen) [#17780](#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [#17273](#17273) * separate missing from default context (Andreas Madsen) [#17273](#17273) * rename initTriggerId (Andreas Madsen) [#17273](#17273) * deprecate undocumented API (Andreas Madsen) [#16972](#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [#16998](#16998) * add trace events to async_hooks (Andreas Madsen) [#15538](#15538) * set HTTPParser trigger to socket (Andreas Madsen) [#18003](#18003) * add provider types for net server (Andreas Madsen) [#17157](#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [#16495](#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [#17033](#17033) * module: * add builtinModules (Jon Moss) [#16386](#16386) * replace default paths in require.resolve() (cjihrig) [#17113](#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * add process.ppid (cjihrig) [#16839](#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [#16267](#16267) * add rawPacket in err of `clientError` event (XadillaX) [#17672](#17672) * better support for IPv6 addresses (Mattias Holmlund) [#14772](#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [#17662](#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [#18463](#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [#17478](#17478) * process: * improve unhandled rejection message (Madara Uchiha) [#17158](#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [#12860](#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [#17196](#17196) PR-URL: #18336

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [nodejs#16413](nodejs#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [nodejs#16413](nodejs#16413) * upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260) * re land npm 5.6.0 (Myles Borins) [nodejs#18625](nodejs#18625) * ICU 60 bump (Steven R. Loomis) [nodejs#16876](nodejs#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [nodejs#16130](nodejs#16130) * warn on invalid authentication tag length (Tobias Nießen) [nodejs#17566](nodejs#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [nodejs#18004](nodejs#18004) * use typed array stack as fast path (Anna Henningsen) [nodejs#17780](nodejs#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [nodejs#17273](nodejs#17273) * separate missing from default context (Andreas Madsen) [nodejs#17273](nodejs#17273) * rename initTriggerId (Andreas Madsen) [nodejs#17273](nodejs#17273) * deprecate undocumented API (Andreas Madsen) [nodejs#16972](nodejs#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [nodejs#16998](nodejs#16998) * add trace events to async_hooks (Andreas Madsen) [nodejs#15538](nodejs#15538) * set HTTPParser trigger to socket (Andreas Madsen) [nodejs#18003](nodejs#18003) * add provider types for net server (Andreas Madsen) [nodejs#17157](nodejs#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [nodejs#16495](nodejs#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [nodejs#17033](nodejs#17033) * module: * add builtinModules (Jon Moss) [nodejs#16386](nodejs#16386) * replace default paths in require.resolve() (cjihrig) [nodejs#17113](nodejs#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * add process.ppid (cjihrig) [nodejs#16839](nodejs#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [nodejs#16267](nodejs#16267) * add rawPacket in err of `clientError` event (XadillaX) [nodejs#17672](nodejs#17672) * better support for IPv6 addresses (Mattias Holmlund) [nodejs#14772](nodejs#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [nodejs#17662](nodejs#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [nodejs#18463](nodejs#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [nodejs#17478](nodejs#17478) * process: * improve unhandled rejection message (Madara Uchiha) [nodejs#17158](nodejs#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [nodejs#12860](nodejs#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [nodejs#17196](nodejs#17196) PR-URL: nodejs#18336

http: add rawPacket in err of clientError event

e9eb792

The `rawPacket` is the current buffer that just parsed. Adding this buffer to the error object of `clientError` event is to make it possible that developers can log the broken packet.

XadillaX force-pushed the add-raw-packet-in-client-error branch from 1fa05b8 to e9eb792 Compare December 14, 2017 04:50

XadillaX mentioned this pull request Dec 14, 2017

feat: add 400 response for broken client request to instead of empty response eggjs/egg#1829

Merged

3 tasks

jasnell added semver-minor PRs that contain new features and should be released in the next minor version. http Issues or PRs related to the http subsystem. labels Dec 14, 2017

addaleax approved these changes Dec 14, 2017

View reviewed changes

update after addaleax' review

5e1369f

XadillaX force-pushed the add-raw-packet-in-client-error branch from a7c6481 to 5e1369f Compare December 15, 2017 02:31

addaleax approved these changes Dec 15, 2017

View reviewed changes

addaleax reviewed Dec 15, 2017

View reviewed changes

bnoordhuis reviewed Dec 15, 2017

View reviewed changes

update after addaleax' review

0e73e2c

XadillaX added the lts-watch-v6.x label Dec 15, 2017

update after bnoordhuis' review

9758fb1

addaleax reviewed Dec 18, 2017

View reviewed changes

update

008e5a8

XadillaX force-pushed the add-raw-packet-in-client-error branch from d4a8ff1 to 008e5a8 Compare December 18, 2017 05:59

XadillaX added the lts-watch-v8.x label Dec 20, 2017

addaleax approved these changes Dec 20, 2017

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 20, 2017

bnoordhuis approved these changes Dec 20, 2017

View reviewed changes

jasnell approved these changes Dec 20, 2017

View reviewed changes

long line

7fb6c62

addaleax closed this Dec 27, 2017

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 27, 2017

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

XadillaX mentioned this pull request Jan 25, 2018

[backport-v8.x] http: add rawPacket in err of clientError event #18370

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: add rawPacket in err of `clientError` event #17672

http: add rawPacket in err of `clientError` event #17672

XadillaX commented Dec 14, 2017 •

edited

Loading

addaleax Dec 14, 2017

XadillaX Dec 20, 2017

addaleax Dec 14, 2017

addaleax Dec 14, 2017

XadillaX Dec 15, 2017

XadillaX commented Dec 15, 2017

addaleax Dec 15, 2017

XadillaX Dec 15, 2017

bnoordhuis Dec 15, 2017

XadillaX Dec 15, 2017

XadillaX commented Dec 18, 2017

XadillaX commented Dec 18, 2017

addaleax Dec 18, 2017

addaleax Dec 18, 2017

XadillaX commented Dec 18, 2017

XadillaX commented Dec 20, 2017

bnoordhuis Dec 20, 2017

XadillaX commented Dec 27, 2017

addaleax commented Dec 27, 2017

http: add rawPacket in err of clientError event #17672

http: add rawPacket in err of clientError event #17672

Conversation

XadillaX commented Dec 14, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX commented Dec 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX commented Dec 18, 2017

XadillaX commented Dec 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX commented Dec 18, 2017

XadillaX commented Dec 20, 2017

Choose a reason for hiding this comment

XadillaX commented Dec 27, 2017

addaleax commented Dec 27, 2017

http: add rawPacket in err of `clientError` event #17672

http: add rawPacket in err of `clientError` event #17672

XadillaX commented Dec 14, 2017 •

edited

Loading