patch: re-work locking in core and fix deadlocks (#143)

core/buffer.go

@@ -23,7 +23,7 @@ Buffer is a structure that holds raw sensitive data.
 The number of Buffers that can exist at one time is limited by how much memory your system's kernel allows each process to mlock/VirtualLock. Therefore you should call DestroyBuffer on Buffers that you no longer need, ideally defering a Destroy call after creating a new one.
 */
 type Buffer struct {
-	sync.RWMutex // Local mutex lock
+	sync.RWMutex // Local mutex lock // TODO: this does not protect 'data' field
 
 	alive   bool // Signals that destruction has not come
 	mutable bool // Mutability state of underlying memory
@@ -44,12 +44,10 @@ NewBuffer is a raw constructor for the Buffer object.
 func NewBuffer(size int) (*Buffer, error) {
 	var err error
 
-	// Return an error if length < 1.
 	if size < 1 {
 		return nil, ErrNullBuffer
 	}
 
-	// Declare and allocate
 	b := new(Buffer)
 
 	// Allocate the total needed memory
@@ -119,18 +117,14 @@ func (b *Buffer) Freeze() {
 }
 
 func (b *Buffer) freeze() error {
-	// Attain lock.
 	b.Lock()
 	defer b.Unlock()
 
-	// Check if destroyed.
 	if !b.alive {
 		return nil
 	}
 
-	// Only do anything if currently mutable.
 	if b.mutable {
-		// Make the memory immutable.
 		if err := memcall.Protect(b.inner, memcall.ReadOnly()); err != nil {
 			return err
 		}
@@ -148,18 +142,14 @@ func (b *Buffer) Melt() {
 }
 
 func (b *Buffer) melt() error {
-	// Attain lock.
 	b.Lock()
 	defer b.Unlock()
 
-	// Check if destroyed.
 	if !b.alive {
 		return nil
 	}
 
-	// Only do anything if currently immutable.
 	if !b.mutable {
-		// Make the memory mutable.
 		if err := memcall.Protect(b.inner, memcall.ReadWrite()); err != nil {
 			return err
 		}
@@ -176,7 +166,6 @@ func (b *Buffer) Scramble() {
 }
 
 func (b *Buffer) scramble() error {
-	// Attain lock.
 	b.Lock()
 	defer b.Unlock()
 	return Scramble(b.Data())
@@ -269,8 +258,6 @@ func (l *bufferList) add(b ...*Buffer) {
 	l.Lock()
 	defer l.Unlock()
 
-	// fmt.Printf("\n\nThere are %d buffers\n\n\n", len(buffers.list))
-
 	l.list = append(l.list, b...)
 }
 
@@ -290,8 +277,6 @@ func (l *bufferList) remove(b *Buffer) {
 	l.Lock()
 	defer l.Unlock()
 
-	// fmt.Printf("\n\nThere are %d buffers\n\n\n", len(buffers.list))
-
 	for i, v := range l.list {
 		if v == b {
 			l.list = append(l.list[:i], l.list[i+1:]...)

core/coffer.go

@@ -16,33 +16,27 @@ var ErrCofferExpired = errors.New("<memguard::core::ErrCofferExpired> attempted
 Coffer is a specialized container for securing highly-sensitive, 32 byte values.
 */
 type Coffer struct {
-	sync.RWMutex
+	sync.Mutex
 
-	left  *Buffer // Left partition.
-	right *Buffer // Right partition.
+	left  *Buffer
+	right *Buffer
 
-	rand *Buffer // Static allocation for fast random bytes reading.
+	rand *Buffer
 }
 
 // NewCoffer is a raw constructor for the *Coffer object.
 func NewCoffer() *Coffer {
-	// Create a new Coffer object.
 	s := new(Coffer)
-
-	// Allocate the partitions.
 	s.left, _ = NewBuffer(32)
 	s.right, _ = NewBuffer(32)
 	s.rand, _ = NewBuffer(32)
 
-	// Initialise with a random 32 byte value.
-	s.Initialise()
+	s.Init()
 
 	go func(s *Coffer) {
-		for {
-			// Sleep for the specified interval.
-			time.Sleep(interval)
+		ticker := time.NewTicker(interval)
 
-			// Re-key the contents, exiting the routine if object destroyed.
+		for range ticker.C {
 			if err := s.Rekey(); err != nil {
 				break
 			}
@@ -52,27 +46,15 @@ func NewCoffer() *Coffer {
 	return s
 }
 
-/*
-Initialise is used to reset the value stored inside a Coffer to a new random 32 byte value, overwriting the old.
-*/
-func (s *Coffer) Initialise() error {
-	// Check if it has been destroyed.
+// Init is used to reset the value stored inside a Coffer to a new random 32 byte value, overwriting the old.
+func (s *Coffer) Init() error {
+	s.Lock()
+	defer s.Unlock()
+
 	if s.Destroyed() {
 		return ErrCofferExpired
 	}
 
-	if err := s.initialise(); err != nil {
-		Panic(err)
-	}
-	return nil
-}
-
-func (s *Coffer) initialise() error {
-	// Attain the mutex.
-	s.Lock()
-	defer s.Unlock()
-
-	// Overwrite the old value with fresh random bytes.
 	if err := Scramble(s.left.Data()); err != nil {
 		return err
 	}
@@ -94,64 +76,37 @@ func (s *Coffer) initialise() error {
 View returns a snapshot of the contents of a Coffer inside a Buffer. As usual the Buffer should be destroyed as soon as possible after use by calling the Destroy method.
 */
 func (s *Coffer) View() (*Buffer, error) {
-	//fmt.Printf("\n\nThere are %d buffers\n\n\n", len(buffers.list))
+	s.Lock()
+	defer s.Unlock()
 
-	// Check if it's destroyed.
 	if s.Destroyed() {
 		return nil, ErrCofferExpired
 	}
-	//fmt.Printf("\n\n%s\n\n\n", "s is not destroyed")
 
-	// Create a new Buffer for the data.
 	b, _ := NewBuffer(32)
 
-	//fmt.Printf("\n\n%s\n\n\n", "made buffer")
-
-	// Attain a read-only lock.
-	s.RLock()
-	defer s.RUnlock()
-
-	//fmt.Printf("\n\n%s\n\n\n", "got lock")
-
-	//fmt.Println(s.right.Data(), s.left.Data())
-
 	// data = hash(right) XOR left
 	h := Hash(s.right.Data())
 
-	//fmt.Printf("\n\n%s\n\n\n", "got hash")
 	for i := range b.Data() {
 		b.Data()[i] = h[i] ^ s.left.Data()[i]
 	}
-	//fmt.Printf("\n\n%s\n\n\n", "computed plaintext")
 	Wipe(h)
 
-	// fmt.Printf("\n\n%s\n\n\n", "got data")
-
-	// Return the view.
 	return b, nil
 }
 
 /*
 Rekey is used to re-key a Coffer. Ideally this should be done at short, regular intervals.
 */
 func (s *Coffer) Rekey() error {
-	// Check if it has been destroyed.
+	s.Lock()
+	defer s.Unlock()
+
 	if s.Destroyed() {
 		return ErrCofferExpired
 	}
 
-	if err := s.rekey(); err != nil {
-		Panic(err)
-	}
-	return nil
-}
-
-func (s *Coffer) rekey() error {
-	// Attain the mutex.
-	s.Lock()
-	defer s.Unlock()
-
-	// Attain 32 bytes of fresh cryptographic buf32.
 	if err := Scramble(s.rand.Data()); err != nil {
 		return err
 	}
@@ -177,28 +132,18 @@ func (s *Coffer) rekey() error {
 /*
 Destroy wipes and cleans up all memory related to a Coffer object. Once this method has been called, the Coffer can no longer be used and a new one should be created instead.
 */
-func (s *Coffer) Destroy() {
-	if err := s.destroy(); err != nil {
-		Panic(err)
-	}
-}
-
-func (s *Coffer) destroy() error {
-	// Attain the mutex.
+func (s *Coffer) Destroy() error {
 	s.Lock()
 	defer s.Unlock()
 
-	// Destroy the partitions.
 	err1 := s.left.destroy()
 	if err1 == nil {
 		buffers.remove(s.left)
 	}
-
 	err2 := s.right.destroy()
 	if err2 == nil {
 		buffers.remove(s.right)
 	}
-
 	err3 := s.rand.destroy()
 	if err3 == nil {
 		buffers.remove(s.rand)
@@ -222,8 +167,5 @@ func (s *Coffer) destroy() error {
 
 // Destroyed returns a boolean value indicating if a Coffer has been destroyed.
 func (s *Coffer) Destroyed() bool {
-	s.RLock()
-	defer s.RUnlock()
-
 	return s.left.data == nil || s.right.data == nil
 }

core/coffer_test.go

@@ -9,7 +9,7 @@ func TestNewCoffer(t *testing.T) {
 	s := NewCoffer()
 
 	// Attain a lock to halt the verify & rekey cycle.
-	s.RLock()
+	s.Lock()
 
 	// Verify that fields are not nil.
 	if s.left == nil || s.right == nil {
@@ -32,11 +32,11 @@ func TestNewCoffer(t *testing.T) {
 		t.Error("right side is zeroed")
 	}
 
-	s.RUnlock() // Release mutex to allow destruction.
+	s.Unlock() // Release mutex to allow destruction.
 	s.Destroy()
 }
 
-func TestCofferInitialise(t *testing.T) {
+func TestCofferInit(t *testing.T) {
 	s := NewCoffer()
 
 	// Get the value stored inside.
@@ -48,8 +48,8 @@ func TestCofferInitialise(t *testing.T) {
 	copy(value, view.Data())
 	view.Destroy()
 
-	// Re-initialise the buffer with a new value.
-	if err := s.Initialise(); err != nil {
+	// Re-init the buffer with a new value.
+	if err := s.Init(); err != nil {
 		t.Error("unexpected error;", err)
 	}
 
@@ -70,7 +70,7 @@ func TestCofferInitialise(t *testing.T) {
 	s.Destroy()
 
 	// Check error condition.
-	if err := s.Initialise(); err != ErrCofferExpired {
+	if err := s.Init(); err != ErrCofferExpired {
 		t.Error("expected ErrCofferExpired; got", err)
 	}
 }
@@ -113,10 +113,7 @@ func TestCofferView(t *testing.T) {
 func TestCofferRekey(t *testing.T) {
 	s := NewCoffer()
 
-	// Halt the rekey cycle.
-	s.RLock()
-
-	// Remember the value stored inside.
+	// remember the value stored inside
 	view, err := s.View()
 	if err != nil {
 		t.Error("unexpected error;", err)
@@ -125,18 +122,16 @@ func TestCofferRekey(t *testing.T) {
 	copy(orgValue, view.Data())
 	view.Destroy()
 
-	// Remember the value of the partitions.
+	// remember the value of the partitions
 	left := make([]byte, 32)
 	right := make([]byte, 32)
+	s.Lock() // halt re-key cycle
 	copy(left, s.left.Data())
 	copy(right, s.right.Data())
+	s.Unlock() // un-halt re-key cycle
 
-	// Manually re-key before we continue.
-	s.RUnlock()
-	s.Rekey()
-	s.RLock()
+	s.Rekey() // force a re-key
 
-	// Get another view of the contents.
 	view, err = s.View()
 	if err != nil {
 		t.Error("unexpected error;", err)
@@ -145,17 +140,14 @@ func TestCofferRekey(t *testing.T) {
 	copy(newValue, view.Data())
 	view.Destroy()
 
-	// Compare the values.
 	if !bytes.Equal(orgValue, newValue) {
 		t.Error("value inside coffer changed!!")
 	}
 
-	// Compare the partition values.
 	if bytes.Equal(left, s.left.Data()) || bytes.Equal(right, s.right.Data()) {
 		t.Error("partition values did not change")
 	}
 
-	s.RUnlock() // Release lock to allow destruction.
 	s.Destroy()
 
 	if err := s.Rekey(); err != ErrCofferExpired {