This is an archive of the discontinued LLVM Phabricator instance.

[Sanitizers][Darwin] Intercept CCRandomGenerateBytes
AbandonedPublic

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

Details

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

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
yln added a comment.Jul 12 2021, 3:46 PM

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)?

devnexen updated this revision to Diff 358328.Jul 13 2021, 10:19 AM

Update unit test case.

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.
yln added a comment.Jul 22 2021, 3:52 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.

devnexen abandoned this revision.Aug 16 2021, 1:43 AM

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