~emersion/soju#55: 
Race in conn.SendMessage

Excerpt of conn.go:

func (c *conn) Close() error {
	if c.isClosed() {
		return fmt.Errorf("connection already closed")
	}
	close(c.closed)
	close(c.outgoing)
	return nil
}

func (c *conn) SendMessage(msg *irc.Message) {
	if c.isClosed() {
		return
	}
	c.outgoing <- msg
}

This is racy. SendMessage can perform the c.isClosed() check, then be pre-empted, then Close can close c.outgoing. When SendMessage is resumed, it'll write to a closed channel.

Status
RESOLVED FIXED
Submitter
~emersion
Assigned to
No-one
Submitted
6 months ago
Updated
6 months ago
Labels
No labels applied.

~emersion REPORTED FIXED 6 months ago

commit e9cebb6fe35cc2e29d501c56234dc1a687a536fb
Author: Simon Ser <contact@emersion.fr>
Date:   Thu Apr 30 10:35:02 2020 +0200

    Use a lock to protect conn.{closed,outgoing}
    
    Unfortunately, I don't think there's a good way to implement net.Conn
    semantics on top of channels. The Close and SendMessage methods should
    gracefully fail without panicking if the connection is already closed.
    Using only channels leads to race conditions.
    
    We could remove the lock if Close and SendMessage are only called from a
    single goroutine. However that's not the case right now.
Register here or Log in to comment, or comment via email.