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

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
Committed: https://crrev.com/32ea7f9a9a2385bd8911bf5bc37748a560a387d5
Cr-Commit-Position: refs/heads/master@{#303251}

[gavinp, 2014-11-06 22:34:57]

thakis: PTAL. As we discussed yesterday.

pasko: FYI.

[Nico, 2014-11-06 22:49:35]

"In file included from external/chromium_org/base/basictypes.h:18:0,
                 from external/chromium_org/base/files/file.h:20,
                 from external/chromium_org/base/files/file_posix.cc:5:
external/chromium_org/base/files/file_posix.cc: In member function 'void
base::File::MemoryCheckingScopedFD::ComputeMemoryChecksum(unsigned int*) const':
external/chromium_org/base/files/file_posix.cc:510:35: error: suggest
parentheses around '-' in operand of '&' [-Werror=parentheses]
   COMPILE_ASSERT((kMultiplier - 1 & 3) == 0, pred_must_be_multiple_of_four);"

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

To be clear, the thinking is that this is a temporary debugging thing to help
track down a crash that you're seeing in the wild but not locally, which is why
you can't use valgrind, right? This will be removed again after you have a few
stacks? If so, say that in the CL description and in a comment in the .h (with a
link to your bug)

https://codereview.chromium.org/702473009/diff/40001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/702473009/diff/40001/base/files/file_posix.cc...
base/files/file_posix.cc:498: unsigned int* out_checksum) const {
instead of

  void foo(int* out)

why not

  int foo()

?

https://codereview.chromium.org/702473009/diff/40001/base/files/file_posix.cc...
base/files/file_posix.cc:503: // These constants were chosen to ensure that the
function is invertible; see
Why is it useful that it's invertible?

https://codereview.chromium.org/702473009/diff/40001/base/files/file_posix.cc...
base/files/file_posix.cc:510: COMPILE_ASSERT((kMultiplier - 1 & 3) == 0,
pred_must_be_multiple_of_four);
move ) a bit to the left, to the right of 1

[gavinp, 2014-11-06 23:35:31]

Patchset #4 (id:60001) has been deleted

[gavinp, 2014-11-06 23:52:55]

Nico,

Thank you so much for such a fast and thorough review. I've made a first pass at
remediating and responding below. What do you think?

On 2014/11/06 22:53:43, Nico wrote:
> To be clear, the thinking is that this is a temporary debugging thing to help
> track down a crash that you're seeing in the wild but not locally, which is
why
> you can't use valgrind, right? This will be removed again after you have a few
> stacks? If so, say that in the CL description and in a comment in the .h (with
a
> link to your bug)

That's right. I have updated the CL description and added a comment in file.h as
you asked.

https://codereview.chromium.org/702473009/diff/40001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/702473009/diff/40001/base/files/file_posix.cc...
base/files/file_posix.cc:498: unsigned int* out_checksum) const {
On 2014/11/06 22:53:43, Nico wrote:
> instead of
> 
>   void foo(int* out)
> 
> why not
> 
>   int foo()
> 
> ?

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.

https://codereview.chromium.org/702473009/diff/40001/base/files/file_posix.cc...
base/files/file_posix.cc:503: // These constants were chosen to ensure that the
function is invertible; see
On 2014/11/06 22:53:43, Nico wrote:
> Why is it useful that it's invertible?

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

https://codereview.chromium.org/702473009/diff/40001/base/files/file_posix.cc...
base/files/file_posix.cc:510: COMPILE_ASSERT((kMultiplier - 1 & 3) == 0,
pred_must_be_multiple_of_four);
On 2014/11/06 22:53:43, Nico wrote:
> move ) a bit to the left, to the right of 1

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

[Nico, 2014-11-07 00:12:09]

lgtm

https://codereview.chromium.org/702473009/diff/100001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/702473009/diff/100001/base/files/file_posix.c...
base/files/file_posix.cc:508: // which implicitly gives us a divisor of 2**32 or
2**64, depending.
(I think unsigneds are 32bit on all platforms we support, even in 64bit builds:
http://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models)

[gavinp, 2014-11-07 00:37:36]

Thank you for your reviews, Nico.

I'm going to let the try jobs bake a bit, then I should CQ this later tonight.

https://codereview.chromium.org/702473009/diff/100001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/702473009/diff/100001/base/files/file_posix.c...
base/files/file_posix.cc:508: // which implicitly gives us a divisor of 2**32 or
2**64, depending.
On 2014/11/07 00:12:08, Nico wrote:
> (I think unsigneds are 32bit on all platforms we support, even in 64bit
builds:
> http://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models)

Good point. I've updated the comment to just say "power of two" since that's
actually what's important here.

[pasko, 2014-11-07 12:35:03]

lgtm thank you

https://codereview.chromium.org/702473009/diff/120001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/702473009/diff/120001/base/files/file_posix.c...
base/files/file_posix.cc:503: // By choosing constants that satisfy the
Hull-Duebell Theorem on lcg cycle
neat choice! are the constants chsen by a number of fair dice rolls?

[gavinp, 2014-11-07 16:42:14]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-11-07 16:42:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/702473009/120001

[gavinp, 2014-11-07 16:47:21]

Thanks for your review Egor. I'm CQing this! Hopefully we'll get this to the
vendor and see what happens.

https://codereview.chromium.org/702473009/diff/120001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/702473009/diff/120001/base/files/file_posix.c...
base/files/file_posix.cc:503: // By choosing constants that satisfy the
Hull-Duebell Theorem on lcg cycle
On 2014/11/07 12:35:03, pasko wrote:
> neat choice! are the constants chsen by a number of fair dice rolls?

I chose a random 14 bit number for my kMultiplier, then * 4 + 1 to insure it
meets the theorem's test. The kIncrement was a random 32 bit number; it came out
odd on my first roll so I took that; I would have advanced any other number to
the next odd one I guess.

I just used the random number generator in emacs-calc. I don't think it's super
vital to choose these constants well, almost any selection will make memory
sprayers obvious. 

My earlier comment that Nico pointed out was pretty bad though; I showed my
mathematics education by calling the function invertible to point out its cycle
length. The worst part is that it's computationally fairly rough to actually
invert it, so it was kind of a dumb way to describe it. Whoops!

[commit-bot: I haz the power, 2014-11-07 17:46:20]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2014-11-07 17:47:10]

Patchset 6 (id:??) landed as
https://crrev.com/32ea7f9a9a2385bd8911bf5bc37748a560a387d5
Cr-Commit-Position: refs/heads/master@{#303251}