charmbracelet / ssh / 31200172697

63%

master: 63%

Pull #56

github

fix: guard handshakeDeadline against concurrent access

handleConn clears the handshake deadline once gossh.NewServerConn returns:

	sshConn, chans, reqs, err := gossh.NewServerConn(conn, srv.config(ctx))
	...
	conn.handshakeDeadline = time.Time{}

By that point NewServerConn has started the crypto/ssh read loop, which
calls serverConn.Read on every packet, and Read consults handshakeDeadline
through updateDeadline whenever IdleTimeout is set. The write and those
reads are unsynchronized, so -race reports a data race on connections that
set both HandshakeTimeout and IdleTimeout.

The window is narrow and post-authentication, so the practical effect is
limited: the handshake deadline has already done its job by the time the
racing write lands, and it cannot widen the pre-auth window. What can go
wrong is deadline bookkeeping, either a stale read leaving a deadline the
connection should have shed, or a torn read handing SetDeadline a garbage
time.Time.

Guard the field with a mutex and route every access through
setHandshakeDeadline, clearHandshakeDeadline, and a locked read in
updateDeadline, so no raw access to it remains. idleTimeout and
maxDeadline are set before the handshake begins and never mutated after,
so they stay lock-free.

The test reproduces the access pattern directly rather than through a live
handshake; driving it via real connections only caught the race about a
third of the time, while the direct version reports it on every run
without this fix and none with it.

21 of 21 new or added lines in 2 files covered. (100.0%)

850 of 1347 relevant lines covered (63.1%)

663.81 hits per line