This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Intercept sem_open/sem_unlink
ClosedPublic

Authored by vitalybuka on Aug 5 2021, 7:50 PM.

Details

Summary

Without interceptor implementation may call strlen on internal
buffers causing false msan errors.

Diff Detail

Event Timeline

vitalybuka requested review of this revision.Aug 5 2021, 7:50 PM
vitalybuka created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2021, 7:50 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
This revision is now accepted and ready to land.Aug 9 2021, 10:14 AM
This revision was landed with ongoing or failed builds.Aug 9 2021, 10:57 AM
This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Aug 9 2021, 11:50 AM
This revision was automatically updated to reflect the committed changes.
ro added a subscriber: ro.Aug 12 2021, 5:27 AM

The new test FAILs on Solaris, and, I believe, rightly so:

For one, there's a warning:

`
compiler-rt/test/sanitizer_common/TestCases/Posix/sem_open.cpp:14:38: warning: format specifies type 'int' but the argument has type 'pid_t' (aka 'long') [-Wformat]
  sprintf(name, "/sem_open_test_%d", getpid());
                                ~~   ^~~~~~~~
                                %ld

pid_t is int for 64-bit complations, but long for 32-bit ones. This can be fixed by printing as %ld and casting the getpid return value to
long to match.

Worse, however, the test execution FAILs:

Assertion failed: sem_close(s2) == 0, file /vol/llvm/src/llvm-project/local/compiler-rt/test/sanitizer_common/TestCases/Posix/sem_open.cpp, line 23

sem_close(s2) succeeds on Linux, but fails on Solaris with EINVAL. Looking closer, s1 and s2 are identical, as expected. On Solaris,
sem_close(s1) frees and deallocates the sem_t, so it has become invalid (unmapped even when looking with gdb) at the time sem_close(s2) is called.

Both Solaris sem_close(3C) and the corresponding XPG7 page state

The effect of subsequent use of the semaphore indicated by sem by this process is undefined.

so this behaviour seems perfectly valid.

In D107615#2941342, @ro wrote:

The new test FAILs on Solaris, and, I believe, rightly so:

For one, there's a warning:

`
compiler-rt/test/sanitizer_common/TestCases/Posix/sem_open.cpp:14:38: warning: format specifies type 'int' but the argument has type 'pid_t' (aka 'long') [-Wformat]
  sprintf(name, "/sem_open_test_%d", getpid());
                                ~~   ^~~~~~~~
                                %ld

pid_t is int for 64-bit complations, but long for 32-bit ones. This can be fixed by printing as %ld and casting the getpid return value to
long to match.

Worse, however, the test execution FAILs:

Assertion failed: sem_close(s2) == 0, file /vol/llvm/src/llvm-project/local/compiler-rt/test/sanitizer_common/TestCases/Posix/sem_open.cpp, line 23

sem_close(s2) succeeds on Linux, but fails on Solaris with EINVAL. Looking closer, s1 and s2 are identical, as expected. On Solaris,
sem_close(s1) frees and deallocates the sem_t, so it has become invalid (unmapped even when looking with gdb) at the time sem_close(s2) is called.

Both Solaris sem_close(3C) and the corresponding XPG7 page state

The effect of subsequent use of the semaphore indicated by sem by this process is undefined.

so this behaviour seems perfectly valid.

There is also

If a process makes multiple successful calls to sem_open() with the same value for name, the same semaphore address is returned for each such successful call, provided that there have been no calls to sem_unlink(3C) for this semaphore.

Opening twice is not essential to the test, so I will remove it and fix type if sprintf.
bf6000dc98df82d9b493e0b5d247538f51d6b9ac

ro added a comment.Aug 13 2021, 5:23 AM

Opening twice is not essential to the test, so I will remove it and fix type if sprintf.
bf6000dc98df82d9b493e0b5d247538f51d6b9ac

Excellent, thanks.