search

JuliaWeb / GnuTLS.jl

Transport Level Security for Julia Streams provided by GnuTLS
Other
8 stars 13 forks source link

Hotfix for something funny with sockets/sessions #31

Closed IainNZ closed 9 years ago

IainNZ commented 9 years ago

@sbromberger can you confirm this works for you? It seems terribly ugly but I think its better than errors being thrown at random.

coveralls commented 9 years ago

Coverage Status

Changes Unknown when pulling 0da301c05a9b525b6c9f4ed722794a52c02f15ab on ird/tempfix into \ on master**.

IainNZ commented 9 years ago

Wooah, "MemoryError" on 0.4, thats a new one

sbromberger commented 9 years ago

Doesn't work in 0.4:

julia> using GnuTLS

julia> sess = GnuTLS.Session()
GnuTLS.Session(Ptr{Void} @0x00007f854e1b2800,false,#undef,#undef)

julia> set_priority_string!(sess)

julia> set_credentials!(sess,GnuTLS.CertificateStore())

julia> associate_stream(sess,connect("github.com",443))

julia> handshake!(sess)

julia> write(sess,"GET / HTTP/1.1\r\n\r\n")
18

julia> z = readall(sess)
ERROR: ArgumentError: stream is closed or unusable
 in check_open at stream.jl:294
 in write at stream.jl:731
 in close at /Users/seth/.julia/v0.4/GnuTLS/src/GnuTLS.jl:97
 in readtobuf at /Users/seth/.julia/v0.4/GnuTLS/src/GnuTLS.jl:379
 in readall at /Users/seth/.julia/v0.4/GnuTLS/src/GnuTLS.jl:390

This is why I guessed in https://github.com/JuliaLang/julia/issues/10182#issuecomment-74173121 that it was something to do with close.

IainNZ commented 9 years ago

Yeah... I just tried doing a POST (trying to file issues for broken packages) and it hangs Julia. Damn. I'm out of my depth!

sbromberger commented 9 years ago

Now I’m thoroughly confused: it’s working with readavailable:

julia> sess = GnuTLS.Session()
GnuTLS.Session(Ptr{Void} @0x00007fd732bc8e00,false,#undef,#undef)

julia> set_credentials!(sess,GnuTLS.CertificateStore())

julia> set_priority_string!(sess)

julia> associate_stream(sess,connect("www.smud.org",443))

julia> handshake!(sess)

julia> write(sess,"GET / HTTP/1.1\r\n\r\n")
18

julia> z = readavailable(sess)
513-element Array{UInt8,1}:

julia> join(char(z))
"HTTP/1.1 400 Bad Request\r\nContent-Type: text/html; charset=us-ascii\r\nServer: Microsoft-HTTPAPI/2.0\r\nDate: Fri, 13 Feb 2015 18:25:14 GMT\r\nConnection: close\r\nContent-Length: 334\r\n\r\n<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.01//EN\"\"http://www.w3.org/TR/html4/strict.dtd\">\r\n<HTML><HEAD><TITLE>Bad Request</TITLE>\r\n<META HTTP-EQUIV=\"Content-Type\" Content=\"text/html; charset=us-ascii\"></HEAD>\r\n<BODY><h2>Bad Request - Invalid Hostname</h2>\r\n<hr><p>HTTP Error 400. The request hostname is invalid.</p>\r\n</BODY></HTML>\r\n”
IainNZ commented 9 years ago

Yes I noticed something like that in my tinkering. The different read methods all look very different to me, readall in particular is very different. readavailable give bytes (I guess because the partial read wouldn't necessarily be a valid UTF8 string), readall gives string. I think maybe both should give bytes, and pacakges using them should bytestring them...

sbromberger commented 9 years ago

I'm not sure returning a vector of bytes is what most folks would want. They'd be converting anyway. Perhaps if we redo this code, we should have an ;as_bytestring=true kwarg.

sbromberger commented 9 years ago

Reopening because I think this is salvageable:

Manually executing the steps in readall(sess); I get this:

julia> const TLS_CHUNK_SIZE = 4096
4096

julia> sess = GnuTLS.Session(); set_priority_string!(sess); set_credentials!(sess,GnuTLS.CertificateStore())

julia> associate_stream(sess,connect("www.smud.org",443)); ^C

julia> buf = IOBuffer(Array(Uint8,TLS_CHUNK_SIZE),true,true,true,true,typemax(Int)); buf.size=0
0

julia> associate_stream(sess,connect("www.smud.org",443)); handshake!(sess); write(sess,"GET / HTTP/1.1\r\n\r\n")
18

julia> while GnuTLS.readtobuf(sess, buf,TLS_CHUNK_SIZE); println("buf.size = $(buf.size)"); end
buf.size = 513
ERROR: GnuTLS Exception: GNUTLS_E_PREMATURE_TERMINATION(-110): The TLS connection was non-properly terminated.
 in readtobuf at /Users/seth/.julia/v0.4/GnuTLS/src/GnuTLS.jl:377
 in anonymous at no file:1

julia> buf
IOBuffer(data=UInt8[...], readable=true, writable=true, seekable=true, append=true, size=513, maxsize=Inf, ptr=1, mark=-1)

julia> readall(buf)
"HTTP/1.1 400 Bad Request\r\nContent-Type: text/html; charset=us-ascii\r\nServer: Microsoft-HTTPAPI/2.0\r\nDate: Fri, 13 Feb 2015 23:01:10 GMT\r\nConnection: close\r\nContent-Length: 334\r\n\r\n<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.01//EN\"\"http://www.w3.org/TR/html4/strict.dtd\">\r\n<HTML><HEAD><TITLE>Bad Request</TITLE>\r\n<META HTTP-EQUIV=\"Content-Type\" Content=\"text/html; charset=us-ascii\"></HEAD>\r\n<BODY><h2>Bad Request - Invalid Hostname</h2>\r\n<hr><p>HTTP Error 400. The request hostname is invalid.</p>\r\n</BODY></HTML>\r\n"

This indicates to me that what really needs to happen is for readtobuf to ignore GNUTLS_E_PREMATURE_TERMINATION(-110). This sort of makes sense to me given https://github.com/JuliaWeb/GnuTLS.jl/blob/master/src/GnuTLS.jl#L51-62 - perhaps read* needs to be using this function.

coveralls commented 9 years ago

Coverage Status

Changes Unknown when pulling 0da301c05a9b525b6c9f4ed722794a52c02f15ab on ird/tempfix into \ on master**.

IainNZ commented 9 years ago

I see what you're saying, and it makes sense. Basically the other end is saying "I don't want to do a proper HTTPS session, so I'm just firing back whatever and ending this", and readall isn't handling this gracefully.

IainNZ commented 9 years ago

Does it work?

sbromberger commented 9 years ago

Heh, you just called me out on my laziness. I was about to test, I swear.

Edit: the issue isn't with close; it's with readtobuf. Let me try checking is_premature_eof there.

Edit 2: can't call is_premature_eof since we're not actually dealing with an exception. This code is ugly but it works for this particular test. Let's clean it up / make it more robust before we even think about using it. :)

NOTE: the -9 || -110 garbage should be abstracted into a function, and should also not be ORed together since the actual value will depend on the gnutls version (see is_premature_eof for the details).

coveralls commented 9 years ago

Coverage Status

Changes Unknown when pulling 2dfc1f8a94a20c1852e5734ea1dc069fb4bcc2ea on ird/tempfix into \ on master**.

coveralls commented 9 years ago

Coverage Status

Changes Unknown when pulling 287aaa499fef1b7f8174c43504050c17a9823a51 on ird/tempfix into \ on master**.

sbromberger commented 9 years ago

Wow. That Travis build got the memoryerror as well:

INFO: Testing GnuTLS
ERROR: LoadError: MemoryError()
while loading /home/travis/.julia/v0.4/GnuTLS/test/runtests.jl, in expression starting on line 7

Edited (taken back my previous suggestion).

IainNZ commented 9 years ago

When I use this PR to do a lot of queries to the Github API (for the package website), it just seems to hang. Master just fails.

sbromberger commented 9 years ago

FYI. Using requests.jl (which uses GnuTLS), I'm getting troubling inconsistent behavior:

julia> get("https://www.github.com")   # I hit ^C after a stall / 100% CPU.
^CERROR: type InterruptException has no field msg
 in close at /Users/seth/.julia/v0.4/GnuTLS/src/GnuTLS.jl:101
 in readtobuf at /Users/seth/.julia/v0.4/GnuTLS/src/GnuTLS.jl:382
 in readavailable at /Users/seth/.julia/v0.4/GnuTLS/src/GnuTLS.jl:405
 in process_response at /Users/seth/.julia/v0.4/Requests/src/Requests.jl:230
 in do_request at /Users/seth/.julia/v0.4/Requests/src/Requests.jl:565
 in get at /Users/seth/.julia/v0.4/Requests/src/Requests.jl:579
 in get at /Users/seth/.julia/v0.4/Requests/src/Requests.jl:578

julia> get("https://www.github.com")
ERROR: type UVError has no field msg
 in close at /Users/seth/.julia/v0.4/GnuTLS/src/GnuTLS.jl:101
 in readtobuf at /Users/seth/.julia/v0.4/GnuTLS/src/GnuTLS.jl:382
 in readavailable at /Users/seth/.julia/v0.4/GnuTLS/src/GnuTLS.jl:405
 in process_response at /Users/seth/.julia/v0.4/Requests/src/Requests.jl:230
 in do_request at /Users/seth/.julia/v0.4/Requests/src/Requests.jl:565
 in get at /Users/seth/.julia/v0.4/Requests/src/Requests.jl:579
 in get at /Users/seth/.julia/v0.4/Requests/src/Requests.jl:578

julia> get("https://www.github.com")
Response(301 Moved Permanently, 11 Headers, 0 Bytes in Body)
coveralls commented 9 years ago

Coverage Status

Changes Unknown when pulling 3f3ea44055ba35e4a0ade927b35faa149476e3c5 on ird/tempfix into \ on master**.

IainNZ commented 9 years ago

Wow Travis passed! So where are we at?

sbromberger commented 9 years ago

This works but it's still a mess. I'm getting ready to post on -users because I'm seeing a ~13-second delay on any check of eof(Session) and I don't think that's right.

The reason this doesn't affect Requests is because that code closes the stream after the HTML is parsed, so it has no need for eof. Does it make sense that an eof check would block like this?

https://groups.google.com/forum/#!topic/julia-users/tsy0s1UHXjg

sbromberger commented 9 years ago

PS: the readall() code is faulty. I don't think there's a way to do this without checking eof(), which causes this delay I'm seeing.

IainNZ commented 9 years ago

Yeah I just tried to run PkgEval and hit some sort of stall - but I'm using Requests...?

sbromberger commented 9 years ago

Requests is intermittently broken right now (see https://github.com/JuliaWeb/Requests.jl/issues/41 and https://github.com/JuliaWeb/Requests.jl/commit/87ad86994dc9585d0cd4f97f6ef187ac22fe1c56).

IainNZ commented 9 years ago

Clearing up some mess, closing in favour of #36