Intercept file Open/Close

Intercept base::File Open/Close

When a file descriptor is opened by the base::File, all calls to close(3) from the same dynamic library will hit a CHECK unless they are made from a whitelist of callsites belonging to base::File.

There is a handy protect_file_posix.gypi introduced to make it easy to enable on Chrome-for-Android.

This 'linker magic' is somewhat crazy, so:
1. it will be *removed *when crbug.com/424562 is fixed
2. it should only be used by a whitelist of binaries/libraries (in the opensource part: libchromeshell only)

BUG=424562
Committed: https://crrev.com/45a0dc0b75b52e026eb15526ef441edc5dbe9ba5
Cr-Commit-Position: refs/heads/master@{#304592}

[gavinp, 2014-10-30 18:40:02]

gavinp@chromium.org changed reviewers:
+ gavinp@chromium.org

[gavinp, 2014-10-30 18:40:03]

Alternative architecture idea: interpose on close as you do in this CL, and save
the stack each time using base::Debug::StackTrace, and provide an interface to
retrieve the prior stack trace.

Then, when we get unexpected failures to close, we can add this output to the
PCHECK output in scoped_file.cc.

The result guards every instance of our scoped files, and shows us all the
offenders.

One question on my mind: I can think of a lot of ways to create an FD. open(2),
creat(2), dup2(2), probably a few others if I spend any time thinking. Is
close(2) the only way to destroy them?

[gavinp, 2014-10-31 12:05:43]

On 2014/10/30 18:40:03, gavinp wrote:
> Alternative architecture idea: interpose on close as you do in this CL, and
save
> the stack each time using base::Debug::StackTrace, and provide an interface to
> retrieve the prior stack trace.
> 
> Then, when we get unexpected failures to close, we can add this output to the
> PCHECK output in scoped_file.cc.
> 
> The result guards every instance of our scoped files, and shows us all the
> offenders.
> 
> One question on my mind: I can think of a lot of ways to create an FD.
open(2),
> creat(2), dup2(2), probably a few others if I spend any time thinking. Is
> close(2) the only way to destroy them?

Actually, after talking this over with Egor offline, we realized this isn't
great: we don't want to lose the early crash and the stack in the crash report
is best if it's the actual stack from the bad deallocation.

So, why not instrument base::File so that it whitelists everything there; then
crash on any attempt to close except from a base::File? This is something we
could leave in longer term, it's more general than just simple cache, and it
punctures fewer abstraction layers.

[pasko, 2014-10-31 13:33:15]

On 2014/10/31 12:05:43, gavinp wrote:
> On 2014/10/30 18:40:03, gavinp wrote:
> > Alternative architecture idea: interpose on close as you do in this CL, and
> save
> > the stack each time using base::Debug::StackTrace, and provide an interface
to
> > retrieve the prior stack trace.
> > 
> > Then, when we get unexpected failures to close, we can add this output to
the
> > PCHECK output in scoped_file.cc.
> > 
> > The result guards every instance of our scoped files, and shows us all the
> > offenders.
> > 
> > One question on my mind: I can think of a lot of ways to create an FD.
> open(2),
> > creat(2), dup2(2), probably a few others if I spend any time thinking. Is
> > close(2) the only way to destroy them?
> 
> Actually, after talking this over with Egor offline, we realized this isn't
> great: we don't want to lose the early crash and the stack in the crash report
> is best if it's the actual stack from the bad deallocation.
> 
> So, why not instrument base::File so that it whitelists everything there; then
> crash on any attempt to close except from a base::File? This is something we
> could leave in longer term, it's more general than just simple cache, and it
> punctures fewer abstraction layers.

there is still a chance that we would use invalid FD from the whitelisted
base::File, in case of memory corruptions, which would be an OK signal to
continue investigating.

I moved the logic to file_posix.cc, need some magic in GYP yet. Also should I
s/Poison/Protect/ maybe?

[pasko, 2014-11-04 14:34:21]

fdegans: can you please do a first pass sanity check? Especially for GYP.

In the meantime I am looking at GN.

[pasko, 2014-11-04 14:34:46]

pasko@chromium.org changed reviewers:
+ fdegans@chromium.org

[pasko, 2014-11-04 14:34:56]

fdegans: can you please do a first pass sanity check? Especially for GYP.

In the meantime I am looking at GN.

[gavinp, 2014-11-04 15:02:02]

This is really good. Awesome!

Do you think this deserves a unit test? It will be a fun one, since it will
crash a lot.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
File base/files/protect_file_posix.cc (right):

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:32: // crazy_linker).  Instead two hooks are
added to base::File, which are
Nit: one space after .

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:33: // implemented as no-op by default. This
file should be linked into the chrome
Nit: capitalise Chrome.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:56: LAZY_INSTANCE_INITIALIZER;
Nit: can this go up on the previous line?

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:59: typedef base::hash_set<int> IntSet;
Do we need this typedef? It's only used once, so it actually makes the file
longer.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:73: base::AutoLock lock(g_lock.Get());
Sad about the lock.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:74: g_protected_fd_set.Get().insert(fd);
Is double protecting also a crash?

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:79: g_protected_fd_set.Get().erase(fd);
Is double erasing also a crash?

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:89: // It is important to crash here and
provide a minidump with necessary context
This comment could be a bit more explanatory. There's two things that I think
it's trying to get across, but I'm not quite seeing them.

1. We crash only if a protected fd is closed without first being unprotected.
This prevents a crash later when we double-close.

2. The earlier crash gives us a much more useful stack, letting us debug the
thread that frees the wrong resources; like using ASAN or TSAN.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:93: PCHECK(0 ==
IGNORE_EINTR(__real_close(fd)));
PCHECK_EQ?

I believe close can fail with EBADF (let's crash!), EINTR (yay UNIX), but what
about EIO? Is that a time to crash?

[Fabrice (no longer in Chrome), 2014-11-04 15:37:22]

If this is only for Android shared library targets, you can disregard my
comments.

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

https://codereview.chromium.org/676873004/diff/60001/base/files/file_posix.cc...
base/files/file_posix.cc:174: // With compilers other than GCC define strong
no-op symbols for simplicity.
Nit: other than GCC and Clang (so msvc, essentially)
COMPILER_GCC is defined for GCC and Clang.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
File base/files/protect_file_posix.cc (right):

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:37: // With compilers other than GCC the
wrapper is trivial. This is to avoid
Nit: GCC and Clang

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
File base/files/protect_file_posix.gypi (right):

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.gypi:24: ['component != "shared_library"', {
Maybe you need to add a test for Windows here if you're going to use that gypi
elsewhere?

[pasko, 2014-11-04 17:48:06]

gavinp, fdegans: thanks for review.

I addressed all your comments, PTAL.

A few more things to do in this CL for me:
* GN (this would be my first GN change, ugh)
* add a test (probably on linux with EXPECT_DEATH, will need more targets to pot
in into wrapping, may appear to be more fun than necessary)
* I am now seeing double-unprotection reproducibly, but the crash does not
reveal enough data for me, will be investigating.

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

https://codereview.chromium.org/676873004/diff/60001/base/files/file_posix.cc...
base/files/file_posix.cc:174: // With compilers other than GCC define strong
no-op symbols for simplicity.
On 2014/11/04 15:37:21, Fabrice de Gans wrote:
> Nit: other than GCC and Clang (so msvc, essentially)
> COMPILER_GCC is defined for GCC and Clang.

Thanks, I did not realize COMPILER_GCC is defined for Clang. That's actually
logical. OK, since !defined(__clang__) is ugly, I'll hope for the best on both
compilers. And of course clang supports this attribute, wow!

Done.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
File base/files/protect_file_posix.cc (right):

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:32: // crazy_linker).  Instead two hooks are
added to base::File, which are
On 2014/11/04 15:02:02, gavinp wrote:
> Nit: one space after .

Done.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:33: // implemented as no-op by default. This
file should be linked into the chrome
On 2014/11/04 15:02:02, gavinp wrote:
> Nit: capitalise Chrome.

Done.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:37: // With compilers other than GCC the
wrapper is trivial. This is to avoid
On 2014/11/04 15:37:21, Fabrice de Gans wrote:
> Nit: GCC and Clang

Done.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:56: LAZY_INSTANCE_INITIALIZER;
On 2014/11/04 15:02:02, gavinp wrote:
> Nit: can this go up on the previous line?

Done.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:59: typedef base::hash_set<int> IntSet;
On 2014/11/04 15:02:01, gavinp wrote:
> Do we need this typedef? It's only used once, so it actually makes the file
> longer.

Done.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:73: base::AutoLock lock(g_lock.Get());
On 2014/11/04 15:02:02, gavinp wrote:
> Sad about the lock.

Acknowledged. Agreed. Let's get drunk.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:74: g_protected_fd_set.Get().insert(fd);
On 2014/11/04 15:02:01, gavinp wrote:
> Is double protecting also a crash?

Yes. Thanks. Done.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:79: g_protected_fd_set.Get().erase(fd);
On 2014/11/04 15:02:02, gavinp wrote:
> Is double erasing also a crash?

Yes! Done.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:89: // It is important to crash here and
provide a minidump with necessary context
On 2014/11/04 15:02:01, gavinp wrote:
> This comment could be a bit more explanatory. There's two things that I think
> it's trying to get across, but I'm not quite seeing them.
> 
> 1. We crash only if a protected fd is closed without first being unprotected.
> This prevents a crash later when we double-close.
> 
> 2. The earlier crash gives us a much more useful stack, letting us debug the
> thread that frees the wrong resources; like using ASAN or TSAN.

Sorry, it was indeed a bit too short. Rephrased closely to what you suggested.

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.cc:93: PCHECK(0 ==
IGNORE_EINTR(__real_close(fd)));
On 2014/11/04 15:02:01, gavinp wrote:
> PCHECK_EQ?
> 
> I believe close can fail with EBADF (let's crash!), EINTR (yay UNIX), but what
> about EIO? Is that a time to crash?

I blindly copied what's in base/files/scoped_file.cc (assuming that if we want
to change this behavior, then it can be done in another CL). I am almost sure we
crashed with EBADF in the bug (since other error messages suggested FDs are out
of control).

It appears there is no PCHECK_EQ, and there is not really much sense in having
it, since the error message will be printed by PCHECK, which has a 1:1
correspondence with the results of close(3).

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
File base/files/protect_file_posix.gypi (right):

https://codereview.chromium.org/676873004/diff/60001/base/files/protect_file_...
base/files/protect_file_posix.gypi:24: ['component != "shared_library"', {
On 2014/11/04 15:37:21, Fabrice de Gans wrote:
> Maybe you need to add a test for Windows here if you're going to use that gypi
> elsewhere?

I am hoping that my scary comments would prevent people from using this on
Windows :)

Yeah, conditioning on Windows here might be useful for target_name=='chrome', on
the other hand we could conditionalize the include itself. Not sure which is
more great, let's just have less code here for now until we care about
target_name=='chrome'.

Currently I care only about android and libchrome DSO, of course.

[gavinp, 2014-11-04 18:34:14]

This looks pretty good to me. I do wonder if a lot of the code belongs in
scoped_file.cc instead of file_posix.cc, but that feels like a minor nit.

I would be made happy by introducing a unit test, even on Linux, just because I
want to see these both not crash and crash in the desired situations; reading
the code is not quite convincing enough.

[pasko, 2014-11-05 19:37:29]

OK, debugging revealed that I did not properly Protect in constructors, where it
is necessary. What a mess. This required me to have weak declarations in an
internal header, since I don't like repeating myself (boilerplate repeating is
not repeating).

Remaining TBD:
1. Crash tests
2. GN

On 2014/11/04 18:34:14, gavinp wrote:
> This looks pretty good to me. I do wonder if a lot of the code belongs in
> scoped_file.cc instead of file_posix.cc, but that feels like a minor nit.

I was considering intercepting at ScopedFD level to add necessary hooks, but
with this path I was concerned by having to extend ScopedGeneric, which is less
trivial to write and review, and resides entirely in a header that gets included
everywhere, not only //base/ which might have caused linking problems that I did
not want to think about.

So if you want to try that path, I will happily leave it to you, and I believe
it could look slightly cleaner.

> I would be made happy by introducing a unit test, even on Linux, just because
I
> want to see these both not crash and crash in the desired situations; reading
> the code is not quite convincing enough.

[pasko, 2014-11-06 16:55:04]

On 2014/11/05 19:37:29, pasko wrote:
> OK, debugging revealed that I did not properly Protect in constructors, where
it
> is necessary. What a mess. This required me to have weak declarations in an
> internal header, since I don't like repeating myself (boilerplate repeating is
> not repeating).
> 
> Remaining TBD:
> 1. Crash tests

Death tests are not supported on Android. On Linux the way base_unittests are
linked, does not play well with --wrap=close because it also wraps tcmalloc.

What's wrong with that?

Well, here is the scenario:
1. base_unittests allocate via tcmalloc
2. tcmalloc acquires its internal lock
3. then attempts ASLR, which requires closing /dev/urandom
4. in close() we attempt to allocate -> deadlock

ways to deal with it:
1. build libbase.so as a dynamic library with --wrap, load it in base_unittests
(crazy amount of work)
2. don't allocate in __wrap_close (i.e. don't use LazyInstance, have a
concurrent lock free map, that's a fun project)
3. use a recursive lock in tcmalloc (well .. what could possibly go wrong)

Ideas welcome. Until then there is no test, sorry.

[gavinp, 2014-11-06 17:48:30]

This lgtm, assuming you write the GN and get good try results using it.

I am sad we can't test on Linux. I'm flustered by some of the discoveries here.

[pasko, 2014-11-07 17:52:12]

pasko@chromium.org changed reviewers:
+ brettw@chromium.org

[pasko, 2014-11-07 17:55:26]

pasko@chromium.org changed reviewers:
+ thakis@chromium.org

[pasko, 2014-11-07 18:00:42]

+owners
thakis: base
brettw: GN

This is a really crazy attempt to catch abuses of base::File in our codebase
(see the bug for details). I would rather consider it temporary due to the
--wrap hack not playing well with too many things. But on the other hand, if it
helps catching bugs, why not have it for longer?

https://codereview.chromium.org/676873004/diff/140001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/676873004/diff/140001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:202: # TODO(pasko): make these 3 lines of code more
reusable.
brettw: I tried to make it a config(), but it did not allow me to add deps. What
is the proper way to do it? Template?

[gavinp, 2014-11-10 19:24:13]

It seems that death tests do run on Android. See
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
. 

FileTest.MemoryCorruption is only built if death tests are supported, and this
log shows the test running and the death test warning output about fork
repeatedly.

[pasko, 2014-11-12 12:30:12]

On 2014/11/10 19:24:13, gavinp wrote:
> It seems that death tests do run on Android. See
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> . 
> 
> FileTest.MemoryCorruption is only built if death tests are supported, and this
> log shows the test running and the death test warning output about fork
> repeatedly.

oh, ah, ugh, my test refused to run because there is this line:
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...

Lovely.

It's there because:
https://codereview.chromium.org/157593007

Apparently the Blink Android Tests (dbg) bot disagreed with trybots back then.
So I don't want to try it now, since not being able to repro failures locally
scares me. If you want to push for deathtests on Android (that would be good,
I'd be happy to l/g/t/m certainly), here is the death test uploaded separately
(without GN, sorry):
https://codereview.chromium.org/720753002/
(it passes locally)

Now given the hacky nature of the --wrap hack, death tests not working etc., I'd
prefer to revert the whole thing after it helps us (or it doesn't help) find the
problem.

[Fabrice (no longer in Chrome), 2014-11-17 16:10:54]

Just a few nits on the gyp, other than that gyp lgtm if the bots are happy.

https://codereview.chromium.org/676873004/diff/140001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/676873004/diff/140001/base/base.gyp#newcode380
base/base.gyp:380: 'target_name': 'protect_file_posix',
Nit: Add the gn file location

https://codereview.chromium.org/676873004/diff/140001/base/files/protect_file...
File base/files/protect_file_posix.gypi (right):

https://codereview.chromium.org/676873004/diff/140001/base/files/protect_file...
base/files/protect_file_posix.gypi:13: #      
'base/files/protect_file_posix.gypi'
Nit: add a comma at the end of the line

[pasko, 2014-11-17 16:25:57]

https://codereview.chromium.org/676873004/diff/140001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/676873004/diff/140001/base/base.gyp#newcode380
base/base.gyp:380: 'target_name': 'protect_file_posix',
On 2014/11/17 16:10:54, Fabrice wrote:
> Nit: Add the gn file location

Done.

https://codereview.chromium.org/676873004/diff/140001/base/files/protect_file...
File base/files/protect_file_posix.gypi (right):

https://codereview.chromium.org/676873004/diff/140001/base/files/protect_file...
base/files/protect_file_posix.gypi:13: #      
'base/files/protect_file_posix.gypi'
On 2014/11/17 16:10:54, Fabrice wrote:
> Nit: add a comma at the end of the line

Done.

[Nico, 2014-11-17 23:55:02]

Please make it clear in both commit message and patch that this is temporary
code to track down a bug (comments in gn, gyp file and at least at the
declaration of ProtectFileDescriptor, with a link to crbug.com/424562)

With that, lgtm

[pasko, 2014-11-18 11:04:30]

On 2014/11/17 23:55:02, Nico wrote:
> Please make it clear in both commit message and patch that this is temporary
> code to track down a bug (comments in gn, gyp file and at least at the
> declaration of ProtectFileDescriptor, with a link to crbug.com/424562)

Done.

[pasko, 2014-11-18 11:04:35]

The CQ bit was checked by pasko@chromium.org

[commit-bot: I haz the power, 2014-11-18 11:05:24]

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

[commit-bot: I haz the power, 2014-11-18 11:36:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-18 11:36:54]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/87710)

[pasko, 2014-11-18 12:51:48]

The CQ bit was checked by pasko@chromium.org

[commit-bot: I haz the power, 2014-11-18 12:52:03]

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

[commit-bot: I haz the power, 2014-11-18 13:31:44]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2014-11-18 13:32:33]

Patchset 12 (id:??) landed as
https://crrev.com/45a0dc0b75b52e026eb15526ef441edc5dbe9ba5
Cr-Commit-Position: refs/heads/master@{#304592}