This is an archive of the discontinued LLVM Phabricator instance.

Dynamically allocate scudo allocation buffer.
ClosedPublic

Authored by fmayer on Jan 3 2023, 5:13 PM.

Details

Summary

This is so we can increase the buffer size for finding elusive bugs.

Tested by hand with this program

int main(int argc, char** argv) {
  if (argc < 2)
    return 1;
  int n = atoi(argv[1]);
  char* x = reinterpret_cast<char*>(malloc(1));
  *((volatile char*)x) = 1;
  free(x);
  for (; n > 0; --n) {
    char* y = reinterpret_cast<char*>(malloc(1024));
    *((volatile char*)y) = 1;
    free(y);
  }
  *x = 2;
  return 0;
}

SCUDO_OPTIONS=allocation_ring_buffer_size=30000 ./uaf 1000000
-> no allocation trace
SCUDO_OPTIONS=allocation_ring_buffer_size=30000000 ./uaf 1000000
-> allocation trace

Diff Detail

Event Timeline

fmayer created this revision.Jan 3 2023, 5:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2023, 5:13 PM
fmayer requested review of this revision.Jan 3 2023, 5:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2023, 5:13 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
fmayer edited the summary of this revision. (Show Details)Jan 3 2023, 5:13 PM
fmayer updated this revision to Diff 486122.Jan 3 2023, 5:14 PM

unneeded includes

Chia-hungDuan added inline comments.Jan 4 2023, 4:57 PM
compiler-rt/lib/scudo/standalone/combined.h
1045

I'm thinking if we do something like the concept in llvm::TrailingObjects, such as

struct TrailingEntries {
  ... GetRingBufferEntry(...);
};

struct AllocationRingBuffer : TrailingEntries {
...
};

Will it look better?

fmayer added inline comments.Jan 9 2023, 4:02 PM
compiler-rt/lib/scudo/standalone/combined.h
1045

I thought about this, but getting this right in a standards-compliant way is a quite subtle exercise. E.g. I think we cannot just use the this pointer from the AllocationRingBuffer, because that would be going over the object boundaries from a C++ standards perspective. This is why I separately retain the RawRingBuffer pointer.

All in all I am not sure it's worth it.

fmayer added a reviewer: hctim.Jan 9 2023, 4:09 PM
Chia-hungDuan added inline comments.Jan 9 2023, 5:17 PM
compiler-rt/lib/scudo/standalone/combined.h
1045

I see. I think casting this to access the memory address before/after it may be safe if we manage the entire raw memory by ourselves. But l agree with you, I'm not so certain if there's any tricky part.

I'm trying to make the allocator(in this combined.h) look simpler so if we can bundle certain functionalities together, the later code restructuring may be easier. That's why I was thinking the trailing structure. But I think it's better to have this first (it reduces the scudo metadata size on certain platforms that don't need a big entry buffer). Thus don't worry about this suggestion, thanks!

fmayer marked 2 inline comments as done.Jan 10 2023, 2:44 PM
hctim added inline comments.Jan 10 2023, 3:44 PM
compiler-rt/lib/scudo/standalone/combined.h
181–182

not sure what extra information this comment is telling me...

947–953

why not just allow the fuzzer to replace the entire ringbuffer?

952

this seems unnecessary, the regular API can be used?

1060–1066

