Http2 throws non-descriptive error "Error [ERR_HTTP2_ERROR]: The user callback function failed"

[blakebyrnes]

• Version: 14.16.0, 15.12.0
• Platform: 18.7.0 Darwin Kernel Version 18.7.0: Mon Aug 31 20:53:32 PDT 2020; root:xnu-4903.278.44~1/RELEASE_X86_64 x86_64
• Subsystem: http2

What steps will reproduce the bug?

async function main() {
  const client = http2.connect('https://www.postgresql.org');
  const stream = client.request({
    ':method': 'GET',
    ':path': '/',
    'accept-encoding': 'gzip, deflate, br',
  });

  const buffer: Buffer[] = [];
  for await (const data of stream) {
    buffer.push(data);
  }
  const response = Buffer.concat(buffer).toString();
  console.log(response);
}

main().catch(err => console.log('Http2 User Error', err));

How often does it reproduce? Is there a required condition?

It will happen every time. When looking deeper into http2 frames, it appears the use of the Varnish? gzip module on the end site is causing an EOF frame to be incorrectly sent. nghttp2 treats any http2 violation as fatal, and so does nodejs.

What is the expected behavior?

Ideally, there would be a way to allow the code to continue since this is really just an invalid EOF code. Chrome's handling of http2 seems to handle this fine. They're obviously more interested in a lenient solution to http2 errors than nodejs.

If there's not a way to provide a lenient mode, or to decide what to do in the case of frame errors, I would have expected this to throw a more descriptive error that says something about the end site having an invalid http2 implementation.

What do you see instead?

The following are a snippet of running the example with NODE_DEBUG=http2*,stream* NODE_DEBUG_NATIVE=http2

STREAM 3518: need readable true
STREAM 3518: length less than watermark true
STREAM 3518: do read
HttpStream 1 (23) [Http2Session client (19)] reading starting
STREAM 3518: read undefined
STREAM 3518: need readable true
STREAM 3518: length less than watermark true
STREAM 3518: reading or ended false
STREAM 3518: read undefined
STREAM 3518: need readable true
STREAM 3518: length less than watermark true
STREAM 3518: reading or ended false
Http2Session client (19) complete frame received: type: 0
Http2Session client (19) handling data frame for stream 1
Http2Session client (19) complete frame received: type: 0
Http2Session client (19) handling data frame for stream 1
Http2Session client (19) fatal error receiving data: -902
HTTP2 3518: Http2Session client: destroying
HTTP2 3518: Http2Session client: start closing/destroying Error [ERR_HTTP2_ERROR]: The user callback function failed
    at Http2Session.onSessionInternalError (internal/http2/core.js:751:26) {
  code: 'ERR_HTTP2_ERROR',
  errno: -902
}
HTTP2 3518: Http2Stream 1 [Http2Session client]: destroying stream

Additional information

[marsonya]

cc @nodejs/http2

[mcollina]

Thanks for reporting. There are not enough information here on how we can reproduce the bug. Can you indicate clear reproducible steps so we can try to understand the problem (and eventually fix it).

[blakebyrnes]

Hi @mcollina, there's a full code example in the ticket. What else are you looking for to be able to reproduce?

[mcollina]

I would prefer to have a server we can run locally instead of a public one.

[addaleax]

@mcollina This is unhelpful. The post contains a snippet that reproduces this problem consistently.

[mcollina]

My experience is that understanding what's going on and reproducing it takes 90% of the time in these kind of issues. Pointing to a remote server is not helpful, as we do not know how that is configured and what triggers the problem.

Anyway, I'm sorry if I sounded negative. I'll leave it to others to fix.

[addaleax]

So, I took a look and it seems that this is caused by 695e38b, which was intended as a security measure to protect against CVE-2019-9518. This was part of a large group of HTTP/2-related security fixes, and I was trying to err on the safe side there.

The security issue was specifically about a flood of 0-length data frames without an EOF flag, but in this case, there’s only two such frames. I think it makes sense to allow a small number of frames of this type, and keep this in line with how we handle other invalid frames: #37875

---

My experience is that understanding what's going on and reproducing it takes 90% of the time in these kind of issues.

@mcollina I absolutely understand where you’re coming from, but @blakebyrnes’s issue description already contains the majority of the work here.

Pointing to a remote server is not helpful, as we do not know how that is configured and what triggers the problem.

I’m not sure how somebody would know the exact configuration of a remote server. The issue description contains the server software (as is indicated by the remote server’s headers) and a best guess about what part of it might be causing this.

[blakebyrnes]

Thanks @addaleax!

