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

[v8.x] cli: add --max-http-header-size flag #25171

Closed

Conversation

MylesBorins
Copy link
Contributor

backport of #24811

Did not backport 436f4de (not applicable to legacy option parser)
632f263 needed to be recreated from scratch PTAL... comments inline about major differences

This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

PR-URL: nodejs#24811
Fixes: nodejs#24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v8.x labels Dec 21, 2018
@@ -110,6 +110,10 @@ Activate inspector on host:port and break at start of user script.
.BR \-\-inspect-port \fI=[host:]port\fR
Set the host:port to be used when the inspector is activated.

.TP
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this needed to be rewritten to match style in 8.x PTAL and lmk if it looks right

@@ -201,6 +201,8 @@ unsigned int reverted = 0;
std::string icu_data_dir; // NOLINT(runtime/string)
#endif

uint64_t max_http_header_size = 8 * 1024;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is now exposed the way options were exposed in the past, PTAL

src/node.h Outdated Show resolved Hide resolved
@@ -88,6 +88,10 @@ static void InitConfig(Local<Object> target,
if (env->abort_on_uncaught_exception())
READONLY_BOOLEAN_PROPERTY("shouldAbortOnUncaughtException");

READONLY_PROPERTY(target,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as there is no longer getOptions I have appended the property of max_http_header_size to the process.bindings('config')... is this the best way to do it?

Copy link
Member

@addaleax addaleax Dec 21, 2018

Choose a reason for hiding this comment

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

If we need to expose this, then yes, this is probably the best way (do we need to, though?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the option is exposed in 10.x and higher via getOptions... so was trying to keep the behavior equivalent.

Don't need to though, open to reverting this

@@ -755,6 +755,9 @@ const struct http_parser_settings Parser::settings = {
nullptr // on_chunk_complete
};

void InitMaxHttpHeaderSizeOnce() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the exact same as the 10.x PR aside from the options parsing

const MAX = 8 * 1024; // 8KB
const MAX = +(process.argv[2] || 8 * 1024); // Command line option, or 8KB.

assert(process.binding('config').maxHTTPHeaderSize,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

extra assert here to confirm that the value is on the process.binding('config')

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Dec 21, 2018

This broke some stuff... digging in

edit: it was a missing comma 🤦🏽‍♀️

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Dec 21, 2018

Allow the maximum size of HTTP headers to be overridden from
the command line.

co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: nodejs#24811
Fixes: nodejs#24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2018
This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

Backport-PR-URL: #25171
PR-URL: #24811
Fixes: #24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2018
Allow the maximum size of HTTP headers to be overridden from
the command line.

Backport-PR-URL: #25171
co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #24811
Fixes: #24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@MylesBorins
Copy link
Contributor Author

landed in 3812a99...d6c029d

@@ -3468,6 +3473,8 @@ static void ParseArgs(int* argc,
new_v8_argc += 1;
} else if (strncmp(arg, "--v8-pool-size=", 15) == 0) {
v8_thread_pool_size = atoi(arg + 15);
} else if (strncmp(arg, "--max-http-header-size=", 23) == 0) {
max_http_header_size = atoi(arg + 23);
Copy link
Member

Choose a reason for hiding this comment

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

this might be strtoul() instead?

MylesBorins pushed a commit that referenced this pull request Dec 21, 2018
This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

Backport-PR-URL: #25171
PR-URL: #24811
Fixes: #24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2018
Allow the maximum size of HTTP headers to be overridden from
the command line.

Backport-PR-URL: #25171
co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #24811
Fixes: #24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 22, 2018
This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

Backport-PR-URL: #25171
PR-URL: #24811
Fixes: #24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 22, 2018
Allow the maximum size of HTTP headers to be overridden from
the command line.

Backport-PR-URL: #25171
co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #24811
Fixes: #24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants