For Linux, override malloc and friends so that we can detect and then stop on...

For Linux, override malloc and friends so that we can detect and then stop on out of memory.

BUG=27222
TEST=new base unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=32395

[willchan no longer on Chromium, 2009-11-13 05:08:49]

On 2009/11/13 03:16:01, vandebo wrote:
> 

I don't know that it makes sense to do this change here, as I am probably going
to turn on tcmalloc for Linux soon (the perfbot results seem compelling).  I
suspect this change is better made in third_party/tcmalloc.  You can add code
that forwards malloc/free/etc and to tc_malloc/tc_free/etc after doing your
check, or to __libc_malloc() and friends if we're not using tcmalloc (I think we
have a LINUX_USE_TCMALLOC preprocessor symbol for that, which
linux_use_tcmalloc=1 sets, but I forget).

For completeness, you should also be overriding __libc_malloc and friends since
some libraries we depend on may be trying to call directly to the libc malloc. 
You'll note that tcmalloc already does this.

[vandebo (ex-Chrome), 2009-11-13 22:50:59]

Will may be out of the office at this point, so I'd appreciate comments from
other reviewers if possible.  Response inline.

On 2009/11/13 05:08:49, willchan wrote:
> I don't know that it makes sense to do this change here, as I am probably
going
> to turn on tcmalloc for Linux soon (the perfbot results seem compelling).  I
> suspect this change is better made in third_party/tcmalloc.  You can add code

It's important not to regress this, as there may be security ramifications. 
I've left in the change for glibc's malloc but also dealt with tcmalloc so that
we're covered either way.

> that forwards malloc/free/etc and to tc_malloc/tc_free/etc after doing your
> check, or to __libc_malloc() and friends if we're not using tcmalloc (I think
we
> have a LINUX_USE_TCMALLOC preprocessor symbol for that, which
> linux_use_tcmalloc=1 sets, but I forget).

tcmalloc actually has an option to induce the intended result, which I've used.

> For completeness, you should also be overriding __libc_malloc and friends
since
> some libraries we depend on may be trying to call directly to the libc malloc.

> You'll note that tcmalloc already does this.

I've overridden those names as well, though it's not clear that anything
actually uses them.

[vandebo (ex-Chrome), 2009-11-16 21:02:05]

ping

[willchan no longer on Chromium, 2009-11-16 21:51:39]

On 2009/11/16 21:02:05, vandebo wrote:
> ping

I'm a bit concerned about the possible performance impact of this change since
we'll be adding an extra function call overhead for each malloc().  After you
land this, please watch the Linux perfbots to make sure there isn't a negative
impact.

LGTM.

[jar (doing other things), 2009-11-16 22:04:37]

Please add unittest cases, per comments below.

Thanks,

Jim

http://codereview.chromium.org/391044/diff/7002/7003
File base/process_util_linux.cc (right):

http://codereview.chromium.org/391044/diff/7002/7003#newcode504
Line 504: CHECK(false) <<
Can you craft this to be testable?  Perchance include a global that indicates
"testing" vs "running," and when you are "testing" just increment a counter, or
set a "was called" flag (that can be validated, and reset, from test code).

Also, please include unit test code for (simulated) crashes on all overridden
functions.

http://codereview.chromium.org/391044/diff/7002/7003#newcode541
Line 541: static void* (*libc_##func)(size_t) = \
It would appear this is the second time you're prepending "libc_" to the name,
and hence you are for example overriding:

libc_malloc
libc_libc_malloc

but not

malloc

http://codereview.chromium.org/391044/diff/7002/7003#newcode542
Line 542: (void* (*)(size_t)) dlsym(RTLD_NEXT, #func); \
nit: Please use a static_cast<>() in all three macros.

I believe you can use the better syntax even though we're in a section with C
style linkage.

Given how error prone it is to read these typenames, I'd also suggest you use a
typedef, both in the cast, and the declaration.

[vandebo (ex-Chrome), 2009-11-18 02:20:42]

http://codereview.chromium.org/391044/diff/7002/7003
File base/process_util_linux.cc (right):

http://codereview.chromium.org/391044/diff/7002/7003#newcode504
Line 504: CHECK(false) <<
On 2009/11/16 22:04:37, jar wrote:
> Can you craft this to be testable?  Perchance include a global that indicates
> "testing" vs "running," and when you are "testing" just increment a counter,
or
> set a "was called" flag (that can be validated, and reset, from test code).
> 
> Also, please include unit test code for (simulated) crashes on all overridden
> functions.

Done.  ASSERT_DEATH() makes this testable. I tried to incorporate testing of
Windows and Mac too, but there were too many platform idiosyncrasies to get it
done in a timely manner.  I've TODO'd myself for that.

http://codereview.chromium.org/391044/diff/7002/7003#newcode541
Line 541: static void* (*libc_##func)(size_t) = \
On 2009/11/16 22:04:37, jar wrote:
> It would appear this is the second time you're prepending "libc_" to the name,

This is the local name for the real function.

> and hence you are for example overriding:
> 
> libc_malloc
> libc_libc_malloc
> 
> but not
> 
> malloc

http://codereview.chromium.org/391044/diff/7002/7003#newcode542
Line 542: (void* (*)(size_t)) dlsym(RTLD_NEXT, #func); \
On 2009/11/16 22:04:37, jar wrote:
> nit: Please use a static_cast<>() in all three macros.
> 
> I believe you can use the better syntax even though we're in a section with C
> style linkage.
> 
> Given how error prone it is to read these typenames, I'd also suggest you use
a
> typedef, both in the cast, and the declaration.

Done.

[jar (doing other things), 2009-11-18 03:20:53]

LGTM (with one nit readability change indicated below).

Sorry about my confusion/error.

http://codereview.chromium.org/391044/diff/7002/7003
File base/process_util_linux.cc (right):

http://codereview.chromium.org/391044/diff/7002/7003#newcode541
Line 541: static void* (*libc_##func)(size_t) = \
Ah... you are correct. I misread.

Please change the local name to something more readable to lessen confusion. 
Since the local variable is not matching anything external at all, it may as
well be a simple name without any macro-concatenation operator, such as:
original_function
or
intercepted_function
or
library_function

There is seemingly no reason to have distinct local variable names in each of
the macros, and IMO it hurt (my) readability.

Thanks!

On 2009/11/18 02:20:43, vandebo wrote:
> On 2009/11/16 22:04:37, jar wrote:
> > It would appear this is the second time you're prepending "libc_" to the
name,
> 
> This is the local name for the real function.
> 
> > and hence you are for example overriding:
> > 
> > libc_malloc
> > libc_libc_malloc
> > 
> > but not
> > 
> > malloc