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

base/files/file_posix.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/files/file.h"
 
 #include <errno.h>
 #include <fcntl.h>
 #include <sys/stat.h>
 #include <unistd.h>
@@ skipping 465 matching lines @@
       return FILE_ERROR_NOT_A_DIRECTORY;
     default:
 #if !defined(OS_NACL)  // NaCl build has no metrics code.
       UMA_HISTOGRAM_SPARSE_SLOWLY("PlatformFile.UnknownErrors.Posix",
                                   saved_errno);
 #endif
       return FILE_ERROR_FAILED;
   }
 }
 
+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);
 }
 
 }  // namespace base