Repository: emersion/go-imap
Pull request #90 (2016-12-24 01:29:02)

Use a mutex to protect access the client Caps map
Hi again!

This is to resolve a panic I encountered:


```
panic: assignment to entry in nil map

goroutine 37 [running]:
panic(0x8253380, 0x18768800)
        /home/will/bin/go-src/src/runtime/panic.go:500 +0x331
github.com/emersion/go-imap/client.(*Client).gotStatusCaps(0x189f8fa0, 0x18788798, 0x14, 0x15)
        /home/will/code/go/src/github.com/emersion/go-imap/client/client.go:234 +0x12c
github.com/emersion/go-imap/client.(*Client).handleUnilateral(0x189f8fa0)
        /home/will/code/go/src/github.com/emersion/go-imap/client/client.go:314 +0xbc5
created by github.com/emersion/go-imap/client.New
        /home/will/code/go/src/github.com/emersion/go-imap/client/client.go:435 +0x3c2
```

From what I can gather, there are at least two goroutines that will access this map. `handleUnilateral()`, and when using functions such as `Login()`.

Thanks for your time!


Patch:
diff --git a/client/client.go b/client/client.go
index e96cae2..ab9c600 100644
--- a/client/client.go
+++ b/client/client.go
@@ -8,6 +8,7 @@ import (
 	"log"
 	"net"
 	"os"
+	"sync"
 	"time"
 
 	"github.com/emersion/go-imap"
@@ -27,8 +28,11 @@ type Client struct {
 	greeted   chan struct{}
 	loggedOut chan struct{}
 
-	// The server capabilities.
-	Caps map[string]bool
+	// The cached server capabilities.
+	caps map[string]bool
+	// The caps map may be accessed in different goroutines. Protect access.
+	capsLocker sync.Mutex
+
 	// The current connection state.
 	State imap.ConnState
 	// The selected mailbox, if there is one.
@@ -228,12 +232,16 @@ func (c *Client) handleContinuationReqs(continues chan<- bool) {
 }
 
 func (c *Client) gotStatusCaps(args []interface{}) {
-	c.Caps = make(map[string]bool)
+	c.capsLocker.Lock()
+
+	c.caps = make(map[string]bool)
 	for _, cap := range args {
 		if cap, ok := cap.(string); ok {
-			c.Caps[cap] = true
+			c.caps[cap] = true
 		}
 	}
+
+	c.capsLocker.Unlock()
 }
 
 // The server can send unilateral data. This function handles it.
diff --git a/client/cmd_any.go b/client/cmd_any.go
index 76c91a6..016b2f5 100644
--- a/client/cmd_any.go
+++ b/client/cmd_any.go
@@ -16,25 +16,47 @@ var ErrAlreadyLoggedOut = errors.New("Already logged out")
 // Capabilities are often returned by the server with the greeting or with the
 // STARTTLS and LOGIN responses, so usually explicitly requesting capabilities
 // isn't needed.
-func (c *Client) Capability() (caps map[string]bool, err error) {
+//
+// Most of the time, Support should be used instead.
+func (c *Client) Capability() (map[string]bool, error) {
 	cmd := &commands.Capability{}
 	res := &responses.Capability{}
 
-	status, err := c.execute(cmd, res)
-	if err != nil {
-		return
-	}
-	if err = status.Err(); err != nil {
-		return
+	if status, err := c.execute(cmd, res); err != nil {
+		return nil, err
+	} else if err := status.Err(); err != nil {
+		return nil, err
 	}
 
-	caps = make(map[string]bool)
+	caps := make(map[string]bool)
 	for _, name := range res.Caps {
 		caps[name] = true
 	}
 
-	c.Caps = caps
-	return
+	c.capsLocker.Lock()
+	c.caps = caps
+	c.capsLocker.Unlock()
+	return caps, nil
+}
+
+// Support checks if cap is a capability supported by the server. If the server
+// hasn't sent its capabilities yet, Support requests them.
+func (c *Client) Support(cap string) (bool, error) {
+	c.capsLocker.Lock()
+	ok := c.caps != nil
+	c.capsLocker.Unlock()
+
+	// If capabilities are not cached, request them
+	if !ok {
+		if _, err := c.Capability(); err != nil {
+			return false, err
+		}
+	}
+
+	c.capsLocker.Lock()
+	supported := c.caps[cap]
+	c.capsLocker.Unlock()
+	return supported, nil
 }
 
 // Noop always succeeds and does nothing.
diff --git a/client/cmd_noauth.go b/client/cmd_noauth.go
index caa76a9..6269b38 100644
--- a/client/cmd_noauth.go
+++ b/client/cmd_noauth.go
@@ -24,15 +24,12 @@ var (
 	ErrLoginDisabled = errors.New("Login is disabled in current state")
 )
 
-// SupportsStartTLS checks if the server supports STARTTLS.
-func (c *Client) SupportsStartTLS() bool {
-	return c.Caps[imap.StartTLS]
+// SupportStartTLS checks if the server supports STARTTLS.
+func (c *Client) SupportStartTLS() (bool, error) {
+	return c.Support(imap.StartTLS)
 }
 
 // StartTLS starts TLS negotiation.
-//
-// This function also resets c.Caps because capabilities change when TLS is
-// enabled.
 func (c *Client) StartTLS(tlsConfig *tls.Config) (err error) {
 	if c.isTLS {
 		err = ErrTLSAlreadyEnabled
@@ -53,7 +50,11 @@ func (c *Client) StartTLS(tlsConfig *tls.Config) (err error) {
 			return nil, err
 		}
 
-		c.Caps = nil
+		// Capabilities change when TLS is enabled
+		c.capsLocker.Lock()
+		c.caps = nil
+		c.capsLocker.Unlock()
+
 		return tlsConn, nil
 	})
 	if err != nil {
@@ -64,9 +65,9 @@ func (c *Client) StartTLS(tlsConfig *tls.Config) (err error) {
 	return
 }
 
-// SupportsAuth checks if the server supports a given authentication mechanism.
-func (c *Client) SupportsAuth(mech string) bool {
-	return c.Caps["AUTH="+mech]
+// SupportAuth checks if the server supports a given authentication mechanism.
+func (c *Client) SupportAuth(mech string) (bool, error) {
+	return c.Support("AUTH="+mech)
 }
 
 // Authenticate indicates a SASL authentication mechanism to the server. If the
@@ -101,24 +102,31 @@ func (c *Client) Authenticate(auth sasl.Client) error {
 	}
 
 	c.State = imap.AuthenticatedState
-	c.Caps = nil
 
-	if status.Code == "CAPABILITY" {
+	// Capabilities change when user is logged in
+	c.capsLocker.Lock()
+	c.caps = nil
+	c.capsLocker.Unlock()
+
+	if status.Code == imap.Capability {
 		c.gotStatusCaps(status.Arguments)
 	}
+
 	return nil
 }
 
 // Login identifies the client to the server and carries the plaintext password
 // authenticating this user.
-func (c *Client) Login(username, password string) (err error) {
+func (c *Client) Login(username, password string) error {
 	if c.State != imap.NotAuthenticatedState {
-		err = ErrAlreadyLoggedIn
-		return
+		return ErrAlreadyLoggedIn
 	}
-	if c.Caps["LOGINDISABLED"] {
-		err = ErrLoginDisabled
-		return
+
+	c.capsLocker.Lock()
+	loginDisabled := c.caps != nil && c.caps["LOGINDISABLED"]
+	c.capsLocker.Unlock()
+	if loginDisabled {
+		return ErrLoginDisabled
 	}
 
 	cmd := &commands.Login{
@@ -128,18 +136,20 @@ func (c *Client) Login(username, password string) (err error) {
 
 	status, err := c.execute(cmd, nil)
 	if err != nil {
-		return
+		return err
 	}
 	if err = status.Err(); err != nil {
-		return
+		return err
 	}
 
 	c.State = imap.AuthenticatedState
-	c.Caps = nil
+
+	c.capsLocker.Lock()
+	c.caps = nil
+	c.capsLocker.Unlock()
 
 	if status.Code == "CAPABILITY" {
 		c.gotStatusCaps(status.Arguments)
 	}
-
-	return
+	return nil
 }


Test patch:
diff --git a/client/client_test.go b/client/client_test.go
index 34373e3..b2fed2d 100644
--- a/client/client_test.go
+++ b/client/client_test.go
@@ -90,7 +90,9 @@ func removeCmdTag(cmd string) string {
 
 func TestClient(t *testing.T) {
 	ct := func(c *client.Client) error {
-		if !c.Caps["IMAP4rev1"] {
+		if ok, err := c.Support("IMAP4rev1"); err != nil {
+			return err
+		} else if !ok {
 			return errors.New("Server hasn't IMAP4rev1 capability")
 		}
 		return nil
diff --git a/client/cmd_noauth_test.go b/client/cmd_noauth_test.go
index d03dd50..cb910bd 100644
--- a/client/cmd_noauth_test.go
+++ b/client/cmd_noauth_test.go
@@ -26,29 +26,28 @@ func TestClient_StartTLS(t *testing.T) {
 		Certificates:       []tls.Certificate{cert},
 	}
 
-	ct := func(c *client.Client) (err error) {
+	ct := func(c *client.Client) error {
 		if c.IsTLS() {
-			err = fmt.Errorf("Client has TLS enabled before STARTTLS")
-			return
+			return fmt.Errorf("Client has TLS enabled before STARTTLS")
 		}
 
-		if !c.SupportsStartTLS() {
-			err = fmt.Errorf("Server doesn't support STARTTLS")
-			return
+
+		if ok, err := c.SupportStartTLS(); err != nil {
+			return err
+		} else if !ok {
+			return fmt.Errorf("Server doesn't support STARTTLS")
 		}
 
-		err = c.StartTLS(tlsConfig)
-		if err != nil {
-			return
+		if err := c.StartTLS(tlsConfig); err != nil {
+			return err
 		}
 
 		if !c.IsTLS() {
-			err = fmt.Errorf("Client has not TLS enabled after STARTTLS")
-			return
+			return fmt.Errorf("Client has not TLS enabled after STARTTLS")
 		}
 
-		_, err = c.Capability()
-		return
+		_, err := c.Capability()
+		return err
 	}
 
 	st := func(c net.Conn) {
@@ -81,24 +80,24 @@ func TestClient_StartTLS(t *testing.T) {
 }
 
 func TestClient_Authenticate(t *testing.T) {
-	ct := func(c *client.Client) (err error) {
-		if !c.SupportsAuth("PLAIN") {
-			err = fmt.Errorf("Server doesn't support AUTH=PLAIN")
-			return
+	ct := func(c *client.Client) error {
+		if ok, err := c.SupportAuth(sasl.Plain); err != nil {
+			return err
+		} else if !ok {
+			return fmt.Errorf("Server doesn't support AUTH=PLAIN")
 		}
 
 		sasl := sasl.NewPlainClient("", "username", "password")
 
-		err = c.Authenticate(sasl)
-		if err != nil {
-			return
+		if err := c.Authenticate(sasl); err != nil {
+			return err
 		}
 
 		if c.State != imap.AuthenticatedState {
 			return fmt.Errorf("Client is not in authenticated state after AUTENTICATE")
 		}
 
-		return
+		return nil
 	}
 
 	st := func(c net.Conn) {