Skip to content

Commit

Permalink
MB-30148: ep_testsuite: ensure Item is reserved before reading
Browse files Browse the repository at this point in the history
TSan reports the follow use-after-free error:

    WARNING: ThreadSanitizer: heap-use-after-free (pid=5347)
      Read of size 1 at 0x7b7400040609 by main thread:
        #0 memcmp <null> (libtsan.so.0+0x000000043643)
        #1 check_key_value() kv_engine/engines/ep/tests/ep_testsuite_common.cc:468 (ep_testsuite.so+0x0000000975ad)
        #2 test_mem_stats kv_engine/engines/ep/tests/ep_testsuite.cc:2036 (ep_testsuite.so+0x00000002c5c6)
        #3 execute_test kv_engine/programs/engine_testapp/engine_testapp.cc:1102 (engine_testapp+0x00000041b0ba)
        #4 main kv_engine/programs/engine_testapp/engine_testapp.cc:1499 (engine_testapp+0x00000041c5b2)

      Previous write of size 8 at 0x7b7400040608 by thread T11 (mutexes: write M3231945710371016):
        #0 operator delete() <null> (libtsan.so.0+0x00000006a7b4)
        #1 Blob::operator delete() kv_engine/engines/ep/src/blob.h:124 (ep.so+0x0000001959c2)
        #2 Blob::Deleter::operator()(TaggedPtr<Blob>) kv_engine/engines/ep/src/blob.h:137 (ep.so+0x0000001959c2)
        #3 SingleThreadedRCPtr<Blob, TaggedPtr<Blob>, Blob::Deleter>::swap(TaggedPtr<Blob>) kv_engine/engines/ep/src/atomic.h:362 (ep.so+0x0000001959c2)
        #4 SingleThreadedRCPtr<Blob, TaggedPtr<Blob>, Blob::Deleter>::reset(TaggedPtr<Blob>) kv_engine/engines/ep/src/atomic.h:298 (ep.so+0x0000001959c2)
        #5 StoredValue::replaceValue(TaggedPtr<Blob>) kv_engine/engines/ep/src/stored-value.h:540 (ep.so+0x0000001959c2)
        #6 StoredValue::storeCompressedBuffer(cb::const_char_buffer) kv_engine/engines/ep/src/stored-value.cc:362 (ep.so+0x0000001959c2)
        #7 HashTable::storeCompressedBuffer(cb::const_char_buffer, StoredValue&) kv_engine/engines/ep/src/hash_table.cc:620 (ep.so+0x00000012be5c)
        #8 ItemCompressorVisitor::visit(HashTable::HashBucketLock const&, StoredValue&) kv_engine/engines/ep/src/item_compressor_visitor.cc:52 (ep.so+0x000000139780)
        #9 HashTable::pauseResumeVisit(HashTableVisitor&, HashTable::Position&) kv_engine/engines/ep/src/hash_table.cc:753 (ep.so+0x000000127b6c)
        #10 PauseResumeVBAdapter::visit(VBucket&) kv_engine/engines/ep/src/vb_visitors.cc:36 (ep.so+0x0000001a81b1)
        #11 KVBucket::pauseResumeVisit(PauseResumeVBVisitor&, KVBucketIface::Position&) kv_engine/engines/ep/src/kv_bucket.cc:2177 (ep.so+0x000000158ec9)
        #12 ItemCompressorTask::run() kv_engine/engines/ep/src/item_compressor.cc:70 (ep.so+0x000000138204)
        #13 ExecutorThread::run() kv_engine/engines/ep/src/executorthread.cc:146 (ep.so+0x00000011da74)
        #14 launch_executor_thread kv_engine/engines/ep/src/executorthread.cc:34 (ep.so+0x00000011e0ae)
        #15 CouchbaseThread::run() platform/src/cb_pthreads.cc:59 (libplatform_so.so.0.1.0+0x000000009cc9)
        #16 platform_thread_wrap platform/src/cb_pthreads.cc:72 (libplatform_so.so.0.1.0+0x000000009cc9)
        #17 <null> <null> (libtsan.so.0+0x000000024feb)

      Mutex M3231945710371016 is already destroyed.

The issue is in In check_key_value(); which uses get_item_info() to
retrieve the address and size of the value (Blob) of the given key
before checking it. get_item_info() doesn't retain a reference on the
underlying engine's Item and consequently Blob. As such it's not safe
to access the underlying Blob.

Fix by explicitly fetching the Item via get(), and retaining the
return value for the duration of accessing the Blob. This ensures the
ref-count is kept and so the Blob cannot be deleted while
check_key_value() is accessing it.

Change-Id: If6cefef4d988ca26c33e798ecc383e94478c474d
Reviewed-on: http://review.couchbase.org/95743
Well-Formed: Build Bot <[email protected]>
Reviewed-by: Tim Bradgate <[email protected]>
Reviewed-by: Sriram Ganesan <[email protected]>
Tested-by: Build Bot <[email protected]>
  • Loading branch information
daverigby committed Jun 20, 2018
1 parent 80650a7 commit e427cb7
Showing 1 changed file with 8 additions and 1 deletion.
9 changes: 8 additions & 1 deletion engines/ep/tests/ep_testsuite_common.cc
Original file line number Diff line number Diff line change
Expand Up @@ -447,8 +447,15 @@ void destroy_buckets(std::vector<BucketHolder> &buckets) {
void check_key_value(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1,
const char* key, const char* val, size_t vlen,
uint16_t vbucket) {
// Fetch item itself, to ensure we maintain a ref-count on the underlying
// Blob while comparing the key.
auto getResult = get(h, h1, NULL, key, vbucket);
checkeq(cb::engine_errc::success,
getResult.first,
"Failed to fetch document");
item_info info;
check(get_item_info(h, h1, &info, key, vbucket), "checking key and value");
check(h1->get_item_info(h, getResult.second.get(), &info),
"Failed to get_item_info");

cb::const_char_buffer payload;
cb::compression::Buffer inflated;
Expand Down

0 comments on commit e427cb7

Please sign in to comment.