[clang] [clang] Warn on umask() argument bits outside 0777 (PR #198130)

[Aaron Ballman via cfe-commits]

================
@@ -1490,6 +1490,49 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD,
                           << FunctionName << DestinationStr << SourceStr);
 }
 
+void Sema::checkFortifiedLibcArgument(FunctionDecl *FD, CallExpr *TheCall) {
+  if (TheCall->isValueDependent() || TheCall->isTypeDependent() ||
+      isConstantEvaluatedContext())
+    return;
+  if (!FD->isExternC())
+    return;
+  const IdentifierInfo *II = FD->getIdentifier();
+  if (!II)
+    return;
+
+  // umask(mode_t): warn when the constant-evaluated argument has bits set
+  // outside the file-permission mask (0777). Those bits are ignored.
+  // Require a matching system-header declaration to avoid warning on
+  // user-defined lookalikes.
+  auto AnyDeclInSystemHeader = [&](const FunctionDecl *F) {
----------------
AaronBallman wrote:

> The thing that scared me off was our experience with read/write. We added those as LibBuiltins and it regressed real user code, ppl who declare their own file-scope read/write in C suddenly got builtin behavior they didn't ask for, and we had to back it out and dispatch by name instead. So I had a general "don't turn libc functions into builtins, you'll step on someone" fear/rule in my head and applied it to umask without thinking it through.

Hmmm so the builtin logic was not accounting for functions named `read` or `write` with internal linkage and treating them as builtins? Or did these have external linkage (at which point the user has UB because the POSIX library they link against also has external linkage symbols with the same names)? Or something else?

> On the target-dependent prototype (mode_t is 16-bit on some platforms, 32-bit on others):
I'd handle that the same way vfork handles pid_t() today, will mark it IgnoreSignature so Clang recognizes it by name and never tries to match the encoded prototype against the libc's real declaration. No per-libc mode_t spelling logic needed.

I think that's reasonable; we can go the extra mile if we find users have their own `umask` function with a different signature.

> I will prototype the Builtins.td entry and repoint the check through the builtin path, and report back on whether anything in the test suite trips.

SGTM!

> P.S. Sorry for long answer

No worries at all, I appreciate the detailed discussion!


https://github.com/llvm/llvm-project/pull/198130