build: make HTTP_MAX_HEADER_SIZE configurable

[mcollina]

The maximum size of headers introduced in the security release of
2018/11/27 is not configurable. This change adds a
--http-max-header-size option to ./configure.

See: #24693

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[nodejs-github-bot]

@mcollina build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1719/pipeline

[mcollina]

CI: https://ci.nodejs.org/job/node-test-pull-request/19032/

[mcollina]

cc @nodejs/lts this will need to be backported on all lines. Sorry about not adding this in the security release.

[mcollina]

cc @ofrobots @zauberpony this should solve your need.

@indutny @rvagg I'm not 100% sure how to pass this down to llhttp. Can you please chime in? I couldn't find where HTTP_MAX_HEADER_SIZE is used in llhttp or if it's another define.

[indutny]

kMaxHeaderSize in src/node_http_parser.cc does this.

[sam-github]

Needs to also modify doc/api/errors.md, probably can't say what the limit is, but can mention that it may be other than the default in a custom node build. The limit could be surfaced through C++, into js as a constant. Isn't there a place already for configure-time constants? @jasnell ?

[rvagg]

#ifdef HTTP_MAX_HEADER_SIZE
static const uint64_t kMaxHeaderSize = HTTP_MAX_HEADER_SIZE;
#else
static const uint64_t kMaxHeaderSize = 8 * 1024;
#endif // HTTP_MAX_HEADER_SIZE

Or do we just ditch kMaxHeaderSize?

When we ditch http_parser we could make this configurable from a runtime API, so maybe retaining it will point us in that direction? We could even do it now for when compiled with llhttp I guess.

[mcollina]

@rvagg can you please clarify if I should pass down that definition to our node.gypi file as well? Or it will be automatically picked up?

When we ditch http_parser we could make this configurable from a runtime API, so maybe retaining it will point us in that direction? We could even do it now for when compiled with llhttp I guess.

I think this a very good reason to switch to the new http parser for 12.

[mcollina]

The limit could be surfaced through C++, into js as a constant. Isn't there a place already for configure-time constants?

I think we should do so, as changing the value at configure time would make our tests fails with the new configuration. How about http. HTTP_MAX_HEADER_SIZE?

[mcollina]

CI: https://ci.nodejs.org/job/node-test-pull-request/19154/

@bnoordhuis @nodejs/build may I get a couple of LGTMs?

[richardlau]

Changes look okay to me, but if we're going to land a runtime configuration option (#24811) I would rather not add more build time configuration options.

[mcollina]

@richardlau I think both are valid. One sets the default, and the other one changes it at runtime.

@cjihrig what do you think?

[cjihrig]

IMO, having a runtime option is desirable because it lets people who are impacted by our recent change continue to run an official build (and not have to build Node themselves). I'm not opposed to also having a build time setting though.

[mcollina]

IMO, having a runtime option is desirable because it lets people who are impacted by our recent change continue to run an official build (and not have to build Node themselves).

I agree.

[lpinca]

Was this superseded by #24811?

[mcollina]

Not really, but there is no urgency to get it landed, and I have other prioritites atm. So, we can close.

[indutny]

Should this be ported to v8.x?