llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Denys Fedoryshchenko (nuclearcat) <details> <summary>Changes</summary> Adds buffer-size diagnostics for `read`, `write`, `pread`, `pread64`, `pwrite`, `pwrite64`, `readlink`, `readlinkat`, and `getcwd` when the buffer size and byte count are known at compile time. The checks use `FortifiedBufferChecker`: reads into a buffer use the existing destination-size diagnostic, while `write` and `pwrite` use the source-overread check. Functions are matched by name and prototype, allowing libc differences in `ssize_t` and `off_t` while excluding incompatible declarations and static helpers. Split out of #<!-- -->196499 so the Sema changes can be reviewed independently of the Static Analyzer changes. Based on Colin Kinloch's work in #<!-- -->161737. Part of #<!-- -->142230. Tested with an assertions-enabled Clang build: all 17 Sema tests selected by `fortify|memcpy|object-size|stringop-overread` passed. --- Full diff: https://github.com/llvm/llvm-project/pull/224979.diff 6 Files Affected: - (modified) clang/docs/ReleaseNotes.md (+5) - (modified) clang/lib/Sema/SemaChecking.cpp (+151-2) - (added) clang/test/Sema/warn-fortify-source-prototype-gate.c (+81) - (added) clang/test/Sema/warn-fortify-source-signed-count.c (+17) - (added) clang/test/Sema/warn-fortify-source-typedef.c (+18) - (modified) clang/test/Sema/warn-fortify-source.c (+68) ``````````diff diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index 7e3e8468914c7..d7bbed8bcb5b7 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -284,6 +284,11 @@ features cannot lower the translation-unit ABI level; ### Improvements to Clang's diagnostics +- `-Wfortify-source` now diagnoses destination buffer-size mismatches in calls to + `read`, `pread`, `pread64`, `readlink`, `readlinkat`, and `getcwd` when the buffer + size and byte count are statically known. `-Wstringop-overread` diagnoses source + buffer over-reads in `write`, `pwrite`, and `pwrite64`. + - `-Wfortify-source` now diagnoses when `strlcat`, `__builtin_strlcat`, `strlcpy`, or `__builtin_strlcpy` is called with a size argument larger than the destination buffer. diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index ea8f3babcb0e0..b5b8a0aaecf75 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -1299,8 +1299,104 @@ class FortifiedBufferChecker { const DiagnoseAsBuiltinAttr *DABAttr; unsigned SizeTypeWidth; }; + +// read/write/readlink/readlinkat/getcwd and pread/pwrite are deliberately not +// Clang builtins: the former are common identifiers whose builtin declarations +// would clash with unrelated user code, and pread/pwrite use off_t, whose +// width is target- and _FILE_OFFSET_BITS-dependent. They are matched by name +// against the POSIX prototype below instead. +struct LibcFuncDesc { + StringRef Name; + QualType Return; + SmallVector<QualType, 4> Params; + // Index of an off_t offset parameter, matched as any signed integer; + // std::nullopt if none. See checkFortifiedBuiltinMemoryFunction. + std::optional<unsigned> OffsetParamIdx; + unsigned BufIdx; + unsigned CountIdx; + // True if CountIdx bytes are read from BufIdx (source over-read, like write); + // false if written into BufIdx (destination mismatch, like read). + bool BufIsSource; +}; } // anonymous namespace +static std::optional<LibcFuncDesc> lookupLibCFunctionDesc(ASTContext &Ctx, + StringRef Name) { + QualType IntTy = Ctx.IntTy; + QualType SizeTy = Ctx.getSizeType(); + QualType VoidPtrTy = Ctx.VoidPtrTy; + QualType ConstVoidPtrTy = Ctx.getPointerType(Ctx.VoidTy.withConst()); + QualType CharPtrTy = Ctx.getPointerType(Ctx.CharTy); + QualType ConstCharPtrTy = Ctx.getPointerType(Ctx.CharTy.withConst()); + // Placeholders: SSizeTy marks an ssize_t return, OffTy an off_t offset; both + // are matched structurally rather than by exact type (see the gate). + QualType SSizeTy = IntTy; + QualType OffTy = IntTy; + + llvm::DenseMap<StringRef, LibcFuncDesc> Table; + Table.insert_range(std::initializer_list<std::pair<StringRef, LibcFuncDesc>>{ + {"getcwd", + {"getcwd", CharPtrTy, {CharPtrTy, SizeTy}, std::nullopt, 0, 1, false}}, + {"read", + {"read", + SSizeTy, + {IntTy, VoidPtrTy, SizeTy}, + std::nullopt, + 1, + 2, + false}}, + {"write", + {"write", + SSizeTy, + {IntTy, ConstVoidPtrTy, SizeTy}, + std::nullopt, + 1, + 2, + true}}, + {"readlink", + {"readlink", + SSizeTy, + {ConstCharPtrTy, CharPtrTy, SizeTy}, + std::nullopt, + 1, + 2, + false}}, + {"readlinkat", + {"readlinkat", + SSizeTy, + {IntTy, ConstCharPtrTy, CharPtrTy, SizeTy}, + std::nullopt, + 2, + 3, + false}}, + {"pread", + {"pread", SSizeTy, {IntTy, VoidPtrTy, SizeTy, OffTy}, 3, 1, 2, false}}, + {"pread64", + {"pread64", SSizeTy, {IntTy, VoidPtrTy, SizeTy, OffTy}, 3, 1, 2, false}}, + {"pwrite", + {"pwrite", + SSizeTy, + {IntTy, ConstVoidPtrTy, SizeTy, OffTy}, + 3, + 1, + 2, + true}}, + {"pwrite64", + {"pwrite64", + SSizeTy, + {IntTy, ConstVoidPtrTy, SizeTy, OffTy}, + 3, + 1, + 2, + true}}, + }); + + auto It = Table.find(Name); + if (It == Table.end()) + return std::nullopt; + return It->second; +} + void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, CallExpr *TheCall) { if (TheCall->isInstantiationDependent() || isConstantEvaluatedContext()) @@ -1309,7 +1405,46 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, FortifiedBufferChecker Checker(*this, FD, TheCall); unsigned BuiltinID = Checker.getBuiltinID(); - if (!BuiltinID) + + // Match the non-builtin libc I/O functions (see LibcFuncDesc) by name. Only + // external-linkage C declarations qualify: a static same-name helper is + // file-local and not the libc function. + std::optional<LibcFuncDesc> LibCMatch; + if (!BuiltinID && FD->isExternC() && FD->hasExternalFormalLinkage() && + FD->getIdentifier() && !FD->isVariadic()) { + ASTContext &Ctx = getASTContext(); + if (auto Desc = + lookupLibCFunctionDesc(Ctx, FD->getIdentifier()->getName())) { + // ssize_t is a signed integer of size_t width whose spelling varies + // across libcs, so a non-pointer return is matched structurally (getcwd, + // the only pointer-returning entry, matches exactly). The off_t offset + // matches any signed integer (its width varies); the int fd, pointer + // buffer, and size_t count must match exactly -- pinning identity and + // keeping a signed count out of the (asserted-unsigned) size-argument + // evaluation. + auto IsSSizeT = [&](QualType T) { + return T->isSignedIntegerType() && + Ctx.getTypeSize(T) == Ctx.getTypeSize(Ctx.getSizeType()); + }; + QualType RetTy = FD->getReturnType(); + bool Matches = FD->getNumParams() == Desc->Params.size() && + TheCall->getNumArgs() == Desc->Params.size() && + (Desc->Return->isPointerType() + ? Ctx.hasSameUnqualifiedType(Desc->Return, RetTy) + : IsSSizeT(RetTy)); + for (unsigned I = 0; Matches && I < Desc->Params.size(); ++I) { + QualType ActualTy = FD->getParamDecl(I)->getType(); + if (Desc->OffsetParamIdx == I) + Matches = ActualTy->isSignedIntegerType(); + else + Matches = Ctx.hasSameUnqualifiedType(Desc->Params[I], ActualTy); + } + if (Matches) + LibCMatch = std::move(*Desc); + } + } + + if (!BuiltinID && !LibCMatch) return; unsigned SizeTypeWidth = Checker.getSizeTypeWidth(); @@ -1318,9 +1453,22 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, std::optional<llvm::APSInt> DestinationSize; unsigned DiagID = 0; + if (LibCMatch) { + if (LibCMatch->BufIsSource) { + // write/pwrite: a source over-read, same as the memcpy family below. + Checker.checkSourceOverread(LibCMatch->BufIdx, LibCMatch->CountIdx); + return; + } + DiagID = diag::warn_fortify_source_size_mismatch; + SourceSize = Checker.ComputeExplicitObjectSizeArgument(LibCMatch->CountIdx); + DestinationSize = Checker.ComputeSizeArgument(LibCMatch->BufIdx); + } + + // For a LibCMatch the sizes are already set above and BuiltinID is zero, so + // the switch falls through its default to the shared size check below. switch (BuiltinID) { default: - return; + break; case Builtin::BI__builtin_strcat: case Builtin::BIstrcat: case Builtin::BI__builtin_stpcpy: @@ -1557,6 +1705,7 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, const Expr *Dest = TheCall->getArg(0)->IgnoreCasts(); IdentifierInfo *FnInfo = FD->getIdentifier(); CheckSizeofMemaccessArgument(LenArg, Dest, FnInfo); + break; } } diff --git a/clang/test/Sema/warn-fortify-source-prototype-gate.c b/clang/test/Sema/warn-fortify-source-prototype-gate.c new file mode 100644 index 0000000000000..75e9d9897ef14 --- /dev/null +++ b/clang/test/Sema/warn-fortify-source-prototype-gate.c @@ -0,0 +1,81 @@ +// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -Werror + +// The fortify dispatch for read/write/pread/pwrite/readlink/readlinkat/getcwd +// is gated on the full POSIX prototype: a function sharing one of these names +// but with a different signature must not be diagnosed as the libc function. + +typedef unsigned long size_t; +typedef long ssize_t; + +// expected-no-diagnostics + +// Variadic: prefix happens to match POSIX read, but the varargs make this +// an unrelated declaration. +ssize_t read(int, void *, size_t, ...); + +void test_read_variadic(void) { + char b[4]; + read(0, b, 8); +} + +// Unsigned return (size_t): only the return differs from POSIX write (which +// returns signed ssize_t), isolating the return-type gate. +size_t write(int fd, const void *buf, size_t n); + +void test_write_unsigned_return(void) { + char buf[4]; + write(0, buf, 8); +} + +// Wrong return type (void) for readlink. +void readlink(const char *p, char *b, size_t n); + +void test_readlink_wrong_return(void) { + char b[4]; + readlink("/", b, 8); +} + +// Wrong second parameter type (int instead of char*) for getcwd. +char *getcwd(int x, size_t n); + +void test_getcwd_wrong_param(void) { + getcwd(42, 8); +} + +// pread64 with 'char *' buffer instead of POSIX 'void *'. Newlib-style +// syscall stubs commonly use this shape; the pointer-type mismatch trips +// the prototype gate. +int pread64(int, char *, size_t, long long); + +void test_pread64_newlib_buffer(void) { + char b[4]; + pread64(0, b, 8, 0); +} + +// pwrite64 with 'const char *' instead of POSIX 'const void *': same shape. +int pwrite64(int, const char *, size_t, long long); + +void test_pwrite64_newlib_buffer(void) { + char b[4]; + pwrite64(0, b, 8, 0); +} + +// Unsigned offset: off_t/off64_t are signed, so only the offset slot differs +// from POSIX pread. It is matched as a signed integer, so this is rejected. +ssize_t pread(int, void *, size_t, unsigned long); + +void test_pread_unsigned_offset(void) { + char b[4]; + pread(0, b, 8, 0); +} + +// static (internal-linkage) lookalike with the exact POSIX signature: it is +// file-local, not the libc function, so it must not be diagnosed. +static ssize_t pwrite(int fd, const void *buf, size_t n, long off) { + return 0; +} + +void test_pwrite_static_lookalike(void) { + char b[4]; + pwrite(0, b, 8, 0); +} diff --git a/clang/test/Sema/warn-fortify-source-signed-count.c b/clang/test/Sema/warn-fortify-source-signed-count.c new file mode 100644 index 0000000000000..e67675621261e --- /dev/null +++ b/clang/test/Sema/warn-fortify-source-signed-count.c @@ -0,0 +1,17 @@ +// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -Werror + +// A read() whose count parameter is a signed 'int' rather than POSIX size_t +// must not be treated as the libc read(): the size_t count slot is matched +// exactly, so this lookalike is not diagnosed. + +typedef unsigned long size_t; +typedef long ssize_t; + +// expected-no-diagnostics + +ssize_t read(int fd, void *buf, int count); + +void test_read_signed_count(void) { + char b[4]; + read(0, b, 8); +} diff --git a/clang/test/Sema/warn-fortify-source-typedef.c b/clang/test/Sema/warn-fortify-source-typedef.c new file mode 100644 index 0000000000000..af7c7565d7658 --- /dev/null +++ b/clang/test/Sema/warn-fortify-source-typedef.c @@ -0,0 +1,18 @@ +// RUN: %clang_cc1 -triple i686-unknown-linux %s -verify +// RUN: %clang_cc1 -triple x86_64-unknown-linux %s -verify + +// Verify that the fortify dispatch is tolerant of libc typedef choices for +// ssize_t. glibc uses `long` while other libcs (musl, bionic) may use a +// type that matches Clang's signed counterpart of size_t. On ILP32 these +// differ canonically (`long` vs `int`) even though both are 32-bit signed +// integers; the gate must accept either as ssize_t. + +typedef __SIZE_TYPE__ size_t; +typedef long ssize_t; + +ssize_t read(int, void *, size_t); + +void test_read(void) { + char b[4]; + read(0, b, 8); // expected-warning {{'read' size argument is too large; destination buffer has size 4, but size argument is 8}} +} diff --git a/clang/test/Sema/warn-fortify-source.c b/clang/test/Sema/warn-fortify-source.c index 8339efddc5b3e..735570725657c 100644 --- a/clang/test/Sema/warn-fortify-source.c +++ b/clang/test/Sema/warn-fortify-source.c @@ -24,6 +24,19 @@ void *memcpy(void *dst, const void *src, size_t c); void bcopy(const void *src, void *dst, size_t n); void bzero(void *dst, size_t n); +typedef long ssize_t; +typedef long off_t; +typedef long long off64_t; +ssize_t read(int fd, void *buf, size_t count); +ssize_t write(int fd, const void *buf, size_t count); +ssize_t pread(int fd, void *buf, size_t count, off_t offset); +ssize_t pread64(int fd, void *buf, size_t count, off64_t offset); +ssize_t pwrite(int fd, const void *buf, size_t count, off_t offset); +ssize_t pwrite64(int fd, const void *buf, size_t count, off64_t offset); +char *getcwd(char *buf, size_t size); +ssize_t readlink(const char *path, char *buf, size_t bufsize); +ssize_t readlinkat(int fd, const char *path, char *buf, size_t bufsize); + #ifdef __cplusplus } #endif @@ -132,6 +145,61 @@ void call_bcopy_bzero(void) { __builtin_bzero(dst, 11); // expected-warning {{'bzero' will always overflow; destination buffer has size 10, but size argument is 11}} } +void call_read(void) { + char buf[10]; + read(0, buf, 10); + read(0, buf, 20); // expected-warning {{'read' size argument is too large; destination buffer has size 10, but size argument is 20}} +} + +void call_pread(void) { + char buf[10]; + pread(0, buf, 10, 0); + pread(0, buf, 20, 0); // expected-warning {{'pread' size argument is too large; destination buffer has size 10, but size argument is 20}} +} + +void call_pread64(void) { + char buf[10]; + pread64(0, buf, 10, 0); + pread64(0, buf, 20, 0); // expected-warning {{'pread64' size argument is too large; destination buffer has size 10, but size argument is 20}} +} + +void call_write(void) { + char buf[10]; + write(0, buf, 10); + write(0, buf, 20); // expected-warning {{'write' reading 20 bytes from a region of size 10}} +} + +void call_pwrite(void) { + char buf[10]; + pwrite(0, buf, 10, 0); + pwrite(0, buf, 20, 0); // expected-warning {{'pwrite' reading 20 bytes from a region of size 10}} +} + +void call_pwrite64(void) { + char buf[10]; + pwrite64(0, buf, 10, 0); + pwrite64(0, buf, 20, 0); // expected-warning {{'pwrite64' reading 20 bytes from a region of size 10}} +} + +void call_getcwd(void) { + char buf[10]; + getcwd(buf, 10); + getcwd(buf, 20); // expected-warning {{'getcwd' size argument is too large; destination buffer has size 10, but size argument is 20}} +} + +void call_readlink(void) { + char buf[10]; + readlink("path", buf, 10); + readlink("path", buf, 20); // expected-warning {{'readlink' size argument is too large; destination buffer has size 10, but size argument is 20}} +} + +void call_readlinkat(void) { + char buf[10]; + readlinkat(0, "path", buf, 10); + readlinkat(0, "path", buf, 20); // expected-warning {{'readlinkat' size argument is too large; destination buffer has size 10, but size argument is 20}} +} + + void call_snprintf(double d, int n) { char buf[10]; __builtin_snprintf(buf, 10, "merp"); `````````` </details> https://github.com/llvm/llvm-project/pull/224979 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
