This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Rework Vector/String
ClosedPublic

Authored by cryptoad on Jun 3 2021, 12:11 PM.

Details

Summary

Some platforms (eg: Trusty) are extremelly memory constrained, which
doesn't necessarily work well with some of Scudo's current assumptions.

Vector by default (and as such String and ScopedString) maps a
page, which is a bit of a waste. This CL changes Vector to use a
buffer local to the class first, then potentially map more memory if
needed (ScopedString currently are all stack based so it would be
stack data). We also want to allow a platform to prevent any dynamic
resizing, so I added a CanGrow templated parameter that for now is
always true but would be set to false on Trusty.

Diff Detail

Event Timeline

cryptoad created this revision.Jun 3 2021, 12:11 PM
cryptoad requested review of this revision.Jun 3 2021, 12:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2021, 12:11 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
cryptoad updated this revision to Diff 349642.Jun 3 2021, 12:13 PM

Correcting a couple of ScopedString constructors.

cryptoad updated this revision to Diff 349646.Jun 3 2021, 12:31 PM

Corrected, with a test now.

hctim added a comment.Jun 3 2021, 1:05 PM

CanGrow templated parameter that for now is
always true but would be set to false on Trusty.

Are we seeing OOMs on Trusty due to ScopedString? I had a quick look and it seemed like there's no non-ephemeral buffers (and really, this shouldn't be called very often at all), and it concerns me a little that push_back can silently fail on a ScopedErrorReport and truncate the text and it seems hard to test "all failure conditions log successfully".

compiler-rt/lib/scudo/standalone/string_utils.h
21–23

With InitialSize == 0, isn't this OOB?

Local buffer is LGTM, but CantGrow looks like unrelated change and a good candidate for a separate patch.

compiler-rt/lib/scudo/standalone/string_utils.h
20–21

Interface will be cleaner without InitialSize, we removed it sanitizer_common some time ago.

maybe for a separate patch:
uptr length() const { return buffer_.size() - 1; } can retire Length

compiler-rt/lib/scudo/standalone/vector.h
20

As is seems all users of VectorNoCtor will need to re-think consequences of failed push_back.
Interface mimics std::vector, but it's unexpected that after N push_back() container may be smaller than N.

Maybe we can we rather set max grow size for Trusty and crash if it's reached. Which also a problem, but at least easy to detect.

another alternative is to assume that Trusty will can handle Grow in most cases and when it cant, we can handle that explicitly on caller side by checking container size before push_back

CanGrow templated parameter that for now is
always true but would be set to false on Trusty.

Are we seeing OOMs on Trusty due to ScopedString? I had a quick look and it seemed like there's no non-ephemeral buffers (and really, this shouldn't be called very often at all), and it concerns me a little that push_back can silently fail on a ScopedErrorReport and truncate the text and it seems hard to test "all failure conditions log successfully".

Some Trusty apps have a heap of 1 page (yup), and all maps end up in the sbrk space. So in it's current shape, a ScopedString is taking way too much space in the heap.
push_back is only used in tests so far, formatString failing might be a little more problematic. I think there will be some truncation for larger strings, but more of the simple ones will go through, and that's probably the price to pay when you have such a small heap.

compiler-rt/lib/scudo/standalone/string_utils.h
21–23

Well we always have at least the local buffer worth of space, so it won't OOB, but I see how this isn't obvious.
I am going to add a check.

hctim added a comment.Jun 3 2021, 2:01 PM

Some Trusty apps have a heap of 1 page (yup),

Wow!

compiler-rt/lib/scudo/standalone/string_utils.h
21–23

Yeah, logical bug. But if I'm reading correctly, it should currently trip the DCHECK :).

cryptoad updated this revision to Diff 349680.Jun 3 2021, 2:07 PM
cryptoad marked 2 inline comments as done.

Based on feedback, remove for now the CanGrow aspect of the vector.

cryptoad updated this revision to Diff 349682.Jun 3 2021, 2:11 PM

Remove stray comment.

cryptoad updated this revision to Diff 349685.Jun 3 2021, 2:17 PM

Remove InitialSize as suggested.

cryptoad updated this revision to Diff 349688.Jun 3 2021, 2:19 PM

and I forgot to copy over the test file.

vitalybuka accepted this revision.Jun 3 2021, 2:26 PM
This revision is now accepted and ready to land.Jun 3 2021, 2:26 PM
hctim accepted this revision.Jun 3 2021, 2:31 PM

LGTM w/ nits.

compiler-rt/lib/scudo/standalone/tests/vector_test.cpp
27

why /s/int/uptr? I mean, okay to keep the change, but if so also change the type of the Vector

compiler-rt/lib/scudo/standalone/vector.h
94

Is stack size a problem?

If so, we could typedef this (template <typename T, int StackBufferBytes = 256> class VectorNoCtor) and in places where we know the upper bound, we can bound the stack buffer size as well.

cryptoad updated this revision to Diff 349696.Jun 3 2021, 2:46 PM

Change type in vector test.

cryptoad updated this revision to Diff 349703.Jun 3 2021, 3:02 PM

Switching 2 lines around.

cryptoad added inline comments.Jun 3 2021, 3:03 PM
compiler-rt/lib/scudo/standalone/vector.h
94

Apparently stack is 1 page as well. 256 should be fine, but I will revisit with CanGrow as well.

This revision was landed with ongoing or failed builds.Jun 3 2021, 6:13 PM
This revision was automatically updated to reflect the committed changes.
kda added a subscriber: kda.Jun 4 2021, 12:44 PM

This looks like it broke the sanitizer-x86_64-linux-qemu.
https://lab.llvm.org/buildbot/#/builders/169/builds/501

This looks like it broke the sanitizer-x86_64-linux-qemu.
https://lab.llvm.org/buildbot/#/builders/169/builds/501

Found the issue, testing the fix.

This looks like it broke the sanitizer-x86_64-linux-qemu.
https://lab.llvm.org/buildbot/#/builders/169/builds/501

Found the issue, testing the fix.

Should be fixed with https://reviews.llvm.org/D103716

kda added a comment.Jun 4 2021, 1:48 PM

thank you!