Page MenuHomePhabricator

[msan] Intercept *getxattr and *listxattr.
ClosedPublic

Authored by earthdok on Jan 28 2014, 9:55 AM.

Details

Reviewers
eugenis

Diff Detail

Event Timeline

Please don't add new interceptors to msan_interceptors.cc. That just creates future work to move them to sanitizer_common.

Does this mean I have to test them under ASan as well?

earthdok updated this revision to Unknown Object (????).Jan 28 2014, 10:38 AM
  • move interceptors to common

You need to update tsan_stat.h and tsan_stat.cc. Run check-tsan to verify.

lib/sanitizer_common/sanitizer_common_interceptors.inc
3074

if (path) COMMON_INTERCEPTOR_READ_RANGE(ctx, path, REAL(strlen)(path) + 1);

3076

if (res > 0 && list)

size => res

Dima has removed tsan_stat.*, hasn't he?

earthdok added inline comments.Jan 29 2014, 9:20 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
3076

We should probably fix this also in sanitizer_common_syscalls.inc, then.

earthdok added inline comments.Jan 29 2014, 9:22 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
3076

Also, I'm fairly sure res > 0 implies list != NULL.

Dima has removed tsan_stat.*, hasn't he?

The file is there but it no longer has interceptor stats.

earthdok added inline comments.Jan 29 2014, 9:46 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
3076

Ok, looking at the man page it actually says:

"An empty buffer of _size_ zero can be passed into these calls to return the current size of the list of extended attribute names"

So we should not attempt to unpoison if size == 0. (The way this is formulated still doesn't allow list == 0, but it's probably best to check.)

earthdok updated this revision to Unknown Object (????).Jan 29 2014, 9:55 AM
  • address comments by eugenis; fix sanitizer_common_syscalls.inc

BTW, the code duplication between sanitizer_common_syscalls.inc and sanitizer_common_interceptors.inc is abhorrent. I think we should be able to reuse the PRE and POST hooks from sanitizer_common_syscalls.inc in sanitizer_common_interceptors.inc, since a lot of intercepted functions are simple syscall wrappers. Anyone disagree?

It would be nice to reuse some code, but we must be careful. A lot of syscalls have subtle difference in semantics with libc wrappers, especially in return value.

Looks like your wrappers are still incorrect. As I understood from man pages, these functions return the desired buffer size, which may be larger than allocated size. The wording is not clear, please verify. In that case we need to write max(size, res), or something like that.

In general, we write interceptors defensively, verifying that arguments make sense even if the doc says they must. Man pages are not specs, they are often wrong; and not everyone codes to specs.

It might be a good idea to move checks inside INTERCEPTOR_WRITE_RANGE / INTERCEPTOR_READ_RANGE in another refactoring CL.

In that case we need to write max(size, res), or something like that.

This would attempt to unpoison more memory than we actually have.

I think the wording is clear: if a buffer of zero size is passed, then the desired size is returned. Nothing is written to the buffer, it just tells us what kind of buffer is wanted. So no need to unpoison anything in this case.

eugenis accepted this revision.Jan 30 2014, 2:48 AM

LGTM

lib/msan/lit_tests/Linux/xattr.cc
9

This will fail if libattr is not installed. I think we need to check it either in configure/cmake or in this test, and pass if not found.

lib/sanitizer_common/sanitizer_common_interceptors.inc
3076

Please add a comment here about size==0 being a special case.

earthdok updated this revision to Unknown Object (????).Jan 30 2014, 4:23 AM
  • do not include libxattr headers; add comment

Committed r200464

earthdok closed this revision.Dec 5 2014, 9:37 AM