Page MenuHomePhabricator

[sanitizer] Support sandboxing in sanitizer coverage.
ClosedPublic

Authored by earthdok on May 12 2014, 8:11 AM.

Details

Reviewers
kcc
Summary

Sandboxed code may now pass additional arguments to
__sanitizer_sandbox_on_notify() to force all coverage data to be dumped to a
single file (the default is one file per module). The user may supply a file or
socket to write to. The latter option can be used to broker out the file writing
functionality. If -1 is passed, we pre-open a file.

Diff Detail

Event Timeline

earthdok updated this revision to Diff 9312.May 12 2014, 8:11 AM
earthdok retitled this revision from to [sanitizer] Support sandboxing in sanitizer coverage..
earthdok updated this object.
earthdok edited the test plan for this revision. (Show Details)
earthdok added a reviewer: kcc.
earthdok added a subscriber: Unknown Object (MLST).
earthdok updated this revision to Diff 9313.May 12 2014, 8:14 AM
  • whitespace
kcc edited edge metadata.May 13 2014, 1:05 AM
kcc added a subscriber: glider.

+glider

kcc added a subscriber: timurrrr.May 13 2014, 1:31 AM
kcc added inline comments.
include/sanitizer/common_interface_defs.h
27

use typedef so that you don't need to repeat struct every time

29

I wonder how this is going to work on Windows

30

comments?

lib/sanitizer_common/sanitizer_coverage.cc
78

comments?

glider added inline comments.May 13 2014, 1:47 AM
include/sanitizer/common_interface_defs.h
27

I think we need some "version" parameter to distinguish between different versions of this structure.

30

Should this really be passed through the sandbox arguments? I don't think the clients need to care much.

lib/sanitizer_common/sanitizer_coverage.cc
46

While at it, can you please replace "AS- safe" with "async-signal-safe"?

55

Please use kInvalidFd.

80

This should be u32, like the offsets.
I'd even suggest to define something like module_off_t in this file.

96

I think it's more common to keep the trailing zeroes, YMMV.

105

s/block_bytes/block_pos ?

116

Please define "proper"

160

Are you adding the leading whitespaces on purpose (here and below)? Why?

184

CHECK(cov_fd != kInvalidFd), remove NOLINT.

lib/sanitizer_common/sanitizer_internal_defs.h
98

You shouldn't need to redefine this struct.

lib/sanitizer_common/scripts/sancov.py
48

Two lines between the function declarations, please.

earthdok added inline comments.May 13 2014, 4:56 AM
include/sanitizer/common_interface_defs.h
29

It's hard to speculate in the absense of any users of __sanitizer_sandbox_on_notify() on Windows.

30

We must not send a longer message than the other side is prepared to receive, otherwise the message gets truncated.

lib/sanitizer_common/sanitizer_coverage.cc
80

This is string length, not module data length. I agree the name is confusing, but it follows pre-existing naming in this file. I should probably add "_name" everywhere.

96

I agree. However, dropping the zero makes the code here and in the parser script a bit more straightforward, and I see no other major reasons to pick one over the other.

160

Following the style set by the two pre-existing Report() calls in this file.

lib/sanitizer_common/sanitizer_internal_defs.h
98

Suggestions? We don't include the public header into private headers, or vice versa.

earthdok added inline comments.May 13 2014, 6:48 AM
include/sanitizer/common_interface_defs.h
27

done

30

done

lib/sanitizer_common/sanitizer_coverage.cc
46

done

55

done

78

done

80

done

105

changed naming

116

replaced with an explicit alignment check

184

done

earthdok updated this revision to Diff 9351.May 13 2014, 6:49 AM
earthdok edited edge metadata.
  • address comments

I think we need some "version" parameter to distinguish between different versions of this structure.

This basically only matters when the runtime is out of sync with the header for some reason. Kostya, do you think safeguarding against this case is worth the trouble?

kcc added a comment.May 13 2014, 7:10 AM

Looks reasonable, but I'd like to see a lit test :)

lib/sanitizer_common/sanitizer_coverage.cc
90

use C++ initializer:

kcc added a comment.May 14 2014, 3:54 AM

This basically only matters when the runtime is out of sync with the header for some reason. Kostya, do you think safeguarding against this case is worth the trouble?

I wouldn't worry at this point.

earthdok updated this revision to Diff 9484.May 16 2014, 8:42 AM
  • whitespace
  • address comments
  • use c++ initializer
  • add test

Test is up. I also switched to 'unsigned int' for the blob size, to keep the file format consistent between 32/64 bit. No one is going to send 4 GB messages over the socket anyway.

rnk added a subscriber: rnk.May 16 2014, 10:01 AM
rnk added inline comments.
include/sanitizer/common_interface_defs.h
29

I think we are going to need it, and it will probably look similar, except this will be a HANDLE (aka void*) instead of an int.

When you add Windows support, you could do #ifdef WIN32 to avoid breaking ABI on Linux, but it might be better to use a uptr instead of an int to make it the same.

kcc accepted this revision.May 19 2014, 4:38 AM
kcc edited edge metadata.

LGTM
Great test.

This revision is now accepted and ready to land.May 19 2014, 4:38 AM
earthdok updated this revision to Diff 9530.May 19 2014, 5:25 AM
earthdok edited edge metadata.
  • whitespace
  • address comments
  • use c++ initializer
  • add test
  • do not leave files after test
  • use intptr_t for fd
earthdok closed this revision.May 19 2014, 6:00 AM

r209121