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

[wintersteiger]

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;
}

[rengolin]

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...

[wintersteiger]

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!

[rengolin]

In D117793#3260730, @wintersteiger wrote:

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.

[wintersteiger]

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]

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.

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp

 //===-- sanitizer_posix_libcdep.cpp ---------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //

 
 #endif  // SANITIZER_GO
 
 bool IsAccessibleMemoryRange(uptr beg, uptr size) {
   uptr page_size = GetPageSizeCached();
   // Checking too large memory ranges is slow.
   CHECK_LT(size, page_size * 10);
   int sock_pair[2];
+  errno = 0;
   if (pipe(sock_pair))
-    return false;
+    return errno == EMFILE || errno == ENFILE;
   uptr bytes_written =
       internal_write(sock_pair[1], reinterpret_cast<void *>(beg), size);
   int write_errno;
   bool result;
   if (internal_iserror(bytes_written, &write_errno)) {
     CHECK_EQ(EFAULT, write_errno);
     result = false;
   } else {