hyper: root cause of curl test 565 failure #8896

pmk21 · 2022-05-22T12:08:18Z

I did this

Ran curl test 565, analyzed the debug trace and generated logs, concluded that the first POST request seems to be missing the terminating chunk which has to be inputted during the read callback. This is possibly causing some issues in hyper.

Initial request from server.input -

POST /565 HTTP/1.1
Host: 127.0.0.1:37051
Accept: */*
Transfer-Encoding: chunked
Content-Type: application/x-www-form-urlencoded

3
one
3
two
5
three
1D
and a final longer crap: four
0

Requests after fix -

POST /565 HTTP/1.1
Host: 127.0.0.1:42461
Accept: */*
Transfer-Encoding: chunked
Content-Type: application/x-www-form-urlencoded

0

POST /565 HTTP/1.1
Host: 127.0.0.1:42461
Authorization: Digest username="foo", realm="testrealm", nonce="1053604144", uri="/565", response="877424f750af047634dbd94f9933217b"
Accept: */*
Transfer-Encoding: chunked
Content-Type: application/x-www-form-urlencoded
Expect: 100-continue

3
one
3
two
5
three
1D
and a final longer crap: four
0

Patched the lib510.c file to allow the test to pass -

diff --git a/tests/libtest/lib510.c b/tests/libtest/lib510.c
index 2b8da207d..a5bd06a9c 100644
--- a/tests/libtest/lib510.c
+++ b/tests/libtest/lib510.c
@@ -36,6 +36,7 @@ struct WriteThis {
   int counter;
 };
 
+#if defined(USE_HYPER) && defined(LIB565)
 static size_t read_callback(char *ptr, size_t size, size_t nmemb, void *userp)
 {
   struct WriteThis *pooh = (struct WriteThis *)userp;
@@ -44,6 +45,11 @@ static size_t read_callback(char *ptr, size_t size, size_t nmemb, void *userp)
   if(size*nmemb < 1)
     return 0;
 
+  if(pooh->counter == -1) {
+    pooh->counter++;
+    return 0;
+  }
+
   data = post[pooh->counter];
 
   if(data) {
@@ -58,6 +64,30 @@ static size_t read_callback(char *ptr, size_t size, size_t nmemb, void *userp)
   }
   return 0;                         /* no more data left to deliver */
 }
