[PATCH] D117793: Fix for spurious UBSan reports when no file descriptors are available.

Fri Jan 21 03:03:05 PST 2022

rengolin added reviewers: eugenis, vitalybuka, samsonov.
rengolin added a comment.

In D117793#3260730 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117793/new/

https://reviews.llvm.org/D117793