Skip to content

In Tls_lwt dont throw EPIPE error if we got close_notify#371

Closed
rymdhund wants to merge 1 commit into
mirleft:masterfrom
rymdhund:broken-pipe-fix
Closed

In Tls_lwt dont throw EPIPE error if we got close_notify#371
rymdhund wants to merge 1 commit into
mirleft:masterfrom
rymdhund:broken-pipe-fix

Conversation

@rymdhund
Copy link
Copy Markdown

@rymdhund rymdhund commented Dec 2, 2017

I don't really have any understanding of this lib, but i think this would fix #347.

According to SSL_shutdown(3):
"The shutdown procedure consists of 2 steps: the sending of the “close notify” shutdown alert and the reception of the peer's “close notify” shutdown alert. According to the TLS standard, it is acceptable for an application to only send its shutdown alert and then close the underlying connection without waiting for the peer's response"
@hannesm
Copy link
Copy Markdown
Member

hannesm commented Dec 17, 2017

this seems to me to handle more EPIPE than intended originally.

Isn't a viable solution to add Sys.(set_signal sigpipe Signal_ignore) to your application (/cc @edwintorok)? That's at least what I learned to do in various OCaml applications (even when not using this TLS library).

@edwintorok
Copy link
Copy Markdown

The test code already sets sigpipe to ignore https://github.com/mirage/ocaml-conduit/blob/b74dbbda7cbcce1b3d398d7499efd99eec7c62b4/tests/unix/cdtest_tls.ml#L62, however that only means that instead of raising a signal you get EPIPE from the system call, which caused the tls code to raise an exception.

The original problem in the referenced bug was that when that exception is raised we actually loose the last message sent by the other side; I haven't checked exactly what happens but my guess is you get a short read and then if the code insists reading again it gets an EPIPE. We should first return whatever the last message was, process it through the TLS state machine, and raise the exception only if we get EPIPE and we haven't reached EOF yet.
In that sense the code in this PR seems to do the right thing, but I haven't tested it yet.

@hannesm
Copy link
Copy Markdown
Member

hannesm commented Dec 19, 2017

@edwintorok thanks for explanation. from what I read in the spec, read should never return EPIPE -- it is write which may return EPIPE.

if you find the time to test this PR, I'd be very interested whether it fixes the issue for you. please report back.

@anmonteiro
Copy link
Copy Markdown

I'm also seeing reproducible EPIPE signals that cause the last chunk of data from the remote endpoint to be lost. I tried this PR, however, without success.

@anmonteiro
Copy link
Copy Markdown

I just opened #387 with an alternative to this PR that I think is less invasive and actually fixes the symptom in my application.

@hannesm
Copy link
Copy Markdown
Member

hannesm commented Jan 7, 2019

thanks, tls 0.9.3 is released (PRed to opam-repository ocaml/opam-repository#13231) which fixes this issue

@hannesm hannesm closed this Jan 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

handle shutdowns and last message sent by server gracefully

4 participants