Page MenuHomePhabricator

[compiler-rt] implement COMMON_INTERCEPTOR_COPY_STRING for asan
AcceptedPublic

Authored by aralisza on Fri, Oct 8, 5:46 PM.

Details

Summary

COMMON_INTERCEPTOR_COPY_STRING only had an implementation in msan, but it would make sense to implement it for asan as well.
This was added due to false negative in ASan on buggy strlcpy usage. Added a regression test for strlcpy as well.

rdar://83965778

Diff Detail

Event Timeline

aralisza created this revision.Fri, Oct 8, 5:46 PM
aralisza requested review of this revision.Fri, Oct 8, 5:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Oct 8, 5:46 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
aralisza updated this revision to Diff 379129.Tue, Oct 12, 11:49 AM

set linux as unsupported

aralisza edited the summary of this revision. (Show Details)

Note we'll need another test case if we choose to add support for checking overlapping params.

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
308 ↗(On Diff #379129)

Given that this macro is supposed to model a copy operation we probably need to model the read operation too with something like.

COMMON_INTERCEPTOR_READ_RANGE(from, to, size);

However, this isn't really enough because it's undefined what happens when to and from overlap for strlcpy.

ASan already has support for this kind of thing in its CHECK_RANGES_OVERLAP macro. So maybe we could introduce a COMMON_INTERCEPTOR_CHECK_RANGES_OVERLAP() macro

// In `asan_interceptors.cpp`
#define COMMON_INTERCEPTOR_CHECK_RANGES_OVERLAP(ctx, name, buffer1, length1, buffer2, length2 ) { \
  CHECK_RANGES_OVERLAP(name, buffer1, length1, buffer2, length2) \
}
// In `sanitizer_common_interceptors.inc`

// FIXME: Figure this out. Maybe we should implement a barebones implementation here?
#ifndef  COMMON_INTERCEPTOR_CHECK_RANGES_OVERLAP
#define COMMON_INTERCEPTOR_CHECK_RANGES_OVERLAP(ctx, name, buffer1, length1, buffer2, length2 ) { }
#endif


#ifndef COMMON_INTERCEPTOR_COPY_STRING
#define COMMON_INTERCEPTOR_COPY_STRING(ctx, name, to, from, size) { \
  COMMON_INTERCEPTOR_CHECK_RANGES_OVERLAP(ctx, name, from, size, to, size) \
  COMMON_INTERCEPTOR_READ_RANGE(ctx, from, size); \
  COMMON_INTERCEPTOR_WRITE_RANGE(ctx, to, size); \
}
#endif

Note I changed COMMON_INTERCEPTOR_CHECK_RANGES_OVERLAP's signature to pass name which is the name of the function being intercepted because the CHECK_RANGES_OVERLAP macro needs it.

compiler-rt/test/asan/TestCases/Posix/strlcpy-buffer-overflow.cpp
3 ↗(On Diff #379129)

Why unsupported on Linux?

Another way we could go about this is just implement an ASan specific implementation, e.g. we already have one for INTERCEPTOR(char *, strcpy, char *to, const char *from) in asan_interceptors.cpp.

I didn't originally add this to asan_interceptors.cpp because there was already an interceptor in the common interceptors. If I want to copy it to asan interceptors, would I need to delete it from common interceptors? That seems like it might be a riskier change

compiler-rt/test/asan/TestCases/Posix/strlcpy-buffer-overflow.cpp
3 ↗(On Diff #379129)

the other strlcpy tests are marked unsupported on linux https://reviews.llvm.org/D44432

also this test fails on debian according to pr testing

aralisza edited the summary of this revision. (Show Details)Wed, Oct 13, 1:19 PM
aralisza updated this revision to Diff 379515.Wed, Oct 13, 1:37 PM

move implementation of macro to asan interceptors

aralisza updated this revision to Diff 379520.Wed, Oct 13, 1:51 PM

add test for overlap

yln added a comment.Wed, Oct 13, 2:51 PM

Please also add a test for strlcat.

compiler-rt/lib/asan/asan_interceptors.cpp
106

Note that the passed size argument is the size of the destination buffer. src can be an arbitrarily long string. I think we should model the touched ranges a bit more precisely.

size_t strlcpy(char * restrict dst, const char * restrict src, size_t dstsize):
--->
src_size = internal_strlen(src) + 1
// Is reading a potentially non-terminated string here okay?  How do we handle this in other interceptors?

check_overlap(dst, dst_size, src, src_size)
read_range(src, src_size)
write_range(dst, min(src_size, dst_size))
compiler-rt/test/asan/TestCases/Posix/strlcpy_test.cpp
18

Note that the size argument is the size of the destination buffer.

Let's try to express the misuse of the API that we are trying to catch in the simplest way possible without complications (ARRAY_SIZE, malloc):

cont char *src = "long string";
const dst_size = 3;
char dst[dst_size];
strlcpy(dst, src, dst_size+1);
aralisza updated this revision to Diff 379538.Wed, Oct 13, 3:52 PM

add test for strlcat and check strlen of src in copy string check

aralisza updated this revision to Diff 379567.Wed, Oct 13, 5:35 PM

fix strndup tests

yln accepted this revision.Thu, Oct 14, 2:06 PM

LGTM, thanks!

This revision is now accepted and ready to land.Thu, Oct 14, 2:06 PM

@krytarowski @vitalybuka Can one of you take a look at this since it is shared with non-Darwin platforms?

delcypher accepted this revision.EditedThu, Oct 14, 3:38 PM

LGTM