SERVER-28977 Coverity analysis defect 101508: Data race condition

(cherry picked from commit 676c69403a0b40686a3c9552c407fdea19bc92e9)

2 files changed, 34 insertions, 31 deletions

diff --git a/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.cpp b/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.cpp
index cea71436ecd..301de9705e0 100644
--- a/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.cpp
+++ b/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.cpp
@@ -297,11 +297,11 @@ const char* EphemeralForTestRecordStore::name() const {
 RecordData EphemeralForTestRecordStore::dataFor(OperationContext* opCtx,
                                                 const RecordId& loc) const {
     stdx::lock_guard<stdx::recursive_mutex> lock(_data->recordsMutex);
-    return recordFor(loc)->toRecordData();
+    return recordFor(lock, loc)->toRecordData();
 }
 
 const EphemeralForTestRecordStore::EphemeralForTestRecord* EphemeralForTestRecordStore::recordFor(
-    const RecordId& loc) const {
+    WithLock, const RecordId& loc) const {
     Records::const_iterator it = _data->records.find(loc);
     if (it == _data->records.end()) {
         error() << "EphemeralForTestRecordStore::recordFor cannot find record for " << ns() << ":"
@@ -312,7 +312,7 @@ const EphemeralForTestRecordStore::EphemeralForTestRecord* EphemeralForTestRecor
 }
 
 EphemeralForTestRecordStore::EphemeralForTestRecord* EphemeralForTestRecordStore::recordFor(
-    const RecordId& loc) {
+    WithLock, const RecordId& loc) {
     Records::iterator it = _data->records.find(loc);
     if (it == _data->records.end()) {
         error() << "EphemeralForTestRecordStore::recordFor cannot find record for " << ns() << ":"
@@ -326,7 +326,6 @@ bool EphemeralForTestRecordStore::findRecord(OperationContext* opCtx,
                                              const RecordId& loc,
                                              RecordData* rd) const {
     stdx::lock_guard<stdx::recursive_mutex> lock(_data->recordsMutex);
-
     Records::const_iterator it = _data->records.find(loc);
     if (it == _data->records.end()) {
         return false;
@@ -338,18 +337,19 @@ bool EphemeralForTestRecordStore::findRecord(OperationContext* opCtx,
 void EphemeralForTestRecordStore::deleteRecord(OperationContext* opCtx, const RecordId& loc) {
     stdx::lock_guard<stdx::recursive_mutex> lock(_data->recordsMutex);
 
-    deleteRecord_inlock(opCtx, loc);
+    deleteRecord(lock, opCtx, loc);
 }
 
-void EphemeralForTestRecordStore::deleteRecord_inlock(OperationContext* opCtx,
-                                                      const RecordId& loc) {
-    EphemeralForTestRecord* rec = recordFor(loc);
+void EphemeralForTestRecordStore::deleteRecord(WithLock lk,
+                                               OperationContext* opCtx,
+                                               const RecordId& loc) {
+    EphemeralForTestRecord* rec = recordFor(lk, loc);
     opCtx->recoveryUnit()->registerChange(new RemoveChange(opCtx, _data, loc, *rec));
     _data->dataSize -= rec->size;
     invariant(_data->records.erase(loc) == 1);
 }
 
-bool EphemeralForTestRecordStore::cappedAndNeedDelete_inlock(OperationContext* opCtx) const {
+bool EphemeralForTestRecordStore::cappedAndNeedDelete(WithLock, OperationContext* opCtx) const {
     if (!_isCapped)
         return false;
 
@@ -362,8 +362,8 @@ bool EphemeralForTestRecordStore::cappedAndNeedDelete_inlock(OperationContext* o
     return false;
 }
 
-void EphemeralForTestRecordStore::cappedDeleteAsNeeded_inlock(OperationContext* opCtx) {
-    while (cappedAndNeedDelete_inlock(opCtx)) {
+void EphemeralForTestRecordStore::cappedDeleteAsNeeded(WithLock lk, OperationContext* opCtx) {
+    while (cappedAndNeedDelete(lk, opCtx)) {
         invariant(!_data->records.empty());
 
         Records::iterator oldest = _data->records.begin();
@@ -373,11 +373,12 @@ void EphemeralForTestRecordStore::cappedDeleteAsNeeded_inlock(OperationContext*
         if (_cappedCallback)
             uassertStatusOK(_cappedCallback->aboutToDeleteCapped(opCtx, id, data));
 
-        deleteRecord_inlock(opCtx, id);
+        deleteRecord(lk, opCtx, id);
     }
 }
 
-StatusWith<RecordId> EphemeralForTestRecordStore::extractAndCheckLocForOplog(const char* data,
+StatusWith<RecordId> EphemeralForTestRecordStore::extractAndCheckLocForOplog(WithLock,
+                                                                             const char* data,
                                                                              int len) const {
     StatusWith<RecordId> status = oploghack::extractKey(data, len);
     if (!status.isOK())
@@ -412,12 +413,12 @@ Status EphemeralForTestRecordStore::insertRecords(OperationContext* opCtx,
         RecordId loc;
         if (_data->isOplog) {
             StatusWith<RecordId> status =
-                extractAndCheckLocForOplog(record->data.data(), record->data.size());
+                extractAndCheckLocForOplog(lock, record->data.data(), record->data.size());
             if (!status.isOK())
                 return status.getStatus();
             loc = status.getValue();
         } else {
-            loc = allocateLoc();
+            loc = allocateLoc(lock);
         }
 
         _data->dataSize += record->data.size();
@@ -425,7 +426,7 @@ Status EphemeralForTestRecordStore::insertRecords(OperationContext* opCtx,
         record->id = loc;
 
         opCtx->recoveryUnit()->registerChange(new InsertChange(opCtx, _data, loc));
-        cappedDeleteAsNeeded_inlock(opCtx);
+        cappedDeleteAsNeeded(lock, opCtx);
 
         return Status::OK();
     };
@@ -459,19 +460,19 @@ Status EphemeralForTestRecordStore::insertRecordsWithDocWriter(OperationContext*
 
         RecordId loc;
         if (_data->isOplog) {
-            StatusWith<RecordId> status = extractAndCheckLocForOplog(rec.data.get(), len);
+            StatusWith<RecordId> status = extractAndCheckLocForOplog(lock, rec.data.get(), len);
             if (!status.isOK())
                 return status.getStatus();
             loc = status.getValue();
         } else {
-            loc = allocateLoc();
+            loc = allocateLoc(lock);
         }
 
         opCtx->recoveryUnit()->registerChange(new InsertChange(opCtx, _data, loc));
         _data->dataSize += len;
         _data->records[loc] = rec;
 
-        cappedDeleteAsNeeded_inlock(opCtx);
+        cappedDeleteAsNeeded(lock, opCtx);
 
         if (idsOut)
             idsOut[i] = loc;
@@ -485,7 +486,7 @@ Status EphemeralForTestRecordStore::updateRecord(OperationContext* opCtx,
                                                  const char* data,
                                                  int len) {
     stdx::lock_guard<stdx::recursive_mutex> lock(_data->recordsMutex);
-    EphemeralForTestRecord* oldRecord = recordFor(loc);
+    EphemeralForTestRecord* oldRecord = recordFor(lock, loc);
     int oldLen = oldRecord->size;
 
     // Documents in capped collections cannot change size. We check that above the storage layer.
@@ -498,7 +499,7 @@ Status EphemeralForTestRecordStore::updateRecord(OperationContext* opCtx,
     _data->dataSize += len - oldLen;
     *oldRecord = newRecord;
 
-    cappedDeleteAsNeeded_inlock(opCtx);
+    cappedDeleteAsNeeded(lock, opCtx);
     return Status::OK();
 }
 
@@ -515,7 +516,7 @@ StatusWith<RecordData> EphemeralForTestRecordStore::updateWithDamages(
 
     stdx::lock_guard<stdx::recursive_mutex> lock(_data->recordsMutex);
 
-    EphemeralForTestRecord* oldRecord = recordFor(loc);
+    EphemeralForTestRecord* oldRecord = recordFor(lock, loc);
     const int len = oldRecord->size;
 
     EphemeralForTestRecord newRecord(len);
@@ -524,7 +525,7 @@ StatusWith<RecordData> EphemeralForTestRecordStore::updateWithDamages(
     opCtx->recoveryUnit()->registerChange(new RemoveChange(opCtx, _data, loc, *oldRecord));
     *oldRecord = newRecord;
 
-    cappedDeleteAsNeeded_inlock(opCtx);
+    cappedDeleteAsNeeded(lock, opCtx);
 
     char* root = newRecord.data.get();
     mutablebson::DamageVector::const_iterator where = damages.begin();
@@ -600,7 +601,7 @@ int64_t EphemeralForTestRecordStore::storageSize(OperationContext* opCtx,
     return _data->dataSize + recordOverhead;
 }
 
-RecordId EphemeralForTestRecordStore::allocateLoc() {
+RecordId EphemeralForTestRecordStore::allocateLoc(WithLock) {
     RecordId out = RecordId(_data->nextId++);
     invariant(out.isNormal());
     return out;
diff --git a/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.h b/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.h
index f1947d5ab70..3998d4904b7 100644
--- a/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.h
+++ b/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.h
@@ -36,6 +36,7 @@
 #include "mongo/db/storage/capped_callback.h"
 #include "mongo/db/storage/record_store.h"
 #include "mongo/platform/mutex.h"
+#include "mongo/util/concurrency/with_lock.h"
 
 namespace mongo {
 
@@ -121,6 +122,7 @@ public:
     virtual void updateStatsAfterRepair(OperationContext* opCtx,
                                         long long numRecords,
                                         long long dataSize) {
+        stdx::lock_guard<stdx::recursive_mutex> lock(_data->recordsMutex);
         invariant(_data->records.size() == size_t(numRecords));
         _data->dataSize = dataSize;
     }
@@ -138,8 +140,8 @@ protected:
         boost::shared_array<char> data;
     };
 
-    virtual const EphemeralForTestRecord* recordFor(const RecordId& loc) const;
-    virtual EphemeralForTestRecord* recordFor(const RecordId& loc);
+    virtual const EphemeralForTestRecord* recordFor(WithLock, const RecordId& loc) const;
+    virtual EphemeralForTestRecord* recordFor(WithLock, const RecordId& loc);
 
 public:
     //
@@ -163,12 +165,12 @@ private:
     class Cursor;
     class ReverseCursor;
 
-    StatusWith<RecordId> extractAndCheckLocForOplog(const char* data, int len) const;
+    StatusWith<RecordId> extractAndCheckLocForOplog(WithLock, const char* data, int len) const;
 
-    RecordId allocateLoc();
-    bool cappedAndNeedDelete_inlock(OperationContext* opCtx) const;
-    void cappedDeleteAsNeeded_inlock(OperationContext* opCtx);
-    void deleteRecord_inlock(OperationContext* opCtx, const RecordId& dl);
+    RecordId allocateLoc(WithLock);
+    bool cappedAndNeedDelete(WithLock, OperationContext* opCtx) const;
+    void cappedDeleteAsNeeded(WithLock lk, OperationContext* opCtx);
+    void deleteRecord(WithLock lk, OperationContext* opCtx, const RecordId& dl);
 
     // TODO figure out a proper solution to metadata
     const bool _isCapped;