Skip to content

Commit 768d424

Browse files
committed
[fix] SIGSEGV when VersionEdit in MANIFEST is corrupted
Summary: This was reported by our customers in task #4295529. Cause: * MANIFEST file contains a VersionEdit, which contains file entries whose 'smallest' and 'largest' internal keys are empty. String with zero characters. Root cause of corruption was not investigated. We should report corruption when this happens. However, we currently SIGSEGV. Here's what happens: * VersionEdit encodes zero-strings happily and stores them in smallest and largest InternalKeys. InternalKey::Encode() does assert when `rep_.empty()`, but we don't assert in production environemnts. Also, we should never assert as a result of DB corruption. * As part of our ConsistencyCheck, we call GetLiveFilesMetaData() * GetLiveFilesMetadata() calls `file->largest.user_key().ToString()` * user_key() function does: 1. assert(size > 8) (ooops, no assert), 2. returns `Slice(internal_key.data(), internal_key.size() - 8)` * since `internal_key.size()` is unsigned int, this call translates to `Slice(whatever, 1298471928561892576182756)`. Bazinga. Fix: * VersionEdit checks if InternalKey is valid in `VersionEdit::GetInternalKey()`. If it's invalid, returns corruption. Lessons learned: * Always keep in mind that even if you `assert()`, production code will continue execution even if assert fails. * Never `assert` based on DB corruption. Assert only if the code should guarantee that assert can't fail. Test Plan: dumped offending manifest. Before: assert. Now: corruption Reviewers: dhruba, haobo, sdong Reviewed By: dhruba CC: leveldb Differential Revision: https://reviews.facebook.net/D18507
1 parent 313b2e5 commit 768d424

File tree

2 files changed

+6
-1
lines changed

2 files changed

+6
-1
lines changed

db/dbformat.h

+5
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,11 @@ class InternalKey {
146146
AppendInternalKey(&rep_, ParsedInternalKey(user_key, s, t));
147147
}
148148

149+
bool Valid() const {
150+
ParsedInternalKey parsed;
151+
return ParseInternalKey(Slice(rep_), &parsed);
152+
}
153+
149154
void DecodeFrom(const Slice& s) { rep_.assign(s.data(), s.size()); }
150155
Slice Encode() const {
151156
assert(!rep_.empty());

db/version_edit.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ static bool GetInternalKey(Slice* input, InternalKey* dst) {
123123
Slice str;
124124
if (GetLengthPrefixedSlice(input, &str)) {
125125
dst->DecodeFrom(str);
126-
return true;
126+
return dst->Valid();
127127
} else {
128128
return false;
129129
}

0 commit comments

Comments
 (0)