can you have just RingBuffer and not have this raw variant? it seems to be only used in two places (there are more references to the name but they're just from the same-named argument).

the invariant doesn't always hold and seems tricky to maintain. for example, it doesn't hold when fuzzing is under operation.

fmayer updated this revision to Diff 488284.Jan 11 2023, 10:40 AM
fmayer marked an inline comment as done.

remove comment

compiler-rt/lib/scudo/standalone/combined.h
947–953

No, this is not how this gets used. The fuzzer creates the buffer for itself, and then uses a STATIC method on that. As such, what's in the Allocator object doesn't matter. What does matter is that the Size member of the RingBuffer is consistent with its size, otherwise we will trivially get loads of crashes.

952

See other comment about fuzzer API.

1060–1066

This is to be strictly standards compliant (and to retain some convenience). I think what we could do is _only_ keep the RawRingBuffer and cast it for all uses of RingBuffer.

fmayer added inline comments.Jan 11 2023, 10:44 AM
compiler-rt/lib/scudo/standalone/combined.h
1060–1066

the invariant doesn't always hold and seems tricky to maintain. for example, it doesn't hold when fuzzing is under operation.

It does always hold. This only gets assigned in one place. Fuzzing is irrelevant because it doesn't use an Allocator object.

hctim added inline comments.Jan 11 2023, 11:33 AM
compiler-rt/lib/scudo/standalone/combined.h
947–953

yeah, so the fuzzer calls getErrorInfo using a fuzzer-generated ringbuffer.

previously getRingBufferErrorInfo used AllocationRingBuffer::NumEntries to determine the size of the ringbuffer, so that definition was ifdef'd.

now the size of the ringbuffer is dynamic and accessed through RingBuffer->Size.

so why not just allow the fuzzer to control the size AND contentx of the ringbuffer, and just set RingBuffer->Size to the right value?

i assumed that there was some reference during getErrorInfo to the ringbuffer via. the allocator, but if that's not the case, just delete these functions (getRingBufferSizeForFuzzer, setRingBufferForFuzzing)?

fmayer updated this revision to Diff 488329.Jan 11 2023, 12:11 PM

change fuzzer

fmayer added inline comments.Jan 11 2023, 12:29 PM
compiler-rt/lib/scudo/standalone/combined.h
947–953

I really disagree to leak the structs to the fuzzer (as discussed offline), it's much less neat than providing one function to it to fix up the Size. Either way we are adding an API for the fuzzer: either through two structs and implicit invariants about them, or with one function and the fuzzer can otherwise still treat the RingBuffer as an opaque blob.

The other suggestion is a good point, I allowed the fuzzer to control both the Size and the content.

hctim accepted this revision.Jan 11 2023, 12:54 PM
This revision is now accepted and ready to land.Jan 11 2023, 12:54 PM

If you could please change InitRingBuffer & RingBufferSizeInBytes to abide to the LLVM coding standard (https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)

fmayer updated this revision to Diff 488353.Jan 11 2023, 1:08 PM

more style

If you could please change InitRingBuffer & RingBufferSizeInBytes to abide to the LLVM coding standard (https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)

Thanks. Also changed GetRingBufferEntry to getRingBufferEntry.

fmayer marked 4 inline comments as done.Jan 11 2023, 1:19 PM
eugenis accepted this revision.Jan 11 2023, 3:52 PM

LGTM

compiler-rt/lib/scudo/standalone/combined.h
1060–1066

I think what we could do is _only_ keep the RawRingBuffer and cast it for all uses of RingBuffer.

I'd like this. Add an accessor function or something - the current code wastes memory, however little.

fmayer updated this revision to Diff 488410.Jan 11 2023, 4:00 PM

evgenii's comment

fmayer marked an inline comment as done.Jan 11 2023, 4:44 PM
This revision was landed with ongoing or failed builds.Jan 11 2023, 4:53 PM
This revision was automatically updated to reflect the committed changes.
shafik added a subscriber: shafik.Jan 11 2023, 5:30 PM
shafik added inline comments.
compiler-rt/lib/scudo/standalone/combined.h
1264

On my local build I am seeing the following diagnostic:

combined.h:1267:47: error: conversion from ‘scudo::uptr’ {aka ‘long unsigned int’} to ‘scudo::u32’ {aka ‘unsigned int’} may change value [-Werror=conversion]
         getRingBufferEntry(RawRingBuffer, Pos % getRingBuffer()->Size);
                                           ~~~~^~~~~~~~~~~~~~~~~~~~~~~
fmayer added inline comments.Jan 11 2023, 5:30 PM
compiler-rt/lib/scudo/standalone/combined.h
1264
amyk added a subscriber: amyk.Jan 16 2023, 9:32 AM

Hi, I realized that the clang-ppc64le-rhel bot is exhibiting the following error when building this patch (https://lab.llvm.org/buildbot/#/builders/57/builds/23848/steps/7/logs/stdio):

FAILED: compiler-rt/lib/scudo/standalone/tests/ScudoUnitTestsObjects.combined_test.cpp.powerpc64le.o /home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/runtimes/runtimes-bins/compiler-rt/lib/scudo/standalone/tests/ScudoUnitTestsObjects.combined_test.cpp.powerpc64le.o 
cd /home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/runtimes/runtimes-bins/compiler-rt/lib/scudo/standalone/tests && /home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/./bin/clang -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Werror -Wno-unused-parameter -Wno-unknown-warning-option -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta -g -Wno-covered-switch-default -Wno-suggest-override -DGTEST_NO_LLVM_SUPPORT=1 -DGTEST_HAS_RTTI=0 -I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/runtimes/../third-party/unittest/googletest/include -I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/runtimes/../third-party/unittest/googletest -I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/include -I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib -I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone -I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/include -DGTEST_HAS_RTTI=0 -g -Wno-mismatched-new-delete -m64 -fno-function-sections -c -o ScudoUnitTestsObjects.combined_test.cpp.powerpc64le.o /home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
In file included from /home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp:10:
In file included from /home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/scudo_unit_test.h:15:
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/runtimes/../third-party/unittest/googletest/include/gtest/gtest.h:1628:28: error: comparison of integers of different signs: 'const unsigned long' and 'const int' [-Werror,-Wsign-compare]
GTEST_IMPL_CMP_HELPER_(GT, >);
~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/runtimes/../third-party/unittest/googletest/include/gtest/gtest.h:1608:12: note: expanded from macro 'GTEST_IMPL_CMP_HELPER_'
  if (val1 op val2) {\
      ~~~~ ^  ~~~~
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp:708:3: note: in instantiation of function template specialization 'testing::internal::CmpHelperGT<unsigned long, int>' requested here
  ASSERT_GT(Size, 0);
  ^
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/runtimes/../third-party/unittest/googletest/include/gtest/gtest.h:2076:32: note: expanded from macro 'ASSERT_GT'
# define ASSERT_GT(val1, val2) GTEST_ASSERT_GT(val1, val2)
                               ^
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/runtimes/../third-party/unittest/googletest/include/gtest/gtest.h:2050:44: note: expanded from macro 'GTEST_ASSERT_GT'
  ASSERT_PRED_FORMAT2(::testing::internal::CmpHelperGT, val1, val2)
                                           ^
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp:705:1: note: in instantiation of member function 'ScudoCombinedTestRingBufferSize<scudo::AndroidSvelteConfig>::Run' requested here
SCUDO_TYPED_TEST(ScudoCombinedTest, RingBufferSize) {
^
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp:129:3: note: expanded from macro 'SCUDO_TYPED_TEST'
  SCUDO_TYPED_TEST_ALL_TYPES(FIXTURE, NAME)                                    \
  ^
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp:115:3: note: expanded from macro 'SCUDO_TYPED_TEST_ALL_TYPES'
  SCUDO_TYPED_TEST_TYPE(FIXTURE, NAME, AndroidSvelteConfig)                    \
  ^
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp:122:70: note: expanded from macro 'SCUDO_TYPED_TEST_TYPE'
  TEST_F(FIXTURE##NAME##_##TYPE, NAME) { FIXTURE##NAME<scudo::TYPE>::Run(); }
                                                                     ^
1 error generated.

Would you be able to assist in resolving this issue?

tislam added a subscriber: tislam.Jan 16 2023, 9:36 AM

Hi, I realized that the clang-ppc64le-rhel bot is exhibiting the following error when building this patch (https://lab.llvm.org/buildbot/#/builders/57/builds/23848/steps/7/logs/stdio):

FAILED: compiler-rt/lib/scudo/standalone/tests/ScudoUnitTestsObjects.combined_test.cpp.powerpc64le.o /home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/runtimes/runtimes-bins/compiler-rt/lib/scudo/standalone/tests/ScudoUnitTestsObjects.combined_test.cpp.powerpc64le.o 
cd /home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/runtimes/runtimes-bins/compiler-rt/lib/scudo/standalone/tests && /home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/./bin/clang -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Werror -Wno-unused-parameter -Wno-unknown-warning-option -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta -g -Wno-covered-switch-default -Wno-suggest-override -DGTEST_NO_LLVM_SUPPORT=1 -DGTEST_HAS_RTTI=0 -I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/runtimes/../third-party/unittest/googletest/include -I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/runtimes/../third-party/unittest/googletest -I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/include -I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib -I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone -I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/include -DGTEST_HAS_RTTI=0 -g -Wno-mismatched-new-delete -m64 -fno-function-sections -c -o ScudoUnitTestsObjects.combined_test.cpp.powerpc64le.o /home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
In file included from /home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp:10:
In file included from /home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/scudo_unit_test.h:15:
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/runtimes/../third-party/unittest/googletest/include/gtest/gtest.h:1628:28: error: comparison of integers of different signs: 'const unsigned long' and 'const int' [-Werror,-Wsign-compare]
GTEST_IMPL_CMP_HELPER_(GT, >);
~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/runtimes/../third-party/unittest/googletest/include/gtest/gtest.h:1608:12: note: expanded from macro 'GTEST_IMPL_CMP_HELPER_'
  if (val1 op val2) {\
      ~~~~ ^  ~~~~
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp:708:3: note: in instantiation of function template specialization 'testing::internal::CmpHelperGT<unsigned long, int>' requested here
  ASSERT_GT(Size, 0);
  ^
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/runtimes/../third-party/unittest/googletest/include/gtest/gtest.h:2076:32: note: expanded from macro 'ASSERT_GT'
# define ASSERT_GT(val1, val2) GTEST_ASSERT_GT(val1, val2)
                               ^
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/runtimes/../third-party/unittest/googletest/include/gtest/gtest.h:2050:44: note: expanded from macro 'GTEST_ASSERT_GT'
  ASSERT_PRED_FORMAT2(::testing::internal::CmpHelperGT, val1, val2)
                                           ^
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp:705:1: note: in instantiation of member function 'ScudoCombinedTestRingBufferSize<scudo::AndroidSvelteConfig>::Run' requested here
SCUDO_TYPED_TEST(ScudoCombinedTest, RingBufferSize) {
^
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp:129:3: note: expanded from macro 'SCUDO_TYPED_TEST'
  SCUDO_TYPED_TEST_ALL_TYPES(FIXTURE, NAME)                                    \
  ^
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp:115:3: note: expanded from macro 'SCUDO_TYPED_TEST_ALL_TYPES'
  SCUDO_TYPED_TEST_TYPE(FIXTURE, NAME, AndroidSvelteConfig)                    \
  ^
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp:122:70: note: expanded from macro 'SCUDO_TYPED_TEST_TYPE'
  TEST_F(FIXTURE##NAME##_##TYPE, NAME) { FIXTURE##NAME<scudo::TYPE>::Run(); }
                                                                     ^
1 error generated.

Would you be able to assist in resolving this issue?

Seems like this was fixed in rG6be602f79cbf501f0e67cee3f926c11fa7490d4f