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.

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

hyper: root cause of curl test 1021 failure #8700

Closed
pmk21 opened this issue Apr 12, 2022 · 0 comments
Closed

hyper: root cause of curl test 1021 failure #8700

pmk21 opened this issue Apr 12, 2022 · 0 comments
Assignees
@bagder
Labels
Hyper

Comments

@pmk21
Copy link
Contributor

pmk21 commented Apr 12, 2022

I did this

Ran curl test 1021, analyzed the debug trace and generated logs, concluded that a request is being sent on a closed socket resulting in an error on hyper's side.

Based on this conclusion, I compared curl's version and hyper's version of the CONNECT function. I found that a specific check in curl's version to be missing in hyper's version of the function.

The missing check -

curl/lib/http_proxy.c

Lines 651 to 656 in 7befbe9

if(s->close_connection && data->req.newurl) {
/* Connection closed by server. Don't use it anymore */
Curl_closesocket(data, conn, conn->sock[sockindex]);
conn->sock[sockindex] = CURL_SOCKET_BAD;
break;
}

Corresponding location in hyper's version of the function -

curl/lib/http_proxy.c

Lines 967 to 974 in 7befbe9

/* If we are supposed to continue and request a new URL, which basically
* means the HTTP authentication is still going on so if the tunnel
* is complete we start over in INIT state */
if(data->req.newurl && (TUNNEL_COMPLETE == s->tunnel_state)) {
infof(data, "CONNECT request done, loop to make another");
connect_init(data, TRUE); /* reinit */
}
} while(data->req.newurl);

Added the check in hyper's version of the function -

--- a/lib/http_proxy.c
+++ b/lib/http_proxy.c
@@ -964,6 +964,13 @@ static CURLcode CONNECT(struct Curl_easy *data,
       break;
     }
 
+    if(conn->bits.close && data->req.newurl) {
+        /* Connection closed by server. Don't use it anymore */
+        Curl_closesocket(data, conn, conn->sock[sockindex]);
+        conn->sock[sockindex] = CURL_SOCKET_BAD;
+        break;
+    }
+
     /* If we are supposed to continue and request a new URL, which basically
      * means the HTTP authentication is still going on so if the tunnel
      * is complete we start over in INIT state */

However, I got the following test output -

1021: data FAILED:
--- log/check-expected  2022-04-10 15:49:04.073572682 +0530
+++ log/check-generated 2022-04-10 15:49:04.073572682 +0530
@@ -2,11 +2,11 @@
 Proxy-Authenticate: NTLM[CR][LF]
 Content-Length: 16[CR][LF]
 Connection: close[CR][LF]
-[LF]
+[CR][LF]
 HTTP/1.1 407 Authorization Required to proxy me my dear[CR][LF]
 Proxy-Authenticate: NTLM TlRMTVNTUAACAAAAAgACADAAAACGggEAc51AYVDgyNcAAAAAAAAAAG4AbgAyAAAAQ0MCAAQAQwBDAAEAEgBFAEwASQBTAEEAQgBFAFQASAAEABgAYwBjAC4AaQBjAGUAZABlAHYALgBuAHUAAwAsAGUAbABpAHMAYQBiAGUAdABoAC4AYwBjAC4AaQBjAGUAZABlAHYALgBuAHUAAAAAAA==[CR][LF]
 Content-Length: 28[CR][LF]
-[LF]
+[CR][LF]
 HTTP/1.1 200 Things are fine in proxy land[CR][LF]
 Server: Microsoft-IIS/5.0[CR][LF]
 Content-Type: text/html; charset=iso-8859-1[CR][LF]

 - abort tests
TESTDONE: 1 tests were considered during 2 seconds.
TESTDONE: 0 tests out of 1 reported OK: 0%

TESTFAIL: These test cases failed: 1021

I expected the following

After adding the missing check in hyper's version of the CONNECT function, I expected the test case to pass, but there is a small mismatch.

curl/libcurl version

curl 7.83.0-DEV (x86_64-pc-linux-gnu) libcurl/7.83.0-DEV OpenSSL/1.1.1m 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.28-1-MANJARO #1 SMP PREEMPT Fri Mar 11 14:12:57 UTC 2022 x86_64 GNU/Linux
@bagder bagder added the Hyper label Apr 14, 2022
@bagder bagder self-assigned this May 6, 2022
bagder added a commit that referenced this issue May 6, 2022
@bagder
http_proxy/hyper: handle closed connections
9c3a683 
Enable test 1021 for hyper builds.

Patched-by: Prithvi MK
Fixes #8700
@bagder bagder mentioned this issue May 6, 2022
Closed
@bagder bagder closed this as completed in a8a1dd8 May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bagder bagder

Labels
Hyper
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

http_proxy/hyper: handle closed connections curl/curl
2 participants
@bagder @pmk21