[Sanitizers][Darwin] Intercept CCRandomGenerateBytes
AbandonedPublic
Actions

Authored by devnexen on Jul 3 2021, 5:55 AM.

Details

Reviewers

yln
delcypher
aralisza
eugenis
vitalybuka

Summary

Intercept the native apple (both iOs and macOS) which is even allowed on App Store (contrary to getentropy for example), thus checking its proper usage in code consumers.

Diff Detail

Unit TestsFailed
View All

	Time	Test
	510 ms	x64 debian > Clang.Driver::cuda-arch-translation.cu Script: -- : 'RUN: at line 8'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -### -target x86_64-linux-gnu -c --cuda-gpu-arch=sm_20 /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/cuda-arch-translation.cu 2>&1 \| /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck -check-prefixes=COMMON,SM20 /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/cuda-arch-translation.cu
	250 ms	x64 debian > Clang.Driver::cuda-bad-arch.cu Script: -- : 'RUN: at line 7'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -### -target x86_64-linux-gnu --cuda-gpu-arch=compute_20 -c /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/cuda-bad-arch.cu 2>&1 \| /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck -check-prefix BAD /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/cuda-bad-arch.cu
	310 ms	x64 debian > Clang.Driver::cuda-flush-denormals-to-zero.cu Script: -- : 'RUN: at line 5'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=sm_20 -fgpu-flush-denormals-to-zero -nocudainc -nocudalib /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/cuda-flush-denormals-to-zero.cu 2>&1 \| /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck -check-prefix=FTZ /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/cuda-flush-denormals-to-zero.cu
	7,780 ms	x64 windows > Clang.Driver::cuda-arch-translation.cu Script: -- : 'RUN: at line 8'; c:\ws\w5\llvm-project\premerge-checks\build\bin\clang.exe -### -target x86_64-linux-gnu -c --cuda-gpu-arch=sm_20 C:\ws\w5\llvm-project\premerge-checks\clang\test\Driver\cuda-arch-translation.cu 2>&1 \| c:\ws\w5\llvm-project\premerge-checks\build\bin\filecheck.exe -check-prefixes=COMMON,SM20 C:\ws\w5\llvm-project\premerge-checks\clang\test\Driver\cuda-arch-translation.cu
	2,920 ms	x64 windows > Clang.Driver::cuda-bad-arch.cu Script: -- : 'RUN: at line 7'; c:\ws\w5\llvm-project\premerge-checks\build\bin\clang.exe -### -target x86_64-linux-gnu --cuda-gpu-arch=compute_20 -c C:\ws\w5\llvm-project\premerge-checks\clang\test\Driver\cuda-bad-arch.cu 2>&1 \| c:\ws\w5\llvm-project\premerge-checks\build\bin\filecheck.exe -check-prefix BAD C:\ws\w5\llvm-project\premerge-checks\clang\test\Driver\cuda-bad-arch.cu
		View Full Test Results (6 Failed)

Event Timeline

devnexen requested review of this revision.Jul 3 2021, 5:55 AM

devnexen created this revision.

Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2021, 5:55 AM

Herald added a subscriber: Restricted Project. · View Herald Transcript

Harbormaster completed remote builds in B112321: Diff 356338.Jul 3 2021, 6:32 AM

Hi David, thanks for the patch for this previously unintercepted API. I am happy with the interceptor implementation itself; it follows our "recipe" nicely! :)

I have two small asks:

In the commit message, can you explain which issue (crash, false positive) for which sanitizer this is addressing?
Can we write a test that shows this issue? I think the current test only shows that we don't crash, right?

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
7246	Nitpick: can we use the parameter names "bytes" and "count" from the header declaration? CCRNGStatus CCRandomGenerateBytes(void *bytes, size_t count) __OSX_AVAILABLE_STARTING(__MAC_10_10, __IPHONE_8_0);
compiler-rt/test/sanitizer_common/TestCases/Darwin/ccrandomgeneratebytes.c
2	Are we testing here simply that we don't crash in our interceptor? Can we add a comment what we intend to test here.
5	I don't think this will work as intended. We would try to compile and run an empty file. However, I think no one is actually testing on macOS version < 10.10, so just omitting this guard should be fine.
13	I don't think we will ever return "false", i.e., failing the test here? success -> n = 0 failure -> n = 1 return n <= 0 // n is never <= 0, right?! Maybe just return the return value of `CCRandomGenerateBytes()` (assuming `kCCSuccess` is 0)?

yln added reviewers: delcypher, aralisza, eugenis, vitalybuka.Jul 12 2021, 3:48 PM

Update unit test case.

Harbormaster completed remote builds in B113779: Diff 358328.Jul 13 2021, 10:20 AM

devnexen edited the summary of this revision. (Show Details)Jul 13 2021, 10:21 AM

devnexen marked 3 inline comments as done.

vitalybuka added inline comments.Jul 13 2021, 11:29 AM

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
9836–9845	Why this renaming? As it it matches https://man7.org/linux/man-pages/man2/getrandom.2.html Either way it looks unrelated to the patch.

It's titled "Intercept CCRandomGenerateBytes", but I don't see changes in interceptors, only new test.

devnexen added inline comments.Jul 13 2021, 11:42 AM

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
9836–9845	My bad wrong lines.

devnexen updated this revision to Diff 358377.Jul 13 2021, 11:48 AM

vitalybuka accepted this revision.Jul 13 2021, 11:51 AM

vitalybuka added 1 blocking reviewer(s): yln.

Harbormaster completed remote builds in B113811: Diff 358377.Jul 13 2021, 1:11 PM

Hi David,

Before I approve this can we do the following?

In the commit message, can you explain which issue (crash, false positive) for which sanitizer this is addressing?
Can we write a test that shows this issue, i.e., a test that fails without the added interceptor? (Maybe this test needs to be for a specific sanitizer instead of sanitizer_common?)

Note: my understanding is that the added test is "green" even without the new interceptor. Ignore my second point if that is not the case.

you re right after second thoughts it does not bring anything on the table.