This is an archive of the discontinued LLVM Phabricator instance.

[scudo][standalone] Reduce Android region sizes to 128MB
ClosedPublic

Authored by cryptoad on Jan 21 2020, 2:39 PM.

Details

Summary

Unity is making irresponsible assumptions as to how clumped up memory
should be. With larger regions, we break those, resulting in errors
like:

"Using memoryadresses from more that 16GB of memory"

This is unfortunately one of those situations where we have to bend to
existing code because we doubt it's going to change any time soon.

128MB should be enough, but we could be flirting with OOMs in the
higher class sizes.

Diff Detail

Unit TestsFailed

Event Timeline

cryptoad created this revision.Jan 21 2020, 2:39 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 21 2020, 2:39 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Unit tests: fail. 62080 tests passed, 1 failed and 784 were skipped.

failed: Clang.Driver/cc-print-options.c

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

hctim added a comment.Jan 21 2020, 2:57 PM

This is unfortunately one of those situations where we have to bend to
existing code because we doubt it's going to change any time soon.

:'(

Apparently this was fixed in Unity 5.4.1 (feedback post only available on WaybackArchive). You can see the patchnotes here which mentions: Linux: Removed 16GB total memory limit..

Instead of bending to old apps, could we consider potentially gating either a Scudo specific config, or Scudo in general (over jemalloc) on AppCompat platform version? We are doing this for Tagged Pointers currently in an attempt not to break old apps. The former seems like a much better option, as the latter requires us to maintain jemalloc on Android for the foreseeable future to use as a fallback allocator for old apps.

eugenis accepted this revision.Jan 21 2020, 3:09 PM

Fallback to a different allocator is hard, because then you need to implement PointerIsMine to redirect to the right allocator in free(), and AFAIK we don't have that in scudo for the larger allocations (i.e. secondary).

This is terrible, but what can you do.

LGTM

This revision is now accepted and ready to land.Jan 21 2020, 3:09 PM
pcc added a comment.Jan 21 2020, 3:12 PM

Fallback to a different allocator is hard, because then you need to implement PointerIsMine to redirect to the right allocator in free(), and AFAIK we don't have that in scudo for the larger allocations (i.e. secondary).

If we really wanted to do this, maybe we could use one of the TBI bits (bit 60-63 so as not to collide with MTE) to mark the allocation as belonging to the new allocator.

This revision was automatically updated to reflect the committed changes.