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.

Sign up for GitHub

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

Jump to bottom

perf: optimize check invalid field-vchar #2889

Merged
merged 4 commits into from
Mar 1, 2024
Merged

perf: optimize check invalid field-vchar #2889

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb3ba94
perf: optimize check invalid field-vchar
tsctx Feb 29, 2024
75fa334
add benchmark, use named import, fix regression
tsctx Feb 29, 2024
1cf6928
fix benchmark
tsctx Feb 29, 2024
e73a0f3
apply suggestions from code review
tsctx Mar 1, 2024
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
24 changes: 6 additions & 18 deletions lib/core/request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,6 @@ const util = require('./util')
const { channels } = require('./diagnostics.js')
const { headerNameLowerCasedRecord } = require('./constants')

// headerCharRegex have been lifted from
// https://github.com/nodejs/node/blob/main/lib/_http_common.js

/**
* Matches if val contains an invalid field-vchar
* field-value = *( field-content / obs-fold )
* field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ]
* field-vchar = VCHAR / obs-text
*/
const headerCharRegex = /[^\t\x20-\x7e\x80-\xff]/

// Verifies that a given path is valid does not contain control chars \x00 to \x20
const invalidPathRegex = /[^\u0021-\u00ff]/

Expand Down Expand Up @@ -166,13 +155,12 @@ class Request {
if (!Array.isArray(header) || header.length !== 2) {
throw new InvalidArgumentError('headers must be in key-value pair format')
}
const [key, value] = header
processHeader(this, key, value)
processHeader(this, header[0], header[1])
}
} else {
const keys = Object.keys(headers)
for (const key of keys) {
processHeader(this, key, headers[key])
for (let i = 0; i < keys.length; ++i) {
processHeader(this, keys[i], headers[keys[i]])
}
}
} else if (headers != null) {
Expand Down Expand Up @@ -315,7 +303,7 @@ class Request {
}
}

function processHeader (request, key, val, skipAppend = false) {
function processHeader (request, key, val) {
if (val && (typeof val === 'object' && !Array.isArray(val))) {
throw new InvalidArgumentError(`invalid ${key} header`)
} else if (val === undefined) {
Expand All @@ -335,7 +323,7 @@ function processHeader (request, key, val, skipAppend = false) {
const arr = []
for (let i = 0; i < val.length; i++) {
if (typeof val[i] === 'string') {
if (headerCharRegex.exec(val[i]) !== null) {
if (!util.isValidHeaderChar(val[i])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have named imports instead of default import?

Copy link
Member

Choose a reason for hiding this comment

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

it'll be slower on longer headers and faster on shorter, a benchmark for every utility function isn't needed

Copy link
Member

Choose a reason for hiding this comment

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

although it would be nice to see if this actually does improve performance, and if it does, maybe upstream it to core

Copy link
Contributor

Choose a reason for hiding this comment

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

a benchmark would help to verify the proposal. e.g. using exec means instantiating an object to return. So why was the regex not using .test(), which always returns true and false?

Copy link
Member Author

Choose a reason for hiding this comment

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

So why was the regex not using .test(), which always returns true and false?

I think this is because exec is faster when primordials are used.

throw new InvalidArgumentError(`invalid ${key} header`)
}
arr.push(val[i])
Expand All @@ -349,7 +337,7 @@ function processHeader (request, key, val, skipAppend = false) {
}
val = arr
} else if (typeof val === 'string') {
if (headerCharRegex.exec(val) !== null) {
if (!util.isValidHeaderChar(val)) {
throw new InvalidArgumentError(`invalid ${key} header`)
}
} else if (val === null) {
Expand Down
20 changes: 19 additions & 1 deletion lib/core/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,24 @@ function isValidHTTPToken (characters) {
return true
}

/**
* @param {string} characters
*/
function isValidHeaderChar (characters) {
tsctx marked this conversation as resolved.
Show resolved Hide resolved
// Validate if val is a valid field-vchar.
// field-value = *( field-content / obs-fold )
// field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ]
// field-vchar = VCHAR / obs-text
for (let i = 0; i < characters.length; ++i) {
const code = characters.charCodeAt(i)
// not \x20-\x7e, \t and \x80-\xff
if ((code < 0x20 && code !== 0x09) || code === 0x7f || code > 0xff) {
return false
}
}
return true
}

// Parsed accordingly to RFC 9110
// https://www.rfc-editor.org/rfc/rfc9110#field.content-range
function parseRangeHeader (range) {
Expand All @@ -516,7 +534,6 @@ kEnumerableProperty.enumerable = true
module.exports = {
kEnumerableProperty,
nop,

isDisturbed,
isErrored,
isReadable,
Expand Down Expand Up @@ -546,6 +563,7 @@ module.exports = {
buildURL,
addAbortListener,
isValidHTTPToken,
isValidHeaderChar,
isTokenCharCode,
parseRangeHeader,
nodeMajor,
Expand Down