Author: Denys Fedoryshchenko Date: 2026-06-30T15:52:42Z New Revision: f49ca7997a4976df71a9defd928fbdc95550a783
URL: https://github.com/llvm/llvm-project/commit/f49ca7997a4976df71a9defd928fbdc95550a783 DIFF: https://github.com/llvm/llvm-project/commit/f49ca7997a4976df71a9defd928fbdc95550a783.diff LOG: [clang] Warn on umask() argument bits outside 0777 (#198130) Add a -Wfortify-source warning when the constant-evaluated argument to umask(mode_t) has bits set outside 0777. Those bits are silently discarded by the kernel, so setting them is almost always a typo (0xFFFF, 7777-as-decimal, etc.). Match the corresponding bionic libc diagnose_if check. The Sema-side dispatch is gated to identify the libc declaration of umask and reject user-supplied lookalikes: at least one redeclaration of the resolved function must come from a system header. That is where libc declares umask. Matching on the `mode_t` typedef name is not used because libcs disagree on the spelling: glibc's <sys/types.h> writes the prototype as `__mode_t umask(__mode_t)` via an internal typedef, bionic and musl use `mode_t` directly, and so on. The system-header origin is the portable libc-identity signal; coarser shape checks (name + extern "C" + non-variadic + 1 integer arg + integer return) fill in the remaining filtering. The negative tests cover: * `int umask(int)` in user code, * `typedef unsigned mode_t; extern mode_t umask(mode_t);` in user code (matches the libc typedef name but not the system-header origin). A positive test mirrors glibc's `__mode_t umask(__mode_t)` shape from a `#pragma GCC system_header`-marked Inputs/ header to lock the gate against regressions on the spelling. No Static Analyzer summary is added: modeling umask's argument as a WithinRange precondition would prune feasible caller branches such as `umask(m); if (m > 0777) ...`, even though umask is defined behavior for any input (the kernel masks off the high bits). The Sema check covers the constant-typo case that bionic's diagnose_if targets. Added: clang/test/Sema/Inputs/warn-fortify-source-umask-darwin.h clang/test/Sema/Inputs/warn-fortify-source-umask-glibc.h clang/test/Sema/Inputs/warn-fortify-source-umask.h clang/test/Sema/warn-fortify-source-umask-darwin.c clang/test/Sema/warn-fortify-source-umask-forward-decl.c clang/test/Sema/warn-fortify-source-umask-glibc.c clang/test/Sema/warn-fortify-source-umask-not-builtin.c clang/test/Sema/warn-fortify-source-umask-record-member.c clang/test/Sema/warn-fortify-source-umask-redecl.c Modified: clang/docs/ReleaseNotes.md clang/include/clang/Basic/BuiltinHeaders.def clang/include/clang/Basic/Builtins.td clang/include/clang/Basic/DiagnosticSemaKinds.td clang/include/clang/Sema/Sema.h clang/lib/Sema/SemaChecking.cpp clang/lib/Sema/SemaExpr.cpp clang/test/Sema/warn-fortify-source.c Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index b372e5b58068b..31e7979612a76 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -656,6 +656,11 @@ latest release, please see the [Clang Web Site](https://clang.llvm.org) or the - Diagnostics for the C++11 range-based for statement now report the correct iterator type in notes for invalid iterator types. +- `-Wfortify-source` now warns when the constant-evaluated argument to + `umask` has bits set outside `0777`. Those bits are silently discarded + by the kernel, so setting them is almost always a typo (matching the + bionic libc `diagnose_if` check). + ### Improvements to Clang's time-trace ### Improvements to Coverage Mapping diff --git a/clang/include/clang/Basic/BuiltinHeaders.def b/clang/include/clang/Basic/BuiltinHeaders.def index 23889a22769ed..b18e470a8bd25 100644 --- a/clang/include/clang/Basic/BuiltinHeaders.def +++ b/clang/include/clang/Basic/BuiltinHeaders.def @@ -38,6 +38,7 @@ HEADER(STDIO_H, "stdio.h") HEADER(STDLIB_H, "stdlib.h") HEADER(STRINGS_H, "strings.h") HEADER(STRING_H, "string.h") +HEADER(SYS_STAT_H, "sys/stat.h") HEADER(UNISTD_H, "unistd.h") HEADER(UTILITY, "utility") HEADER(WCHAR_H, "wchar.h") diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td index 87ba4c6c17efd..506f47fb597af 100644 --- a/clang/include/clang/Basic/Builtins.td +++ b/clang/include/clang/Basic/Builtins.td @@ -3860,6 +3860,14 @@ def VFork : LibBuiltin<"unistd.h"> { let Prototype = "pid_t()"; } +// POSIX sys/stat.h + +def Umask : LibBuiltin<"sys/stat.h"> { + let Spellings = ["umask"]; + let Attributes = [IgnoreSignature, NoThrow]; + let Prototype = ""; // mode_t(mode_t); mode_t is target-specific +} + // POSIX pthread.h def PthreadCreate : GNULibBuiltin<"pthread.h"> { diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 7360c9bbab60a..7e4a9338d5b1f 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -978,6 +978,11 @@ def warn_fortify_strlen_overflow: Warning< " but the source string has length %2 (including NUL byte)">, InGroup<FortifySource>; +def warn_fortify_umask_unused_bits : Warning< + "'umask' argument sets non-file-permission bits (0%0); those bits are " + "ignored">, + InGroup<FortifySource>; + def subst_format_overflow : TextSubstitution< "'%0' will always overflow; destination buffer has size %1," " but format string expands to at least %2">; diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 2d606b1551ceb..3b1668df540d2 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -2973,6 +2973,10 @@ class Sema final : public SemaBase { void checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, CallExpr *TheCall); + /// Argument-value fortify checks for libc functions that are not builtins, + /// dispatched by name (e.g. umask). Diagnostics belong to -Wfortify-source. + void checkFortifiedLibcArgument(FunctionDecl *FD, CallExpr *TheCall); + /// Check the arguments to '__builtin_va_start', '__builtin_ms_va_start', /// or '__builtin_c23_va_start' for validity. Emit an error and return true /// on failure; return false on success. diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index d3eb71e1afe88..01b1f4c26f017 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -1571,6 +1571,42 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, << FunctionName << DestinationStr << SourceStr); } +void Sema::checkFortifiedLibcArgument(FunctionDecl *FD, CallExpr *TheCall) { + if (TheCall->isValueDependent() || TheCall->isTypeDependent()) + return; + + // Recognize the libc function by builtin identity rather than by name and + // system-header origin. umask is a LibBuiltin marked IgnoreSignature, so the + // builtin id is attached to any file-scope, C-linkage declaration of umask + // regardless of the libc's mode_t spelling -- including a hand-written + // forward declaration without <sys/stat.h>. A static/local lookalike or a + // C++ (non-extern-"C") declaration keeps a zero builtin id and is ignored. + if (FD->getBuiltinID() != Builtin::BIumask) + return; + + // umask(mode_t): warn when the constant-evaluated argument has bits set + // outside the file-permission mask (0777). Those bits are ignored. + if (TheCall->getNumArgs() != 1) + return; + Expr *Arg = TheCall->getArg(0); + if (!Arg->getType()->isIntegerType()) + return; + Expr::EvalResult R; + if (!Arg->EvaluateAsInt(R, getASTContext())) + return; + // Operate on the raw two's-complement bit pattern so that negative literals + // (which convert to large unsigned mode_t values) are caught. + llvm::APInt RawValue = R.Val.getInt(); + llvm::APInt Mask(RawValue.getBitWidth(), 0777); + llvm::APInt Extra = RawValue & ~Mask; + if (Extra == 0) + return; + SmallString<16> ExtraStr; + Extra.toString(ExtraStr, /*Radix=*/8, /*Signed=*/false); + Diag(TheCall->getBeginLoc(), diag::warn_fortify_umask_unused_bits) + << ExtraStr; +} + static bool BuiltinSEHScopeCheck(Sema &SemaRef, CallExpr *TheCall, Scope::ScopeFlags NeededScopeFlags, unsigned DiagID) { diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 0cd95ec311218..237ab0ad7c3f6 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -7394,6 +7394,7 @@ ExprResult Sema::BuildResolvedCallExpr(Expr *Fn, NamedDecl *NDecl, return ExprError(); checkFortifiedBuiltinMemoryFunction(FDecl, TheCall); + checkFortifiedLibcArgument(FDecl, TheCall); if (BuiltinID) return CheckBuiltinFunctionCall(FDecl, BuiltinID, TheCall); diff --git a/clang/test/Sema/Inputs/warn-fortify-source-umask-darwin.h b/clang/test/Sema/Inputs/warn-fortify-source-umask-darwin.h new file mode 100644 index 0000000000000..878706c4ca3d1 --- /dev/null +++ b/clang/test/Sema/Inputs/warn-fortify-source-umask-darwin.h @@ -0,0 +1,17 @@ +#pragma GCC system_header + +// Darwin and the BSDs spell mode_t as a 16-bit type (__uint16_t), unlike +// glibc's unsigned int. umask is recognized by builtin name with no prototype +// to match, so the width the libc picks for mode_t does not matter. + +#ifdef __cplusplus +extern "C" { +#endif + +typedef unsigned short __uint16_t; +typedef __uint16_t mode_t; +mode_t umask(mode_t); + +#ifdef __cplusplus +} +#endif diff --git a/clang/test/Sema/Inputs/warn-fortify-source-umask-glibc.h b/clang/test/Sema/Inputs/warn-fortify-source-umask-glibc.h new file mode 100644 index 0000000000000..40071c2982c8a --- /dev/null +++ b/clang/test/Sema/Inputs/warn-fortify-source-umask-glibc.h @@ -0,0 +1,19 @@ +#pragma GCC system_header + +// Mimic glibc's typedef chain: the prototype is written in terms of +// __mode_t, an internal typedef that itself aliases unsigned int. +// `mode_t` is then defined as an alias of __mode_t. This shape would +// reject any check that walks one layer of typedef sugar and matches +// on the typedef name `mode_t`. + +#ifdef __cplusplus +extern "C" { +#endif + +typedef unsigned int __mode_t; +typedef __mode_t mode_t; +extern __mode_t umask(__mode_t __mask); + +#ifdef __cplusplus +} +#endif diff --git a/clang/test/Sema/Inputs/warn-fortify-source-umask.h b/clang/test/Sema/Inputs/warn-fortify-source-umask.h new file mode 100644 index 0000000000000..620ea723a072d --- /dev/null +++ b/clang/test/Sema/Inputs/warn-fortify-source-umask.h @@ -0,0 +1,12 @@ +#pragma GCC system_header + +#ifdef __cplusplus +extern "C" { +#endif + +typedef unsigned mode_t; +mode_t umask(mode_t cmask); + +#ifdef __cplusplus +} +#endif diff --git a/clang/test/Sema/warn-fortify-source-umask-darwin.c b/clang/test/Sema/warn-fortify-source-umask-darwin.c new file mode 100644 index 0000000000000..22df37517d6e8 --- /dev/null +++ b/clang/test/Sema/warn-fortify-source-umask-darwin.c @@ -0,0 +1,19 @@ +// RUN: %clang_cc1 -triple arm64-apple-macosx11.0 %s -verify + +// Regression test for the macOS/BSD case. There mode_t is a 16-bit type, so an +// earlier attempt that gave umask a fixed unsigned-int builtin prototype turned +// the libc's own <sys/stat.h> declaration into an incompatible library +// redeclaration and silently dropped its builtin identity -- the fortify check +// went dead, even for the system header. umask now has no prototype to match +// (it is recognized purely by name), so the check fires regardless of how wide +// the libc makes mode_t. + +#include "Inputs/warn-fortify-source-umask-darwin.h" + +void call_umask_darwin(mode_t runtime_mode) { + umask(0); + umask(0644); + umask(01000); // expected-warning {{'umask' argument sets non-file-permission bits (01000); those bits are ignored}} + umask(0xFFFF); // expected-warning {{'umask' argument sets non-file-permission bits (0177000); those bits are ignored}} + umask(runtime_mode); // no warning, not a constant +} diff --git a/clang/test/Sema/warn-fortify-source-umask-forward-decl.c b/clang/test/Sema/warn-fortify-source-umask-forward-decl.c new file mode 100644 index 0000000000000..f3df651d07cb0 --- /dev/null +++ b/clang/test/Sema/warn-fortify-source-umask-forward-decl.c @@ -0,0 +1,19 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu %s -verify + +// umask forward-declared in user code without including <sys/stat.h>. Because +// umask is a LibBuiltin recognized by name (not by system-header origin), the +// fortify check still fires here -- this is the case the earlier system-header +// gate went silent on. umask has no prototype to match against, so the +// declaration is never reported as an incompatible library redeclaration +// regardless of how this target spells mode_t. + +typedef unsigned mode_t; +mode_t umask(mode_t); + +void call_user_umask(mode_t runtime_mode) { + umask(0); + umask(0644); + umask(01000); // expected-warning {{'umask' argument sets non-file-permission bits (01000); those bits are ignored}} + umask(0xFFFF); // expected-warning {{'umask' argument sets non-file-permission bits (0177000); those bits are ignored}} + umask(runtime_mode); // no warning, not a constant +} diff --git a/clang/test/Sema/warn-fortify-source-umask-glibc.c b/clang/test/Sema/warn-fortify-source-umask-glibc.c new file mode 100644 index 0000000000000..6d5d4dbab79c5 --- /dev/null +++ b/clang/test/Sema/warn-fortify-source-umask-glibc.c @@ -0,0 +1,16 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu %s -verify + +// Verify the builtin-based fortify check fires on a glibc-style declaration +// of umask whose prototype is written in terms of the internal __mode_t +// typedef (= unsigned int) rather than mode_t directly. Recognition is by +// builtin name, so the libc's particular mode_t spelling does not matter. + +#include "Inputs/warn-fortify-source-umask-glibc.h" + +void call_umask_glibc(mode_t runtime_mode) { + umask(0); + umask(0644); + umask(01000); // expected-warning {{'umask' argument sets non-file-permission bits (01000); those bits are ignored}} + umask(0xFFFF); // expected-warning {{'umask' argument sets non-file-permission bits (0177000); those bits are ignored}} + umask(runtime_mode); // no warning, not a constant +} diff --git a/clang/test/Sema/warn-fortify-source-umask-not-builtin.c b/clang/test/Sema/warn-fortify-source-umask-not-builtin.c new file mode 100644 index 0000000000000..57e3706a5f1b1 --- /dev/null +++ b/clang/test/Sema/warn-fortify-source-umask-not-builtin.c @@ -0,0 +1,21 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -x c %s -verify +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -x c++ %s -verify +// expected-no-diagnostics + +// Declarations that are not the POSIX umask keep a zero builtin id, so the +// fortify check is skipped even for out-of-range constants: +// * a file-local umask with internal linkage; +// * in C++, a umask without C language linkage (e.g. a user's own overload). + +static int umask(int m) { return m; } + +void call_static_umask(void) { + (void)umask(0xFFFF); +} + +#ifdef __cplusplus +namespace user { +int umask(int); +void call(void) { (void)umask(0xFFFF); } +} // namespace user +#endif diff --git a/clang/test/Sema/warn-fortify-source-umask-record-member.c b/clang/test/Sema/warn-fortify-source-umask-record-member.c new file mode 100644 index 0000000000000..ca1a36d1b3bc2 --- /dev/null +++ b/clang/test/Sema/warn-fortify-source-umask-record-member.c @@ -0,0 +1,15 @@ +// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify + +typedef unsigned mode_t; + +// A struct member with function type is rejected at ActOnField and becomes +// a FieldDecl, not a record-context FunctionDecl, so the call below never +// reaches the fortify gate. Regression test that this error-recovery path +// does not crash (would otherwise risk the assert in Decl.cpp isDeclExternC). +struct S { + mode_t umask(mode_t); // expected-error {{field 'umask' declared as a function}} +}; + +void call_member_umask(struct S *s) { + (void)s->umask(0xFFFF); +} diff --git a/clang/test/Sema/warn-fortify-source-umask-redecl.c b/clang/test/Sema/warn-fortify-source-umask-redecl.c new file mode 100644 index 0000000000000..c125a6a17936e --- /dev/null +++ b/clang/test/Sema/warn-fortify-source-umask-redecl.c @@ -0,0 +1,16 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu %s -verify + +// A user declaration of umask whose integer type diff ers from the historical +// unsigned-int spelling -- here a plain int, as in a K&R-era or sloppy +// prototype -- is still recognized as the libc umask and checked. Because +// umask is a LibBuiltin with no prototype to match, the declaration is not +// flagged as an incompatible library redeclaration (it would have been when +// the builtin carried a fixed unsigned-int prototype), and the fortify check +// still fires on an out-of-range constant. + +extern int umask(int); + +void call_user_umask(void) { + umask(0644); + umask(0xFFFF); // expected-warning {{'umask' argument sets non-file-permission bits (0177000); those bits are ignored}} +} diff --git a/clang/test/Sema/warn-fortify-source.c b/clang/test/Sema/warn-fortify-source.c index 55a54b46a1175..0a6c44f59af9e 100644 --- a/clang/test/Sema/warn-fortify-source.c +++ b/clang/test/Sema/warn-fortify-source.c @@ -28,6 +28,10 @@ void bzero(void *dst, size_t n); } #endif +// umask is recognized as a builtin by name; this header just supplies a +// realistic system-header declaration and the mode_t typedef used below. +#include "Inputs/warn-fortify-source-umask.h" + void call_memcpy(void) { char dst[10]; char src[20]; @@ -242,6 +246,18 @@ void call_sprintf(void) { sprintf(buf, "5%.1e", 9.f); // expected-warning {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 8}} } +void call_umask(mode_t runtime_mode) { + umask(0); + umask(022); + umask(0644); + umask(0777); + umask(01000); // expected-warning {{'umask' argument sets non-file-permission bits (01000); those bits are ignored}} + umask(0xFFFF); // expected-warning {{'umask' argument sets non-file-permission bits (0177000); those bits are ignored}} + umask(7777); // expected-warning {{'umask' argument sets non-file-permission bits (017000); those bits are ignored}} + umask(-1); // expected-warning {{'umask' argument sets non-file-permission bits (}} + umask(runtime_mode); // no warning, not a constant +} + #ifdef __cplusplus template <class> struct S { void mf() const { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
