Skip to content

Commit 6e3002c

Browse files
committed
[-Wunsafe-buffer-usage] Fix a bug in "warning libc functions (llvm#101583)"
The commit d7dd2c4 crashes for such an example: ``` void printf() { printf(); } ``` Because it assumes `printf` must have arguments. This commit fixes this issue. (rdar://117182250) (cherry picked from commit de88d7d)
1 parent d589d28 commit 6e3002c

File tree

2 files changed

+53
-30
lines changed

2 files changed

+53
-30
lines changed

clang/lib/Analysis/UnsafeBufferUsage.cpp

Lines changed: 51 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include "clang/AST/RecursiveASTVisitor.h"
1515
#include "clang/AST/Stmt.h"
1616
#include "clang/AST/StmtVisitor.h"
17+
#include "clang/AST/Type.h"
1718
#include "clang/ASTMatchers/ASTMatchFinder.h"
1819
#include "clang/ASTMatchers/ASTMatchers.h"
1920
#include "clang/Basic/SourceLocation.h"
@@ -763,32 +764,27 @@ AST_MATCHER(FunctionDecl, isNormalPrintfFunc) {
763764
AST_MATCHER_P(CallExpr, hasUnsafePrintfStringArg,
764765
clang::ast_matchers::internal::Matcher<Expr>,
765766
UnsafeStringArgMatcher) {
766-
// Determine what printf it is:
767-
const Expr *FirstArg = Node.getArg(0);
768-
ASTContext &Ctx = Finder->getASTContext();
767+
// Determine what printf it is by examining formal parameters:
768+
const FunctionDecl *FD = Node.getDirectCallee();
769769

770-
if (isa<StringLiteral>(FirstArg->IgnoreParenImpCasts())) {
771-
// It is a printf/kprintf. And, the format is a string literal:
772-
bool isKprintf = false;
773-
const Expr *UnsafeArg;
770+
assert(FD && "It should have been checked that FD is non-null.");
774771

775-
if (auto *Callee = Node.getDirectCallee())
776-
if (auto *II = Callee->getIdentifier())
777-
isKprintf = II->getName() == "kprintf";
778-
if (hasUnsafeFormatOrSArg(&Node, UnsafeArg, 0, Ctx, isKprintf))
779-
return UnsafeStringArgMatcher.matches(*UnsafeArg, Finder, Builder);
780-
return false;
781-
}
772+
unsigned NumParms = FD->getNumParams();
773+
774+
if (NumParms < 1)
775+
return false; // possibly some user-defined printf function
782776

783-
QualType PtrTy = FirstArg->getType();
777+
ASTContext &Ctx = Finder->getASTContext();
778+
QualType FristParmTy = FD->getParamDecl(0)->getType();
784779

785-
assert(PtrTy->isPointerType());
780+
if (!FristParmTy->isPointerType())
781+
return false; // possibly some user-defined printf function
786782

787-
QualType PteTy = (cast<PointerType>(PtrTy))->getPointeeType();
783+
QualType FirstPteTy = (cast<PointerType>(FristParmTy))->getPointeeType();
788784

789-
if (!Ctx.getFILEType().isNull() /* If `FILE *` is not ever in the ASTContext,
790-
there can't be any file pointer then */
791-
&& PteTy.getCanonicalType() == Ctx.getFILEType().getCanonicalType()) {
785+
if (!Ctx.getFILEType()
786+
.isNull() && //`FILE *` must be in the context if it is fprintf
787+
FirstPteTy.getCanonicalType() == Ctx.getFILEType().getCanonicalType()) {
792788
// It is a fprintf:
793789
const Expr *UnsafeArg;
794790

@@ -797,17 +793,32 @@ AST_MATCHER_P(CallExpr, hasUnsafePrintfStringArg,
797793
return false;
798794
}
799795

800-
const Expr *SecondArg = Node.getArg(1);
801-
802-
if (SecondArg->getType()->isIntegerType()) {
803-
// It is a snprintf:
796+
if (FirstPteTy.isConstQualified()) {
797+
// If the first parameter is a `const char *`, it is a printf/kprintf:
798+
bool isKprintf = false;
804799
const Expr *UnsafeArg;
805800

806-
if (hasUnsafeFormatOrSArg(&Node, UnsafeArg, 2, Ctx, false))
801+
if (auto *II = FD->getIdentifier())
802+
isKprintf = II->getName() == "kprintf";
803+
if (hasUnsafeFormatOrSArg(&Node, UnsafeArg, 0, Ctx, isKprintf))
807804
return UnsafeStringArgMatcher.matches(*UnsafeArg, Finder, Builder);
808805
return false;
809806
}
810-
// It is printf but the format string is passed by pointer. The only thing we
807+
808+
if (NumParms > 2) {
809+
QualType SecondParmTy = FD->getParamDecl(1)->getType();
810+
811+
if (!FirstPteTy.isConstQualified() && SecondParmTy->isIntegerType()) {
812+
// If the first parameter type is non-const qualified `char *` and the
813+
// second is an integer, it is a snprintf:
814+
const Expr *UnsafeArg;
815+
816+
if (hasUnsafeFormatOrSArg(&Node, UnsafeArg, 2, Ctx, false))
817+
return UnsafeStringArgMatcher.matches(*UnsafeArg, Finder, Builder);
818+
return false;
819+
}
820+
}
821+
// We don't really recognize this "normal" printf, the only thing we
811822
// can do is to require all pointers to be null-terminated:
812823
for (auto Arg : Node.arguments())
813824
if (Arg->getType()->isPointerType() && !isNullTermPointer(Arg))
@@ -826,12 +837,23 @@ AST_MATCHER_P(CallExpr, hasUnsafePrintfStringArg,
826837
// size:= DRE.size()/DRE.size_bytes()
827838
// And DRE is a hardened container or view.
828839
AST_MATCHER(CallExpr, hasUnsafeSnprintfBuffer) {
829-
if (Node.getNumArgs() < 3)
830-
return false; // not an snprintf call
840+
const FunctionDecl *FD = Node.getDirectCallee();
841+
842+
assert(FD && "It should have been checked that FD is non-null.");
843+
844+
if (FD->getNumParams() < 3)
845+
return false; // Not an snprint
846+
847+
QualType FirstParmTy = FD->getParamDecl(0)->getType();
848+
849+
if (!FirstParmTy->isPointerType())
850+
return false; // Not an snprint
831851

852+
QualType FirstPteTy = cast<PointerType>(FirstParmTy)->getPointeeType();
832853
const Expr *Buf = Node.getArg(0), *Size = Node.getArg(1);
833854

834-
if (!Buf->getType()->isPointerType() || !Size->getType()->isIntegerType())
855+
if (FirstPteTy.isConstQualified() || !Buf->getType()->isPointerType() ||
856+
!Size->getType()->isIntegerType())
835857
return false; // not an snprintf call
836858

837859
static StringRef SizedObjs[] = {"span", "array", "vector",

clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ int snwprintf_s( char* buffer, unsigned buf_size, const char* format, ... );
2020
int vsnprintf( char* buffer, unsigned buf_size, const char* format, ... );
2121
int sscanf_s(const char * buffer, const char * format, ...);
2222
int sscanf(const char * buffer, const char * format, ... );
23+
int __asan_printf();
2324

2425
namespace std {
2526
template< class InputIt, class OutputIt >
@@ -64,7 +65,6 @@ void f(char * p, char * q, std::span<char> s, std::span<char> s2) {
6465
strcpy_s(); // expected-warning{{function 'strcpy_s' is unsafe}}
6566
wcscpy_s(); // expected-warning{{function 'wcscpy_s' is unsafe}}
6667

67-
6868
/* Test printfs */
6969
fprintf((FILE*)p, "%s%d", p, *p); // expected-warning{{function 'fprintf' is unsafe}} expected-note{{string argument is not guaranteed to be null-terminated}}
7070
printf("%s%d", // expected-warning{{function 'printf' is unsafe}}
@@ -90,6 +90,7 @@ void f(char * p, char * q, std::span<char> s, std::span<char> s2) {
9090
snwprintf(s.data(), s.size_bytes(), "%s%d", __PRETTY_FUNCTION__, *p); // no warn
9191
snwprintf_s(s.data(), s.size_bytes(), "%s%d", __PRETTY_FUNCTION__, *p); // no warn
9292
strlen("hello");// no warn
93+
__asan_printf();// a printf but no argument, so no warn
9394
}
9495

9596
void v(std::string s1, int *p) {

0 commit comments

Comments
 (0)