From bc526f18a4b053aa111dc170a08f157b50526d3e Mon Sep 17 00:00:00 2001
From: Dev Ojha <ValarDragon@users.noreply.github.com>
Date: Thu, 26 Jul 2018 19:21:08 -0700
Subject: [PATCH] libs: Make bitarray functions lock parameters that aren't the
 caller (#2081)

This now makes bit array functions which take in a second bit array, thread
safe. Previously there was a warning on bitarray.Update to be lock the
second parameter externally if thread safety wasrequired.
This was not done within the codebase, so it was fine to change here.

Closes #2080
---
 CHANGELOG_PENDING.md     |  1 +
 libs/common/bit_array.go | 36 ++++++++++++++++++++++++++----------
 2 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md
index 3bb4940ce..627632945 100644
--- a/CHANGELOG_PENDING.md
+++ b/CHANGELOG_PENDING.md
@@ -17,6 +17,7 @@ IMPROVEMENTS:
     - tweak params
     - only process one block at a time to avoid starving
 - [crypto] Switch hkdfchachapoly1305 to xchachapoly1305
+- [common] bit array functions which take in another parameter are now thread safe
 
 BUG FIXES:
 - [privval] fix a deadline for accepting new connections in socket private
diff --git a/libs/common/bit_array.go b/libs/common/bit_array.go
index 50c6f12e3..abf6110d8 100644
--- a/libs/common/bit_array.go
+++ b/libs/common/bit_array.go
@@ -118,7 +118,11 @@ func (bA *BitArray) Or(o *BitArray) *BitArray {
 		return bA.Copy()
 	}
 	bA.mtx.Lock()
-	defer bA.mtx.Unlock()
+	o.mtx.Lock()
+	defer func() {
+		bA.mtx.Unlock()
+		o.mtx.Unlock()
+	}()
 	c := bA.copyBits(MaxInt(bA.Bits, o.Bits))
 	for i := 0; i < len(c.Elems); i++ {
 		c.Elems[i] |= o.Elems[i]
@@ -134,7 +138,11 @@ func (bA *BitArray) And(o *BitArray) *BitArray {
 		return nil
 	}
 	bA.mtx.Lock()
-	defer bA.mtx.Unlock()
+	o.mtx.Lock()
+	defer func() {
+		bA.mtx.Unlock()
+		o.mtx.Unlock()
+	}()
 	return bA.and(o)
 }
 
@@ -153,6 +161,10 @@ func (bA *BitArray) Not() *BitArray {
 	}
 	bA.mtx.Lock()
 	defer bA.mtx.Unlock()
+	return bA.not()
+}
+
+func (bA *BitArray) not() *BitArray {
 	c := bA.copy()
 	for i := 0; i < len(c.Elems); i++ {
 		c.Elems[i] = ^c.Elems[i]
@@ -169,7 +181,11 @@ func (bA *BitArray) Sub(o *BitArray) *BitArray {
 		return nil
 	}
 	bA.mtx.Lock()
-	defer bA.mtx.Unlock()
+	o.mtx.Lock()
+	defer func() {
+		bA.mtx.Unlock()
+		o.mtx.Unlock()
+	}()
 	if bA.Bits > o.Bits {
 		c := bA.copy()
 		for i := 0; i < len(o.Elems)-1; i++ {
@@ -178,13 +194,12 @@ func (bA *BitArray) Sub(o *BitArray) *BitArray {
 		i := len(o.Elems) - 1
 		if i >= 0 {
 			for idx := i * 64; idx < o.Bits; idx++ {
-				// NOTE: each individual GetIndex() call to o is safe.
-				c.setIndex(idx, c.getIndex(idx) && !o.GetIndex(idx))
+				c.setIndex(idx, c.getIndex(idx) && !o.getIndex(idx))
 			}
 		}
 		return c
 	}
-	return bA.and(o.Not()) // Note degenerate case where o == nil
+	return bA.and(o.not()) // Note degenerate case where o == nil
 }
 
 // IsEmpty returns true iff all bits in the bit array are 0
@@ -332,15 +347,16 @@ func (bA *BitArray) Bytes() []byte {
 
 // Update sets the bA's bits to be that of the other bit array.
 // The copying begins from the begin of both bit arrays.
-// Note, the other bitarray o is not locked when reading,
-// so if necessary, caller must copy or lock o prior to calling Update.
-// If bA is nil, does nothing.
 func (bA *BitArray) Update(o *BitArray) {
 	if bA == nil || o == nil {
 		return
 	}
 	bA.mtx.Lock()
-	defer bA.mtx.Unlock()
+	o.mtx.Lock()
+	defer func() {
+		bA.mtx.Unlock()
+		o.mtx.Unlock()
+	}()
 
 	copy(bA.Elems, o.Elems)
 }