trusty.cpp and trusty.h define Trusty implementations of map and other
platform-specific functions. In addition to adding Trusty configurations
in allocator_config.h and size_class_map.h, MapSizeIncrement and
PrimaryEnableRandomOffset are added as configurable options in
allocator_config.h.
Background on Trusty: https://source.android.com/security/trusty
Details
- Reviewers
cryptoad vitalybuka - Commits
- rG2551053e8d8d: [scudo] Add Scudo support for Trusty OS
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thank you Daniel, this looks like a good start!
I'll work on my side on reducing some of the memory footprint of some structures and whatnot (scoped strings come to mind).
As discussed, we can re-introduce the use-separate-class-for-batches distinction for Trusty, which can be a large win to get rid of a class size.
I still think the Primary32 might be better suited here, but we'll follow up.
compiler-rt/lib/scudo/standalone/allocator_config.h | ||
---|---|---|
48 | I think this should be PrimaryMapSizeIncrement to be homogeneous with the naming of all the Primary settings. | |
compiler-rt/lib/scudo/standalone/primary64.h | ||
79 | Maybe use a ternary here? | |
275 | If it's only used once, you might just want to use Config::PrimaryEnableRandomOffset and not add the extra definition. | |
compiler-rt/lib/scudo/standalone/trusty.cpp | ||
34 | Addr is UNUSED | |
37 | The variables in this function don't have the "proper" casing. Should be ProgramBreak and so on. |
Updated scudo/standalone/tests with primary64 configurable
options PrimaryEnableRandomOffset and PrimaryMapSizeIncrement.
compiler-rt/lib/scudo/standalone/memtag.h | ||
---|---|---|
58–60 | Even if I make this suggested change, there is non-Linux-specific AArch64 assembly which cannot be compiled with Trusty's build system (see lines 119-265). | |
compiler-rt/lib/scudo/standalone/trusty.cpp | ||
63–67 | Trusty has its own build system, so the existing Scudo tests would need to be ported to Trusty as test apps. We have a Trusty test app for Scudo running downstream on the Trusty side, and we can consider porting other Scudo tests to Trusty downstream. |
compiler-rt/lib/scudo/standalone/memtag.h | ||
---|---|---|
58–60 | Isn't it a compiler issue rather than a build system issue? Can you resolve it by upgrading the compiler? (I think we technically should be making this conditional on the compiler version since our minimum supported compiler version is older than the version that added MTE support, but that's a separate issue.) |
compiler-rt/lib/scudo/standalone/memtag.h | ||
---|---|---|
58–60 | You're right, I meant the Trusty compiler was out of date. As it turns out, the compiler was just updated so now this change is unnecessary and memtag.h can be left as it was before. |
compiler-rt/lib/scudo/standalone/trusty.cpp | ||
---|---|---|
94 | Isn't printf using malloc (or others)? If so this will recurse. |
compiler-rt/lib/scudo/standalone/trusty.cpp | ||
---|---|---|
53 | Can _trusty_brk set the errno to ENOMEM? |
compiler-rt/lib/scudo/standalone/trusty.cpp | ||
---|---|---|
53 | It turns out that _trusty_brk does not set errno, so I modify map to set it manually. This is consistent with how Trusty implements sbrk using _trusty_brk as well. Also, since we know errno equals ENOMEM in this if branch, we just directly call dieOnMapUnmapError(Size) instead of dieOnMapUnmapError(errno == ENOMEM ? Size : 0); | |
94 | Trusty uses Musl's implementation of printf which uses a fixed size buffer allocated on the stack. |
I patched it locally and everything passes for me.
Given the current timeline, I am OK with landing it now as is, and improve gradually on it.
I will pick up some of the action items, notably to try and reduce the memory footprint.
compiler-rt/lib/scudo/standalone/tests/combined_test.cpp | ||
---|---|---|
112 | Enabling the Trusty config on non-trusty config is wonky: #4 0x0000555555438f23 in scudo::map (Addr=0x7fffcfaab400, Size=1024, Name=0x7ffff7a68237 "scudo:primary", Flags=5, Data=0x172aff200210) at third_party/llvm/llvm-project/compiler-rt/lib/scudo/standalone/linux.cpp:69 It ends up trying to map at a non-page boundary a non-multiple-of-page size, ending up in an abort. |
LGTM but upstream testing could be very helpful for those who change code without TrustyOS setup
I think this should be PrimaryMapSizeIncrement to be homogeneous with the naming of all the Primary settings.