I've been poking through nghttp2 code to figure out root sources for where this could be blowing up from. It seemed like node's nghttp2 wrapper was re-broadcasting any underlying http2 issues as this "user callback" error.
->

node/lib/internal/http2/core.js

Line 781 in 0a77830

this[kOwner].destroy(new NghttpError(code));

->

node/lib/internal/http2/core.js

Line 3326 in 0a77830

onSessionInternalError,

-> https://github.com/nghttp2/nghttp2/blob/2e44f23b053dac556d222674d1dccd0341e72280/lib/nghttp2_session.c#L3313

Looks like I was barking up the wrong tree though! I didn't get to controlling the flow yet.

I wonder if this error message "NGHTTP2_ERR_CALLBACK_FAILURE" shouldn't also be re-translated. It's referring to nodejs callbacks injected into nghttp2, which is confusing as a "nodejs" user.

In any case, thanks for looking into this. I was only a small step down the path of trying to figure out how to get a correct server configuration working.

[addaleax]

It's referring to nodejs callbacks injected into nghttp2, which is confusing as a "nodejs" user.

Yeah, that's fair. We're currently just forwarding whatever nghttp2_strerror gives us:

node/src/node_http2.cc

Line 2438 in 0a77830

reinterpret_cast<const uint8_t*>(nghttp2_strerror(val))));

and I agree that the error message is generally not helpful. Would you be interested in sending a PR to address this?

We could probably also make the error message for the specific case of an invalid frame like this one better, but that's not completely trivial, because, as you saw, nghttp2 just turns any non-zero value for the frame callback into NGHTTP2_ERR_CALLBACK_FAILURE, so we'd have to store any information that goes beyond "callback failure" on the session itself and make sure that it's up to date when we handle the return value of nghttp2_session_mem_recv().

[blakebyrnes]

What you just wrote is where I got lost thinking about fixing this ;) There seem to be 20 ways nghttp will return that error code, and the "right" solution probably involves doing a good bit more work to figure out what's wrong.

Perhaps the simple solution is just to translate into a generic error. Is there anything else in nodejs that you can think of that resembles this one? Is the standard to "blame" the underlying library? Or to blame the upstream site? Ie, I could see this going in a few different directions:

• HTTP2_NGHTTP_ERROR
• HTTP2_INTERNAL_ERROR
• HTTP2_FRAME_ERROR
• HTTP2_SPECIFICATION_ERROR

[addaleax]

@blakebyrnes Yeah, I don't think we'd want to cover every possible case that NGHTTP2_ERR_CALLBACK_FAILURE fails here, that would probably be too much work to realistically happen.

If you're looking for an error code for this specific condition, I think HTTP2_FRAME_ERROR would be fine, or generally anything that indicates that the remote send some parsable but invalid data.

[blakebyrnes]

If that's the case, wouldn't we be replacing the NghttpError where the code === 902 with a new error type? It seems that's the only "simple" place to fix this.

Ie, here:

node/lib/internal/http2/core.js

Line 781 in 0a77830

this[kOwner].destroy(new NghttpError(code));

Would translate to:

let nghttpError;
if (code === -902){
  nghttp2Error = new HTTP2_FRAME_ERROR(code);
} else {
  nghttp2Error = new NghttpError(code);
} 
this[kOwner].destroy(nghttp2Error); 

[addaleax]

@blakebyrnes So, what I imagine we could do to get something like HTTP2_FRAME_ERROR for this case...

• Add a const char* custom_error_code_; field to Http2Session
• Set custom_error_code = "HTTP2_FRAME_ERROR" in this specific case inside HandleDataFrame
• Before calling ConsumeHTTP2Data(), reset it to nullptr, and check afterwards here whether the flag has been set, and in that case, specify it as a second string argument to the http2session_on_error_function callback
• Fix up the JS code in basically the way that you're suggesting (except that it checks for the value of that second argument instead of code === -902, because the latter would probably be too broad)

How does that sound?

[blakebyrnes]

This approach sounds very logically sound. I'm still not quite up to speed on running node on the c side of things though.

I was just looking at your PR for the EOF frames to see how this would fit in. Were you imagining this as a separate PR, or on top of your new one?

[addaleax]

@blakebyrnes I think separate PR would be fine :) If you're interested in opening one, I think you can either cherry-pick my second commit (to avoid the minor conflict that would show up/re-use the test that it introduces) or just rebase later. If you'd prefer for me to open a PR, I can probably do that some time this week, too. :)

[blakebyrnes]

I was intrigued by the idea of taking this on, but I don't think I can realistically get to it soon. I dislike asking if you could take it on, but I feel like I probably should 🤦

[addaleax]

@blakebyrnes No problem! I landed the first PR, so here’s one for the error code update: #37936