Problematic http2 header causes an assertion fail: Diagnosis and Fix

[ywave620]

Version

From v14.17.2 to latest

Platform

Darwin Kernel Version 19.6.0

Subsystem

http2

What steps will reproduce the bug?

In summary,

http2.connect(`http://127.0.0.1:80`, { ... }).request({
    'A': 'A'.repeat(87382),  // a header including an unreasonable long name or value will trigger this bug
});

For more details, please refer to #39844

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

consistently produced

What is the expected behavior?

the assertion pass

What do you see instead?

the assertion fail

node[82270]: ../src/node_http2.cc:525:virtual node::http2::Http2Session::~Http2Sessi
on(): Assertion `(current_nghttp2_memory_) == (0)' failed.

Additional information

Source involved: node_http2.cc and nghttp2 lib

First of all, the assertion(Http2Session::current_nghttp2_memory_ == 0) means that when destructing a Http2Session instance, any memory allocated by nghttp2 lib should have been freed or transferred to(and hence controlled by) JS land.

When a complete HEADERS frame is received, Http2Session::HandleHeadersFrame() will be called and all buffered headers(temporarily stored in Http2Stream::current_headers_ ) will be emitted to JS land through 'headers' event, as we mentioned above, after this action, all headers are transferred to JS land and the memory they occupy will not contribute to Http2Session::current_nghttp2_memory_ any more.

However, when there is any problematic http2 header, Http2Session::HandleHeadersFrame() won't be called by nghttp2 lib so that the buffered headers are always kept in a Http2Stream instance. In addition, the Http2Stream instance has not been destructed when the Http2Session instance destructs so that the buffered headers has not been freed, too.

Altogether, when the Http2Session instance destructs, the buffered headers has either not been freed or transferred to JS land. In this way, the assertion fails. This is definitely a bug, we forget to take care of the case where we have buffered some headers but the Http2Session::HandleHeadersFrame() never executes.

Now I switch to another interesting issue (maybe a bug) I found when debugging, and this issue explains why the Http2Stream instance has not been destructed when the Http2Session instance destructs.

A problematic http2 header will make nghttp2 lib decide to queued a RST_STREAM and also a GOAWAY. However, the nghttp2 lib intentionally enforces that when a GOAWAY is queued, the queued RST_STREAM won' t be sent and hence the Http2Session::OnStreamClose won' t be called. We should keep an eye on this issue because under this circumtance, we have no way to close and destory the stream. Note that the stream has never been exposed to JS land(i.e. the 'headers' event has not been emitted to JS land), so it is impossible to rely on user to actively call close() and destory(). Given this, the only way to destruct a Http2Stream instance is to destruct its cooresponding Http2Session instance, after which, the Http2Stream instance becomes a subject to GC.

Finally, I give my version of fix, consisting of two parts:

• Node.js must be notified that a RST_STREAM is sent or receiverd under all circumtances, because Node.js takes this as a signal indicating that a stream is closed.
• Node.js must free all buffered headers actively in the case where the stream is closed and the stream has never been exposed to JS land.

For the first part, we either change the design of nghttp2 lib or make use of the Http2Session::OnFrameNotSent() callback, which will be called when any frame is dropped by nghttp2 lib. My idea is that, inside this callback, we figure out whether the dropped frame is a RST_STREAM and whether the reason why it's drop is a GOAWAY is already queued, if both are true, call Http2Session::OnStreamClose().

For the second part, just destruct current_headers_ actively in Http2Stream::close(), which will be called by Http2Session::OnStreamClose(). This is safe because when the buffered headers are already transferred to JS land, the current_headers_ will be empty.

[RafaelGSS]

It was fixed by: #41502