New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 633966 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Malloc/new failure is non fatal on Android and it should be.

Project Member Reported by primiano@chromium.org, Aug 3 2016

Issue description

Forking OOM crash issues from Issue 633313

Historically the OnNoMemory suicide on malloc/new failure was not
enabled on Android. This seems to be due to the fact that
set_new_handler was not avilable on Android back in the days of
pre-libcxx. See  crbug.com/317791  .

After the libcxx switch, however, the combination of operator new
throwing bad_alloc and chrome building with -fno-exception made
operator new (but not malloc) inadvertently suicidal, by virtue of
ending up calling the default exception handler.
See crbug.com/633313#c28 .

crrev.com/1883093005 (first seen in M52) introduced a shim layer
wrapping malloc and operator new, which was intending, among the
various things, to make malloc / new finally secure on Android.
This good intend, however, failed to materialize because the
set_new_handler call in memory_linux.cc was still #ifdef-ed out
on Android. Similarly the memory_unittests.cc were excluded on
Android for the same reason (Android was deemed to not possibly
be secure since 1.).

In summary here's what went wrong:
- When we switched to libcxx, nobody realized that we could have
  finally taken advantage of set_new_handler.
- When I enabled the android shim I didn't realize about the
  missing set_new_handler call. I was assuming that the memory
  tests would have screamed red if I did something wrong, but I
  didn't realize that they were disabled on Android.

This is being fixed in https://codereview.chromium.org/2201363002/
 
Labels: -ReleaseBlock-Beta ReleaseBlock-Stable
Spoke with primiano@; since this regressed between 51 and 52, holding 53 beta for this won't help much; that said we should get fixed before 53 moves to stable.  Updating labels to match.
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/227dbd3dc564004471f146ef655fad35c52704c3

commit 227dbd3dc564004471f146ef655fad35c52704c3
Author: primiano <primiano@chromium.org>
Date: Wed Aug 03 16:31:03 2016

android: Enable death on malloc/operator new failure.

1. Historically the OnNoMemory suicide on malloc/new failure was not
enabled on Android. This seems to be due to the fact that
set_new_handler was not avilable on Android back in the days of
pre-libcxx. See  crbug.com/317791  .

2. After the libcxx switch, however, the combination of operator new
throwing bad_alloc and chrome building with -fno-exception made
operator new (but not malloc) inadvertently suicidal, by virtue of
ending up calling the default exception handler.
See crbug.com/633313#c28 .

3. crrev.com/1883093005 (first seen in M52) introduced a shim layer
wrapping malloc and operator new, which was intending, among the
various things, to make malloc / new finally secure on Android.
This good intend, however, failed to materialize because the
set_new_handler call in memory_linux.cc was still #ifdef-ed out
on Android. Similarly the memory_unittests.cc were excluded on
Android for the same reason (Android was deemed to not possibly
be secure since 1.).

In summary here's what went wrong:
- When we switched to libcxx, nobody realized that we could have
  finally taken advantage of set_new_handler.
- When I enabled the android shim I didn't realize about the
  missing set_new_handler call. I was assuming that the memory
  tests would have screamed red if I did something wrong, but I
  didn't realize that they were disabled on Android.

This CL fixes all this, enabling set_new_handler on Android and
enabling the tests.

Note also that this CL is just about inducing a hard crash on malloc failure.
This does not change the situation about disallowing large allocations
(>2GB) that might cause int signed/unsigned bugs
(see  crbug.com/169327 ). As things stand today, Android never had that
check and still doesn't yet after this CL.

BUG= 633966 , 317791 
TEST=base_unittests --gtest_filter=OutOfMemory*

Review-Url: https://codereview.chromium.org/2201363002
Cr-Commit-Position: refs/heads/master@{#409531}

[modify] https://crrev.com/227dbd3dc564004471f146ef655fad35c52704c3/base/process/memory_linux.cc
[modify] https://crrev.com/227dbd3dc564004471f146ef655fad35c52704c3/base/process/memory_unittest.cc
[modify] https://crrev.com/227dbd3dc564004471f146ef655fad35c52704c3/build/android/pylib/gtest/filter/base_unittests_disabled

Labels: Merge-Request-53

Comment 4 by dimu@chromium.org, Aug 3 2016

Labels: -Merge-Request-53 Merge-Approved-53 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M53 (branch: 2785)
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 4 2016

Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f27b21f990ba0636f3589e7fe118e0d3cc9e3bb9

commit f27b21f990ba0636f3589e7fe118e0d3cc9e3bb9
Author: primiano <primiano@chromium.org>
Date: Thu Aug 04 09:19:37 2016

android: Enable death on malloc/operator new failure (m53 cherry-pick)

Original CL: https://codereview.chromium.org/2201363002/
Original CL Description:
1. Historically the OnNoMemory suicide on malloc/new failure was not
enabled on Android. This seems to be due to the fact that
set_new_handler was not avilable on Android back in the days of
pre-libcxx. See  crbug.com/317791  .

2. After the libcxx switch, however, the combination of operator new
throwing bad_alloc and chrome building with -fno-exception made
operator new (but not malloc) inadvertently suicidal, by virtue of
ending up calling the default exception handler.
See crbug.com/633313#c28 .

3. crrev.com/1883093005 (first seen in M52) introduced a shim layer
wrapping malloc and operator new, which was intending, among the
various things, to make malloc / new finally secure on Android.
This good intend, however, failed to materialize because the
set_new_handler call in memory_linux.cc was still #ifdef-ed out
on Android. Similarly the memory_unittests.cc were excluded on
Android for the same reason (Android was deemed to not possibly
be secure since 1.).

In summary here's what went wrong:
- When we switched to libcxx, nobody realized that we could have
  finally taken advantage of set_new_handler.
- When I enabled the android shim I didn't realize about the
  missing set_new_handler call. I was assuming that the memory
  tests would have screamed red if I did something wrong, but I
  didn't realize that they were disabled on Android.

This CL fixes all this, enabling set_new_handler on Android and
enabling the tests.

Note also that this CL is just about inducing a hard crash on malloc failure.
This does not change the situation about disallowing large allocations
(>2GB) that might cause int signed/unsigned bugs
(see  crbug.com/169327 ). As things stand today, Android never had that
check and still doesn't yet after this CL.

BUG= 633966 , 317791 
TEST=base_unittests --gtest_filter=OutOfMemory*

Review-Url: https://codereview.chromium.org/2201363002
Cr-Commit-Position: refs/heads/master@{#409531}

TBR=thakis@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2210233002
Cr-Commit-Position: refs/branch-heads/2785@{#499}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/f27b21f990ba0636f3589e7fe118e0d3cc9e3bb9/base/process/memory_linux.cc

Status: Fixed (was: Started)

Sign in to add a comment