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

crypto/tls: Set{Read,Write}Deadline implementation is surprising/ineffective #13828

Open
rburchell opened this Issue Jan 5, 2016 · 3 comments

Comments

Projects
None yet
4 participants
@rburchell
Copy link

rburchell commented Jan 5, 2016

Bear with me, this is a little long-winded, but I'm not sure how I can explain this more succinctly..

The current implementation of these functions is to pass them directly through to c.conn. Judging by my understanding of how TLS works, I think this is wrong. Picture a TLS listener socket which has just accepted a client (which won't ever complete a handshake, say, telnet or something).

Given:

type aClient struct {
    conn net.Conn
}

On accept of an incoming connection, an aClient instance is created (with conn populated from the TLS listener) & the server is instructed to time out the client after X seconds of not sending anything (through a goroutine, a Timer, and a channel to cancel the goroutine), something like this:

func authTimeout(client *aClient, controlChan chan int) {
    for {
        timer := time.NewTimer(time.Second * 5)
        select {
        case <-timer.C:
            client.Write("You didn't authenticate in time.")
            client.conn.Close()
            return
        case <-controlChan:
            return
        }
    }
}

client.Write's implementation looks something like this:

func (this *aClient) Write(format string, args ...interface{}) {
    // one would expect this to time out...
    this.conn.SetWriteDeadline(time.Now().Add(10 * time.Second))
    this.conn.Write(fmsg)
    // but if the client doesn't handshake, it'll hang!
}

authTimeout's timer will successfully fire, Write will be called, and set the Write deadline, and attempt to write. However, because the connection hasn't been read/written previously, it needs a handshake. And as handshaking requires reading from the underlying socket, this write deadline won't ever actually trigger.

This, to me, is incorrect/surprising. I'm not entirely sure what the behaviour of deadlines should be before handshake (maybe take whichever is the lowest? Maybe offer an explicit handshake deadline?) but I'm pretty sure there needs to be something done here even if that something is just a warning in the documentation. This bit me when blindly porting some code over from net/tcp to TLS.

It's worth noting, also, that net/http/transport.go tries to work around this very issue, but at least my own attempts to implement a similar pattern (handshake right after accepting, with a goroutine to time out the handshake and Close() on fail) seemed to suffer similar problems with Close() never returning unless I set both a ReadDeadline and WriteDeadline. I didn't (yet) analyze this problem, though.

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jan 5, 2016

@rburchell

This comment has been minimized.

Copy link
Author

rburchell commented Jan 27, 2016

It looks like Close() hangs in my case because it also locks the handshake mutex. So I had one goroutine stuck in a handshake that would never progress (with the handshake mutex locked), and another tried to Close() - and blocked on the same mutex.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Jan 27, 2016

Git rev 4ffba76 might help ("crypto/tls: don't block in Conn.Close if Writes are in-flight")

@rburchell

This comment has been minimized.

Copy link
Author

rburchell commented Jan 27, 2016

Thanks, yes, that looks like that would fix that particular problem. Then we're just left with the original report :)

@agl agl self-assigned this Mar 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.