Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable SYS_close syscall when Thread Sanitizer is active #3112

Conversation

oerdnj
Copy link
Contributor

@oerdnj oerdnj commented Feb 19, 2021

Thread Sanitizer doesn't intercept syscall(SYS_close, fd) that's used
instead of close(fd); on Linux. That leads to false positives as Thread
Sanitizer thinks the descriptor is still being used by the thread.

Disabling the usage of SYS_close won't help in common case where the
libuv is not instrumented, but at least it gives the developers an
option to prepare a Thread Sanitizer compatible build by compiling an
instrumented version.

Some extra notes:

  • Is there a place where to document this?
  • Is the performance benefit (for streams) that high that it justifies the use of SYS_close? I understand the need of syscalls() for interfaces that might not be in libc, but is that really necessary for close()?

@bnoordhuis
Copy link
Member

syscall(SYS_close) is for correctness, not performance. The commit log for 2138dd2 has a bit more detail but, in a nutshell, close() is a cancellation point and therefore a potential resource leak.

@oerdnj oerdnj force-pushed the v1.x-disable-SYS_close-under-thread-sanitizer branch from c08b5a9 to 82c614e Compare February 19, 2021 12:27
@oerdnj
Copy link
Contributor Author

oerdnj commented Feb 19, 2021

@bnoordhuis Thanks for the explanation, somehow I missed the comment at the top of the function.

Meanwhile, after more chat with LLVM/TSAN developer, I changed the commit to use pre- and post- sanitizer wrappers, which has the same effect, but keeps the syscall(SYS_close, ...) instead of calling close(...).

src/unix/core.c Outdated Show resolved Hide resolved
Thread Sanitizer can't intercept syscall(SYS_close, fd) that's used
instead of close(fd); on Linux.  That leads to false positives as Thread
Sanitizer thinks the descriptor is still being used by the thread.

clang defines pre- and post- syscall actions, so wrap the close
syscall() into the action macros.  For gcc, use close() from glibc
instead of the syscall. This allows the thread sanitizer to intercept
closing of the file descriptor when libuv is compiled with Thread
Sanitizer.
@oerdnj oerdnj force-pushed the v1.x-disable-SYS_close-under-thread-sanitizer branch from 82c614e to 9de7e7f Compare February 20, 2021 10:26
@stale
Copy link

stale bot commented Mar 20, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 20, 2021
@oerdnj
Copy link
Contributor Author

oerdnj commented Mar 20, 2021

bump

@stale stale bot removed the stale label Mar 20, 2021
santigimeno pushed a commit that referenced this pull request Apr 4, 2021
Thread Sanitizer can't intercept syscall(SYS_close, fd) that's used
instead of close(fd); on Linux.  That leads to false positives as Thread
Sanitizer thinks the descriptor is still being used by the thread.

clang defines pre- and post- syscall actions, so wrap the close
syscall() into the action macros.  For gcc, use close() from glibc
instead of the syscall. This allows the thread sanitizer to intercept
closing of the file descriptor when libuv is compiled with Thread
Sanitizer.

PR-URL: #3112
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@santigimeno
Copy link
Member

Landed in c3fe3cf. Thanks!

@santigimeno santigimeno closed this Apr 4, 2021
JeffroMF pushed a commit to JeffroMF/libuv that referenced this pull request May 16, 2022
Thread Sanitizer can't intercept syscall(SYS_close, fd) that's used
instead of close(fd); on Linux.  That leads to false positives as Thread
Sanitizer thinks the descriptor is still being used by the thread.

clang defines pre- and post- syscall actions, so wrap the close
syscall() into the action macros.  For gcc, use close() from glibc
instead of the syscall. This allows the thread sanitizer to intercept
closing of the file descriptor when libuv is compiled with Thread
Sanitizer.

PR-URL: libuv#3112
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants