CORE-589: In Core Crypto, Address TSAN #3

The FileService holds an RLP Coder and a file service is now called via multiple threads.  Hence TSAN issues have arisen.

Author: Ed Gamble

ethereum/rlp/BRRlpCoder.c

@@ -16,9 +16,6 @@
 #include "ethereum/util/BRUtil.h"
 #include "BRRlpCoder.h"
 
-static void
-rlpCoderReclaimInternal (BRRlpCoder coder) ;
-
 static int
 rlpDecodeStringEmptyCheck (BRRlpCoder coder, BRRlpItem item);
 
@@ -125,21 +122,8 @@ rlpCoderCreate (void) {
     return coder;
 }
 
-extern void
-rlpCoderRelease (BRRlpCoder coder) {
-    pthread_mutex_lock(&coder->lock);
-
-    // Every single Item must be returned!
-    assert (NULL == coder->busy);
-    rlpCoderReclaimInternal (coder);
-
-    pthread_mutex_unlock(&coder->lock);
-    pthread_mutex_destroy(&coder->lock);
-    free (coder);
-}
-
 static void
-rlpCoderReclaimInternal (BRRlpCoder coder) {
+_rlpCoderReclaimInternal (BRRlpCoder coder) {
     BRRlpItem item = coder->free;
     while (item != NULL) {
         BRRlpItem next = item->next;   // save 'next' before release...
@@ -153,22 +137,26 @@ rlpCoderReclaimInternal (BRRlpCoder coder) {
 extern void
 rlpCoderReclaim (BRRlpCoder coder) {
     pthread_mutex_lock(&coder->lock);
-    rlpCoderReclaimInternal (coder);
+    _rlpCoderReclaimInternal (coder);
     pthread_mutex_unlock(&coder->lock);
 }
 
-extern int
-rlpCoderBusyCount (BRRlpCoder coder) {
-    int count = 0;
-    for (BRRlpItem found = coder->busy; NULL != found; found = found->next)
-        count++;
-    return count;
+extern void
+rlpCoderRelease (BRRlpCoder coder) {
+    pthread_mutex_lock(&coder->lock);
+
+    // Every single Item must be returned!
+    assert (NULL == coder->busy);
+    _rlpCoderReclaimInternal (coder);
+
+    pthread_mutex_unlock(&coder->lock);
+    pthread_mutex_destroy(&coder->lock);
+    free (coder);
 }
 
 static BRRlpItem
-rlpCoderAcquireItem (BRRlpCoder coder) {
+_rlpCoderAcquireItemInternal (BRRlpCoder coder) {
     BRRlpItem item = NULL;
-    pthread_mutex_lock(&coder->lock);
 
     // Get `item` from `coder->free` or `calloc`
     if (NULL != coder->free) {
@@ -188,13 +176,19 @@ rlpCoderAcquireItem (BRRlpCoder coder) {
     // Update `coder` to show `item` as busy.
     coder->busy = item;
 
+    return item;
+}
+
+static BRRlpItem
+rlpCoderAcquireItem (BRRlpCoder coder) {
+    pthread_mutex_lock(&coder->lock);
+    BRRlpItem item = _rlpCoderAcquireItemInternal (coder);
     pthread_mutex_unlock(&coder->lock);
     return item;
 }
 
 static void
-rlpCoderReturnItem (BRRlpCoder coder, BRRlpItem prev, BRRlpItem item, BRRlpItem next) {
-    pthread_mutex_lock(&coder->lock);
+_rlpCoderReturnItemInternal (BRRlpCoder coder, BRRlpItem prev, BRRlpItem item, BRRlpItem next) {
     assert (NULL == item->next       && NULL == item->prev &&
             0    == item->bytesCount && 0    == item->itemsCount);
 
@@ -215,17 +209,26 @@ rlpCoderReturnItem (BRRlpCoder coder, BRRlpItem prev, BRRlpItem item, BRRlpItem
 
     // Update `coder` to show `item` as free.
     coder->free = item;
-
-    pthread_mutex_unlock(&coder->lock);
 }
 
 static void
-itemRelease (BRRlpCoder coder, BRRlpItem item) {
+_rlpCoderReleaseItemInternal (BRRlpCoder coder, BRRlpItem item) {
+    for (size_t index = 0; index < item->itemsCount; index++)
+        _rlpCoderReleaseItemInternal (coder, item->items[index]);
+
+    // Surely get these before itemReleaseMemory() blows them away.
     BRRlpItem prev = item->prev;
     BRRlpItem next = item->next;
 
     itemReleaseMemory(item);
-    rlpCoderReturnItem(coder, prev, item, next);
+    _rlpCoderReturnItemInternal (coder, prev, item, next);
+}
+
+static void
+rlpCoderReleaseItem (BRRlpCoder coder, BRRlpItem item) {
+    pthread_mutex_lock(&coder->lock);
+    _rlpCoderReleaseItemInternal (coder, item);
+    pthread_mutex_unlock(&coder->lock);
 }
 
 static int
@@ -277,84 +280,6 @@ rlpCoderHasFailed (BRRlpCoder coder) {
     return coder->failed;
 }
 
-#if 0
-static BRRlpItem
-itemCreate (BRRlpCoder coder,
-            uint8_t *bytes, size_t bytesCount, int takeBytes) {
-    BRRlpItem item = calloc (1, sizeof (struct BRRlpItemRecord));
-
-    item->type = CODER_ITEM;
-    item->bytesCount = bytesCount;
-    if (takeBytes)
-        item->bytes = bytes;
-    else {
-        item->bytes = (item->bytesCount > ITEM_DEFAULT_BYTES_COUNT
-                       ? malloc (item->bytesCount)
-                       : item->bytesArray);
-        memcpy (item->bytes, bytes, item->bytesCount);
-    }
-    item->itemsCount = 0;
-    item->items = NULL;
-
-    return item;
-}
-
-static BRRlpItem
-itemCreateList (BRRlpCoder coder,
-                uint8_t *bytes, size_t bytesCount, int takeBytes,
-                BRRlpItem *items, size_t itemsCount) {
-    BRRlpItem item = itemCreate(coder, bytes, bytesCount, takeBytes);
-    item->type = CODER_LIST;
-    item->itemsCount = itemsCount;
-    item->items = (item->itemsCount > ITEM_DEFAULT_ITEMS_COUNT
-                   ? calloc (item->itemsCount, sizeof (BRRlpItem))
-                   : item->itemsArray);
-    for (int i = 0; i < itemsCount; i++)
-        item->items[i] = items[i];
-    
-    return item;
-}
-#endif
-
-/**
- * Return a new BRRlpContext by appending the two provided contexts.  Both provided contexts
- * must be for CODER_ITEM (othewise an 'assert' is raised); the appending is performed by simply
- * concatenating the two context's byte arrays.
- *
- * If release is TRUE, then both the provided contexts are released; thereby freeing their memory.
- *
- */
-#if 0
-static BRRlpItem
-itemCreateAppend (BRRlpCoder coder, BRRlpItem context1, BRRlpItem context2, int release) {
-    assert (CODER_ITEM == context1->type && CODER_ITEM == context2->type);
-
-    BRRlpItem item = calloc (1, sizeof (struct BRRlpItemRecord));
-
-    item->type = CODER_ITEM;
-    
-    item->bytesCount = context1->bytesCount + context2->bytesCount;
-    item->bytes = (item->bytesCount > ITEM_DEFAULT_BYTES_COUNT
-                   ? malloc (item->bytesCount)
-                   : item->bytesArray);
-
-    memcpy (&item->bytes[0], context1->bytes, context1->bytesCount);
-    memcpy (&item->bytes[context1->bytesCount], context2->bytes, context2->bytesCount);
-
-    item->itemsCount = 0;
-    item->items = NULL;
-    
-    if (release) {
-        //        assert (context2.bytes != context1.bytes || context2.bytes == NULL || context1.bytes == NULL);
-        //        assert (context2.items != context1.items || context2.items == NULL || context1.items == NULL);
-        itemRelease(coder, context1);
-        itemRelease(coder, context2);
-    }
-    
-    return item;
-}
-#endif
-
 // The largest number supported for encoding is a UInt256 - which is representable as 32 bytes.
 #define CODER_NUMBER_BYTES_LIMIT    (256/8)
 
@@ -1071,9 +996,7 @@ rlpShowItemInternal (BRRlpCoder coder, BRRlpItem context, const char *topic, int
 extern void
 rlpReleaseItem (BRRlpCoder coder, BRRlpItem item) {
     assert (itemIsValid(coder, item));
-    for (size_t index = 0; index < item->itemsCount; index++)
-        rlpReleaseItem(coder, item->items[index]);
-    itemRelease(coder, item);
+    rlpCoderReleaseItem (coder, item);
 }
 
 extern void