diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index f5660f9670eae..df1589aec7d36 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -506,6 +506,11 @@ Improvements to Clang's diagnostics - Added ``-Wattribute-alias`` to diagnose type mismatches between an alias and its aliased function. (#GH195550) +- ``-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 ---------------------------------- diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index d7dd20d6a45e4..e2f7e26d0fcfb 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -971,6 +971,10 @@ def warn_fortify_strlen_overflow: Warning< " but the source string has length %2 (including NUL byte)">, InGroup; +def warn_fortify_umask_unused_bits : Warning< + "'umask' argument has bits outside 0777 (0%0); those bits are ignored">, + InGroup; + 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 5202244cee2a7..cbc2a9aeb895c 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -2972,6 +2972,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 cc834bbee23c4..c2356281a2606 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -1490,6 +1490,52 @@ 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 0777 (those bits are silently discarded by the kernel). + // Require a matching system-header declaration to avoid warning on + // user-defined lookalikes. Do not match the `mode_t` typedef spelling: + // libc headers disagree on whether the prototype uses `mode_t` directly. + auto AnyDeclInSystemHeader = [&](const FunctionDecl *F) { + SourceManager &SM = getSourceManager(); + for (const FunctionDecl *R : F->redecls()) + if (SM.isInSystemHeader(R->getLocation())) + return true; + return false; + }; + if (II->isStr("umask") && !FD->isVariadic() && TheCall->getNumArgs() == 1 && + FD->getNumParams() == 1 && + FD->getParamDecl(0)->getType()->isIntegerType() && + FD->getReturnType()->isIntegerType() && AnyDeclInSystemHeader(FD)) { + 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 5b89236b11824..f74223f9abf8d 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -7368,6 +7368,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-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-glibc.c b/clang/test/Sema/warn-fortify-source-umask-glibc.c new file mode 100644 index 0000000000000..e2df251e8e75d --- /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 fortify gate accepts a glibc-style declaration of umask +// whose prototype is written in terms of the internal __mode_t typedef +// (not mode_t directly). Earlier versions of this check matched on the +// `mode_t` typedef name and silently failed to fire on glibc. + +#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 has bits outside 0777 (01000); those bits are ignored}} + umask(0xFFFF); // expected-warning {{'umask' argument has bits outside 0777 (0177000); those bits are ignored}} + umask(runtime_mode); // no warning, not a constant +} diff --git a/clang/test/Sema/warn-fortify-source-umask-non-libc.c b/clang/test/Sema/warn-fortify-source-umask-non-libc.c new file mode 100644 index 0000000000000..e464024aaf243 --- /dev/null +++ b/clang/test/Sema/warn-fortify-source-umask-non-libc.c @@ -0,0 +1,14 @@ +// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify +// expected-no-diagnostics + +// User-defined umask declared in user code (not a system header). The +// fortify gate requires the resolved function to have at least one +// declaration in a system header, so no -Wfortify-source warning fires. + +extern int umask(int); + +void call_user_umask(void) { + (void)umask(01000); + (void)umask(0xFFFF); + (void)umask(7777); +} diff --git a/clang/test/Sema/warn-fortify-source-umask-non-system.c b/clang/test/Sema/warn-fortify-source-umask-non-system.c new file mode 100644 index 0000000000000..42e53a6dc86ca --- /dev/null +++ b/clang/test/Sema/warn-fortify-source-umask-non-system.c @@ -0,0 +1,17 @@ +// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify +// expected-no-diagnostics + +// User-defined umask whose signature spells the typedef `mode_t` +// (matching libc on systems where the prototype isn't written in terms +// of a libc-internal alias), but the declaration is in user code rather +// than a system header. The fortify gate requires a system-header +// origin, so no warning fires. + +typedef unsigned mode_t; +extern mode_t umask(mode_t cmask); + +void call_user_mode_umask(void) { + (void)umask(01000); + (void)umask(0xFFFF); + (void)umask(7777); +} diff --git a/clang/test/Sema/warn-fortify-source.c b/clang/test/Sema/warn-fortify-source.c index d0b519a516545..4becdfb042b63 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 +// Declare umask via a system header so the libc gate (system-header origin) +// matches without needing the real on every test host. +#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 has bits outside 0777 (01000); those bits are ignored}} + umask(0xFFFF); // expected-warning {{'umask' argument has bits outside 0777 (0177000); those bits are ignored}} + umask(7777); // expected-warning {{'umask' argument has bits outside 0777 (017000); those bits are ignored}} + umask(-1); // expected-warning {{'umask' argument has bits outside 0777 (}} + umask(runtime_mode); // no warning, not a constant +} + #ifdef __cplusplus template struct S { void mf() const {