Fix CRC seqNo race across concurrent Notecard requests

n_request.c

@@ -491,6 +491,12 @@ J *_noteTransactionShouldLock(J *req, bool lockNotecard)
     // Calculate the transaction timeout based on the parameters in the request.
     const uint32_t transactionTimeoutMs = _noteTransaction_calculateTimeoutMs(req, reqFound);
 
+    // Take the lock on the Notecard.  This is required to ensure that we don't
+    // have multiple threads trying to access the Notecard at the same time.
+    if (lockNotecard) {
+        _LockNote();
+    }
+
 #ifndef NOTE_C_LOW_MEM
     /*
     * Add a CRC value, so the request may be retried if it is received
@@ -510,9 +516,10 @@ J *_noteTransactionShouldLock(J *req, bool lockNotecard)
     *    1     0      1
     *    1     1    1 (UB)
     */
+    const uint16_t transactionSeqNo = seqNo;
     bool crcAddedToRequest = false;
     if (reqFound) {
-        char *newJson = _crcAdd(json, seqNo);
+        char *newJson = _crcAdd(json, transactionSeqNo);
         if (newJson != NULL) {
             _Free(json);
             json = newJson;
@@ -521,12 +528,6 @@ J *_noteTransactionShouldLock(J *req, bool lockNotecard)
     }
 #endif // !NOTE_C_LOW_MEM
 
-    // Take the lock on the Notecard.  This is required to ensure that we don't
-    // have multiple threads trying to access the Notecard at the same time.
-    if (lockNotecard) {
-        _LockNote();
-    }
-
     // If a reset of the I/O interface is required for any reason, do it now.
     if (resetRequired) {
         NOTE_C_LOG_DEBUG("Resetting Notecard I/O Interface...");
@@ -627,7 +628,7 @@ J *_noteTransactionShouldLock(J *req, bool lockNotecard)
         // If we sent a CRC in the request, examine the response JSON to see if
         // it has a CRC error.  Note that the CRC is stripped from the
         // rspJsonStr as a side-effect of this method.
-        if (crcAddedToRequest && _crcError(rspJsonStr, seqNo)) {
+        if (crcAddedToRequest && _crcError(rspJsonStr, transactionSeqNo)) {
             _Free(rspJsonStr);
             errStr = ERRSTR("CRC error {io}", c_iobad);
             NOTE_C_LOG_WARN(ERRSTR("retrying... CRC error", c_iobad));

test/src/NoteTransaction_test.cpp

@@ -166,16 +166,39 @@ class MultipleHeartbeatsThenValid
 };
 int MultipleHeartbeatsThenValid::call_count = 0;
 
+bool noteTransactionTestNotecardLocked = false;
+bool crcAddCalledWhileNotecardLocked = false;
+
+void noteTransactionTestLockNote(void)
+{
+    noteTransactionTestNotecardLocked = true;
+}
+
+void noteTransactionTestUnlockNote(void)
+{
+    noteTransactionTestNotecardLocked = false;
+}
+
 char * _crcAdd_customFake(char *json, uint16_t seqno)
 {
     // Custom fake implementation for _crcAdd
+    (void)seqno;
     return strdup(json);
 }
 
+char * _crcAdd_lockCheckCustomFake(char *json, uint16_t seqno)
+{
+    crcAddCalledWhileNotecardLocked = noteTransactionTestNotecardLocked;
+    return _crcAdd_customFake(json, seqno);
+}
+
 SCENARIO("NoteTransaction")
 {
     NoteSetFnDefault(malloc, free, NULL, NULL);
+    NoteSetFnNoteMutex(NULL, NULL);
     _crcAdd_fake.custom_fake = _crcAdd_customFake;
+    noteTransactionTestNotecardLocked = false;
+    crcAddCalledWhileNotecardLocked = false;
 
     // NoteReset's mock should succeed unless the test explicitly instructs
     // it to fail.
@@ -449,6 +472,26 @@ SCENARIO("NoteTransaction")
     }
 
 #ifndef NOTE_C_LOW_MEM
+    SECTION("CRC is added while the Notecard lock is held") {
+        J *req = NoteNewRequest("note.add");
+        REQUIRE(req != NULL);
+        _noteJSONTransaction_fake.custom_fake = _noteJSONTransactionValid;
+        _crcAdd_fake.custom_fake = _crcAdd_lockCheckCustomFake;
+        NoteSetFnNoteMutex(noteTransactionTestLockNote, noteTransactionTestUnlockNote);
+
+        J *resp = NoteTransaction(req);
+
+        CHECK(_crcAdd_fake.call_count == 1);
+        CHECK(crcAddCalledWhileNotecardLocked);
+        CHECK(resp != NULL);
+        CHECK(!NoteResponseError(resp));
+        CHECK(!noteTransactionTestNotecardLocked);
+
+        NoteSetFnNoteMutex(NULL, NULL);
+        JDelete(req);
+        JDelete(resp);
+    }
+
     SECTION("Bad CRC") {
         J *req = NoteNewRequest("note.add");
         REQUIRE(req != NULL);