From 7c6a7e8fa8e0a77207a82c59a886230d2c086172 Mon Sep 17 00:00:00 2001 From: sdong Date: Thu, 23 Sep 2021 11:59:33 -0700 Subject: [PATCH] FaultInjectionTestFS::InjectThreadSpecificReadError() should not corrupt mmaped bytes (#8952) Summary: Right now FaultInjectionTestFS::InjectThreadSpecificReadError() might try to corrupt return bytes, but these bytes might be from mmapped files, which would cause segfault. Instead FaultInjectionTestFS::InjectThreadSpecificReadError() should never corrupt data unless it is in caller's buffer. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8952 Test Plan: See db_stress still runs and make sure in a test run failurs are still injected in non-mmap cases. Reviewed By: ajkr, ltamasi Differential Revision: D31147318 fbshipit-source-id: 9484a64ff2aaa36685557203f449286e694e65f9 --- utilities/fault_injection_fs.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/utilities/fault_injection_fs.cc b/utilities/fault_injection_fs.cc index 7aae89479..69b32a120 100644 --- a/utilities/fault_injection_fs.cc +++ b/utilities/fault_injection_fs.cc @@ -768,7 +768,7 @@ void FaultInjectionTestFS::UntrackFile(const std::string& f) { } IOStatus FaultInjectionTestFS::InjectThreadSpecificReadError( - ErrorOperation op, Slice* result, bool direct_io, char* /*scratch*/, + ErrorOperation op, Slice* result, bool direct_io, char* scratch, bool need_count_increase, bool* fault_injected) { bool dummy_bool; bool& ret_fault_injected = fault_injected ? *fault_injected : dummy_bool; @@ -803,11 +803,15 @@ IOStatus FaultInjectionTestFS::InjectThreadSpecificReadError( *result = Slice(); ctx->message += "inject empty result; "; ret_fault_injected = true; - } else if (!direct_io && Random::GetTLSInstance()->OneIn(7)) { + } else if (!direct_io && Random::GetTLSInstance()->OneIn(7) && + scratch != nullptr && result->data() == scratch) { assert(result); // With direct I/O, many extra bytes might be read so corrupting // one byte might not cause checksum mismatch. Skip checksum // corruption injection. + // We only corrupt data if the result is filled to `scratch`. For other + // cases, the data might not be able to be modified (e.g mmaped files) + // or has unintended side effects. // For a small chance, set the failure to status but corrupt the // result in a way that checksum checking is supposed to fail. // Corrupt the last byte, which is supposed to be a checksum byte -- GitLab