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

Hyper Curl Test 718 #2783

Closed
DanielVZ96 opened this issue Mar 19, 2022 · 6 comments
Closed

Hyper Curl Test 718 #2783

DanielVZ96 opened this issue Mar 19, 2022 · 6 comments
Labels
C-bug Category: bug. Something is wrong. This is bad!

Comments

@DanielVZ96
Copy link

Version
hyper::ffi current version

Platform

5.13.0-7614-generic #14~1631647151~20.04~930e87c-Ubuntu SMP Fri Sep 17 00:26:31 UTC  x86_64 x86_64 x86_64 GNU/Linux

but i suppose it's also for any platform

Description

The test 718 is failing with the following error from hyper: 0curl: (56) Hyper: [3] connection closed before message completed.

After inspecting the test, I noticed that there's no EOF before closing the connection, so I guess there's no issue on hyper's side. My intuition of the underlying problem is that the test expects hyper to process the data from the connection, but hyper isn't doing anything after noticing there's no EOF.

Test Case:

# Server-side
<reply>

# this is returned first since we get no proxy-auth
<connect>
HTTP/1.1 407 Authorization Required to proxy me swsclose
Proxy-Authenticate: Digest realm="weirdorealm", nonce="12345"
</connect>

<datacheck>
HTTP/1.1 407 Authorization Required to proxy me swsclose
Proxy-Authenticate: Digest realm="weirdorealm", nonce="12345"
</datacheck>
</reply>

If i add the EOF to both connect and datacheck the test passes both with and without hyper.

Updated Test case:

# Server-side
<reply>

# this is returned first since we get no proxy-auth
<connect>
HTTP/1.1 407 Authorization Required to proxy me swsclose
Proxy-Authenticate: Digest realm="weirdorealm", nonce="12345"

</connect>

<datacheck>
HTTP/1.1 407 Authorization Required to proxy me swsclose
Proxy-Authenticate: Digest realm="weirdorealm", nonce="12345"

</datacheck>
</reply>

With hyper:

********* System characteristics ******** 
* curl 7.83.0-DEV (x86_64-pc-linux-gnu) 
* libcurl/7.83.0-DEV OpenSSL/1.1.1f zlib/1.2.11 brotli/1.0.7 libidn2/2.2.0 libpsl/0.21.0 (+libidn2/2.2.0) Hyper/0.14.17
* Features: alt-svc AsynchDNS brotli Debug HSTS HTTP2 HTTPS-proxy IDN IPv6 Largefile libz NTLM NTLM_WB PSL SSL TLS-SRP TrackMemory UnixSockets
* Disabled: 
* Host: daniel
* System: Linux daniel 5.13.0-7614-generic #14~1631647151~20.04~930e87c-Ubuntu SMP Fri Sep 17 00:26:31 UTC  x86_64 x86_64 x86_64 GNU/Linux
* OS: linux
*
*** DISABLES memory tracking when using threaded resolver
*
* Servers: HTTP-IPv6 HTTP-unix FTP-IPv6 
* Env: 
* Seed: 259306
***************************************** 
Warning: test718 is explicitly disabled
test 0718...[HTTP proxy CONNECT (no auth) with proxy returning 407 and closing]
--pd---e--- OK (1   out of 1  , remaining: 00:00, took 1.071s, duration: 00:01)
TESTDONE: 1 tests were considered during 1 seconds.
TESTDONE: 1 tests out of 1 reported OK: 100%

Without hyper

********* System characteristics ******** 
* curl 7.83.0-DEV (x86_64-pc-linux-gnu) 
* libcurl/7.83.0-DEV OpenSSL/1.1.1f zlib/1.2.11 brotli/1.0.7 libidn2/2.2.0 libpsl/0.21.0 (+libidn2/2.2.0)
* Features: alt-svc AsynchDNS brotli Debug HSTS HTTPS-proxy IDN IPv6 Largefile libz NTLM NTLM_WB PSL SSL TLS-SRP TrackMemory UnixSockets
* Disabled: 
* Host: daniel
* System: Linux daniel 5.13.0-7614-generic #14~1631647151~20.04~930e87c-Ubuntu SMP Fri Sep 17 00:26:31 UTC  x86_64 x86_64 x86_64 GNU/Linux
* OS: linux
*
*** DISABLES memory tracking when using threaded resolver
*
* Servers: HTTP-IPv6 HTTP-unix FTP-IPv6 
* Env: 
* Seed: 224910
***************************************** 
test 0718...[HTTP proxy CONNECT (no auth) with proxy returning 407 and closing]
--pd---e--- OK (1   out of 1  , remaining: 00:00, took 1.143s, duration: 00:01)
TESTDONE: 1 tests were considered during 1 seconds.
TESTDONE: 1 tests out of 1 reported OK: 100%

@seanmonstar would updating the test case be a solution?

@DanielVZ96 DanielVZ96 added the C-bug Category: bug. Something is wrong. This is bad! label Mar 19, 2022
@seanmonstar
Copy link
Member

Oh, I see what you mean. That the test case doesn't include a blank line after the headers indicating the headers are done. Ya, so hyper thinks that means the connection terminated before the headers were finished.

I... don't know if hyper is wrong here. I think it's right, that's not a complete headers without the newline...

@seanmonstar
Copy link
Member

It might that the unit test could be updated, I don't know if it was meant to test this specific case. The one who could answer that is @bagder. It looks like the unit test is expecting error code 56, which i think is CURLE_RECV_ERROR. So, I think it's expected to be an error, but the handling is just slightly different.

@bagder
Copy link
Contributor

bagder commented Mar 20, 2022

The 718 test case shows hyper being more picky on HTTP than native libcurl. It can be fixed by editing the test case like this: curl/curl#8614

(and maybe it also shows that hyper isn't always making it very easy to understand why it returns error...)

bagder added a commit to curl/curl that referenced this issue Mar 20, 2022
Since hyper is picky and won't play ball otherwise.

Bug: hyperium/hyper#2783
Reported-by: Daniel Valenzuela
Closes #8614
@bagder
Copy link
Contributor

bagder commented Mar 20, 2022

Now fixed in curl's git repo.

@DanielVZ96
Copy link
Author

Sorry for not responding earlier, i had a tight weekend

@bagder thanks for fixing the test so promptly!

@seanmonstar I've been thinking about this and maybe hyper should support only having CR as an EOF in order to match the compatibility of libcurl. it could be under a #[cfg(feature = "ffi")]. i can't pin where this change should be implemented though.

@seanmonstar
Copy link
Member

hyper should support only having CR as an EOF

I don't think we want to do that. The specs and real world that I've seen only ever have LF or CRLF.

Otherwise, seems this test case is fixed, ya? Thanks for helping us figure it out!! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug. Something is wrong. This is bad!
Projects
Status: Done
Development

No branches or pull requests

3 participants