From 02689579200d9d0749edd746b26ebf920fb5be1d Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 10 Feb 2023 08:53:52 +0100 Subject: [PATCH 01/11] http: correctly calculate strict content length Fixes some logical errors related to strict content length. Also, previously Buffer.byteLength (which is slow) was run regardless of whether or not the len was required. --- lib/_http_outgoing.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index ea3d7e62556212..4219d4679f6bcf 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -873,14 +873,9 @@ function write_(msg, chunk, encoding, callback, fromEnd) { if (typeof callback !== 'function') callback = nop; - let len; if (chunk === null) { throw new ERR_STREAM_NULL_VALUES(); - } else if (typeof chunk === 'string') { - len = Buffer.byteLength(chunk, encoding); - } else if (isUint8Array(chunk)) { - len = chunk.length; - } else { + } else if (typeof chunk !== 'string' && !isUint8Array(chunk)) { throw new ERR_INVALID_ARG_TYPE( 'chunk', ['string', 'Buffer', 'Uint8Array'], chunk); } @@ -901,8 +896,11 @@ function write_(msg, chunk, encoding, callback, fromEnd) { return false; } + let len; + if (!msg._header) { if (fromEnd) { + len ??= typeof chunk === 'string' ? Buffer.byteLength(chunk) : chunk.byteLength; msg._contentLength = len; } msg._implicitHeader(); @@ -922,6 +920,7 @@ function write_(msg, chunk, encoding, callback, fromEnd) { let ret; if (msg.chunkedEncoding && chunk.length !== 0) { + len ??= typeof chunk === 'string' ? Buffer.byteLength(chunk) : chunk.byteLength; msg._send(NumberPrototypeToString(len, 16), 'latin1', null); msg._send(crlf_buf, null, null); msg._send(chunk, encoding, null); From 23d253ab1e555235d60885dfed23f3757253a18e Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 10 Feb 2023 09:00:33 +0100 Subject: [PATCH 02/11] http: speedup strict content length --- lib/_http_outgoing.js | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 4219d4679f6bcf..620e8ce66924fb 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -351,7 +351,7 @@ OutgoingMessage.prototype.destroy = function destroy(error) { // This abstract either writing directly to the socket or buffering it. -OutgoingMessage.prototype._send = function _send(data, encoding, callback) { +OutgoingMessage.prototype._send = function _send(data, encoding, callback, byteLength) { // This is a shameful hack to get the headers and first body chunk onto // the same packet. Future versions of Node are going to take care of // this at a lower level and in a more general way. @@ -373,20 +373,11 @@ OutgoingMessage.prototype._send = function _send(data, encoding, callback) { } this._headerSent = true; } - return this._writeRaw(data, encoding, callback); + return this._writeRaw(data, encoding, callback, byteLength); }; -function _getMessageBodySize(chunk, headers, encoding) { - if (Buffer.isBuffer(chunk)) return chunk.length; - const chunkLength = chunk ? Buffer.byteLength(chunk, encoding) : 0; - const headerLength = headers ? headers.length : 0; - if (headerLength === chunkLength) return 0; - if (headerLength < chunkLength) return MathAbs(chunkLength - headerLength); - return chunkLength; -} - OutgoingMessage.prototype._writeRaw = _writeRaw; -function _writeRaw(data, encoding, callback) { +function _writeRaw(data, encoding, callback, size) { const conn = this.socket; if (conn && conn.destroyed) { // The socket was destroyed. If we're still trying to write to it, @@ -400,12 +391,10 @@ function _writeRaw(data, encoding, callback) { } // TODO(sidwebworks): flip the `strictContentLength` default to `true` in a future PR - if (this.strictContentLength && conn && conn.writable && !this._removedContLen && this._hasBody) { + if (this.strictContentLength && size != null && conn && conn.writable && !this._removedContLen && this._hasBody) { const skip = conn._httpMessage.statusCode === 304 || (this.hasHeader('transfer-encoding') || this.chunkedEncoding); if (typeof this._contentLength === 'number' && !skip) { - const size = _getMessageBodySize(data, conn._httpMessage._header, encoding); - if ((size + this[kBytesWritten]) > this._contentLength) { throw new ERR_HTTP_CONTENT_LENGTH_MISMATCH(size + this[kBytesWritten], this._contentLength); } @@ -898,6 +887,10 @@ function write_(msg, chunk, encoding, callback, fromEnd) { let len; + if (this.strictContentLength) { + len = typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength; + } + if (!msg._header) { if (fromEnd) { len ??= typeof chunk === 'string' ? Buffer.byteLength(chunk) : chunk.byteLength; @@ -920,13 +913,13 @@ function write_(msg, chunk, encoding, callback, fromEnd) { let ret; if (msg.chunkedEncoding && chunk.length !== 0) { - len ??= typeof chunk === 'string' ? Buffer.byteLength(chunk) : chunk.byteLength; + len ??= typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength; msg._send(NumberPrototypeToString(len, 16), 'latin1', null); msg._send(crlf_buf, null, null); - msg._send(chunk, encoding, null); + msg._send(chunk, encoding, null, len); ret = msg._send(crlf_buf, null, callback); } else { - ret = msg._send(chunk, encoding, callback); + ret = msg._send(chunk, encoding, callback, len); } debug('write ret = ' + ret); @@ -1039,7 +1032,7 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) { if (this._hasBody && this.chunkedEncoding) { this._send('0\r\n' + this._trailer + '\r\n', 'latin1', finish); } else if (!this._headerSent || this.writableLength || chunk) { - this._send('', 'latin1', finish); + this._send('', 'latin1', finish, 0); } else { process.nextTick(finish); } From 1a3683bf990e7a53ab9f340a682aa2bb428525ab Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 10 Feb 2023 09:22:20 +0100 Subject: [PATCH 03/11] http: make sure to pass encoding to Buffer.byteLength --- lib/_http_outgoing.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 620e8ce66924fb..4b859a154a2ec0 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -893,7 +893,7 @@ function write_(msg, chunk, encoding, callback, fromEnd) { if (!msg._header) { if (fromEnd) { - len ??= typeof chunk === 'string' ? Buffer.byteLength(chunk) : chunk.byteLength; + len ??= typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength; msg._contentLength = len; } msg._implicitHeader(); From db11211cd9fce7a1c896b6858000a1897c1b22d0 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 10 Feb 2023 09:35:34 +0100 Subject: [PATCH 04/11] http: count writtenBytes even if socket is not ready --- lib/_http_outgoing.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 4b859a154a2ec0..7e4d3073e9f212 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -391,7 +391,7 @@ function _writeRaw(data, encoding, callback, size) { } // TODO(sidwebworks): flip the `strictContentLength` default to `true` in a future PR - if (this.strictContentLength && size != null && conn && conn.writable && !this._removedContLen && this._hasBody) { + if (this.strictContentLength && size != null && !this._removedContLen && this._hasBody) { const skip = conn._httpMessage.statusCode === 304 || (this.hasHeader('transfer-encoding') || this.chunkedEncoding); if (typeof this._contentLength === 'number' && !skip) { From 30a9aebc3c840365b3982cd7144bb021f59cef0c Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 10 Feb 2023 09:38:45 +0100 Subject: [PATCH 05/11] fixup! http: speedup strict content length --- lib/_http_outgoing.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 7e4d3073e9f212..9608b2acb6be4c 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -1032,7 +1032,7 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) { if (this._hasBody && this.chunkedEncoding) { this._send('0\r\n' + this._trailer + '\r\n', 'latin1', finish); } else if (!this._headerSent || this.writableLength || chunk) { - this._send('', 'latin1', finish, 0); + this._send('', 'latin1', finish); } else { process.nextTick(finish); } From 0729f475d72a3a7a586a54d2f93081e3733b0699 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 10 Feb 2023 09:57:43 +0100 Subject: [PATCH 06/11] fixup! http: speedup strict content length --- lib/_http_outgoing.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 9608b2acb6be4c..e8afa7226af8a6 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -25,7 +25,6 @@ const { Array, ArrayIsArray, ArrayPrototypeJoin, - MathAbs, MathFloor, NumberPrototypeToString, ObjectDefineProperty, From fe6f80a43b38990a413959a8b84e1d09b6acb515 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 15 Feb 2023 08:43:25 +0100 Subject: [PATCH 07/11] fixuP --- lib/_http_outgoing.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index e8afa7226af8a6..c0733a4606561e 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -886,7 +886,7 @@ function write_(msg, chunk, encoding, callback, fromEnd) { let len; - if (this.strictContentLength) { + if (msg.strictContentLength) { len = typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength; } From ab2c162a778b177345d9b8d9dc74862ea728a58f Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Thu, 16 Feb 2023 11:15:28 +0100 Subject: [PATCH 08/11] fixuP --- lib/_http_outgoing.js | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index c0733a4606561e..eee2ecb77d689f 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -85,7 +85,6 @@ const HIGH_WATER_MARK = getDefaultHighWaterMark(); const kCorked = Symbol('corked'); const kUniqueHeaders = Symbol('kUniqueHeaders'); const kBytesWritten = Symbol('kBytesWritten'); -const kEndCalled = Symbol('kEndCalled'); const kErrored = Symbol('errored'); const nop = () => {}; @@ -128,7 +127,6 @@ function OutgoingMessage() { this.strictContentLength = false; this[kBytesWritten] = 0; - this[kEndCalled] = false; this._contentLength = null; this._hasBody = true; this._trailer = ''; @@ -389,21 +387,18 @@ function _writeRaw(data, encoding, callback, size) { encoding = null; } - // TODO(sidwebworks): flip the `strictContentLength` default to `true` in a future PR - if (this.strictContentLength && size != null && !this._removedContLen && this._hasBody) { - const skip = conn._httpMessage.statusCode === 304 || (this.hasHeader('transfer-encoding') || this.chunkedEncoding); - - if (typeof this._contentLength === 'number' && !skip) { - if ((size + this[kBytesWritten]) > this._contentLength) { - throw new ERR_HTTP_CONTENT_LENGTH_MISMATCH(size + this[kBytesWritten], this._contentLength); - } - - if (this[kEndCalled] && (size + this[kBytesWritten]) !== this._contentLength) { - throw new ERR_HTTP_CONTENT_LENGTH_MISMATCH(size + this[kBytesWritten], this._contentLength); - } - - this[kBytesWritten] += size; + if (size != null) { + if ( + this.strictContentLength && + this._hasBody && + !this._removedContLen && + !this.chunkedEncoding && + this[kBytesWritten] + size > this._contentLength && + !this.hasHeader('transfer-encoding') + ) { + throw new ERR_HTTP_CONTENT_LENGTH_MISMATCH(size + this[kBytesWritten], this._contentLength); } + this[kBytesWritten] += size; } if (conn && conn._httpMessage === this && conn.writable) { @@ -990,8 +985,6 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) { encoding = null; } - this[kEndCalled] = true; - if (chunk) { if (this.finished) { onError(this, @@ -1026,6 +1019,17 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) { if (typeof callback === 'function') this.once('finish', callback); + if ( + this.strictContentLength && + this._hasBody && + !this._removedContLen && + !this.chunkedEncoding && + this[kBytesWritten] !== this._contentLength && + !this.hasHeader('transfer-encoding') + ) { + throw new ERR_HTTP_CONTENT_LENGTH_MISMATCH(this[kBytesWritten], this._contentLength); + } + const finish = onFinish.bind(undefined, this); if (this._hasBody && this.chunkedEncoding) { From a1c9fc7891e71c9865af7389e234c491181ada4b Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Thu, 16 Feb 2023 11:31:22 +0100 Subject: [PATCH 09/11] fixup --- lib/_http_outgoing.js | 45 +++++++++---------- .../test-http-content-length-mismatch.js | 2 +- 2 files changed, 21 insertions(+), 26 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index eee2ecb77d689f..c7f13ddc0f71a2 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -387,20 +387,6 @@ function _writeRaw(data, encoding, callback, size) { encoding = null; } - if (size != null) { - if ( - this.strictContentLength && - this._hasBody && - !this._removedContLen && - !this.chunkedEncoding && - this[kBytesWritten] + size > this._contentLength && - !this.hasHeader('transfer-encoding') - ) { - throw new ERR_HTTP_CONTENT_LENGTH_MISMATCH(size + this[kBytesWritten], this._contentLength); - } - this[kBytesWritten] += size; - } - if (conn && conn._httpMessage === this && conn.writable) { // There might be pending data in the this.output buffer. if (this.outputData.length) { @@ -852,6 +838,17 @@ function emitErrorNt(msg, err, callback) { } } +function strictContentLength(msg) { + return ( + msg._contentLength != null && + msg.strictContentLength && + msg._hasBody && + !msg._removedContLen && + !msg.chunkedEncoding && + !msg.hasHeader('transfer-encoding') + ) +} + function write_(msg, chunk, encoding, callback, fromEnd) { if (typeof callback !== 'function') callback = nop; @@ -879,10 +876,15 @@ function write_(msg, chunk, encoding, callback, fromEnd) { return false; } - let len; + let len = msg.strictContentLength ? + typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength : null; - if (msg.strictContentLength) { - len = typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength; + if (len != null) { + if (strictContentLength(msg) && (fromEnd ? msg[kBytesWritten] + len !== msg._contentLength : msg[kBytesWritten] + len > msg._contentLength) + ) { + throw new ERR_HTTP_CONTENT_LENGTH_MISMATCH(len + msg[kBytesWritten], msg._contentLength); + } + msg[kBytesWritten] += len; } if (!msg._header) { @@ -1019,14 +1021,7 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) { if (typeof callback === 'function') this.once('finish', callback); - if ( - this.strictContentLength && - this._hasBody && - !this._removedContLen && - !this.chunkedEncoding && - this[kBytesWritten] !== this._contentLength && - !this.hasHeader('transfer-encoding') - ) { + if (strictContentLength(this) && this[kBytesWritten] !== this._contentLength) { throw new ERR_HTTP_CONTENT_LENGTH_MISMATCH(this[kBytesWritten], this._contentLength); } diff --git a/test/parallel/test-http-content-length-mismatch.js b/test/parallel/test-http-content-length-mismatch.js index fc294db16407bc..540acbe759d84a 100644 --- a/test/parallel/test-http-content-length-mismatch.js +++ b/test/parallel/test-http-content-length-mismatch.js @@ -57,7 +57,7 @@ function shouldThrowOnFewerBytes() { res.write('a'); res.statusCode = 200; assert.throws(() => { - res.end(); + res.end('aaa'); }, { code: 'ERR_HTTP_CONTENT_LENGTH_MISMATCH' }); From e259ee6810e702bc6cfa6aaeb2a027ef2ed61334 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Thu, 16 Feb 2023 20:06:10 +0100 Subject: [PATCH 10/11] fixup: linting --- lib/_http_outgoing.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index c7f13ddc0f71a2..7ac93bd3d8f498 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -840,13 +840,13 @@ function emitErrorNt(msg, err, callback) { function strictContentLength(msg) { return ( - msg._contentLength != null && msg.strictContentLength && + msg._contentLength != null && msg._hasBody && !msg._removedContLen && !msg.chunkedEncoding && !msg.hasHeader('transfer-encoding') - ) + ); } function write_(msg, chunk, encoding, callback, fromEnd) { @@ -880,7 +880,9 @@ function write_(msg, chunk, encoding, callback, fromEnd) { typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength : null; if (len != null) { - if (strictContentLength(msg) && (fromEnd ? msg[kBytesWritten] + len !== msg._contentLength : msg[kBytesWritten] + len > msg._contentLength) + if ( + strictContentLength(msg) && + (fromEnd ? msg[kBytesWritten] + len !== msg._contentLength : msg[kBytesWritten] + len > msg._contentLength) ) { throw new ERR_HTTP_CONTENT_LENGTH_MISMATCH(len + msg[kBytesWritten], msg._contentLength); } From 20c7f2faa5f433953584320fd1c3cc6ab63864ed Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 17 Feb 2023 10:39:49 +0100 Subject: [PATCH 11/11] fixup: cleanup --- lib/_http_outgoing.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 7ac93bd3d8f498..d8296ec34a1cc0 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -876,16 +876,18 @@ function write_(msg, chunk, encoding, callback, fromEnd) { return false; } - let len = msg.strictContentLength ? - typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength : null; + let len; + + if (msg.strictContentLength) { + len ??= typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength; - if (len != null) { if ( strictContentLength(msg) && (fromEnd ? msg[kBytesWritten] + len !== msg._contentLength : msg[kBytesWritten] + len > msg._contentLength) ) { throw new ERR_HTTP_CONTENT_LENGTH_MISMATCH(len + msg[kBytesWritten], msg._contentLength); } + msg[kBytesWritten] += len; }