Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(115)

Issue 1675143004: Allocator shim skeleton + Linux impl behind a build flag (Closed)

Created:
4 years, 10 months ago by Primiano Tucci (use gerrit)
Modified:
4 years, 9 months ago
Reviewers:
Nico, Will Harris
CC:
chromium-reviews, wfh+watch_chromium.org, Dai Mikurube (NOT FULLTIME), vmpstr+watch_chromium.org, chrisha
Base URL:
https://chromium.googlesource.com/chromium/src.git@shim_exp_flag
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Allocator shim skeleton + Linux impl behind a build flag TL;DR ----- This CL introduces the skeleton for the unified allocator shim and an actual working implementation for Linux. The Linux implementation is really just taking the headers that today define the malloc symbols in tcmalloc and copying them to base. All the changes introduced are conditioned behind the build-time flag use_experimental_allocator_shim, which is disabled by default. Background Context ------------------ There are two reasons why we want to intercept allocations in Chrome: 1) To enforce some security properties (suicide on malloc failure via std::new_handler, preventing allocations > 2GB which can trigger signed vs. unsigned bugs in third_party libraries). 2) For diagnostic purposes (heap profiling). Today (before this CL) allocation calls are already intercepted in most Chrome platforms, but that happens in a disorganized and inconsistent fashion. More in details: On Linux: TCMalloc redefines the malloc()/new() symbols and we added code to the internal fork of tcmalloc to achieve 1). On Mac: we inject our hooks in the default malloc zone in EnableTerminationOnOutOfMemory() (memory_mac.mm) On Windows: we strip the malloc symbols out of libcmt and re-define them in allocator_shim_win.cc On Android: we don't have 1) The purpose of this work is to refactor and uniform this. The goal is to have all the OS call into a single module (herein allocator_shim.h) which performs 1) in base (as opposite to forking allocator-specific code) and which offers a uniform interface for 2). Why is this good? ----------------- - Makes the allocator code more readable. - Allows to unfork code from tcmalloc. - Allows to design a more maintainable heap profiler, which shares the same code paths of the security enforcement layer. Notes about execution --------------------- Essentially on each platform we need to do three things: - Dismantle the code that defines the malloc()/new symbols. - Let the malloc()/new symbols route through the shim. - Have the shim ultimately invoke the actual allocator (tcmalloc, winheap, system allocator) as defined by the build config. This clearly cannot happen atomically in one CL. The plan, therefore, is to make the above happen behind a build-time flag (use_experimental_allocator_shim), so the shim can be easily tested / rolled-back in case of failures, and ultimately drop the build-time flag (and remove the dead cde) once everything works. This also will make dealing with reverts very manageable. Therefore this CL (and likely all the future ones) is meant to be a no-op until use_experimental_allocator_shim==true. Design doc: bit.ly/allocator-shim BUG=550886 TEST=build with use_experimental_allocator_shim=true, base_unittests --gtest_filter=AllocatorShimTest.* Committed: https://crrev.com/4e68ed2d51f897d12a570e16f7adbcb7595fa031 Cr-Commit-Position: refs/heads/master@{#380196}

Patch Set 1 : rebase #

Patch Set 2 : Remove use of c++ classes + test #

Patch Set 3 : cleanup #

Total comments: 1

Patch Set 4 : Rebase + some thoughts overnight #

Patch Set 5 : rebase #

Patch Set 6 : rebase + remove removal #

Patch Set 7 : rebase #

Total comments: 32

Patch Set 8 : Rebase + address thakis review #

