Merge pull request #1503 from fjl/fix-accounts-race

accounts: fix data race when key is locked after the unlock timeout

accounts/account_manager.go

@@ -78,8 +78,8 @@ func (am *Manager) DeleteAccount(address common.Address, auth string) error {
 
 func (am *Manager) Sign(a Account, toSign []byte) (signature []byte, err error) {
 	am.mutex.RLock()
+	defer am.mutex.RUnlock()
 	unlockedKey, found := am.unlocked[a.Address]
-	am.mutex.RUnlock()
 	if !found {
 		return nil, ErrLocked
 	}
@@ -87,14 +87,17 @@ func (am *Manager) Sign(a Account, toSign []byte) (signature []byte, err error)
 	return signature, err
 }
 
-// unlock indefinitely
+// Unlock unlocks the given account indefinitely.
 func (am *Manager) Unlock(addr common.Address, keyAuth string) error {
 	return am.TimedUnlock(addr, keyAuth, 0)
 }
 
-// Unlock unlocks the account with the given address. The account
-// stays unlocked for the duration of timeout
-// it timeout is 0 the account is unlocked for the entire session
+// TimedUnlock unlocks the account with the given address. The account
+// stays unlocked for the duration of timeout. A timeout of 0 unlocks the account
+// until the program exits.
+//
+// If the accout is already unlocked, TimedUnlock extends or shortens
+// the active unlock timeout.
 func (am *Manager) TimedUnlock(addr common.Address, keyAuth string, timeout time.Duration) error {
 	key, err := am.keyStore.GetKey(addr, keyAuth)
 	if err != nil {

accounts/accounts_test.go

@@ -23,9 +23,10 @@ import (
 	"time"
 
 	"github.com/ethereum/go-ethereum/crypto"
-	"github.com/ethereum/go-ethereum/crypto/randentropy"
 )
 
+var testSigData = make([]byte, 32)
+
 func TestSign(t *testing.T) {
 	dir, ks := tmpKeyStore(t, crypto.NewKeyStorePlain)
 	defer os.RemoveAll(dir)
@@ -33,26 +34,24 @@ func TestSign(t *testing.T) {
 	am := NewManager(ks)
 	pass := "" // not used but required by API
 	a1, err := am.NewAccount(pass)
-	toSign := randentropy.GetEntropyCSPRNG(32)
 	am.Unlock(a1.Address, "")
 
-	_, err = am.Sign(a1, toSign)
+	_, err = am.Sign(a1, testSigData)
 	if err != nil {
 		t.Fatal(err)
 	}
 }
 
 func TestTimedUnlock(t *testing.T) {
-	dir, ks := tmpKeyStore(t, crypto.NewKeyStorePassphrase)
+	dir, ks := tmpKeyStore(t, crypto.NewKeyStorePlain)
 	defer os.RemoveAll(dir)
 
 	am := NewManager(ks)
 	pass := "foo"
 	a1, err := am.NewAccount(pass)
-	toSign := randentropy.GetEntropyCSPRNG(32)
 
 	// Signing without passphrase fails because account is locked
-	_, err = am.Sign(a1, toSign)
+	_, err = am.Sign(a1, testSigData)
 	if err != ErrLocked {
 		t.Fatal("Signing should've failed with ErrLocked before unlocking, got ", err)
 	}
@@ -63,28 +62,26 @@ func TestTimedUnlock(t *testing.T) {
 	}
 
 	// Signing without passphrase works because account is temp unlocked
-	_, err = am.Sign(a1, toSign)
+	_, err = am.Sign(a1, testSigData)
 	if err != nil {
 		t.Fatal("Signing shouldn't return an error after unlocking, got ", err)
 	}
 
 	// Signing fails again after automatic locking
 	time.Sleep(150 * time.Millisecond)
-	_, err = am.Sign(a1, toSign)
+	_, err = am.Sign(a1, testSigData)
 	if err != ErrLocked {
 		t.Fatal("Signing should've failed with ErrLocked timeout expired, got ", err)
 	}
-
 }
 
 func TestOverrideUnlock(t *testing.T) {
-	dir, ks := tmpKeyStore(t, crypto.NewKeyStorePassphrase)
+	dir, ks := tmpKeyStore(t, crypto.NewKeyStorePlain)
 	defer os.RemoveAll(dir)
 
 	am := NewManager(ks)
 	pass := "foo"
 	a1, err := am.NewAccount(pass)
-	toSign := randentropy.GetEntropyCSPRNG(32)
 
 	// Unlock indefinitely
 	if err = am.Unlock(a1.Address, pass); err != nil {
@@ -92,7 +89,7 @@ func TestOverrideUnlock(t *testing.T) {
 	}
 
 	// Signing without passphrase works because account is temp unlocked
-	_, err = am.Sign(a1, toSign)
+	_, err = am.Sign(a1, testSigData)
 	if err != nil {
 		t.Fatal("Signing shouldn't return an error after unlocking, got ", err)
 	}
@@ -103,20 +100,45 @@ func TestOverrideUnlock(t *testing.T) {
 	}
 
 	// Signing without passphrase still works because account is temp unlocked
-	_, err = am.Sign(a1, toSign)
+	_, err = am.Sign(a1, testSigData)
 	if err != nil {
 		t.Fatal("Signing shouldn't return an error after unlocking, got ", err)
 	}
 
 	// Signing fails again after automatic locking
 	time.Sleep(150 * time.Millisecond)
-	_, err = am.Sign(a1, toSign)
+	_, err = am.Sign(a1, testSigData)
 	if err != ErrLocked {
 		t.Fatal("Signing should've failed with ErrLocked timeout expired, got ", err)
 	}
 }
 
-//
+// This test should fail under -race if signing races the expiration goroutine.
+func TestSignRace(t *testing.T) {
+	dir, ks := tmpKeyStore(t, crypto.NewKeyStorePlain)
+	defer os.RemoveAll(dir)
+
+	// Create a test account.
+	am := NewManager(ks)
+	a1, err := am.NewAccount("")
+	if err != nil {
+		t.Fatal("could not create the test account", err)
+	}
+
+	if err := am.TimedUnlock(a1.Address, "", 15*time.Millisecond); err != nil {
+		t.Fatalf("could not unlock the test account", err)
+	}
+	end := time.Now().Add(80 * time.Millisecond)
+	for time.Now().Before(end) {
+		if _, err := am.Sign(a1, testSigData); err == ErrLocked {
+			return
+		} else if err != nil {
+			t.Errorf("Sign error: %v", err)
+			return
+		}
+	}
+	t.Errorf("Account did not lock within the timeout")
+}
 
 func tmpKeyStore(t *testing.T, new func(string) crypto.KeyStore) (string, crypto.KeyStore) {
 	d, err := ioutil.TempDir("", "eth-keystore-test")

crypto/key_store_passphrase.go

@@ -147,7 +147,6 @@ func (ks keyStorePassphrase) DeleteKey(keyAddr common.Address, auth string) (err
 }
 
 func decryptKeyFromFile(keysDirPath string, keyAddr common.Address, auth string) (keyBytes []byte, keyId []byte, err error) {
-	fmt.Printf("%v\n", keyAddr.Hex())
 	m := make(map[string]interface{})
 	err = getKey(keysDirPath, keyAddr, &m)
 	if err != nil {