fix(server): end range responses when the file stream errors - #42716
Sebastien Tardif (SebTardif) wants to merge 2 commits into
Conversation
A Range request used createReadStream without an error handler. If the file became unreadable after stat, the unhandled error could crash the process. End the HTTP response instead. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Do not write 206 until the file is open. An early read error now ends with 500 instead of a hung client. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Test results for "tests 1"1 failed 6 flaky51737 passed, 1247 skipped Merge workflow run. |
Test results for "MCP"8564 passed, 1446 skipped Merge workflow run. |
|
|
||
| const readable = fs.createReadStream(absoluteFilePath, { start, end }); | ||
| readable.pipe(response); | ||
| readable.on('error', error => { |
There was a problem hiding this comment.
is this something that can actually be hit by real user code? as far as i can tell we haven't had any reports of this being an issue so im not sure it's actually necessary to fix (especially if it's not possible to hit due to other existing guards elsewhere)
There was a problem hiding this comment.
is this something that can actually be hit by real user code?
No path we can show through show-report / show-trace without a chmod race after statSync. I am closing this.
|
Closing: no public / path without a post-stat race. |
|
Closing: no public |
2 participants
Summary
ReadStreamerrors, instead of leaving an unhandled'error'.Problem
HttpServer._serveRangeFileusedcreateReadStream(...).pipe(response)with no'error'handler. The non-range path usesreadFileSyncinside try/catch. If the file became unreadable afterstatSync, Node could emit an unhandled stream error and crash the HTML report or trace-viewer server.Change
Wait for
'open'before sending206. If the stream errors first, respond500. If headers were already sent, destroy the socket.Validation
tests/library/http-server.spec.ts: happy-pathRange: bytes=2-5returnscdef.500(skipped on Windows and as root).