This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Move recvmsg and recv interceptors to sanitizer_common.
ClosedPublic

Authored by m.ostapenko on Feb 20 2016, 2:59 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

m.ostapenko retitled this revision from to [asan] Add new recv and recvfrom interceptors..
m.ostapenko updated this object.
m.ostapenko added reviewers: kcc, eugenis, samsonov.
m.ostapenko set the repository for this revision to rL LLVM.
m.ostapenko added subscribers: llvm-commits, ygribov.
kcc edited edge metadata.Feb 21 2016, 11:45 PM

I think this should go into sanitizer_common, to also cover tsan.
(msan has one, so merge msan's one into sanitizer_common).
also, would be nice to have a test in test/asan/TestCases/Linux

m.ostapenko retitled this revision from [asan] Add new recv and recvfrom interceptors. to [sanitizer] Move recvmsg and recv interceptors to sanitizer_common..
m.ostapenko updated this object.
m.ostapenko added a reviewer: dvyukov.

OK, let's try more. Moved recvfrom and recv interceptors from TSan and MSan to sanitizer_common. I'm adding Dmitry to reviewers because this change affects TSan. The testcase for ASan may look scary at first glance, but actually it's quite simple:

  1. Create simple UDP server in thread 1 and wait for a message. The size of allocated destination buffer is less then expected message length.
  2. Create simple UDP client in thread 0 and send the message.
  3. Crash in recvfrom function due to buffer overflow.
dvyukov added inline comments.Feb 24 2016, 5:42 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
5352

this is copied from recv, needs to be updated

5354

this var is unused

5357

res > 0

5359

this is different from what msan did
why?

5361

drop this
we add such synchronization lazily when somebody needs it, otherwise we will need to synchronize too much

m.ostapenko added inline comments.Feb 24 2016, 6:09 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
5359

You are right. This is msan specific code, so perhaps we could wrap the whole if to macro (say, COMMON_INTERCEPTOR_UNPOISON_COND) and define it like this:

#define COMMON_INTERCEPTOR_UNPOISON_COND(srcaddr, sz, srcaddr_sz)  \
  do {                                                             \
    if (srcaddr) {                                                 \
      SIZE_T sz = *addrlen;                                        \
        __msan_unpoison(srcaddr, Min(sz, srcaddr_sz));             \
    }                                                              \
  } while (false)

for msan and empty macros for others (just like COMMON_INTERCEPTOR_UNPOISON_PARAM)?

dvyukov added inline comments.Feb 24 2016, 7:00 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
5359

__msan_unpoison(srcaddr, Min(sz, *addrlen)) looks simpler to me

Dmitry, thank you for your nits. Updating the diff.

dvyukov edited edge metadata.Feb 24 2016, 10:12 AM

Otherwise looks good.

lib/sanitizer_common/sanitizer_common_interceptors.inc
5360

I would just do:

if (srcaddr)
    __msan_unpoison(srcaddr, Min(srcaddr_sz, *addrlen));

The macro makes code more convoluted without adding significant value. Every time I read this function I will need to go and check macro definition.

m.ostapenko added inline comments.Feb 25 2016, 12:05 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
5360

Every time I read this function I will need to go and check macro definition.

Agree here, but wouldn't this lead to use of undeclared identifier '__msan_unpoison' errors in {a, t}san_interceptors.cc? I used the macro just to avoid such issues. Perhaps I can avoid introducing a new macro and write something like this:

if (srcaddr)
  COMMON_INTERCEPTOR_INITIALIZE_RANGE(srcaddr, Min(srcaddr_sz, (SIZE_T)*addrlen));
dvyukov added inline comments.Feb 25 2016, 12:13 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
5360

Right.
Don't we already have it? I would expect that we should have something like this already. We already have COMMON_INTERCEPTOR_INITIALIZE_RANGE, maybe that's what you are looking for?
If we don't have it yet, then yes, introduce such macro.

m.ostapenko edited edge metadata.

Use COMMON_INTERCEPTOR_INITIALIZE_RANGE instead of introducing new macro for MSan.

dvyukov accepted this revision.Feb 25 2016, 12:33 AM
dvyukov edited edge metadata.

LGTM
Thanks!

This revision is now accepted and ready to land.Feb 25 2016, 12:33 AM
This revision was automatically updated to reflect the committed changes.
m.ostapenko added inline comments.Feb 25 2016, 6:52 AM
compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc
5343 ↗(On Diff #49020)

I've just noticed, that here and below in recvfrom we should pass res as third parameter, not len. Should I create a new revision on Phabricator?

5359 ↗(On Diff #49020)

And here.

Yes, please.

I've also disabled recvfrom.cc testcase (http://reviews.llvm.org/rL261870) for now due to buildbot failure:

FAIL: AddressSanitizer-i386-linux :: TestCases/Linux/recvfrom.cc (524 of 1166)
******************** TEST 'AddressSanitizer-i386-linux :: TestCases/Linux/recvfrom.cc' FAILED ********************
Script:
--
/mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/clang_build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m32 /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/test/asan/TestCases/Linux/recvfrom.cc -o /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/asan/I386LinuxConfig/TestCases/Linux/Output/recvfrom.cc.tmp && not  /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/asan/I386LinuxConfig/TestCases/Linux/Output/recvfrom.cc.tmp 2>&1 | FileCheck /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/test/asan/TestCases/Linux/recvfrom.cc
--
Exit Code: 2

Command Output (stderr):
--
FileCheck error: '-' is empty.

--
eugenis added inline comments.Feb 25 2016, 11:12 AM
compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc
5343 ↗(On Diff #49020)

I think you can submit such an obvious fix directly.

m.ostapenko added inline comments.Feb 25 2016, 11:24 AM
compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc
5343 ↗(On Diff #49020)

Thanks, I've already done it (approved by Dmitry): http://reviews.llvm.org/D17608.