Page MenuHomePhabricator

Fix for spurious UBSan reports when no file descriptors are available.
Needs RevisionPublic

Authored by wintersteiger on Jan 20 2022, 8:34 AM.

Details

Reviewers
eugenis
vitalybuka
samsonov
Group Reviewers
Restricted Project
Summary

I'm working with a test case that actively exhausts the number of file descriptors to check that our system is resilient to that, and UBSan produces spurious undefined behaviour reports. This is due to the call to pipe in IsAccessibleMemoryRange in sanitizer_posix_libcdep.cpp, which attempts to create a pipe to check whether a particular block of memory is accessible. When there are no more file descriptors, that call will fail (returns -1), but it does not mean that the memory is in fact inaccessible. In these circumstances, I think it's best to give the program under test the benefit of the doubt and not report undefined behaviour. With this patch, IsAccessibleMemoryRange returns true if errno indicates that pipe failed because there are no file descriptors left.

Trivial example:

// Compile with: -fsanitize=undefined -fno-omit-frame-pointer
//
// Run with: bash -c "ulimit -n 4 ; ./ubsan-issue"
// (triggers the issue with -n 4, but not with -n 5; your numbers may vary)
//
// Complaint:
// ubsan-issue.cpp:16:23: runtime error: member call on address 0x000000639c00 which does not point to an object of type 'std::basic_ostream<char>'
// 0x000000639c00: note: object has invalid vptr
// <memory cannot be printed>
// SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ubsan-issue.cpp:16:23 in

int main(void) {
  std::cout << "text" << std::endl;
  return 0;
}

Diff Detail

Event Timeline

wintersteiger requested review of this revision.Jan 20 2022, 8:34 AM
wintersteiger created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2022, 8:34 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
wintersteiger edited the summary of this revision. (Show Details)Jan 20 2022, 8:35 AM
wintersteiger added a reviewer: Restricted Project.Jan 20 2022, 8:39 AM
rengolin added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
278

This just means the pipe failed for a "known" problem, not that the memory range is accessible, which is what the return value means.

If there are no file descriptors left, then this function needs to calculate the accessibility of the memory range in some other way...

That's correct, but without a different way to determine accessibility, we should not report likely false positives. The goal for UBSan is not to report too many false positives, and in the given circumstances, many reports would be false positives. If there's a better way to determine accessibility, I'm all for it though!

That's correct, but without a different way to determine accessibility, we should not report likely false positives. The goal for UBSan is not to report too many false positives, and in the given circumstances, many reports would be false positives.

I think the question here is slightly different: In general, when the sanitizers own infrastructure breaks down (due to internal logic or system error), do we report true or false?

I agree with you that we should report "not-hit" as in "I haven't been able to prove there's a problem".

I think you're right that only EMFILE and ENFILE are likely real world causes for error (EFAULT and EINVAL would be program errors), but if the overall approach is the conservative "only return hit when we can prove it", then I think this patch should be even simpler:

// Avoid false positives on system errors
if (pipe(sock_pair))
  return false;

We could even use the Report function to show the error but I'm not sure how that would play with the other users of the same function (LSAN also uses it).

If there's a better way to determine accessibility, I'm all for it though!

Right, but I think this would be a separate patch.

It would require intimate knowledge of UBSAN to devise a generic and "fast-enough" implementation in case the system runs out of file descriptors.

And it would only be warranted if the sanitizer developers deem important to handle that case. We really want to avoid extra complexity in the sanitizers, which already slow down applications considerably as they are.

Adding Evgenii and Vitaly as reviewers directly, who have recently worked on UBSAN, and Alexey who wrote this code originally.

Sure, if there's a guideline or ongoing discussion about system errors for sanitizers, this should clearly be in line with them. Thanks for adding reviewers directly!

eugenis added inline comments.Jan 21 2022, 10:43 AM
compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
278

Why check errno at all? I don't think *any* error from the pipe() call should be treated as the given memory being inaccessible.

It would be good to print something. Silently failing to detect a bug can be confusing to the user, too. We often do CHECK() in such cases, which is fatal, but it should be fine to keep going in this case - the worst that can happen is we try to access the memory and crash anyway.

vitalybuka requested changes to this revision.May 9 2022, 11:36 AM
This revision now requires changes to proceed.May 9 2022, 11:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 11:36 AM