Page MenuHomePhabricator

[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 ↗(On Diff #48907)

this is copied from recv, needs to be updated

5354 ↗(On Diff #48907)

this var is unused

5357 ↗(On Diff #48907)

res > 0

5359 ↗(On Diff #48907)

this is different from what msan did
why?

5361 ↗(On Diff #48907)

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 ↗(On Diff #48907)

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 ↗(On Diff #48907)

__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
5364 ↗(On Diff #48944)

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
5364 ↗(On Diff #48944)

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
5364 ↗(On Diff #48944)

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

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

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

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

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