Remove sync point from Block destructor

[sagar0]

A sync point was introduced in Block destructor in f3d91a0, to test that iterators release all data blocks at the end of their lifetime.

However, a user who initializes a static ColumnFamilyOptions saw use-after-free issue with SyncPoint Data.

1. The order of static initializations/destructions across compilation units is undefined in C++ runtime. (and hence the suggestion of moving to folly::singleton, as far as I understand).
2. Moving rocksdb::SyncPoint static initialization to some other place (like ColumnFamilyOptions or BlockBasedTableFactory or LRUCache) still would not future proof us, as RocksDB cannot control the order in which a user initializes some RocksDB objects. (E.g. Users can create LRUCache, BlockBasedTableFactory and ColumnFamilyOptions in any order, and moreover as static too).

Since we don't have a good enough solution to solve all cases, I am inclining towards just removing TEST_SYNC_POINT("Block::~Block"); from ~Block() destructor, and disable IteratorPinnedMemory test. This will temporarily move RocksDB to the previous state of not having a unit test for checking that all data blocks are released by iterators. We should spend more time thinking of the right way to fix the static order initialization, but that will take much longer warranting more discussion.

Call Stack:

=================================================================
==1798517==ERROR: AddressSanitizer: heap-use-after-free on address 0x614000015df0 at pc 0x7f4d9d0269f9 bp 0x7ffe5a8d8450 sp 0x7ffe5a8d8448
=== How to use this, how to get the raw stack trace, and more: ASAN ===
READ of size 1 at 0x614000015df0 thread T0
SCARINESS: 40 (1-byte-read-heap-use-after-free)
     #2 rocksdb/src/util/sync_point_impl.cc:92   rocksdb::SyncPoint::Data::Process(std::__cxx11::basic_string<...> const&, void*)
     #3 rocksdb/src/util/sync_point.cc:64        rocksdb::SyncPoint::Process(std::__cxx11::basic_string<...> const&, void*)
     #4 rocksdb/src/table/block.cc:765           rocksdb::Block::~Block()
     #5 rocksdb/src/table/block_based_table_reader.cc:107 void rocksdb::(anonymous namespace)::DeleteCachedEntry<...>(rocksdb::Slice const&, void*)
     #6 rocksdb/src/cache/lru_cache.h:111        rocksdb::LRUHandle::Free()
     #7 rocksdb/src/cache/lru_cache.cc:32        rocksdb::LRUHandleTable::~LRUHandleTable()::$_0::operator()(rocksdb::LRUHandle*) const
     #8 rocksdb/src/cache/lru_cache.h:138        void rocksdb::LRUHandleTable::ApplyToAllCacheEntries<...>(rocksdb::LRUHandleTable::~LRUHandleTable()::$_0)
     #9 rocksdb/src/cache/lru_cache.cc:30        rocksdb::LRUHandleTable::~LRUHandleTable()
    #10 rocksdb/src/cache/lru_cache.cc:118       rocksdb::LRUCacheShard::~LRUCacheShard()
    #11 rocksdb/src/cache/lru_cache.cc:480       rocksdb::LRUCache::~LRUCache()
    #19 rocksdb/src/include/rocksdb/table.h:52   rocksdb::BlockBasedTableOptions::~BlockBasedTableOptions()
    #20 rocksdb/src/table/block_based_table_factory.h:50 rocksdb::BlockBasedTableFactory::~BlockBasedTableFactory()
    #21 rocksdb/src/table/block_based_table_factory.h:50 rocksdb::BlockBasedTableFactory::~BlockBasedTableFactory()
    #27 rocksdb/options.h:82                     rocksdb::ColumnFamilyOptions::~ColumnFamilyOptions()

0x614000015df0 is located 432 bytes inside of 440-byte region [0x614000015c40,0x614000015df8)
freed by thread T0 here:
     #0 shard_based_mapper_test+0x694790         operator delete(void*)
     #1 rocksdb/src/util/sync_point_impl.h:28    rocksdb::SyncPoint::Data::~Data()
     #2 rocksdb/src/util/sync_point.cc:25        rocksdb::SyncPoint::~SyncPoint()

previously allocated by thread T0 here:
     #0 shard_based_mapper_test+0x693a18         operator new(unsigned long)
     #1 rocksdb/src/util/sync_point.cc:21        rocksdb::SyncPoint::SyncPoint()
     #2 rocksdb/src/util/sync_point.cc:16        rocksdb::SyncPoint::GetInstance()
     #3 rocksdb/src/env/env_posix.cc:1074        rocksdb::Env::Default()
     #4 rocksdb/src/include/rocksdb/options.h:379 rocksdb::DBOptions::DBOptions()
     #5  .... RocksWrapper::defaultDBOptions()
     #6  ... Test SetUp()
    #17 common/gtest/LightMain.cpp:19            main

SUMMARY: AddressSanitizer: heap-use-after-free in std::__atomic_base<bool>::load(std::memory_order) const
==1798517==ABORTING

Test Plan:

• make check
• verified that it fixes the issue

[facebook-github-bot]

sagar0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[ajkr]

lgtm