This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer_common] Define __sanitizer_FILE on NetBSD
ClosedPublic

Authored by mgorny on Dec 27 2018, 12:08 PM.

Diff Detail

Event Timeline

mgorny created this revision.Dec 27 2018, 12:08 PM
krytarowski added inline comments.Dec 27 2018, 12:19 PM
lib/sanitizer_common/sanitizer_platform_limits_netbsd.cc
2249

Duplicate with L2231

mgorny updated this revision to Diff 179570.Dec 27 2018, 12:22 PM
mgorny marked 2 inline comments as done.
mgorny added inline comments.
lib/sanitizer_common/sanitizer_platform_limits_netbsd.cc
2249

Fixed.

As a subtask please add missing calls to unpoison_file in interceptors (in sanitizer_common_interceptors.inc)

krytarowski added inline comments.Dec 27 2018, 3:09 PM
lib/sanitizer_common/sanitizer_common_interceptors.inc
5698

maybe if (fp->_bf._base && fp->_bf._size)

mgorny marked 2 inline comments as done.Dec 28 2018, 8:09 AM
mgorny added inline comments.
lib/sanitizer_common/sanitizer_common_interceptors.inc
5698

Ok. I'll even take it further, and since _size is signed, check for > 0.

mgorny updated this revision to Diff 179641.Dec 28 2018, 9:16 AM
mgorny marked an inline comment as done.

We've been discussing this, and I think we're doing this the wrong way. Could you help me a little understand this?

In particular, what is the purpose of unpoisoning file? Is it in order to account for stdio functions being implemented inline or as macros, and therefore user code accessing internal FILE members? Or is there some other use case for this?

If only the former, then I think there is no purpose in definining __sanitizer_FILE on NetBSD, as we support only reentrant interfaces which are all implemented as libc routine calls.

If we understand it correctly, unpoision FILE would be only for inlined routines accessing FILE buffer(s) directly. On NetBSD we enforce _REENTRANT for all sanitizers in order to support only _REENTRANT variations of calls that go through libc calls.

There is however an exception with *_unlocked functions that are still inlined (putc_unlocked(), getc_unlocked(),...) and probably for them we still want to keep SANITIZER_HAS_STRUCT_FILE=1. Such interfaces are niche but still used according to https://codesearch.debian.net/

@vitalybuka @kcc @dvyukov @eugenis

Please help to make it clearer.

krytarowski added inline comments.Dec 28 2018, 4:47 PM
lib/tsan/rtl/tsan_interceptors.cc
45

We might want to go for emulating _unlocked:

#define	fileno_unlocked(p)	\
    488     ((p)->_file == -1 ? -1 : (int)(unsigned short)(p)->_file)
mgorny updated this revision to Diff 179681.Dec 29 2018, 2:24 AM
mgorny marked an inline comment as done.

Implemented fileno_unlocked as requested.

We've been discussing this, and I think we're doing this the wrong way. Could you help me a little understand this?

In particular, what is the purpose of unpoisoning file? Is it in order to account for stdio functions being implemented inline or as macros, and therefore user code accessing internal FILE members? Or is there some other use case for this?

If only the former, then I think there is no purpose in definining __sanitizer_FILE on NetBSD, as we support only reentrant interfaces which are all implemented as libc routine calls.

Yes, AFAIR unpoisoning of struct FILE was done specifically to support inlined and _unlocked stdio functions.

krytarowski accepted this revision.Jan 9 2019, 1:26 PM
This revision is now accepted and ready to land.Jan 9 2019, 1:26 PM
This revision was automatically updated to reflect the committed changes.