Patch Set 9 : Make self the 1st arg #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1129 lines, -4 lines) Patch
M base/BUILD.gn View 1 2 3 4 5 6 7 4 chunks +12 lines, -0 lines 0 comments Download
M base/allocator/BUILD.gn View 1 2 3 4 5 6 7 2 chunks +33 lines, -0 lines 0 comments Download
M base/allocator/allocator.gyp View 3 chunks +46 lines, -1 line 0 comments Download
A base/allocator/allocator_shim.h View 1 2 3 4 5 6 7 8 1 chunk +96 lines, -0 lines 0 comments Download
A base/allocator/allocator_shim.cc View 1 2 3 4 5 6 7 8 1 chunk +246 lines, -0 lines 0 comments Download
A base/allocator/allocator_shim_default_dispatch_to_glibc.cc View 1 2 3 4 5 6 7 8 1 chunk +52 lines, -0 lines 0 comments Download
A base/allocator/allocator_shim_default_dispatch_to_tcmalloc.cc View 1 2 3 4 5 6 7 8 1 chunk +76 lines, -0 lines 0 comments Download
A base/allocator/allocator_shim_internals.h View 1 chunk +23 lines, -0 lines 0 comments Download
A base/allocator/allocator_shim_override_cpp_symbols.h View 1 chunk +42 lines, -0 lines 0 comments Download
A base/allocator/allocator_shim_override_glibc_weak_symbols.h View 1 chunk +109 lines, -0 lines 0 comments Download
A base/allocator/allocator_shim_override_libc_symbols.h View 1 chunk +54 lines, -0 lines 0 comments Download
A base/allocator/allocator_shim_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +314 lines, -0 lines 0 comments Download
M base/base.gyp View 1 2 3 4 5 6 7 3 chunks +7 lines, -0 lines 0 comments Download
M base/process/memory_linux.cc View 1 2 4 chunks +14 lines, -3 lines 0 comments Download
M build/common.gypi View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 47 (31 generated)
Primiano Tucci (use gerrit)
Well this is going to be .... fun! Thanks in advance. https://codereview.chromium.org/1675143004/diff/420001/base/allocator/allocator_shim.cc File base/allocator/allocator_shim.cc (right): ...
4 years, 10 months ago (2016-02-16 22:58:06 UTC) #24
Primiano Tucci (use gerrit)
+chrisha as follow-up to the SS16 chat.
4 years, 9 months ago (2016-02-29 20:55:04 UTC) #25
Nico
Nice design doc and CL description! I'm a bit worried about a) the amount of ...
4 years, 9 months ago (2016-03-08 03:08:42 UTC) #27
Primiano Tucci (use gerrit)
Super thanks for the review. Will get to the actual inline comments later today, in ...
4 years, 9 months ago (2016-03-08 11:55:05 UTC) #28
scottmg
On 2016/03/08 11:55:05, Primiano wrote: > Super thanks for the review. > Will get to ...
4 years, 9 months ago (2016-03-08 17:02:28 UTC) #29
Primiano Tucci (use gerrit)
Thanks for the review, really! New patchset + comments inline. https://codereview.chromium.org/1675143004/diff/500001/base/allocator/BUILD.gn File base/allocator/BUILD.gn (right): https://codereview.chromium.org/1675143004/diff/500001/base/allocator/BUILD.gn#newcode307 ...
4 years, 9 months ago (2016-03-08 20:54:05 UTC) #30
Nico
A'ight, lgtm, let's see how this goes :-) https://codereview.chromium.org/1675143004/diff/500001/base/allocator/allocator_shim.h File base/allocator/allocator_shim.h (right): https://codereview.chromium.org/1675143004/diff/500001/base/allocator/allocator_shim.h#newcode44 base/allocator/allocator_shim.h:44: using ...
4 years, 9 months ago (2016-03-09 05:34:25 UTC) #32
Primiano Tucci (use gerrit)
https://codereview.chromium.org/1675143004/diff/500001/base/allocator/allocator_shim.h File base/allocator/allocator_shim.h (right): https://codereview.chromium.org/1675143004/diff/500001/base/allocator/allocator_shim.h#newcode44 base/allocator/allocator_shim.h:44: using AllocFn = void*(size_t size, const AllocatorDispatch* self); On ...
4 years, 9 months ago (2016-03-09 10:51:59 UTC) #33
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1675143004/540001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1675143004/540001
4 years, 9 months ago (2016-03-09 10:52:33 UTC) #35
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 9 months ago (2016-03-09 12:42:11 UTC) #37
Primiano Tucci (use gerrit)
On 2016/03/09 12:42:11, commit-bot: I haz the power wrote: > Dry run: This issue passed ...
4 years, 9 months ago (2016-03-09 16:21:14 UTC) #38
Nico
I'd do 1 and reconsider if there are problems.
4 years, 9 months ago (2016-03-09 18:38:07 UTC) #39
Primiano Tucci (use gerrit)
On 2016/03/09 18:38:07, Nico wrote: > I'd do 1 and reconsider if there are problems. ...
4 years, 9 months ago (2016-03-09 18:41:21 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1675143004/540001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1675143004/540001
4 years, 9 months ago (2016-03-09 20:08:17 UTC) #43
commit-bot: I haz the power
Committed patchset #9 (id:540001)
4 years, 9 months ago (2016-03-09 20:13:57 UTC) #45
commit-bot: I haz the power
4 years, 9 months ago (2016-03-09 20:15:55 UTC) #47
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/4e68ed2d51f897d12a570e16f7adbcb7595fa031
Cr-Commit-Position: refs/heads/master@{#380196}

Powered by Google App Engine
This is Rietveld 408576698