http2: let http2_recv handle the h2c-upgrade stream even if closed

[starrify]

This may be deemed necessary since some servers (e.g. nghttp2's reverse proxy) may start sending the response payload immediately following the server-side connection preface, other than waiting for the client's connection preface and possible setting negotiations.

Resolves #7036. Please refer to that issue for further details.

This revision 1a99208 passes all checks when being tested on my local machine (with valgrind enabled by default):

TESTDONE: 1426 tests were considered during 1398 seconds.
TESTDONE: 1133 tests out of 1133 reported OK: 100%

This change introduces the possibly unwanted behavior that http2_recv may be attempted (passing the changed lines) for every initial payload received after the protocol switching (h2c-upgrade).

In normal cases such initial payload may contain only one or a few SETTINGS frame, which shall cause http2_recv to finish silently with no error / warning reported.

It may be a good idea to add one or a few test cases for this specific behavior (server sending payload before receiving client's connection preface, as described in #7036), assuming curl does intend support such behavior despite its being RFC-incompliant.

However given my limited knowledge with this project and the tests, I'm yet unsure how to properly implement such a test. Therefore this PR does not yet come with proper tests. (and sorry for this)

[bagder]

The condition for continuing isn't really that it got a 101 back though is it? Isn't it rather that there are more bytes queued for this stream to process?

[starrify]

Hi @bagder , thanks for the review! I have to apologize in advance for my lack of expertise of this project, and thus the possibility that I'm having incorrect assumptions or giving improper suggestions.

Based on my current understanding, the major problem we're facing here is that when curl handles the protocol switching for HTTP/2 and submits the payload for nghttp2 to parse, there is a chance that stream 1 is fulfilled and closed right away if the server sends it back too early:

curl/lib/http2.c

Lines 2327 to 2339 in 1e19ece

	infof(data, "Copying HTTP/2 data in stream buffer to connection buffer"
	" after upgrade: len=%zu\n",
	nread);
	if(nread)
	memcpy(httpc->inbuf, mem, nread);
	httpc->inbuflen = nread;
	DEBUGASSERT(httpc->nread_inbuf == 0);
	if(-1 == h2_process_pending_input(data, httpc, &result))
	return CURLE_HTTP2;

Once that happens, the residual payload (parsed by nghttp2 and pushed to the stream buffer by the callbacks) would still await retrieval while being marked as dirty (data->state.drain being non-zero).

Also such payload gets ignored by curl due to the new shortcut added in 252790c:

curl/lib/http2.c

Lines 1622 to 1624 in 1e19ece

	if(stream->closed)
	/* closed overrides paused */
	return http2_handle_stream_close(conn, data, stream, err);

My motivation is therefore to allow stream 1 to go beyond this shortcut and reach the draining branch:

curl/lib/http2.c

Lines 1658 to 1661 in 1e19ece

	if((data->state.drain) && stream->memlen) {
	H2BUGF(infof(data, "http2_recv: DRAIN %zu bytes stream %u!! (%p => %p)\n",
	stream->memlen, stream->stream_id,
	stream->mem, mem));

Therefore the payload may be picked up at a later time at:

curl/lib/http2.c

Lines 1780 to 1791 in 1e19ece

	if(stream->memlen) {
	ssize_t retlen = stream->memlen;
	H2BUGF(infof(data, "http2_recv: returns %zd for stream %u\n",
	retlen, stream->stream_id));
	stream->memlen = 0;
	if(httpc->pause_stream_id == stream->stream_id) {
	/* data for this stream is returned now, but this stream caused a pause
	already so we need it called again asap */
	H2BUGF(infof(data, "Data returned for PAUSED stream %u\n",
	stream->stream_id));
	}

Isn't it rather that there are more bytes queued for this stream to process?

This is because I wanted to avoid altering too much of the logic (accidentally covering other cases) and the only case I know of that shall be allowed to go beyond the shortcut is upon UPGR101_RECEIVED.

Actually I wanted to also include stream->stream_id == 1 to the condition, but that cannot be done because the stream ID would have been destroyed already as part of the on_stream_close callback.

[starrify]

I have just submitted #7040, which is an alternative attempt to resolve the same issue. Please do not merge both of the two.

[bagder]

I just merged #7040 instead of this, closing here. Thanks!