In CrMallocErrorBreak, do not kill the process if errno is ENOMEM.

In CrMallocErrorBreak, do not kill the process if errno is ENOMEM.

This will allow large JPEG decodes to be handled and optionally killed by the
OOM killer instead. Based on a sampling of the other malloc_error_break() bugs
("unaligned pointer", "freed was not allocated", "double free", and "incorrect
checksum"), this will only affect the "allocate region" error, as those others
happen at free(), rather than malloc().

BUG=103980
TEST=Covered by ProcessUtilTest.MacTerminateOnHeapCorruption


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

[Scott Hess - ex-Googler, 2012-03-05 20:34:20]

https://chromiumcodereview.appspot.com/9597031/diff/1/base/process_util_mac.mm
File base/process_util_mac.mm (right):

https://chromiumcodereview.appspot.com/9597031/diff/1/base/process_util_mac.m...
base/process_util_mac.mm:553: // Out of memory is certainly not heap corruption,
and not necessairly
s/necessairly/correct spelling/

Also, I think the OOM killer uniformly decides to crash in case of NULL.

https://chromiumcodereview.appspot.com/9597031/diff/1/base/process_util_unitt...
File base/process_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/9597031/diff/1/base/process_util_unitt...
base/process_util_unittest.cc:459: void* buf = malloc(pow(10, 15));
This isn't really 32-bit safe, is it?  It just happens to be 2764472320 on
32-bit, and I see failures above 2^31.  If that was intentional, I think you
should code it more intentionally.

Well, also, 1<<50 would let you get away without cmath.  Or (size_t)(-1).

https://chromiumcodereview.appspot.com/9597031/diff/1/base/process_util_unitt...
base/process_util_unittest.cc:461: EXPECT_EQ(ENOMEM, errno) << "Expected no
memory error";
Is the death-test below considered proof enough that CrMallocErrorBreak() has
been installed?

[Scott Hess - ex-Googler, 2012-03-05 20:36:53]

Hmm, and I also was wondering if other cases were setting errno in other ways? 
In practical terms, I don't think that would be a problem, if calls where
random.  But I worry about whether callers might do bad things after seeing the
ENOMEM case, which this code ignores because of ENOMEM.  I will admit that this
may not be solvable.

[Robert Sesek, 2012-03-06 01:16:27]

https://chromiumcodereview.appspot.com/9597031/diff/1/base/process_util_mac.mm
File base/process_util_mac.mm (right):

https://chromiumcodereview.appspot.com/9597031/diff/1/base/process_util_mac.m...
base/process_util_mac.mm:553: // Out of memory is certainly not heap corruption,
and not necessairly
On 2012/03/05 20:34:20, shess wrote:
> s/necessairly/correct spelling/
> 
> Also, I think the OOM killer uniformly decides to crash in case of NULL.

Spalling es ovarraited.

https://chromiumcodereview.appspot.com/9597031/diff/1/base/process_util_unitt...
File base/process_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/9597031/diff/1/base/process_util_unitt...
base/process_util_unittest.cc:459: void* buf = malloc(pow(10, 15));
On 2012/03/05 20:34:20, shess wrote:
> This isn't really 32-bit safe, is it?  It just happens to be 2764472320 on
> 32-bit, and I see failures above 2^31.  If that was intentional, I think you
> should code it more intentionally.
> 
> Well, also, 1<<50 would let you get away without cmath.  Or (size_t)(-1).

Done in a C++ way.

https://chromiumcodereview.appspot.com/9597031/diff/1/base/process_util_unitt...
base/process_util_unittest.cc:461: EXPECT_EQ(ENOMEM, errno) << "Expected no
memory error";
On 2012/03/05 20:34:20, shess wrote:
> Is the death-test below considered proof enough that CrMallocErrorBreak() has
> been installed?

Yes.

[Scott Hess - ex-Googler, 2012-03-06 01:27:51]

LGTM.

https://chromiumcodereview.appspot.com/9597031/diff/3002/base/process_util_un...
File base/process_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/9597031/diff/3002/base/process_util_un...
base/process_util_unittest.cc:458: void* buf =
malloc(std::numeric_limits<size_t>::max());
Whoa, that's super shiny!

BTW, did you see the comment in EnableTerminationOnOutOfMemory()?  It seems
relevant.

[Robert Sesek, 2012-03-06 01:55:43]

https://chromiumcodereview.appspot.com/9597031/diff/3002/base/process_util_un...
File base/process_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/9597031/diff/3002/base/process_util_un...
base/process_util_unittest.cc:458: void* buf =
malloc(std::numeric_limits<size_t>::max());
On 2012/03/06 01:27:51, shess wrote:
> Whoa, that's super shiny!
> 
> BTW, did you see the comment in EnableTerminationOnOutOfMemory()?  It seems
> relevant.

Very, very relevant. That explains the behavior I was seeing. Updated to get the
malloc_error_break() behavior.

[Scott Hess - ex-Googler, 2012-03-06 01:56:55]

STILL LGTM

[Mark Mentovai, 2012-03-06 22:08:52]

LGTM

[commit-bot: I haz the power, 2012-03-06 23:40:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/9597031/10001

[commit-bot: I haz the power, 2012-03-07 01:20:14]

Try job failure for 9597031-10001 (retry) on mac_rel for step "base_unittests".
It's a second try, previously, step "base_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Robert Sesek, 2012-03-07 17:51:21]

Mark: PTAL

[Mark Mentovai, 2012-03-07 17:54:12]

LGTM otherwise

http://codereview.chromium.org/9597031/diff/17001/base/process_util_unittest.cc
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/9597031/diff/17001/base/process_util_unittest....
base/process_util_unittest.cc:464: void* buf =
malloc(std::numeric_limits<size_t>::max() - (2*PAGE_SIZE) - 1);
You should have spaces around the * operator.

http://codereview.chromium.org/9597031/diff/17001/base/process_util_unittest....
base/process_util_unittest.cc:470: EXPECT_EQ(ENOMEM, errno) << "Expected no
memory error";
Now that I’m reading this again, I think the string is confusing. Is it “(no
memory) error” or “no (memory error)?” I think it’s fine without the string at
all.

[Robert Sesek, 2012-03-07 17:58:16]

http://codereview.chromium.org/9597031/diff/17001/base/process_util_unittest.cc
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/9597031/diff/17001/base/process_util_unittest....
base/process_util_unittest.cc:464: void* buf =
malloc(std::numeric_limits<size_t>::max() - (2*PAGE_SIZE) - 1);
On 2012/03/07 17:54:12, Mark Mentovai wrote:
> You should have spaces around the * operator.

Per C++ guide, "it's okay to remove spaces around factors"
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo....

But Done.

http://codereview.chromium.org/9597031/diff/17001/base/process_util_unittest....
base/process_util_unittest.cc:470: EXPECT_EQ(ENOMEM, errno) << "Expected no
memory error";
On 2012/03/07 17:54:12, Mark Mentovai wrote:
> Now that I’m reading this again, I think the string is confusing. Is it “(no
> memory) error” or “no (memory error)?” I think it’s fine without the string at
> all.

Yeah, removed string.

[commit-bot: I haz the power, 2012-03-07 17:58:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/9597031/17003

[commit-bot: I haz the power, 2012-03-07 20:33:25]

Change committed as 125441

[Scott Hess - ex-Googler, 2012-03-07 23:00:59]

On 2012/03/05 20:36:53, shess wrote:
> Hmm, and I also was wondering if other cases were setting errno in other ways?

> In practical terms, I don't think that would be a problem, if calls where
> random.  But I worry about whether callers might do bad things after seeing
the
> ENOMEM case, which this code ignores because of ENOMEM.  I will admit that
this
> may not be solvable.

PROVEN!