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

fix: premature end large body #805

Merged
merged 12 commits into from May 12, 2021
Merged
Show file tree
Hide file tree
Changes from 10 commits
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
450 changes: 225 additions & 225 deletions deps/llhttp/src/llhttp.c

Large diffs are not rendered by default.

25 changes: 13 additions & 12 deletions docs/api/Errors.md
Expand Up @@ -7,15 +7,16 @@ You can find all the error objects inside the `errors` key.
const { errors } = require('undici')
```

| Error | Error Codes | Description |
| ------------------------------------|---------------------------------------|---------------------------------------------------|
| `InvalidArgumentError` | `UND_ERR_INVALID_ARG` | passed an invalid argument. |
| `InvalidReturnValueError` | `UND_ERR_INVALID_RETURN_VALUE` | returned an invalid value. |
| `RequestAbortedError` | `UND_ERR_ABORTED` | the request has been aborted by the user |
| `ClientDestroyedError` | `UND_ERR_DESTROYED` | trying to use a destroyed client. |
| `ClientClosedError` | `UND_ERR_CLOSED` | trying to use a closed client. |
| `SocketError` | `UND_ERR_SOCKET` | there is an error with the socket. |
| `NotSupportedError` | `UND_ERR_NOT_SUPPORTED` | encountered unsupported functionality. |
| `RequestContentLengthMismatchError` | `UND_ERR_REQ_CONTENT_LENGTH_MISMATCH`| request body does not match content-length header |
| `InformationalError` | `UND_ERR_INFO` | expected error with reason |
| `TrailerMismatchError` | `UND_ERR_TRAILER_MISMATCH` | trailers did not match specification |
| Error | Error Codes | Description |
| ------------------------------------|---------------------------------------|----------------------------------------------------|
| `InvalidArgumentError` | `UND_ERR_INVALID_ARG` | passed an invalid argument. |
| `InvalidReturnValueError` | `UND_ERR_INVALID_RETURN_VALUE` | returned an invalid value. |
| `RequestAbortedError` | `UND_ERR_ABORTED` | the request has been aborted by the user |
| `ClientDestroyedError` | `UND_ERR_DESTROYED` | trying to use a destroyed client. |
| `ClientClosedError` | `UND_ERR_CLOSED` | trying to use a closed client. |
| `SocketError` | `UND_ERR_SOCKET` | there is an error with the socket. |
| `NotSupportedError` | `UND_ERR_NOT_SUPPORTED` | encountered unsupported functionality. |
| `RequestContentLengthMismatchError` | `UND_ERR_REQ_CONTENT_LENGTH_MISMATCH`| request body does not match content-length header |
| `RequestContentLengthMismatchError` | `UND_ERR_RES_CONTENT_LENGTH_MISMATCH`| response body does not match content-length header |
| `InformationalError` | `UND_ERR_INFO` | expected error with reason |
| `TrailerMismatchError` | `UND_ERR_TRAILER_MISMATCH` | trailers did not match specification |
18 changes: 17 additions & 1 deletion lib/client.js
Expand Up @@ -9,6 +9,7 @@ const Request = require('./core/request')
const Dispatcher = require('./dispatcher')
const {
RequestContentLengthMismatchError,
ResponseContentLengthMismatchError,
TrailerMismatchError,
InvalidArgumentError,
RequestAbortedError,
Expand Down Expand Up @@ -448,8 +449,11 @@ class Parser {
this.paused = false
this.resume = this.resume.bind(this)

this.bytesRead = 0

this.trailer = ''
this.keepAlive = ''
this.contentLength = ''
}

setTimeout (value, type) {
Expand Down Expand Up @@ -590,6 +594,8 @@ class Parser {
this.keepAlive += buf.toString()
} else if (key.length === 7 && key.toString().toLowerCase() === 'trailer') {
this.trailer += buf.toString()
} else if (key.length === 14 && key.toString().toLowerCase() === 'content-length') {
this.contentLength += buf.toString()
}

this.trackHeader(buf.length)
Expand Down Expand Up @@ -780,6 +786,8 @@ class Parser {

assert(statusCode >= 200)

this.bytesRead += buf.length

try {
if (request.onData(buf) === false) {
return constants.ERROR.PAUSED
Expand All @@ -790,7 +798,7 @@ class Parser {
}

onMessageComplete () {
const { client, socket, statusCode, upgrade, trailer, headers, shouldKeepAlive } = this
const { client, socket, statusCode, upgrade, trailer, headers, contentLength, bytesRead, shouldKeepAlive } = this

if (socket.destroyed && (!statusCode || shouldKeepAlive)) {
return -1
Expand All @@ -806,6 +814,8 @@ class Parser {
assert(statusCode >= 100)

this.statusCode = null
this.bytesRead = 0
this.contentLength = ''
this.trailer = ''
this.keepAlive = ''

Expand Down Expand Up @@ -834,6 +844,12 @@ class Parser {
}
}

/* istanbul ignore next */
if (request.method !== 'HEAD' && contentLength && bytesRead !== parseInt(contentLength, 10)) {
util.destroy(socket, new ResponseContentLengthMismatchError())
return -1
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this ignored by istanbul? Can't we reproduce?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think llhttp doesn't actually allow it, i.e. it shouldn't be possible.

Copy link
Member Author

@ronag ronag May 12, 2021

Choose a reason for hiding this comment

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

I will have to dig into it further. Just to tired right now. Only slept 6 hours in total this week 😞 .

Copy link
Contributor

Choose a reason for hiding this comment

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

Please take some rest 🙏🏻


try {
request.onComplete(headers)
} catch (err) {
Expand Down
15 changes: 13 additions & 2 deletions lib/core/errors.js
Expand Up @@ -94,7 +94,17 @@ class RequestContentLengthMismatchError extends UndiciError {
Error.captureStackTrace(this, RequestContentLengthMismatchError)
this.name = 'RequestContentLengthMismatchError'
this.message = message || 'Request body length does not match content-length header'
this.code = 'UND_ERR_CONTENT_LENGTH_MISMATCH'
this.code = 'UND_ERR_REQ_CONTENT_LENGTH_MISMATCH'
}
}

class ResponseContentLengthMismatchError extends UndiciError {
constructor (message) {
super(message)
Error.captureStackTrace(this, ResponseContentLengthMismatchError)
this.name = 'ResponseContentLengthMismatchError'
this.message = message || 'Response body length does not match content-length header'
this.code = 'UND_ERR_RES_CONTENT_LENGTH_MISMATCH'
}
}

Expand Down Expand Up @@ -163,5 +173,6 @@ module.exports = {
ClientClosedError,
InformationalError,
SocketError,
NotSupportedError
NotSupportedError,
ResponseContentLengthMismatchError
}
2 changes: 1 addition & 1 deletion lib/core/request.js
Expand Up @@ -189,7 +189,7 @@ function processHeader (request, key, val) {
key.length === 14 &&
key.toLowerCase() === 'content-length'
) {
request.contentLength = parseInt(val)
request.contentLength = parseInt(val, 10)
if (!Number.isFinite(request.contentLength)) {
throw new InvalidArgumentError('invalid content-length header')
}
Expand Down
2 changes: 1 addition & 1 deletion lib/core/util.js
Expand Up @@ -147,7 +147,7 @@ function destroy (stream, err) {
const KEEPALIVE_TIMEOUT_EXPR = /timeout=(\d+)/
function parseKeepAliveTimeout (val) {
const m = val.toString().match(KEEPALIVE_TIMEOUT_EXPR)
return m ? parseInt(m[1]) * 1000 : null
return m ? parseInt(m[1], 10) * 1000 : null
}

function parseHeaders (headers, obj = {}) {
Expand Down
Binary file modified lib/llhttp/llhttp.wasm
Binary file not shown.
Binary file modified lib/llhttp/llhttp_simd.wasm
Binary file not shown.
2 changes: 1 addition & 1 deletion test/errors.js
Expand Up @@ -21,7 +21,7 @@ const scenarios = [
createScenario(errors.InvalidReturnValueError, 'Invalid Return Value Error', 'InvalidReturnValueError', 'UND_ERR_INVALID_RETURN_VALUE'),
createScenario(errors.RequestAbortedError, 'Request aborted', 'RequestAbortedError', 'UND_ERR_ABORTED'),
createScenario(errors.InformationalError, 'Request information', 'InformationalError', 'UND_ERR_INFO'),
createScenario(errors.RequestContentLengthMismatchError, 'Request body length does not match content-length header', 'RequestContentLengthMismatchError', 'UND_ERR_CONTENT_LENGTH_MISMATCH'),
createScenario(errors.RequestContentLengthMismatchError, 'Request body length does not match content-length header', 'RequestContentLengthMismatchError', 'UND_ERR_REQ_CONTENT_LENGTH_MISMATCH'),
createScenario(errors.TrailerMismatchError, 'Trailers does not match trailer header', 'TrailerMismatchError', 'UND_ERR_TRAILER_MISMATCH'),
createScenario(errors.ClientDestroyedError, 'The client is destroyed', 'ClientDestroyedError', 'UND_ERR_DESTROYED'),
createScenario(errors.ClientClosedError, 'The client is closed', 'ClientClosedError', 'UND_ERR_CLOSED'),
Expand Down
47 changes: 47 additions & 0 deletions test/issue-803.js
@@ -0,0 +1,47 @@
'use strict'

const { test } = require('tap')
const { Client } = require('..')
const { createServer } = require('http')
const EE = require('events')

test('https://github.com/nodejs/undici/issues/803', (t) => {
t.plan(2)

const SIZE = 5900373096

const server = createServer(async (req, res) => {
res.setHeader('content-length', SIZE)
let pos = 0
while (pos < SIZE) {
const len = Math.min(SIZE - pos, 65536)
if (!res.write(Buffer.allocUnsafe(len))) {
await EE.once(res, 'drain')
}
pos += len
}

res.end()
})
t.teardown(server.close.bind(server))

server.listen(0, () => {
const client = new Client(`http://localhost:${server.address().port}`)
t.teardown(client.close.bind(client))

client.request({
path: '/',
method: 'GET'
}, (err, data) => {
t.error(err)

let pos = 0
data.body.on('data', (buf) => {
pos += buf.length
})
data.body.on('end', () => {
t.equal(pos, SIZE)
})
})
})
})
2 changes: 1 addition & 1 deletion test/types/errors.test-d.ts
Expand Up @@ -36,7 +36,7 @@ expectAssignable<'UND_ERR_INFO'>(new errors.InformationalError().code)
expectAssignable<errors.UndiciError>(new errors.RequestContentLengthMismatchError())
expectAssignable<errors.RequestContentLengthMismatchError>(new errors.RequestContentLengthMismatchError())
expectAssignable<'RequestContentLengthMismatchError'>(new errors.RequestContentLengthMismatchError().name)
expectAssignable<'UND_ERR_CONTENT_LENGTH_MISMATCH'>(new errors.RequestContentLengthMismatchError().code)
expectAssignable<'UND_ERR_REQ_CONTENT_LENGTH_MISMATCH'>(new errors.RequestContentLengthMismatchError().code)

expectAssignable<errors.UndiciError>(new errors.ClientDestroyedError())
expectAssignable<errors.ClientDestroyedError>(new errors.ClientDestroyedError())
Expand Down