+#else
+static size_t read_callback(char *ptr, size_t size, size_t nmemb, void *userp)
+{
+  struct WriteThis *pooh = (struct WriteThis *)userp;
+  const char *data;
+
+  if(size*nmemb < 1)
+    return 0;
+
+  data = post[pooh->counter];
+
+  if(data) {
+    size_t len = strlen(data);
+    if(size*nmemb < len) {
+      fprintf(stderr, "read buffer is too small to run test\n");
+      return 0;
+    }
+    memcpy(ptr, data, len);
+    pooh->counter++; /* advance pointer */
+    return len;
+  }
+  return 0;                         /* no more data left to deliver */
+}
+#endif
 
 int test(char *URL)
 {
@@ -65,7 +95,11 @@ int test(char *URL)
   CURLcode res = CURLE_OK;
   struct curl_slist *slist = NULL;
   struct WriteThis pooh;
+#if defined(USE_HYPER) && defined(LIB565)
+  pooh.counter = -1;
+#else
   pooh.counter = 0;
+#endif
 
   if(curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK) {
     fprintf(stderr, "curl_global_init() failed\n");

curl/libcurl version

curl 7.83.1-DEV (x86_64-pc-linux-gnu) libcurl/7.83.1-DEV OpenSSL/1.1.1o zlib/1.2.12 brotli/1.0.9 zstd/1.5.2 libidn2/2.3.2 libpsl/0.21.1 (+libidn2/2.3.0) librtmp/2.3 Hyper/0.14.18 OpenLDAP/2.6.1
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher gophers http https imap imaps ldap ldaps mqtt pop3 pop3s rtmp smb smbs smtp smtps telnet tftp 
Features: alt-svc AsynchDNS brotli Debug HSTS HTTP2 HTTPS-proxy IDN Largefile libz NTLM NTLM_WB PSL SSL TLS-SRP TrackMemory UnixSockets zstd

operating system

Linux msi 5.15.38-1-MANJARO #1 SMP PREEMPT Mon May 9 07:52:21 UTC 2022 x86_64 GNU/Linux

The text was updated successfully, but these errors were encountered:

bagder · 2022-05-23T15:07:24Z

Test 565 uses (explicit) Digest authentication. That's a multi-request mechanism and curl will then not issue the full request in the first request but just sends a zero length one to get the auth done, then send the full body in the second request.

When hyper is not used, libcurl does not call the callback until it wants to read (and send) actual data. Your research here shows that with hyper enabled, the read callback wrongly gets called already in the first request and then things go wrong.

pmk21 · 2022-05-28T13:59:51Z

Thanks for the explanation!

I did some more digging and found this check in libcurl -

curl/lib/http.c

Lines 2663 to 2670 in 10cd696

    
                 if(data->req.upload_chunky && conn->bits.authneg) { 
        
                   /* Chunky upload is selected and we're negotiating auth still, send 
        
                      end-of-data only */ 
        
                   result = Curl_dyn_addn(r, (char *)STRCONST("\x30\x0d\x0a\x0d\x0a")); 
        
                   /* 0  CR  LF  CR  LF */ 
        
                   if(result) 
        
                     return result; 
        
                 }

I guess there has to be some similar logic in c-hyper.c as well. I found that data->req.upload_chunky is being set to false always.

curl/lib/c-hyper.c

Line 1132 in 10cd696

data->req.upload_chunky = FALSE;

Removing that line causes failures in other tests, so I added a messy fix -

--- a/lib/c-hyper.c
+++ b/lib/c-hyper.c
@@ -671,6 +671,7 @@ static int uploadstreamed(void *userdata, hyper_context *ctx,
 {
   size_t fillcount;
   struct Curl_easy *data = (struct Curl_easy *)userdata;
+  struct connectdata *conn = (struct connectdata *)data->conn;
   CURLcode result;
   (void)ctx;
 
@@ -685,7 +686,15 @@ static int uploadstreamed(void *userdata, hyper_context *ctx,
     return HYPER_POLL_PENDING;
   }
 
-  result = Curl_fillreadbuffer(data, data->set.upload_buffer_size, &fillcount);
+  if(data->req.upload_chunky && conn->bits.authneg) {
+    fillcount = 0;
+    data->req.upload_chunky = FALSE;
+    result = CURLE_OK;
+  }
+  else {
+    result = Curl_fillreadbuffer(data, data->set.upload_buffer_size,
+                                 &fillcount);
+  }
   if(result) {
     data->state.hresult = result;
     return HYPER_POLL_ERROR;
@@ -1129,7 +1138,12 @@ CURLcode Curl_http(struct Curl_easy *data, bool *done)
   if(result)
     goto error;
 
-  data->req.upload_chunky = FALSE;
+  if(data->req.upload_chunky && conn->bits.authneg) {
+    data->req.upload_chunky = TRUE;
+  }
+  else {
+    data->req.upload_chunky = FALSE;
+  }
   sendtask = hyper_clientconn_send(client, req);
   if(!sendtask) {
     failf(data, "hyper_clientconn_send");

Is there a better way to handle this?

bagder · 2022-12-08T17:05:51Z

I'm not sure there's a better way, but I think that if this works I think it is better to move ahead with this and enable the test rather than waiting and trying to find a nicer solution. Can you turn this into a PR?

pmk21 · 2022-12-10T16:22:58Z

Yes, I'll open a PR.

It make test 565 run fine. Fixes curl#8896 Assisted-by: Daniel Stenberg

It makes test 565 run fine. Fixes curl#8896 Assisted-by: Daniel Stenberg

bagder added the Hyper label May 23, 2022

pmk21 added a commit to pmk21/curl that referenced this issue Dec 11, 2022

c-hyper: fix multi-request mechanism

8e705cd

It make test 565 run fine. Fixes curl#8896 Assisted-by: Daniel Stenberg

pmk21 added a commit to pmk21/curl that referenced this issue Dec 11, 2022

c-hyper: fix multi-request mechanism

427db97

It makes test 565 run fine. Fixes curl#8896 Assisted-by: Daniel Stenberg

pmk21 mentioned this issue Dec 11, 2022

c-hyper: fix multi-request mechanism #10080

Closed

bagder closed this as completed in b80dae2 Dec 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hyper: root cause of curl test 565 failure #8896

hyper: root cause of curl test 565 failure #8896

pmk21 commented May 22, 2022

bagder commented May 23, 2022

pmk21 commented May 28, 2022

bagder commented Dec 8, 2022 •

edited

pmk21 commented Dec 10, 2022

hyper: root cause of curl test 565 failure #8896

hyper: root cause of curl test 565 failure #8896

Comments

pmk21 commented May 22, 2022

I did this

curl/libcurl version

operating system

bagder commented May 23, 2022

pmk21 commented May 28, 2022

bagder commented Dec 8, 2022 • edited

pmk21 commented Dec 10, 2022

bagder commented Dec 8, 2022 •

edited