Add memory corruption checking to base::File().

base/files/file_posix.cc

Index: base/files/file_posix.cc
diff --git a/base/files/file_posix.cc b/base/files/file_posix.cc
index 43684b5dabfe3df9b2a6907cd5dfbecb8e3582c3..a7e5be60a7da7c7d7fab4bfad5708375e5b9cd40 100644
--- a/base/files/file_posix.cc
+++ b/base/files/file_posix.cc
@@ -483,6 +483,48 @@ 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 {

[Nico, 2014/11/06 22:53:43]

instead of

  void foo(int* out)

why not

  int foo()

?

[gavinp, 2014/11/06 23:52:54]

I put a significant comment in the .h explaining the reasoning behind this.
There's a lot of discussion in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types
about the dangers of implicit unsigned conversions. In particular, because this
unsigned int can be larger than numeric_limits<int>::max(), it's possible to
have an undefined conversion to int; and yet that conversion can be generated
implicitly. That scares me, and so I coded as carefully as possible to
demonstrate a lack of implicit conversions. That's why the out pointer
interface; we avoid dangerous integral conversions.

It's very, very cautious. This code isn't a big public interface, and we do know
it's being used safely. We also have the compiler warning about implicit
conversions to unsigned elevated to an error as well, making this unneeded on at
least the platforms we do that (all of them?). So I'm open to being told I'm
being paranoid and obfuscatory and this function should just return an unsigned.

+  // Use a single iteration of a linear congruentional generator to provide a
+  // cheap checksum unlikely to be accidentally matched by a random memory
+  // corruption.
+
+  // These constants were chosen to ensure that the function is invertible; see

[Nico, 2014/11/06 22:53:43]

Why is it useful that it's invertible?

[gavinp, 2014/11/06 23:52:54]

I've changed the comment to say more directly what I meant here: no two fds map
to the same checksum, maximising its utility.

+  // The Hull-Dubell Theorem for a proof.
+
+  // This code uses "unsigned int" throughout for its defined modular semantics,
+  // which implicitly gives us a divisor of 2**32 or 2**64, depending.
+
+  const unsigned int kMultiplier = 13035 * 4 + 1;
+  COMPILE_ASSERT((kMultiplier - 1 & 3) == 0, pred_must_be_multiple_of_four);

[Nico, 2014/11/06 22:53:43]

move ) a bit to the left, to the right of 1

[gavinp, 2014/11/06 23:52:54]

That ) is needed, since & is lower priority than == in normal C++ precedence.
I've added the parenthesis you are asking for as well.

+  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);