Skip to content

Commit

Permalink
Remove memory corruption testing from base::File().
Browse files Browse the repository at this point in the history 
This was intended to help debug issue 424562 (see Issue 702473009 for
more details), and it isn't needed any more.

R=thakis@chromium.org,pasko@chromium.org,thestig@chromium.org
BUG=424562

Review URL: https://codereview.chromium.org/1372113002

Cr-Commit-Position: refs/heads/master@{#351194}
  • Loading branch information
gavinp authored and Commit bot committed Sep 28, 2015
1 parent 32f7bac commit ac9c196
Show file tree
Hide file tree
Showing 3 changed files with 2 additions and 163 deletions.
53 changes: 2 additions & 51 deletions base/files/file.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,13 @@
#include "base/files/file_path.h"
#include "base/files/file_tracing.h"
#include "base/files/scoped_file.h"
#include "base/gtest_prod_util.h"
#include "base/move.h"
#include "base/time/time.h"

#if defined(OS_WIN)
#include "base/win/scoped_handle.h"
#endif

FORWARD_DECLARE_TEST(FileTest, MemoryCorruption);

namespace base {

#if defined(OS_WIN)
Expand Down Expand Up @@ -306,55 +303,8 @@ class BASE_EXPORT File {
static std::string ErrorToString(Error error);

private:
FRIEND_TEST_ALL_PREFIXES(::FileTest, MemoryCorruption);

friend class FileTracing::ScopedTrace;

#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

// Creates or opens the given file. Only called if |path| has no
// traversal ('..') components.
void DoInitialize(const FilePath& path, uint32 flags);
Expand All @@ -368,7 +318,7 @@ class BASE_EXPORT File {
#if defined(OS_WIN)
win::ScopedHandle file_;
#elif defined(OS_POSIX)
MemoryCheckingScopedFD file_;
ScopedFD file_;
#endif

// A path to use for tracing purposes. Set if file tracing is enabled during
Expand All @@ -386,3 +336,4 @@ class BASE_EXPORT File {
} // namespace base

#endif // BASE_FILES_FILE_H_

43 changes: 0 additions & 43 deletions base/files/file_posix.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -420,49 +420,6 @@ File::Error File::OSErrorToFileError(int saved_errno) {
}
}

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_);
}

// NaCl doesn't implement system calls to open files directly.
#if !defined(OS_NACL)
// TODO(erikkay): does it make sense to support FLAG_EXCLUSIVE_* here?
Expand Down
69 changes: 0 additions & 69 deletions base/files/file_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
#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"

Expand Down Expand Up @@ -510,71 +509,3 @@ TEST(FileTest, GetInfoForDirectory) {
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_,
static_cast<unsigned int>(file.GetPlatformFile()));
file.file_.file_memory_checksum_ = file.GetPlatformFile();
EXPECT_DEATH(file.IsValid(), "");

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(), "");

// 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(), "");

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(), "");

// 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()), "");

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), "");
EXPECT_DEATH(file.Read(0, NULL, 0), "");
EXPECT_DEATH(file.ReadAtCurrentPos(NULL, 0), "");
EXPECT_DEATH(file.Write(0, NULL, 0), "");

ignore_result(file.file_.file_.release());
file.file_.UpdateChecksum();
}
}
#endif // defined(OS_POSIX)

0 comments on commit ac9c196

Please sign in to comment.