Skip to content

Commit

Permalink
Fix two nasty use-after-free-bugs
Browse files Browse the repository at this point in the history
Summary:
These bugs were caught by ASAN crash test.
1. The first one, in table/filter_block.cc is very nasty. We first reference entries_ and store the reference to Slice prev. Then, we call entries_.append(), which can change the reference. The Slice prev now points to junk.
2. The second one is a bug in a test, so it's not very serious. Once we set read_opts.prefix, we never clear it, so some other function might still reference it.

Test Plan: asan crash test now runs more than 5 mins. Before, it failed immediately. I will run the full one, but the full one takes quite some time (5 hours)

Reviewers: dhruba, haobo, kailiu

Reviewed By: dhruba

CC: leveldb

Differential Revision: https://reviews.facebook.net/D14223
  • Loading branch information
igorcanadi committed Nov 20, 2013
1 parent 8906ab5 commit 469a9f3
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 8 deletions.
15 changes: 9 additions & 6 deletions table/filter_block.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,19 +52,22 @@ bool FilterBlockBuilder::SamePrefix(const Slice &key1,
void FilterBlockBuilder::AddKey(const Slice& key) {
// get slice for most recently added entry
Slice prev;
if (start_.size() > 0) {
size_t prev_start = start_[start_.size() - 1];
const char* base = entries_.data() + prev_start;
size_t length = entries_.size() - prev_start;
prev = Slice(base, length);
}
size_t added_to_start = 0;

// add key to filter if needed
if (whole_key_filtering_) {
start_.push_back(entries_.size());
++added_to_start;
entries_.append(key.data(), key.size());
}

if (start_.size() > added_to_start) {
size_t prev_start = start_[start_.size() - 1 - added_to_start];
const char* base = entries_.data() + prev_start;
size_t length = entries_.size() - prev_start;
prev = Slice(base, length);
}

// add prefix to filter if needed
if (prefix_extractor_ && prefix_extractor_->InDomain(ExtractUserKey(key))) {
// If prefix_extractor_, this filter_block layer assumes we only
Expand Down
5 changes: 3 additions & 2 deletions tools/db_stress.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1118,6 +1118,7 @@ class StressTest {
} else {
MultiPrefixScan(thread, read_opts, prefix);
}
read_opts.prefix = nullptr;
} else if (prefixBound <= prob_op && prob_op < writeBound) {
// OPERATION write
uint32_t value_base = thread->rand.Next();
Expand All @@ -1126,8 +1127,8 @@ class StressTest {
if (!FLAGS_test_batches_snapshots) {
MutexLock l(thread->shared->GetMutexForKey(rand_key));
if (FLAGS_verify_before_write) {
std::string keystr = Key(rand_key);
Slice k = keystr;
std::string keystr2 = Key(rand_key);
Slice k = keystr2;
Status s = db_->Get(read_opts, k, &from_db);
VerifyValue(rand_key,
read_opts,
Expand Down

0 comments on commit 469a9f3

Please sign in to comment.