Avoid forkIO in bye

[FinleyMcIlwaine]

If the peer has disconnected, bye may throw resource vanished exceptions from a forked thread, whose top-level exception handler will print them to stdout. We are addressing this by removing the forkIOs from bye. The patch now exists at kazu's new-bye branch. This one has been marked as a draft in the meantime, and should be closed when that branch is merged.

[FinleyMcIlwaine]

Let me know if the changes I made are what you had in mind!

[kazu-yamamoto]

Now LGTM!

[kazu-yamamoto]

Rebased and merged.
Thank you for your contribution!

[FinleyMcIlwaine]

Thanks @kazu-yamamoto! In the meantime, we realized that the behavior introduced by this PR might not be desirable. In particular, we might not want to swallow synchronous exceptions if the shutdown sequence itself is not the result of some other exception. I will open another PR tomorrow that attempts to address this.

[kazu-yamamoto]

If you was able to provide test cases, it would be great.

[edsko]

A bit of background for this PR. Suppose we have something like

bracket (... setup ...) bye $ body

If an exception is thrown in body, say because the connection was broken, then it's very likely that bye will not be able to properly conduct the TLS shutdown sequence, and throw an exception of its own; but that means that the exception that is thrown is the one from bye, not the one from body. That's what motivated this PR.

However, what we realized only after submitting: what happens if body runs just fine, but bye throws a legitimate exception of its own (i.e., everything was okay, but the TLS shutdown sequence was not). This could potentially be some kind of truncation attack, in which case we might want to know that something went wrong, and suppressing the exception, as is done in this PR, is not the right thing.

It's not entirely clear to me however what the right solution is. Even if we tell bye if an exception had occurred prior to calling bye, we still don't know if that means that that exception meant that the connection was broken. So if we decide that it is important that we see "legitimate" exceptions, then perhaps we have no choice but to throw an exception from bye regardless of what happened prior to it.

So, the path going forward as I see it:

1. Is it important that legitimate exceptions that occur in bye (i.e., connection is fine, but TLS shutdown fails) are visible? If not, then everything is fine as-is after this PR, and we are done.

2. If that is not fine, then we need to change things:

   • Instead of swallowing exceptions in bye, we should throw them as normal; but the definition of bye as it was prior to this PR was already swallowing some of those exceptions; the ones thrown in the forked threads. They would be caught only in the default top-levle exception handler and therefore likely printed to the console, but would not be thrown to the main program. We should instead catch those exceptions and throw them to the thread that bye itself runs in.
   • We need to think a bit about the http2 case (which is the context in which we are considering this). In http2, if the connection is closed suddenly; the backgrounds threads terminate normally on ConnectionIsClosed, but then we get the exception anyway from bye. Not sure what to do about this, other than just to accept this and say that you will get an exception in this case.

@FinleyMcIlwaine , could you verify please if doing this throw-to-main-thread thing in bye is actually sufficient for our purposes?

[FinleyMcIlwaine]

Implemented the throw-to-main-thread thing in #478, it also solves the exception output we were seeing in grapesy.

[kazu-yamamoto]

I should have written this first. I think the usage bracket setup bye body is simply wrong. body should be:

body = do
   ctx <- contextNew
   handshake ctx
   something
   bye ctx

However, even I made this mistake. You can find my wrong examples in http2-tls.
So, I introduced forkIOWithUnmask so that a new thread can receive an asynchronous exception from timeout even if bye is used in bracket.

It's time to revisit this issue.
If we can ask users to change their code to not use bye in bracket, the issue would get simpler.

Recently I'm not a fan of asynchronous exceptions, so I don't want to use throwTo.

[edsko]

If we can ask users to change their code to not use bye in bracket, the issue would get simpler.

I agree, that avoids the whole problem of trying to call bye after an exception has already happened. This makes sense to me.

However, we still then have the possibility of an exception arising in one of the auxiliary threads spawned by bye. I am not 100% sure why you were spawning those threads in the first place; I'd be fine with a solution which

1. Avoids bye in bracket
2. Avoids forking new threads in bye (just do everything synchronously).

Would that be okay with you?

[kazu-yamamoto]

I agree, that avoids the whole problem of trying to call bye after an exception has already happened. This makes sense to me.

However, we still then have the possibility of an exception arising in one of the auxiliary threads spawned by bye. I am not 100% sure why you were spawning those threads in the first place; I'd be fine with a solution which

bye was actually used in bracket where asynchronous exceptions don't reach. To catch the timeout exception, a new thread must be spawned.

1. Avoids bye in bracket
2. Avoids forking new threads in bye (just do everything synchronously).

Would that be okay with you?

My plan is 1) AND 2). Not 1) OR 2).

[edsko]

My plan is 1) AND 2). Not 1) OR 2).

Yes, that is what I meant also :)

[kazu-yamamoto]

For the first step, I removed forkIO:

• https://github.com/kazu-yamamoto/hs-tls/tree/new-bye
• https://github.com/kazu-yamamoto/http2-tls/tree/new-bye

[kazu-yamamoto]

@edsko @FinleyMcIlwaine What should we do next?

[edsko]

@kazu-yamamoto sorry for the delay.

• The patch to http2-tls in https://github.com/kazu-yamamoto/http2-tls/tree/new-bye looks good.
• The patch to tls in https://github.com/kazu-yamamoto/hs-tls/tree/new-bye I think is not quite right yet: the call to swallow in bye should go entirely. Rationale: we added it because when bye is called in bracket, it will be called also on abnormal termination (for example, when the connection is broken), which would then result in new exceptions that would hide the old exceptions. However, now that we have concluded that it should not be used in bracket at all (as your new documentation also clearly states), then we don't want to swallow exceptions anymore: if everything goes well up until the call to bye (no exception is raised yet), then we want to know if the shutdown sequence fails (potential truncation attack attempt); if an exception is raised before the call to bye, bye will not be called at all.

So I think once that call to swallow is gone (and swallow is then redundant and can be removed), this is good to go.

[kazu-yamamoto]

@edsko Thanks. That's what I want to hear. Would you send a PR?

[edsko]

Yup, will do!

[FinleyMcIlwaine]

@kazu-yamamoto I opened a PR removing the swallowSync from bye on your new-bye branch: kazu-yamamoto#6. Once that patch is merged into your new-bye branch and the new-bye branch is merged into main, this PR can be closed as well. Alternatively, I could change this PR to include all of those changes. Just let me know if that is what you would prefer. I've changed this to a draft in the meantime to indicate that the changes proposed on this PR should not be merged as-is.

[kazu-yamamoto]

Closing with #480.