pipe() is better to be replaced by pipe2() with O_CLOEXEC flag to avoid file descriptor leakage.
Thanks for this!
I don't have great context on tidy, so I can't stamp this, but I do have a few drive-by nits for you.
We probably don't want to commit commented out code
simplicity nit: can this be a std::string?
nit: should probably update this comment
(Do we have a CHECK-FIXES-NOT or CHECK-MESSAGES-NOT to apply to the things below? Or are the CHECKs here meant to be complete like clang's -verify?)
replaceFunc takes a llvm::StringRef as the third parameter. StringRef is "expected to be used in situations where the character data resides in some other buffer, whose lifetime extends past that of the StringRef", which is true in this case, so I think it should be fine.
Thanks for the comments. I have fixed them accordingly.
Based on Testing Checks https://clang.llvm.org/extra/clang-tidy/Contributing.html, CHECK-MASSAGES and CHECK-FIXES are sufficient for clang-tidy checks. I have double checked the tests for similar checks and they don't seem to have additional FileCheck invocations other than these two.
Both should be functionally equivalent in this code. My point was that it's not common to rely on temporary lifetime extension when writing the non-const& type would be equivalent (barring maybe cases with auto, but that's not applicable), and I don't see why we should break with that here.
the message doesn't seem to explain the reason, especially to the people who are not familiar with the pipe API, maybe prefer pipe2() to pipe() to avoid file descriptor leakage is clearer?
Ah, it looks like you are following the existing CloexecCreatCheck check, no need to do it in this patch.
nit: no need to do it explicitly, if a warning is shown unexpectedly, the test will fail.
"prefer pipe2() with O_CLOEXEC to avoid leaking file descriptors to child processes"
No need to change in this patch if you're following an existing pattern, but please do update the message in all checks in a separate patch to explain things better.
"Suggests to replace calls to pipe() with calls to pipe2()."
WDYT about this rewrite?
The check also adds the O_CLOEXEC flag that marks the file descriptor to be closed in child processes. Without this flag a sensitive file descriptor can be leaked to a child process, potentially into a lower-privileged SELinux domain.
Also use a separate code block for the replacement.