Page MenuHomePhabricator

sanitizer_common: Introduce internal_madvise and start using it.
ClosedPublic

Authored by pcc on Aug 12 2020, 7:56 PM.

Details

Summary

A recent change to sanitizer_common caused us to issue the syscall
madvise(MADV_HUGEPAGE) during HWASAN initialization. This may lead to a
problem if madvise is instrumented (e.g. because libc is instrumented
or the user intercepted it). For example, on Android the syscall may
fail if the kernel does not support transparent hugepages, which leads
to an attempt to set errno in a HWASAN instrumented function. Avoid
this problem by introducing a syscall wrapper and using it to issue
this syscall.

Tested only on Linux; includes untested updates for the other
platforms.

Diff Detail

Event Timeline

pcc created this revision.Aug 12 2020, 7:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2020, 7:56 PM
Herald added subscribers: Restricted Project, fedor.sergeev. · View Herald Transcript
pcc requested review of this revision.Aug 12 2020, 7:56 PM
This revision is now accepted and ready to land.Aug 13 2020, 12:36 PM
This revision was landed with ongoing or failed builds.Aug 13 2020, 1:10 PM
This revision was automatically updated to reflect the committed changes.
ro added a subscriber: ro.Aug 13 2020, 1:49 PM
ro added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_solaris.cpp
79

I don't know why but you reintroduced an older/broken version of the madvise declaration here. The one you removed from sanitizer_posix_libcdep.cpp had the first arg as void * for a reason.

pcc added inline comments.Aug 13 2020, 4:14 PM
compiler-rt/lib/sanitizer_common/sanitizer_solaris.cpp
79

Oh, I thought that was a mistake and that madvise was supposed to be defined like this on Solaris (according to the comment that I removed on sanitizer_posix_libcdep.cpp lines 70-72, and man pages that I found online, e.g. https://docs.oracle.com/cd/E36784_01/html/E36874/madvise-3c.html). If it's actually defined to take void * on Solaris then I can fix it.

ro added inline comments.Aug 18 2020, 5:03 AM
compiler-rt/lib/sanitizer_common/sanitizer_solaris.cpp
79

On the contrary: I had provided a code snippet using `void *', carefully tested on both Solaris 11.4 and Illumos in D84046. Unfortunately, the author of that patch chose to make the same change as you did, silently and at the last minute and breaking the Solaris build ;-(

I had to restore the build in D84388 and thus was particularly dismayed to see the same change going in again for no apparent reason and without testing.

Strangely, 1-stage builds with gcc 9 or 10 still do work, as can be seen on the Solaris buildbots, but when I tried a 2-stage build last night, it failed again as it had before:

/vol/llvm/src/llvm-project/dist/compiler-rt/lib/sanitizer_common/sanitizer_solaris.cpp:79:16: error: conflicting types for 'madvise'
extern "C" int madvise(caddr_t, size_t, int);
               ^
/usr/include/sys/mman.h:232:12: note: previous declaration is here
extern int madvise(void *, size_t, int);
           ^
1 error generated.

The man page you link is from Solaris 11.2, about 8 years ago. The signature has been changed in 11.4 exactly to avoid unnecessary incompatibilities with other systems.

So yes, please restore the void * version.

pcc added inline comments.Aug 18 2020, 1:11 PM
compiler-rt/lib/sanitizer_common/sanitizer_solaris.cpp
79

Okay, I sent D86166 to fix this.

I have a followup question which I will raise there, since I think the comment really needs to say *why* the declaration is this way, otherwise someone else may come along and "fix" it again.

ro added inline comments.Aug 18 2020, 1:41 PM
compiler-rt/lib/sanitizer_common/sanitizer_solaris.cpp
79

Okay, I sent D86166 to fix this.

I have a followup question which I will raise there, since I think the comment really needs to say *why* the declaration is this way, otherwise someone else may come along and "fix" it again.

But why on earth do people insist on changing/breaking working code on platforms that by their own admission they know nothing about instead of first asking someone that does for clarification? That's completely beyond me!

I do run Solaris buildbots for a reason.