Add memory corruption checking to base::File().
Since we crash on invalid fd in ScopedFD::Close(), memory corruption
in a base::File() object can cause crashes; perhaps long after the memory
is actually corrupt.
This CL puts a checksum in memory near the fd on POSIX systems. The
checksum is different enough to be very unlikely to be correct after a
random memory write, but also very cheap to compute, so we can have
most operations on a file double quickly double check its integrity,
for earlier (and more useful for debugging) crashes.
This is intended to help us debug issue 424562. Once we have found out
if and where memory is being corrupted, this instrumentation code should
be removed.
R=thakis@chromium.org,pasko@chromium.org
BUG=424562
Review URL: https://codereview.chromium.org/702473009
Cr-Commit-Position: refs/heads/master@{#303251}
CrOS-Libchrome-Original-Commit: 32ea7f9a9a2385bd8911bf5bc37748a560a387d5
diff --git a/base/files/file.h b/base/files/file.h
index 4110d51..7b6366c 100644
--- a/base/files/file.h
+++ b/base/files/file.h
@@ -19,6 +19,7 @@
#include "base/base_export.h"
#include "base/basictypes.h"
#include "base/files/scoped_file.h"
+#include "base/gtest_prod_util.h"
#include "base/move.h"
#include "base/time/time.h"
@@ -26,6 +27,8 @@
#include "base/win/scoped_handle.h"
#endif
+FORWARD_DECLARE_TEST(FileTest, MemoryCorruption);
+
namespace base {
class FilePath;
@@ -296,12 +299,59 @@
static std::string ErrorToString(Error error);
private:
+ FRIEND_TEST_ALL_PREFIXES(::FileTest, MemoryCorruption);
+
+#if defined(OS_POSIX)
+ // Encloses a single ScopedFD, saving a cheap tamper resistent memory checksum
+ // alongside it. This checksum is validated at every access, allowing early
+ // detection of memory corruption.
+
+ // TODO(gavinp): This is in place temporarily to help us debug
+ // https://crbug.com/424562 , which can't be reproduced in valgrind. Remove
+ // this code after we have fixed this issue.
+ class MemoryCheckingScopedFD {
+ public:
+ MemoryCheckingScopedFD();
+ MemoryCheckingScopedFD(int fd);
+ ~MemoryCheckingScopedFD();
+
+ bool is_valid() const { Check(); return file_.is_valid(); }
+ int get() const { Check(); return file_.get(); }
+
+ void reset() { Check(); file_.reset(); UpdateChecksum(); }
+ void reset(int fd) { Check(); file_.reset(fd); UpdateChecksum(); }
+ int release() {
+ Check();
+ int fd = file_.release();
+ UpdateChecksum();
+ return fd;
+ }
+
+ private:
+ FRIEND_TEST_ALL_PREFIXES(::FileTest, MemoryCorruption);
+
+ // Computes the checksum for the current value of |file_|. Returns via an
+ // out parameter to guard against implicit conversions of unsigned integral
+ // types.
+ void ComputeMemoryChecksum(unsigned int* out_checksum) const;
+
+ // Confirms that the current |file_| and |file_memory_checksum_| agree,
+ // failing a CHECK if they do not.
+ void Check() const;
+
+ void UpdateChecksum();
+
+ ScopedFD file_;
+ unsigned int file_memory_checksum_;
+ };
+#endif
+
void SetPlatformFile(PlatformFile file);
#if defined(OS_WIN)
win::ScopedHandle file_;
#elif defined(OS_POSIX)
- ScopedFD file_;
+ MemoryCheckingScopedFD file_;
#endif
Error error_details_;
diff --git a/base/files/file_posix.cc b/base/files/file_posix.cc
index 43684b5..3d229e4 100644
--- a/base/files/file_posix.cc
+++ b/base/files/file_posix.cc
@@ -483,6 +483,49 @@
}
}
+File::MemoryCheckingScopedFD::MemoryCheckingScopedFD() {
+ UpdateChecksum();
+}
+
+File::MemoryCheckingScopedFD::MemoryCheckingScopedFD(int fd) : file_(fd) {
+ UpdateChecksum();
+}
+
+File::MemoryCheckingScopedFD::~MemoryCheckingScopedFD() {}
+
+// static
+void File::MemoryCheckingScopedFD::ComputeMemoryChecksum(
+ unsigned int* out_checksum) const {
+ // Use a single iteration of a linear congruentional generator (lcg) to
+ // provide a cheap checksum unlikely to be accidentally matched by a random
+ // memory corruption.
+
+ // By choosing constants that satisfy the Hull-Duebell Theorem on lcg cycle
+ // length, we insure that each distinct fd value maps to a distinct checksum,
+ // which maximises the utility of our checksum.
+
+ // This code uses "unsigned int" throughout for its defined modular semantics,
+ // which implicitly gives us a divisor that is a power of two.
+
+ const unsigned int kMultiplier = 13035 * 4 + 1;
+ COMPILE_ASSERT(((kMultiplier - 1) & 3) == 0, pred_must_be_multiple_of_four);
+ const unsigned int kIncrement = 1595649551;
+ COMPILE_ASSERT(kIncrement & 1, must_be_coprime_to_powers_of_two);
+
+ *out_checksum =
+ static_cast<unsigned int>(file_.get()) * kMultiplier + kIncrement;
+}
+
+void File::MemoryCheckingScopedFD::Check() const {
+ unsigned int computed_checksum;
+ ComputeMemoryChecksum(&computed_checksum);
+ CHECK_EQ(file_memory_checksum_, computed_checksum) << "corrupted fd memory";
+}
+
+void File::MemoryCheckingScopedFD::UpdateChecksum() {
+ ComputeMemoryChecksum(&file_memory_checksum_);
+}
+
void File::SetPlatformFile(PlatformFile file) {
DCHECK(!file_.is_valid());
file_.reset(file);
diff --git a/base/files/file_unittest.cc b/base/files/file_unittest.cc
index 6616f6a..9f57974 100644
--- a/base/files/file_unittest.cc
+++ b/base/files/file_unittest.cc
@@ -5,6 +5,7 @@
#include "base/files/file.h"
#include "base/files/file_util.h"
#include "base/files/scoped_temp_dir.h"
+#include "base/memory/scoped_ptr.h"
#include "base/time/time.h"
#include "testing/gtest/include/gtest/gtest.h"
@@ -466,3 +467,71 @@
EXPECT_EQ(0, info.size);
}
#endif // defined(OS_WIN)
+
+#if defined(OS_POSIX) && defined(GTEST_HAS_DEATH_TEST)
+TEST(FileTest, MemoryCorruption) {
+ {
+ // Test that changing the checksum value is detected.
+ base::File file;
+ EXPECT_NE(file.file_.file_memory_checksum_,
+ implicit_cast<unsigned int>(file.GetPlatformFile()));
+ file.file_.file_memory_checksum_ = file.GetPlatformFile();
+ EXPECT_DEATH(file.IsValid(), "corrupted fd memory");
+
+ file.file_.UpdateChecksum(); // Do not crash on File::~File().
+ }
+
+ {
+ // Test that changing the file descriptor value is detected.
+ base::File file;
+ file.file_.file_.reset(17);
+ EXPECT_DEATH(file.IsValid(), "corrupted fd memory");
+
+ // Do not crash on File::~File().
+ ignore_result(file.file_.file_.release());
+ file.file_.UpdateChecksum();
+ }
+
+ {
+ // Test that GetPlatformFile() checks for corruption.
+ base::File file;
+ file.file_.file_memory_checksum_ = file.GetPlatformFile();
+ EXPECT_DEATH(file.GetPlatformFile(), "corrupted fd memory");
+
+ file.file_.UpdateChecksum(); // Do not crash on File::~File().
+ }
+
+ {
+ // Test that the base::File destructor checks for corruption.
+ scoped_ptr<base::File> file(new File());
+ file->file_.file_memory_checksum_ = file->GetPlatformFile();
+ EXPECT_DEATH(file.reset(), "corrupted fd memory");
+
+ // Do not crash on this thread's destructor call.
+ file->file_.UpdateChecksum();
+ }
+
+ {
+ // Test that the base::File constructor checks for corruption.
+ base::File file;
+ file.file_.file_memory_checksum_ = file.GetPlatformFile();
+ EXPECT_DEATH(File f(file.Pass()), "corrupted fd memory");
+
+ file.file_.UpdateChecksum(); // Do not crash on File::~File().
+ }
+
+ {
+ // Test that doing IO checks for corruption.
+ base::File file;
+ file.file_.file_.reset(17); // A fake open FD value.
+
+ EXPECT_DEATH(file.Seek(File::FROM_BEGIN, 0), "corrupted fd memory");
+ EXPECT_DEATH(file.Read(0, NULL, 0), "corrupted fd memory");
+ EXPECT_DEATH(file.ReadAtCurrentPos(NULL, 0), "corrupted fd memory");
+ EXPECT_DEATH(file.Write(0, NULL, 0), "corrupted fd memory");
+
+ ignore_result(file.file_.file_.release());
+ file.file_.UpdateChecksum();
+ }
+}
+#endif // defined(OS_POSIX)