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.

Sign up for GitHub

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

Jump to bottom

HTTP/2 downgrade to HTTP/1.1 not working #11357

Closed
jay opened this issue Jun 21, 2023 · 2 comments
Closed

HTTP/2 downgrade to HTTP/1.1 not working #11357

jay opened this issue Jun 21, 2023 · 2 comments
Labels
HTTP/2 regression

Comments

@jay
Copy link
Member

jay commented Jun 21, 2023 •
edited

I did this

Investigating #11034 which has a URL that when requested via HTTP/2 replies with RST_STREAM error HTTP_1_1_REQUIRED. libcurl should retry the transfer with HTTP/1.1 but doesn't. (Note the reporter in that issue builds libcurl without HTTP/2 so it is an unrelated issue).

curld -v https://wsnfse.vitoria.es.gov.br/producao/NotaFiscalService.asmx?WSDL

...
* HTTP/2 stream 1 was reset
* [CONN-0] readwrite_data() -> 56
* [CONN-0] Curl_readwrite() -> 56
* multi_done: status: 56 prem: 1 done: 0
* Connection #0 to host wsnfse.vitoria.es.gov.br left intact
* Expire cleared (transfer 0x526ee8)
curl: (56) HTTP/2 stream 1 was reset

Bisected to cab2d56 @icing

That commit changes http2_handle_stream_close to return a generic error before checking for CURLE_HTTP2_STREAM, which means CURLE_HTTP2_STREAM is never returned, the error is no longer properly output in verbose mode as HTTP_1_1_REQUIRED, and later code in libcurl to handle the downgrade is never reached:

curl/lib/http2.c

Lines 1579 to 1598 in 51f6a0d

if(stream->error == NGHTTP2_REFUSED_STREAM) {
DEBUGF(LOG_CF(data, cf, "[h2sid=%d] REFUSED_STREAM, try again on a new "
"connection", stream->id));
connclose(cf->conn, "REFUSED_STREAM"); /* don't use this anymore */
data->state.refused_stream = TRUE;
*err = CURLE_SEND_ERROR; /* trigger Curl_retry_request() later */
return -1;
}
else if(stream->reset) {
failf(data, "HTTP/2 stream %u was reset", stream->id);
*err = stream->bodystarted? CURLE_PARTIAL_FILE : CURLE_RECV_ERROR;
return -1;
}
else if(stream->error != NGHTTP2_NO_ERROR) {
failf(data, "HTTP/2 stream %u was not closed cleanly: %s (err %u)",
stream->id, nghttp2_http2_strerror(stream->error),
stream->error);
*err = CURLE_HTTP2_STREAM;
return -1;
}

curl/lib/multi.c

Lines 2479 to 2500 in 51f6a0d

else if((CURLE_HTTP2_STREAM == result) &&
Curl_h2_http_1_1_error(data)) {
CURLcode ret = Curl_retry_request(data, &newurl);
if(!ret) {
infof(data, "Downgrades to HTTP/1.1");
streamclose(data->conn, "Disconnect HTTP/2 for HTTP/1");
data->state.httpwant = CURL_HTTP_VERSION_1_1;
/* clear the error message bit too as we ignore the one we got */
data->state.errorbuf = FALSE;
if(!newurl)
/* typically for HTTP_1_1_REQUIRED error on first flight */
newurl = strdup(data->state.url);
/* if we are to retry, set the result to OK and consider the request
as done */
retry = TRUE;
result = CURLE_OK;
done = TRUE;
}
else
result = ret;
}

It's unclear to me why else if(stream->reset) block needs to come before. Changing it back works but I don't know why it was changed in the first place there may be a good reason.

diff --git a/lib/http2.c b/lib/http2.c
index 9da3cae..0894561 100644
--- a/lib/http2.c
+++ b/lib/http2.c
@@ -1523,22 +1523,22 @@ static ssize_t http2_handle_stream_close(struct Curl_cfilter *cf,
     connclose(cf->conn, "REFUSED_STREAM"); /* don't use this anymore */
     data->state.refused_stream = TRUE;
     *err = CURLE_SEND_ERROR; /* trigger Curl_retry_request() later */
     return -1;
   }
-  else if(stream->reset) {
-    failf(data, "HTTP/2 stream %u was reset", stream->id);
-    *err = stream->bodystarted? CURLE_PARTIAL_FILE : CURLE_RECV_ERROR;
-    return -1;
-  }
   else if(stream->error != NGHTTP2_NO_ERROR) {
     failf(data, "HTTP/2 stream %u was not closed cleanly: %s (err %u)",
           stream->id, nghttp2_http2_strerror(stream->error),
           stream->error);
     *err = CURLE_HTTP2_STREAM;
     return -1;
   }
+  else if(stream->reset) {
+    failf(data, "HTTP/2 stream %u was reset", stream->id);
+    *err = stream->bodystarted? CURLE_PARTIAL_FILE : CURLE_RECV_ERROR;
+    return -1;
+  }
 
   if(!stream->bodystarted) {
     failf(data, "HTTP/2 stream %u was closed cleanly, but before getting "
           " all response header fields, treated as error",
           stream->id);

/cc @icing
@jay jay added the HTTP/2 label Jun 21, 2023
@bagder
Copy link
Member

bagder commented Jun 21, 2023

@icing this feels like something we should be able to add a test for these days, right?

icing added a commit to icing/curl that referenced this issue Jun 21, 2023
@icing
HTTP/2, error stream resets with code CURLE_HTTP2_STREAM
0d41221 
- refs curl#11357, where it was reported that HTTP/1.1 downgrades
  no longer works
- fixed with suggested change
- added test_05_03 and a new handler in the curltest module
  to reproduce that downgrades work
@icing icing mentioned this issue Jun 21, 2023
Closed
@icing
Copy link
Contributor

icing commented Jun 21, 2023

Added #11362 in the now more depressing github UI.

@bagder bagder closed this as completed in d435bf1 Jun 22, 2023
bch pushed a commit to bch/curl that referenced this issue Jul 19, 2023
@icing @bch
http2: error stream resets with code CURLE_HTTP2_STREAM
f13fb0d 
- refs curl#11357, where it was reported that HTTP/1.1 downgrades
  no longer works
- fixed with suggested change
- added test_05_03 and a new handler in the curltest module
  to reproduce that downgrades work

Fixes curl#11357
Closes curl#11362
Reported-by: Jay Satiro
ptitSeb pushed a commit to wasix-org/curl that referenced this issue Sep 25, 2023
@icing @ptitSeb
http2: error stream resets with code CURLE_HTTP2_STREAM
603b508 
- refs curl#11357, where it was reported that HTTP/1.1 downgrades
  no longer works
- fixed with suggested change
- added test_05_03 and a new handler in the curltest module
  to reproduce that downgrades work

Fixes curl#11357
Closes curl#11362
Reported-by: Jay Satiro
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
HTTP/2 regression
Milestone
No milestone
Development

No branches or pull requests
3 participants
@icing @bagder @jay