[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rC342832: [CStringSyntaxChecker] Check strlcat sizeof check
(authored by devnexen, committed by ).
Repository:
rC Clang
https://reviews.llvm.org/D49722
Files:
lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
test/Analysis/cstring-syntax.c
Index: test/Analysis/cstring-syntax.c
===
--- test/Analysis/cstring-syntax.c
+++ test/Analysis/cstring-syntax.c
@@ -7,6 +7,7 @@
char *strncat(char *, const char *, size_t);
size_t strlen (const char *s);
size_t strlcpy(char *, const char *, size_t);
+size_t strlcat(char *, const char *, size_t);
void testStrncat(const char *src) {
char dest[10];
@@ -27,9 +28,27 @@
strlcpy(dest, src, sizeof(dest));
strlcpy(dest, src, destlen);
strlcpy(dest, src, 10);
- strlcpy(dest, src, 20); // expected-warning {{The third argument is larger than the size of the input buffer. Replace with the value 'sizeof(dest)` or lower}}
- strlcpy(dest, src, badlen); // expected-warning {{The third argument is larger than the size of the input buffer. Replace with the value 'sizeof(dest)` or lower}}
+ strlcpy(dest, src, 20); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(dest) or lower}}
+ strlcpy(dest, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(dest) or lower}}
strlcpy(dest, src, ulen);
strlcpy(dest + 5, src, 5);
- strlcpy(dest + 5, src, 10); // expected-warning {{The third argument is larger than the size of the input buffer.}}
+ strlcpy(dest + 5, src, 10); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof() or lower}}
+}
+
+void testStrlcat(const char *src) {
+ char dest[10];
+ size_t badlen = 20;
+ size_t ulen;
+ strlcpy(dest, "a", sizeof("a") - 1);
+ strlcat(dest, "", (sizeof("") - 1) - sizeof(dest) - 1);
+ strlcpy(dest, "012345678", sizeof(dest));
+ strlcat(dest, "910", sizeof(dest));
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcat(dest, "0123456789", badlen / 2);
+ strlcat(dest, "0123456789", badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(dest) or lower}}
+ strlcat(dest, "0123456789", badlen - strlen(dest) - 1);
+ strlcat(dest, src, ulen);
+ strlcpy(dest, src, 5);
+ strlcat(dest + 5, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof() or lower}}
}
Index: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -90,7 +90,16 @@
/// strlcpy(dst, "abcd", 4);
/// strlcpy(dst + 3, "abcd", 2);
/// strlcpy(dst, "abcd", cpy);
- bool containsBadStrlcpyPattern(const CallExpr *CE);
+ /// Identify erroneous patterns in the last argument to strlcat - the number
+ /// of bytes to copy.
+ /// The bad pattern checked is when the last argument is basically
+ /// pointing to the destination buffer size or argument larger or
+ /// equal to.
+ /// char dst[2];
+ /// strlcat(dst, src2, sizeof(dst));
+ /// strlcat(dst, src2, 2);
+ /// strlcat(dst, src2, 10);
+ bool containsBadStrlcpyStrlcatPattern(const CallExpr *CE);
public:
WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
@@ -142,15 +151,19 @@
return false;
}
-bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) {
+bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE) {
if (CE->getNumArgs() != 3)
return false;
+ const FunctionDecl *FD = CE->getDirectCallee();
+ bool Append = CheckerContext::isCLibraryFunction(FD, "strlcat");
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
const auto *DstArgDecl = dyn_cast(DstArg->IgnoreParenImpCasts());
const auto *LenArgDecl = dyn_cast(LenArg->IgnoreParenLValueCasts());
uint64_t DstOff = 0;
+ if (isSizeof(LenArg, DstArg))
+return false;
// - size_t dstlen = sizeof(dst)
if (LenArgDecl) {
const auto *LenArgVal = dyn_cast(LenArgDecl->getDecl());
@@ -181,8 +194,14 @@
if (const auto *Buffer = dyn_cast(DstArgDecl->getType())) {
ASTContext &C = BR.getContext();
uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
-if ((BufferLen - DstOff) < ILRawVal)
- return true;
+auto RemainingBufferLen = BufferLen - DstOff;
+if (Append) {
+ if (RemainingBufferLen <= ILRawVal)
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
MaskRay accepted this revision. MaskRay added a comment. LG https://reviews.llvm.org/D49722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen updated this revision to Diff 166628.
https://reviews.llvm.org/D49722
Files:
lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
test/Analysis/cstring-syntax.c
Index: test/Analysis/cstring-syntax.c
===
--- test/Analysis/cstring-syntax.c
+++ test/Analysis/cstring-syntax.c
@@ -7,6 +7,7 @@
char *strncat(char *, const char *, size_t);
size_t strlen (const char *s);
size_t strlcpy(char *, const char *, size_t);
+size_t strlcat(char *, const char *, size_t);
void testStrncat(const char *src) {
char dest[10];
@@ -27,9 +28,27 @@
strlcpy(dest, src, sizeof(dest));
strlcpy(dest, src, destlen);
strlcpy(dest, src, 10);
- strlcpy(dest, src, 20); // expected-warning {{The third argument is larger than the size of the input buffer. Replace with the value 'sizeof(dest)` or lower}}
- strlcpy(dest, src, badlen); // expected-warning {{The third argument is larger than the size of the input buffer. Replace with the value 'sizeof(dest)` or lower}}
+ strlcpy(dest, src, 20); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(dest) or lower}}
+ strlcpy(dest, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(dest) or lower}}
strlcpy(dest, src, ulen);
strlcpy(dest + 5, src, 5);
- strlcpy(dest + 5, src, 10); // expected-warning {{The third argument is larger than the size of the input buffer.}}
+ strlcpy(dest + 5, src, 10); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof() or lower}}
+}
+
+void testStrlcat(const char *src) {
+ char dest[10];
+ size_t badlen = 20;
+ size_t ulen;
+ strlcpy(dest, "a", sizeof("a") - 1);
+ strlcat(dest, "", (sizeof("") - 1) - sizeof(dest) - 1);
+ strlcpy(dest, "012345678", sizeof(dest));
+ strlcat(dest, "910", sizeof(dest));
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcat(dest, "0123456789", badlen / 2);
+ strlcat(dest, "0123456789", badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(dest) or lower}}
+ strlcat(dest, "0123456789", badlen - strlen(dest) - 1);
+ strlcat(dest, src, ulen);
+ strlcpy(dest, src, 5);
+ strlcat(dest + 5, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof() or lower}}
}
Index: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -90,7 +90,16 @@
/// strlcpy(dst, "abcd", 4);
/// strlcpy(dst + 3, "abcd", 2);
/// strlcpy(dst, "abcd", cpy);
- bool containsBadStrlcpyPattern(const CallExpr *CE);
+ /// Identify erroneous patterns in the last argument to strlcat - the number
+ /// of bytes to copy.
+ /// The bad pattern checked is when the last argument is basically
+ /// pointing to the destination buffer size or argument larger or
+ /// equal to.
+ /// char dst[2];
+ /// strlcat(dst, src2, sizeof(dst));
+ /// strlcat(dst, src2, 2);
+ /// strlcat(dst, src2, 10);
+ bool containsBadStrlcpyStrlcatPattern(const CallExpr *CE);
public:
WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
@@ -142,15 +151,19 @@
return false;
}
-bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) {
+bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE) {
if (CE->getNumArgs() != 3)
return false;
+ const FunctionDecl *FD = CE->getDirectCallee();
+ bool Append = CheckerContext::isCLibraryFunction(FD, "strlcat");
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
const auto *DstArgDecl = dyn_cast(DstArg->IgnoreParenImpCasts());
const auto *LenArgDecl = dyn_cast(LenArg->IgnoreParenLValueCasts());
uint64_t DstOff = 0;
+ if (isSizeof(LenArg, DstArg))
+return false;
// - size_t dstlen = sizeof(dst)
if (LenArgDecl) {
const auto *LenArgVal = dyn_cast(LenArgDecl->getDecl());
@@ -181,8 +194,14 @@
if (const auto *Buffer = dyn_cast(DstArgDecl->getType())) {
ASTContext &C = BR.getContext();
uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
-if ((BufferLen - DstOff) < ILRawVal)
- return true;
+auto RemainingBufferLen = BufferLen - DstOff;
+if (Append) {
+ if (RemainingBufferLen <= ILRawVal)
+return true;
+} else {
+ if (RemainingBufferLen < ILRawVal)
+return true;
+}
}
}
}
@@ -219,8 +238,9 @@
"C String API", os.str(), Loc,
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp:275
+os << "sizeof(" << DstName << ")";
+ else
+os << "sizeof()";
MaskRay wrote:
> devnexen wrote:
> > MaskRay wrote:
> > > Why can't this `else if` case be folded into the `strlcpy` case? There
> > > are lots of duplication.
> > >
> > > `strlcpy` does not check `DstName.empty()` but this one does. Is there
> > > any cases I am missing?
> > strlcpy does but agreed with your first statement, this handling case for
> > both are more different than my initial plan defined them.
> Not sure the description of `strlcat` should be different from `strlcpy`...
> For both of them, `len` should be less or equal to the size of `dst`. They
> may just use the same description.
>
> I think your description of `strlcat` (`"The third argument allows to
> potentially copy more bytes than it should. ")` is better while the existing
> description of `strlcpy` is problematic:
>
> os << "The third argument is larger than the size of the input buffer. ";
>
> input => output
Fair enough. Code reduction is always nice anyway.
https://reviews.llvm.org/D49722
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
MaskRay added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp:275
+os << "sizeof(" << DstName << ")";
+ else
+os << "sizeof()";
devnexen wrote:
> MaskRay wrote:
> > Why can't this `else if` case be folded into the `strlcpy` case? There are
> > lots of duplication.
> >
> > `strlcpy` does not check `DstName.empty()` but this one does. Is there any
> > cases I am missing?
> strlcpy does but agreed with your first statement, this handling case for
> both are more different than my initial plan defined them.
Not sure the description of `strlcat` should be different from `strlcpy`... For
both of them, `len` should be less or equal to the size of `dst`. They may
just use the same description.
I think your description of `strlcat` (`"The third argument allows to
potentially copy more bytes than it should. ")` is better while the existing
description of `strlcpy` is problematic:
os << "The third argument is larger than the size of the input buffer. ";
input => output
https://reviews.llvm.org/D49722
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen updated this revision to Diff 166626.
https://reviews.llvm.org/D49722
Files:
lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
test/Analysis/cstring-syntax.c
Index: test/Analysis/cstring-syntax.c
===
--- test/Analysis/cstring-syntax.c
+++ test/Analysis/cstring-syntax.c
@@ -7,6 +7,7 @@
char *strncat(char *, const char *, size_t);
size_t strlen (const char *s);
size_t strlcpy(char *, const char *, size_t);
+size_t strlcat(char *, const char *, size_t);
void testStrncat(const char *src) {
char dest[10];
@@ -33,3 +34,21 @@
strlcpy(dest + 5, src, 5);
strlcpy(dest + 5, src, 10); // expected-warning {{The third argument is larger than the size of the input buffer.}}
}
+
+void testStrlcat(const char *src) {
+ char dest[10];
+ size_t badlen = 20;
+ size_t ulen;
+ strlcpy(dest, "a", sizeof("a") - 1);
+ strlcat(dest, "", (sizeof("") - 1) - sizeof(dest) - 1);
+ strlcpy(dest, "012345678", sizeof(dest));
+ strlcat(dest, "910", sizeof(dest));
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcat(dest, "0123456789", badlen / 2);
+ strlcat(dest, "0123456789", badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(dest) or lower}}
+ strlcat(dest, "0123456789", badlen - strlen(dest) - 1);
+ strlcat(dest, src, ulen);
+ strlcpy(dest, src, 5);
+ strlcat(dest + 5, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof() or lower}}
+}
Index: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -90,7 +90,16 @@
/// strlcpy(dst, "abcd", 4);
/// strlcpy(dst + 3, "abcd", 2);
/// strlcpy(dst, "abcd", cpy);
- bool containsBadStrlcpyPattern(const CallExpr *CE);
+ /// Identify erroneous patterns in the last argument to strlcat - the number
+ /// of bytes to copy.
+ /// The bad pattern checked is when the last argument is basically
+ /// pointing to the destination buffer size or argument larger or
+ /// equal to.
+ /// char dst[2];
+ /// strlcat(dst, src2, sizeof(dst));
+ /// strlcat(dst, src2, 2);
+ /// strlcat(dst, src2, 10);
+ bool containsBadStrlcpyStrlcatPattern(const CallExpr *CE);
public:
WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
@@ -142,15 +151,19 @@
return false;
}
-bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) {
+bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE) {
if (CE->getNumArgs() != 3)
return false;
+ const FunctionDecl *FD = CE->getDirectCallee();
+ bool Append = CheckerContext::isCLibraryFunction(FD, "strlcat");
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
const auto *DstArgDecl = dyn_cast(DstArg->IgnoreParenImpCasts());
const auto *LenArgDecl = dyn_cast(LenArg->IgnoreParenLValueCasts());
uint64_t DstOff = 0;
+ if (isSizeof(LenArg, DstArg))
+return false;
// - size_t dstlen = sizeof(dst)
if (LenArgDecl) {
const auto *LenArgVal = dyn_cast(LenArgDecl->getDecl());
@@ -181,8 +194,14 @@
if (const auto *Buffer = dyn_cast(DstArgDecl->getType())) {
ASTContext &C = BR.getContext();
uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
-if ((BufferLen - DstOff) < ILRawVal)
- return true;
+auto RemainingBufferLen = BufferLen - DstOff;
+if (Append) {
+ if (RemainingBufferLen <= ILRawVal)
+return true;
+} else {
+ if (RemainingBufferLen < ILRawVal)
+return true;
+}
}
}
}
@@ -219,8 +238,9 @@
"C String API", os.str(), Loc,
LenArg->getSourceRange());
}
- } else if (CheckerContext::isCLibraryFunction(FD, "strlcpy")) {
-if (containsBadStrlcpyPattern(CE)) {
+ } else if (CheckerContext::isCLibraryFunction(FD, "strlcpy") ||
+ CheckerContext::isCLibraryFunction(FD, "strlcat")) {
+if (containsBadStrlcpyStrlcatPattern(CE)) {
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
PathDiagnosticLocation Loc =
@@ -230,13 +250,27 @@
SmallString<256> S;
llvm::raw_svector_ostream os(S);
- os << "The third argument is larger than the size of the input buffer. ";
- if (!DstName.empty())
-os << "Replace with the value 'sizeof(" << DstName << ")` or lower";
-
- BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument",
- "C String API", os.str(), Loc,
- LenArg->getSourceRange());
+ if (CheckerContext::isCLibraryFunction(FD, "str
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp:275
+os << "sizeof(" << DstName << ")";
+ else
+os << "sizeof()";
MaskRay wrote:
> Why can't this `else if` case be folded into the `strlcpy` case? There are
> lots of duplication.
>
> `strlcpy` does not check `DstName.empty()` but this one does. Is there any
> cases I am missing?
strlcpy does but agreed with your first statement, this handling case for both
are more different than my initial plan defined them.
https://reviews.llvm.org/D49722
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
MaskRay added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp:275
+os << "sizeof(" << DstName << ")";
+ else
+os << "sizeof()";
Why can't this `else if` case be folded into the `strlcpy` case? There are lots
of duplication.
`strlcpy` does not check `DstName.empty()` but this one does. Is there any
cases I am missing?
https://reviews.llvm.org/D49722
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen added a comment. ping :) https://reviews.llvm.org/D49722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen updated this revision to Diff 165604.
https://reviews.llvm.org/D49722
Files:
lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
test/Analysis/cstring-syntax.c
Index: test/Analysis/cstring-syntax.c
===
--- test/Analysis/cstring-syntax.c
+++ test/Analysis/cstring-syntax.c
@@ -7,6 +7,7 @@
char *strncat(char *, const char *, size_t);
size_t strlen (const char *s);
size_t strlcpy(char *, const char *, size_t);
+size_t strlcat(char *, const char *, size_t);
void testStrncat(const char *src) {
char dest[10];
@@ -33,3 +34,21 @@
strlcpy(dest + 5, src, 5);
strlcpy(dest + 5, src, 10); // expected-warning {{The third argument is larger than the size of the input buffer.}}
}
+
+void testStrlcat(const char *src) {
+ char dest[10];
+ size_t badlen = 20;
+ size_t ulen;
+ strlcpy(dest, "a", sizeof("a") - 1);
+ strlcat(dest, "", (sizeof("") - 1) - sizeof(dest) - 1);
+ strlcpy(dest, "012345678", sizeof(dest));
+ strlcat(dest, "910", sizeof(dest));
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcat(dest, "0123456789", badlen / 2);
+ strlcat(dest, "0123456789", badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(dest) or lower}}
+ strlcat(dest, "0123456789", badlen - strlen(dest) - 1);
+ strlcat(dest, src, ulen);
+ strlcpy(dest, src, 5);
+ strlcat(dest + 5, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof() or lower}}
+}
Index: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -90,7 +90,16 @@
/// strlcpy(dst, "abcd", 4);
/// strlcpy(dst + 3, "abcd", 2);
/// strlcpy(dst, "abcd", cpy);
- bool containsBadStrlcpyPattern(const CallExpr *CE);
+ /// Identify erroneous patterns in the last argument to strlcat - the number
+ /// of bytes to copy.
+ /// The bad pattern checked is when the last argument is basically
+ /// pointing to the destination buffer size or argument larger or
+ /// equal to.
+ /// char dst[2];
+ /// strlcat(dst, src2, sizeof(dst));
+ /// strlcat(dst, src2, 2);
+ /// strlcat(dst, src2, 10);
+ bool containsBadStrlcpyStrlcatPattern(const CallExpr *CE);
public:
WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
@@ -142,15 +151,19 @@
return false;
}
-bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) {
+bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE) {
if (CE->getNumArgs() != 3)
return false;
+ const FunctionDecl *FD = CE->getDirectCallee();
+ bool Append = CheckerContext::isCLibraryFunction(FD, "strlcat");
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
const auto *DstArgDecl = dyn_cast(DstArg->IgnoreParenImpCasts());
const auto *LenArgDecl = dyn_cast(LenArg->IgnoreParenLValueCasts());
uint64_t DstOff = 0;
+ if (isSizeof(LenArg, DstArg))
+return false;
// - size_t dstlen = sizeof(dst)
if (LenArgDecl) {
const auto *LenArgVal = dyn_cast(LenArgDecl->getDecl());
@@ -181,8 +194,14 @@
if (const auto *Buffer = dyn_cast(DstArgDecl->getType())) {
ASTContext &C = BR.getContext();
uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
-if ((BufferLen - DstOff) < ILRawVal)
- return true;
+auto RemainingBufferLen = BufferLen - DstOff;
+if (Append) {
+ if (RemainingBufferLen <= ILRawVal)
+return true;
+} else {
+ if (RemainingBufferLen < ILRawVal)
+return true;
+}
}
}
}
@@ -220,7 +239,7 @@
LenArg->getSourceRange());
}
} else if (CheckerContext::isCLibraryFunction(FD, "strlcpy")) {
-if (containsBadStrlcpyPattern(CE)) {
+if (containsBadStrlcpyStrlcatPattern(CE)) {
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
PathDiagnosticLocation Loc =
@@ -234,6 +253,29 @@
if (!DstName.empty())
os << "Replace with the value 'sizeof(" << DstName << ")` or lower";
+ BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument",
+ "C String API", os.str(), Loc,
+ LenArg->getSourceRange());
+}
+ } else if (CheckerContext::isCLibraryFunction(FD, "strlcat")) {
+if (containsBadStrlcpyStrlcatPattern(CE)) {
+ const Expr *DstArg = CE->getArg(0);
+ const Expr *LenArg = CE->getArg(2);
+ PathDiagnosticLocation Loc =
+PathDiagnosticLocation::createBegin(LenArg, BR.getSourceManager(), AC);
+
+ StringRef DstName = getPrintableName(DstArg);
+
+ SmallS
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
MaskRay added inline comments.
Comment at: test/Analysis/cstring-syntax.c:49
+ strlcat(dest, "0123456789", badlen / 2);
+ strlcat(dest, "0123456789", badlen); // expected-warning {{The third
argument allows to potentially copy more bytes than it should. Replace with the
value 'badlen' - strlen(dest) - 1 or lower}}
+ strlcat(dest, "0123456789", badlen - strlen(dest) - 1);
devnexen wrote:
> NoQ wrote:
> > The suggested fix is a bit weird.
> >
> > The correct code for appending `src` to `dst` is either `strlcat(dst, src,
> > sizeof(dst));` (the approach suggested by the man page) or `strlcat(dst +
> > strlen(dst) + 1, src, sizeof(dst) - strlen(dst) - 1)` (which is equivalent
> > but faster if you already know `strlen(dst)`). In both cases you can
> > specify a smaller value but not a larger value.
> In fact in this case the message is misleading/a bit wrong.
I think `strlcat` is very clumsy to you if you need to add an offset to
`dest`...
For
`strlcat(dst + strlen(dst) + 1, src, sizeof(dst) - strlen(dst) - 1)`
I suppose you meant:
`strlcpy(dst + strlen(dst), src, sizeof(dst) - strlen(dst))`
... but the suggestion does not look very appealing. `strlcat(dst, ...,
sizeof(dst)` if good enough as a suggested fix.
https://reviews.llvm.org/D49722
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen added inline comments. Comment at: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp:199 +if (Append) + RemainingBufferLen -= 1; +if (RemainingBufferLen < ILRawVal) MaskRay wrote: > `RemainingBufferLen` is `uint64_t`. Can the `-= 1` overflow? That s a good point. I may redo as it was before. https://reviews.llvm.org/D49722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
MaskRay added inline comments. Comment at: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp:164 + // - sizeof(dst) + if (Append && isSizeof(LenArg, DstArg)) +return true; george.karpenkov wrote: > george.karpenkov wrote: > > george.karpenkov wrote: > > > I am confused on what is happening in this branch and why is it bad. > > > Could we add a commend? > > Sorry I'm still confused about this branch: could you clarify? > Ah, OK, I see it needs one more byte due to null-termination. I think the `if (isSizeof(LenArg, DstArg))` check is fine so it returns `false`. `strlcat(dst, src, sizeof dst)` is good. Comment at: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp:199 +if (Append) + RemainingBufferLen -= 1; +if (RemainingBufferLen < ILRawVal) `RemainingBufferLen` is `uint64_t`. Can the `-= 1` overflow? https://reviews.llvm.org/D49722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen updated this revision to Diff 164355.
devnexen added a comment.
- Correcting misleading message and advising proper fix.
https://reviews.llvm.org/D49722
Files:
lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
test/Analysis/cstring-syntax.c
Index: test/Analysis/cstring-syntax.c
===
--- test/Analysis/cstring-syntax.c
+++ test/Analysis/cstring-syntax.c
@@ -7,6 +7,7 @@
char *strncat(char *, const char *, size_t);
size_t strlen (const char *s);
size_t strlcpy(char *, const char *, size_t);
+size_t strlcat(char *, const char *, size_t);
void testStrncat(const char *src) {
char dest[10];
@@ -33,3 +34,21 @@
strlcpy(dest + 5, src, 5);
strlcpy(dest + 5, src, 10); // expected-warning {{The third argument is larger than the size of the input buffer.}}
}
+
+void testStrlcat(const char *src) {
+ char dest[10];
+ size_t badlen = 20;
+ size_t ulen;
+ strlcpy(dest, "a", sizeof("a") - 1);
+ strlcat(dest, "", (sizeof("") - 1) - sizeof(dest) - 1);
+ strlcpy(dest, "012345678", sizeof(dest));
+ strlcat(dest, "910", sizeof(dest));
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcat(dest, "0123456789", badlen / 2);
+ strlcat(dest, "0123456789", badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(dest) - strlen(dest) - 1 or lower}}
+ strlcat(dest, "0123456789", badlen - strlen(dest) - 1);
+ strlcat(dest, src, ulen);
+ strlcpy(dest, src, 5);
+ strlcat(dest + 5, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof() - strlen() - 1 or lower}}
+}
Index: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -90,7 +90,16 @@
/// strlcpy(dst, "abcd", 4);
/// strlcpy(dst + 3, "abcd", 2);
/// strlcpy(dst, "abcd", cpy);
- bool containsBadStrlcpyPattern(const CallExpr *CE);
+ /// Identify erroneous patterns in the last argument to strlcat - the number
+ /// of bytes to copy.
+ /// The bad pattern checked is when the last argument is basically
+ /// pointing to the destination buffer size or argument larger or
+ /// equal to.
+ /// char dst[2];
+ /// strlcat(dst, src2, sizeof(dst));
+ /// strlcat(dst, src2, 2);
+ /// strlcat(dst, src2, 10);
+ bool containsBadStrlcpyStrlcatPattern(const CallExpr *CE);
public:
WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
@@ -142,15 +151,19 @@
return false;
}
-bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) {
+bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE) {
if (CE->getNumArgs() != 3)
return false;
+ const FunctionDecl *FD = CE->getDirectCallee();
+ bool Append = CheckerContext::isCLibraryFunction(FD, "strlcat");
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
const auto *DstArgDecl = dyn_cast(DstArg->IgnoreParenImpCasts());
const auto *LenArgDecl = dyn_cast(LenArg->IgnoreParenLValueCasts());
uint64_t DstOff = 0;
+ if (isSizeof(LenArg, DstArg))
+return false;
// - size_t dstlen = sizeof(dst)
if (LenArgDecl) {
const auto *LenArgVal = dyn_cast(LenArgDecl->getDecl());
@@ -181,7 +194,10 @@
if (const auto *Buffer = dyn_cast(DstArgDecl->getType())) {
ASTContext &C = BR.getContext();
uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
-if ((BufferLen - DstOff) < ILRawVal)
+auto RemainingBufferLen = BufferLen - DstOff;
+if (Append)
+ RemainingBufferLen -= 1;
+if (RemainingBufferLen < ILRawVal)
return true;
}
}
@@ -220,7 +236,7 @@
LenArg->getSourceRange());
}
} else if (CheckerContext::isCLibraryFunction(FD, "strlcpy")) {
-if (containsBadStrlcpyPattern(CE)) {
+if (containsBadStrlcpyStrlcatPattern(CE)) {
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
PathDiagnosticLocation Loc =
@@ -234,6 +250,29 @@
if (!DstName.empty())
os << "Replace with the value 'sizeof(" << DstName << ")` or lower";
+ BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument",
+ "C String API", os.str(), Loc,
+ LenArg->getSourceRange());
+}
+ } else if (CheckerContext::isCLibraryFunction(FD, "strlcat")) {
+if (containsBadStrlcpyStrlcatPattern(CE)) {
+ const Expr *DstArg = CE->getArg(0);
+ const Expr *LenArg = CE->getArg(2);
+ PathDiagnosticLocation Loc =
+PathDiagnosticLocation::createBegin(LenArg, BR.getSourceManager(), AC);
+
+ StringRef DstName = getPrintableName(DstArg
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen added inline comments.
Comment at: test/Analysis/cstring-syntax.c:49
+ strlcat(dest, "0123456789", badlen / 2);
+ strlcat(dest, "0123456789", badlen); // expected-warning {{The third
argument allows to potentially copy more bytes than it should. Replace with the
value 'badlen' - strlen(dest) - 1 or lower}}
+ strlcat(dest, "0123456789", badlen - strlen(dest) - 1);
NoQ wrote:
> The suggested fix is a bit weird.
>
> The correct code for appending `src` to `dst` is either `strlcat(dst, src,
> sizeof(dst));` (the approach suggested by the man page) or `strlcat(dst +
> strlen(dst) + 1, src, sizeof(dst) - strlen(dst) - 1)` (which is equivalent
> but faster if you already know `strlen(dst)`). In both cases you can specify
> a smaller value but not a larger value.
In fact in this case the message is misleading/a bit wrong.
https://reviews.llvm.org/D49722
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
NoQ added inline comments.
Comment at: test/Analysis/cstring-syntax.c:49
+ strlcat(dest, "0123456789", badlen / 2);
+ strlcat(dest, "0123456789", badlen); // expected-warning {{The third
argument allows to potentially copy more bytes than it should. Replace with the
value 'badlen' - strlen(dest) - 1 or lower}}
+ strlcat(dest, "0123456789", badlen - strlen(dest) - 1);
The suggested fix is a bit weird.
The correct code for appending `src` to `dst` is either `strlcat(dst, src,
sizeof(dst));` (the approach suggested by the man page) or `strlcat(dst +
strlen(dst) + 1, src, sizeof(dst) - strlen(dst) - 1)` (which is equivalent but
faster if you already know `strlen(dst)`). In both cases you can specify a
smaller value but not a larger value.
https://reviews.llvm.org/D49722
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen added a comment. ping @george.karpenkov after that I won t bother you for a long time :) https://reviews.llvm.org/D49722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen added a comment. ping https://reviews.llvm.org/D49722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen added a comment. ping https://reviews.llvm.org/D49722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen updated this revision to Diff 162139.
devnexen added a comment.
- Returns immediately for both case when sizeof destination.
- Adding few more cases.
https://reviews.llvm.org/D49722
Files:
lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
test/Analysis/cstring-syntax.c
Index: test/Analysis/cstring-syntax.c
===
--- test/Analysis/cstring-syntax.c
+++ test/Analysis/cstring-syntax.c
@@ -7,6 +7,7 @@
char *strncat(char *, const char *, size_t);
size_t strlen (const char *s);
size_t strlcpy(char *, const char *, size_t);
+size_t strlcat(char *, const char *, size_t);
void testStrncat(const char *src) {
char dest[10];
@@ -33,3 +34,21 @@
strlcpy(dest + 5, src, 5);
strlcpy(dest + 5, src, 10); // expected-warning {{The third argument is larger than the size of the input buffer.}}
}
+
+void testStrlcat(const char *src) {
+ char dest[10];
+ size_t badlen = 20;
+ size_t ulen;
+ strlcpy(dest, "a", sizeof("a") - 1);
+ strlcat(dest, "", (sizeof("") - 1) - sizeof(dest) - 1);
+ strlcpy(dest, "012345678", sizeof(dest));
+ strlcat(dest, "910", sizeof(dest));
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcat(dest, "0123456789", badlen / 2);
+ strlcat(dest, "0123456789", badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value 'badlen' - strlen(dest) - 1 or lower}}
+ strlcat(dest, "0123456789", badlen - strlen(dest) - 1);
+ strlcat(dest, src, ulen);
+ strlcpy(dest, src, 5);
+ strlcat(dest + 5, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value 'badlen' - strlen() - 1 or lower}}
+}
Index: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -90,7 +90,16 @@
/// strlcpy(dst, "abcd", 4);
/// strlcpy(dst + 3, "abcd", 2);
/// strlcpy(dst, "abcd", cpy);
- bool containsBadStrlcpyPattern(const CallExpr *CE);
+ /// Identify erroneous patterns in the last argument to strlcat - the number
+ /// of bytes to copy.
+ /// The bad pattern checked is when the last argument is basically
+ /// pointing to the destination buffer size or argument larger or
+ /// equal to.
+ /// char dst[2];
+ /// strlcat(dst, src2, sizeof(dst));
+ /// strlcat(dst, src2, 2);
+ /// strlcat(dst, src2, 10);
+ bool containsBadStrlcpyStrlcatPattern(const CallExpr *CE);
public:
WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
@@ -142,15 +151,19 @@
return false;
}
-bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) {
+bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE) {
if (CE->getNumArgs() != 3)
return false;
+ const FunctionDecl *FD = CE->getDirectCallee();
+ bool Append = CheckerContext::isCLibraryFunction(FD, "strlcat");
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
const auto *DstArgDecl = dyn_cast(DstArg->IgnoreParenImpCasts());
const auto *LenArgDecl = dyn_cast(LenArg->IgnoreParenLValueCasts());
uint64_t DstOff = 0;
+ if (isSizeof(LenArg, DstArg))
+return false;
// - size_t dstlen = sizeof(dst)
if (LenArgDecl) {
const auto *LenArgVal = dyn_cast(LenArgDecl->getDecl());
@@ -181,7 +194,10 @@
if (const auto *Buffer = dyn_cast(DstArgDecl->getType())) {
ASTContext &C = BR.getContext();
uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
-if ((BufferLen - DstOff) < ILRawVal)
+auto RemainingBufferLen = BufferLen - DstOff;
+if (Append)
+ RemainingBufferLen -= 1;
+if (RemainingBufferLen < ILRawVal)
return true;
}
}
@@ -220,7 +236,7 @@
LenArg->getSourceRange());
}
} else if (CheckerContext::isCLibraryFunction(FD, "strlcpy")) {
-if (containsBadStrlcpyPattern(CE)) {
+if (containsBadStrlcpyStrlcatPattern(CE)) {
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
PathDiagnosticLocation Loc =
@@ -234,6 +250,34 @@
if (!DstName.empty())
os << "Replace with the value 'sizeof(" << DstName << ")` or lower";
+ BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument",
+ "C String API", os.str(), Loc,
+ LenArg->getSourceRange());
+}
+ } else if (CheckerContext::isCLibraryFunction(FD, "strlcat")) {
+if (containsBadStrlcpyStrlcatPattern(CE)) {
+ const Expr *DstArg = CE->getArg(0);
+ const Expr *LenArg = CE->getArg(2);
+ PathDiagnosticLocation Loc =
+PathDiagnosticLocation::createBegin(LenArg, BR.getSourceManager(), AC);
+
+ StringRef DstNam
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
george.karpenkov reopened this revision. george.karpenkov added a comment. This revision is now accepted and ready to land. This is not correct, `strlcat(dest, "mystr", sizeof(dest))` is perfectly fine, as can be seen in many examples including strlcat man page. Repository: rL LLVM https://reviews.llvm.org/D49722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
NoQ added inline comments.
Comment at: cfe/trunk/test/Analysis/cstring-syntax.c:45
+ strlcpy(dest, "012345678", sizeof(dest));
+ strlcat(dest, "910", sizeof(dest)); // expected-warning {{The third argument
allows to potentially copy more bytes than it should. Replace with the value
- strlen(dest) - 1 or lower}}
+ strlcpy(dest, "0123456789", sizeof(dest));
There seem to be two spaces around ``.
Repository:
rL LLVM
https://reviews.llvm.org/D49722
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
This revision was automatically updated to reflect the committed changes.
Closed by commit rL339641: [CStringSyntaxChecker] Check strlcat sizeof check
(authored by devnexen, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D49722?vs=160272&id=160513#toc
Repository:
rL LLVM
https://reviews.llvm.org/D49722
Files:
cfe/trunk/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
cfe/trunk/test/Analysis/cstring-syntax.c
Index: cfe/trunk/test/Analysis/cstring-syntax.c
===
--- cfe/trunk/test/Analysis/cstring-syntax.c
+++ cfe/trunk/test/Analysis/cstring-syntax.c
@@ -7,6 +7,7 @@
char *strncat(char *, const char *, size_t);
size_t strlen (const char *s);
size_t strlcpy(char *, const char *, size_t);
+size_t strlcat(char *, const char *, size_t);
void testStrncat(const char *src) {
char dest[10];
@@ -33,3 +34,19 @@
strlcpy(dest + 5, src, 5);
strlcpy(dest + 5, src, 10); // expected-warning {{The third argument is larger than the size of the input buffer.}}
}
+
+void testStrlcat(const char *src) {
+ char dest[10];
+ size_t badlen = 10;
+ size_t ulen;
+ strlcpy(dest, "a", sizeof("a") - 1);
+ strlcat(dest, "", (sizeof("") - 1) - sizeof(dest) - 1);
+ strlcpy(dest, "012345678", sizeof(dest));
+ strlcat(dest, "910", sizeof(dest)); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value- strlen(dest) - 1 or lower}}
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcat(dest, "0123456789", badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value 'badlen' - strlen(dest) - 1 or lower}}
+ strlcat(dest, "0123456789", badlen - strlen(dest) - 1);
+ strlcat(dest, src, ulen);
+ strlcpy(dest, src, 5);
+ strlcat(dest + 5, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value 'badlen' - strlen() - 1 or lower}}
+}
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -90,7 +90,16 @@
/// strlcpy(dst, "abcd", 4);
/// strlcpy(dst + 3, "abcd", 2);
/// strlcpy(dst, "abcd", cpy);
- bool containsBadStrlcpyPattern(const CallExpr *CE);
+ /// Identify erroneous patterns in the last argument to strlcat - the number
+ /// of bytes to copy.
+ /// The bad pattern checked is when the last argument is basically
+ /// pointing to the destination buffer size or argument larger or
+ /// equal to.
+ /// char dst[2];
+ /// strlcat(dst, src2, sizeof(dst));
+ /// strlcat(dst, src2, 2);
+ /// strlcat(dst, src2, 10);
+ bool containsBadStrlcpyStrlcatPattern(const CallExpr *CE);
public:
WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
@@ -142,15 +151,21 @@
return false;
}
-bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) {
+bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE) {
if (CE->getNumArgs() != 3)
return false;
+ const FunctionDecl *FD = CE->getDirectCallee();
+ bool Append = CheckerContext::isCLibraryFunction(FD, "strlcat");
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
const auto *DstArgDecl = dyn_cast(DstArg->IgnoreParenImpCasts());
const auto *LenArgDecl = dyn_cast(LenArg->IgnoreParenLValueCasts());
uint64_t DstOff = 0;
+ // - sizeof(dst)
+ // strlcat appends at most size - strlen(dst) - 1
+ if (Append && isSizeof(LenArg, DstArg))
+return true;
// - size_t dstlen = sizeof(dst)
if (LenArgDecl) {
const auto *LenArgVal = dyn_cast(LenArgDecl->getDecl());
@@ -181,7 +196,10 @@
if (const auto *Buffer = dyn_cast(DstArgDecl->getType())) {
ASTContext &C = BR.getContext();
uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
-if ((BufferLen - DstOff) < ILRawVal)
+auto RemainingBufferLen = BufferLen - DstOff;
+if (Append)
+ RemainingBufferLen -= 1;
+if (RemainingBufferLen < ILRawVal)
return true;
}
}
@@ -220,7 +238,7 @@
LenArg->getSourceRange());
}
} else if (CheckerContext::isCLibraryFunction(FD, "strlcpy")) {
-if (containsBadStrlcpyPattern(CE)) {
+if (containsBadStrlcpyStrlcatPattern(CE)) {
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
PathDiagnosticLocation Loc =
@@ -238,6 +256,34 @@
"C String API", os.str(), Loc,
LenArg->getSourceRange());
}
+ } else if (CheckerContext::isCLibraryFunction(FD, "strlcat")) {
+if (containsBadStrlcpyStrlcatPattern(CE)) {
+
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen updated this revision to Diff 160272.
https://reviews.llvm.org/D49722
Files:
lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
test/Analysis/cstring-syntax.c
Index: test/Analysis/cstring-syntax.c
===
--- test/Analysis/cstring-syntax.c
+++ test/Analysis/cstring-syntax.c
@@ -7,6 +7,7 @@
char *strncat(char *, const char *, size_t);
size_t strlen (const char *s);
size_t strlcpy(char *, const char *, size_t);
+size_t strlcat(char *, const char *, size_t);
void testStrncat(const char *src) {
char dest[10];
@@ -33,3 +34,19 @@
strlcpy(dest + 5, src, 5);
strlcpy(dest + 5, src, 10); // expected-warning {{The third argument is larger than the size of the input buffer.}}
}
+
+void testStrlcat(const char *src) {
+ char dest[10];
+ size_t badlen = 10;
+ size_t ulen;
+ strlcpy(dest, "a", sizeof("a") - 1);
+ strlcat(dest, "", (sizeof("") - 1) - sizeof(dest) - 1);
+ strlcpy(dest, "012345678", sizeof(dest));
+ strlcat(dest, "910", sizeof(dest)); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value- strlen(dest) - 1 or lower}}
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcat(dest, "0123456789", badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value 'badlen' - strlen(dest) - 1 or lower}}
+ strlcat(dest, "0123456789", badlen - strlen(dest) - 1);
+ strlcat(dest, src, ulen);
+ strlcpy(dest, src, 5);
+ strlcat(dest + 5, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value 'badlen' - strlen() - 1 or lower}}
+}
Index: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -90,7 +90,16 @@
/// strlcpy(dst, "abcd", 4);
/// strlcpy(dst + 3, "abcd", 2);
/// strlcpy(dst, "abcd", cpy);
- bool containsBadStrlcpyPattern(const CallExpr *CE);
+ /// Identify erroneous patterns in the last argument to strlcat - the number
+ /// of bytes to copy.
+ /// The bad pattern checked is when the last argument is basically
+ /// pointing to the destination buffer size or argument larger or
+ /// equal to.
+ /// char dst[2];
+ /// strlcat(dst, src2, sizeof(dst));
+ /// strlcat(dst, src2, 2);
+ /// strlcat(dst, src2, 10);
+ bool containsBadStrlcpyStrlcatPattern(const CallExpr *CE);
public:
WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
@@ -142,15 +151,21 @@
return false;
}
-bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) {
+bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE) {
if (CE->getNumArgs() != 3)
return false;
+ const FunctionDecl *FD = CE->getDirectCallee();
+ bool Append = CheckerContext::isCLibraryFunction(FD, "strlcat");
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
const auto *DstArgDecl = dyn_cast(DstArg->IgnoreParenImpCasts());
const auto *LenArgDecl = dyn_cast(LenArg->IgnoreParenLValueCasts());
uint64_t DstOff = 0;
+ // - sizeof(dst)
+ // strlcat appends at most size - strlen(dst) - 1
+ if (Append && isSizeof(LenArg, DstArg))
+return true;
// - size_t dstlen = sizeof(dst)
if (LenArgDecl) {
const auto *LenArgVal = dyn_cast(LenArgDecl->getDecl());
@@ -181,7 +196,10 @@
if (const auto *Buffer = dyn_cast(DstArgDecl->getType())) {
ASTContext &C = BR.getContext();
uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
-if ((BufferLen - DstOff) < ILRawVal)
+auto RemainingBufferLen = BufferLen - DstOff;
+if (Append)
+ RemainingBufferLen -= 1;
+if (RemainingBufferLen < ILRawVal)
return true;
}
}
@@ -220,7 +238,7 @@
LenArg->getSourceRange());
}
} else if (CheckerContext::isCLibraryFunction(FD, "strlcpy")) {
-if (containsBadStrlcpyPattern(CE)) {
+if (containsBadStrlcpyStrlcatPattern(CE)) {
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
PathDiagnosticLocation Loc =
@@ -234,6 +252,34 @@
if (!DstName.empty())
os << "Replace with the value 'sizeof(" << DstName << ")` or lower";
+ BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument",
+ "C String API", os.str(), Loc,
+ LenArg->getSourceRange());
+}
+ } else if (CheckerContext::isCLibraryFunction(FD, "strlcat")) {
+if (containsBadStrlcpyStrlcatPattern(CE)) {
+ const Expr *DstArg = CE->getArg(0);
+ const Expr *LenArg = CE->getArg(2);
+ PathDiagnosticLocation Loc =
+PathDiagnosticLocation::createBegin(LenArg, BR.getSourceManager(), A
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
george.karpenkov requested changes to this revision.
george.karpenkov added inline comments.
This revision now requires changes to proceed.
Comment at: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp:164
+ // - sizeof(dst)
+ if (Append && isSizeof(LenArg, DstArg))
+return true;
george.karpenkov wrote:
> george.karpenkov wrote:
> > I am confused on what is happening in this branch and why is it bad. Could
> > we add a commend?
> Sorry I'm still confused about this branch: could you clarify?
Ah, OK, I see it needs one more byte due to null-termination.
Comment at: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp:199
uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
-if ((BufferLen - DstOff) < ILRawVal)
- return true;
+auto RemainingBufferLen = BufferLen - DstOff;
+if (Append) {
Probably better expressed as:
```
if (Append)
RemainingBufferLen -= 1;
```
Then you don't need branching.
https://reviews.llvm.org/D49722
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
george.karpenkov added inline comments. Comment at: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp:164 + // - sizeof(dst) + if (Append && isSizeof(LenArg, DstArg)) +return true; george.karpenkov wrote: > I am confused on what is happening in this branch and why is it bad. Could we > add a commend? Sorry I'm still confused about this branch: could you clarify? https://reviews.llvm.org/D49722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen added a comment. ping but will be for 8.0 :) https://reviews.llvm.org/D49722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen added a comment. ping :) https://reviews.llvm.org/D49722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen updated this revision to Diff 158062.
https://reviews.llvm.org/D49722
Files:
lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
test/Analysis/cstring-syntax.c
Index: test/Analysis/cstring-syntax.c
===
--- test/Analysis/cstring-syntax.c
+++ test/Analysis/cstring-syntax.c
@@ -7,6 +7,7 @@
char *strncat(char *, const char *, size_t);
size_t strlen (const char *s);
size_t strlcpy(char *, const char *, size_t);
+size_t strlcat(char *, const char *, size_t);
void testStrncat(const char *src) {
char dest[10];
@@ -33,3 +34,19 @@
strlcpy(dest + 5, src, 5);
strlcpy(dest + 5, src, 10); // expected-warning {{The third argument is larger than the size of the input buffer.}}
}
+
+void testStrlcat(const char *src) {
+ char dest[10];
+ size_t badlen = 10;
+ size_t ulen;
+ strlcpy(dest, "a", sizeof("a") - 1);
+ strlcat(dest, "", (sizeof("") - 1) - sizeof(dest) - 1);
+ strlcpy(dest, "012345678", sizeof(dest));
+ strlcat(dest, "910", sizeof(dest)); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value- strlen(dest) - 1 or lower}}
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcat(dest, "0123456789", badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value 'badlen' - strlen(dest) - 1 or lower}}
+ strlcat(dest, "0123456789", badlen - strlen(dest) - 1);
+ strlcat(dest, src, ulen);
+ strlcpy(dest, src, 5);
+ strlcat(dest + 5, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value 'badlen' - strlen() - 1 or lower}}
+}
Index: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -90,7 +90,16 @@
/// strlcpy(dst, "abcd", 4);
/// strlcpy(dst + 3, "abcd", 2);
/// strlcpy(dst, "abcd", cpy);
- bool containsBadStrlcpyPattern(const CallExpr *CE);
+ /// Identify erroneous patterns in the last argument to strlcat - the number
+ /// of bytes to copy.
+ /// The bad pattern checked is when the last argument is basically
+ /// pointing to the destination buffer size or argument larger or
+ /// equal to.
+ /// char dst[2];
+ /// strlcat(dst, src2, sizeof(dst));
+ /// strlcat(dst, src2, 2);
+ /// strlcat(dst, src2, 10);
+ bool containsBadStrlcpyStrlcatPattern(const CallExpr *CE);
public:
WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
@@ -142,15 +151,21 @@
return false;
}
-bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) {
+bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE) {
if (CE->getNumArgs() != 3)
return false;
+ const FunctionDecl *FD = CE->getDirectCallee();
+ bool Append = CheckerContext::isCLibraryFunction(FD, "strlcat");
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
const auto *DstArgDecl = dyn_cast(DstArg->IgnoreParenImpCasts());
const auto *LenArgDecl = dyn_cast(LenArg->IgnoreParenLValueCasts());
uint64_t DstOff = 0;
+ // - sizeof(dst)
+ // strlcat appends at most size - strlen(dst) - 1
+ if (Append && isSizeof(LenArg, DstArg))
+return true;
// - size_t dstlen = sizeof(dst)
if (LenArgDecl) {
const auto *LenArgVal = dyn_cast(LenArgDecl->getDecl());
@@ -181,8 +196,14 @@
if (const auto *Buffer = dyn_cast(DstArgDecl->getType())) {
ASTContext &C = BR.getContext();
uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
-if ((BufferLen - DstOff) < ILRawVal)
- return true;
+auto RemainingBufferLen = BufferLen - DstOff;
+if (Append) {
+ if (RemainingBufferLen <= ILRawVal)
+return true;
+} else {
+ if (RemainingBufferLen < ILRawVal)
+return true;
+}
}
}
}
@@ -220,7 +241,7 @@
LenArg->getSourceRange());
}
} else if (CheckerContext::isCLibraryFunction(FD, "strlcpy")) {
-if (containsBadStrlcpyPattern(CE)) {
+if (containsBadStrlcpyStrlcatPattern(CE)) {
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
PathDiagnosticLocation Loc =
@@ -234,6 +255,34 @@
if (!DstName.empty())
os << "Replace with the value 'sizeof(" << DstName << ")` or lower";
+ BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument",
+ "C String API", os.str(), Loc,
+ LenArg->getSourceRange());
+}
+ } else if (CheckerContext::isCLibraryFunction(FD, "strlcat")) {
+if (containsBadStrlcpyStrlcatPattern(CE)) {
+ const Expr *DstArg = CE->getArg(0);
+ const Expr *LenArg = CE->getArg(2);
+ PathD
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
george.karpenkov requested changes to this revision.
george.karpenkov added inline comments.
This revision now requires changes to proceed.
Comment at: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp:154
-bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) {
+bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE, bool
Append) {
if (CE->getNumArgs() != 3)
`Append` can be deduced from a call expression; I think it would be better to
drop it from the parameter list and calculate again in the function body.
Comment at: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp:164
+ // - sizeof(dst)
+ if (Append && isSizeof(LenArg, DstArg))
+return true;
I am confused on what is happening in this branch and why is it bad. Could we
add a commend?
Comment at: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp:196
uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
-if ((BufferLen - DstOff) < ILRawVal)
- return true;
+BufferLen -= DstOff;
+if (Append) {
Better to introduce a new variable, e.g. `RemainingBufferLen`
https://reviews.llvm.org/D49722
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen added a comment. ping :) https://reviews.llvm.org/D49722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen updated this revision to Diff 157381.
https://reviews.llvm.org/D49722
Files:
lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
test/Analysis/cstring-syntax.c
Index: test/Analysis/cstring-syntax.c
===
--- test/Analysis/cstring-syntax.c
+++ test/Analysis/cstring-syntax.c
@@ -7,6 +7,7 @@
char *strncat(char *, const char *, size_t);
size_t strlen (const char *s);
size_t strlcpy(char *, const char *, size_t);
+size_t strlcat(char *, const char *, size_t);
void testStrncat(const char *src) {
char dest[10];
@@ -33,3 +34,19 @@
strlcpy(dest + 5, src, 5);
strlcpy(dest + 5, src, 10); // expected-warning {{The third argument is larger than the size of the input buffer.}}
}
+
+void testStrlcat(const char *src) {
+ char dest[10];
+ size_t badlen = 10;
+ size_t ulen;
+ strlcpy(dest, "a", sizeof("a") - 1);
+ strlcat(dest, "", (sizeof("") - 1) - sizeof(dest) - 1);
+ strlcpy(dest, "012345678", sizeof(dest));
+ strlcat(dest, "910", sizeof(dest)); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value- strlen(dest) - 1 or lower}}
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcat(dest, "0123456789", badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value 'badlen' - strlen(dest) - 1 or lower}}
+ strlcat(dest, "0123456789", badlen - strlen(dest) - 1);
+ strlcat(dest, src, ulen);
+ strlcpy(dest, src, 5);
+ strlcat(dest + 5, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value 'badlen' - strlen() - 1 or lower}}
+}
Index: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -90,7 +90,16 @@
/// strlcpy(dst, "abcd", 4);
/// strlcpy(dst + 3, "abcd", 2);
/// strlcpy(dst, "abcd", cpy);
- bool containsBadStrlcpyPattern(const CallExpr *CE);
+ /// Identify erroneous patterns in the last argument to strlcat - the number
+ /// of bytes to copy.
+ /// The bad pattern checked is when the last argument is basically
+ /// pointing to the destination buffer size or argument larger or
+ /// equal to.
+ /// char dst[2];
+ /// strlcat(dst, src2, sizeof(dst));
+ /// strlcat(dst, src2, 2);
+ /// strlcat(dst, src2, 10);
+ bool containsBadStrlcpyStrlcatPattern(const CallExpr *CE, bool Append = false);
public:
WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
@@ -142,15 +151,18 @@
return false;
}
-bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) {
+bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE, bool Append) {
if (CE->getNumArgs() != 3)
return false;
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
const auto *DstArgDecl = dyn_cast(DstArg->IgnoreParenImpCasts());
const auto *LenArgDecl = dyn_cast(LenArg->IgnoreParenLValueCasts());
uint64_t DstOff = 0;
+ // - sizeof(dst)
+ if (Append && isSizeof(LenArg, DstArg))
+return true;
// - size_t dstlen = sizeof(dst)
if (LenArgDecl) {
const auto *LenArgVal = dyn_cast(LenArgDecl->getDecl());
@@ -181,8 +193,14 @@
if (const auto *Buffer = dyn_cast(DstArgDecl->getType())) {
ASTContext &C = BR.getContext();
uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
-if ((BufferLen - DstOff) < ILRawVal)
- return true;
+BufferLen -= DstOff;
+if (Append) {
+ if (BufferLen <= ILRawVal)
+return true;
+} else {
+ if (BufferLen < ILRawVal)
+return true;
+}
}
}
}
@@ -220,7 +238,7 @@
LenArg->getSourceRange());
}
} else if (CheckerContext::isCLibraryFunction(FD, "strlcpy")) {
-if (containsBadStrlcpyPattern(CE)) {
+if (containsBadStrlcpyStrlcatPattern(CE)) {
const Expr *DstArg = CE->getArg(0);
const Expr *LenArg = CE->getArg(2);
PathDiagnosticLocation Loc =
@@ -234,6 +252,34 @@
if (!DstName.empty())
os << "Replace with the value 'sizeof(" << DstName << ")` or lower";
+ BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument",
+ "C String API", os.str(), Loc,
+ LenArg->getSourceRange());
+}
+ } else if (CheckerContext::isCLibraryFunction(FD, "strlcat")) {
+if (containsBadStrlcpyStrlcatPattern(CE, true)) {
+ const Expr *DstArg = CE->getArg(0);
+ const Expr *LenArg = CE->getArg(2);
+ PathDiagnosticLocation Loc =
+PathDiagnosticLocation::createBegin(LenArg, BR.getSourceManager(), AC);
+
+ StringRef DstName = getPrintableName(DstArg);
+ StringRe
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
george.karpenkov requested changes to this revision. george.karpenkov added a comment. This revision now requires changes to proceed. This has too much duplication with respect to your previous patch, and even Phabricator highlights it in yellow. Repository: rC Clang https://reviews.llvm.org/D49722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen added a comment. Hopefully will try to push it before the freeze just announced, that s my last change in this area (except potential fixes) :) Repository: rC Clang https://reviews.llvm.org/D49722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen created this revision.
devnexen added reviewers: george.karpenkov, NoQ.
devnexen created this object with visibility "All Users".
Herald added a subscriber: cfe-commits.
- Assuming strlcat is used with strlcpy we check as we can if the last argument
does not equal os not larger than the buffer.
- Advising the proper usual pattern.
Repository:
rC Clang
https://reviews.llvm.org/D49722
Files:
lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
test/Analysis/cstring-syntax.c
Index: test/Analysis/cstring-syntax.c
===
--- test/Analysis/cstring-syntax.c
+++ test/Analysis/cstring-syntax.c
@@ -7,6 +7,7 @@
char *strncat(char *, const char *, size_t);
size_t strlen (const char *s);
size_t strlcpy(char *, const char *, size_t);
+size_t strlcat(char *, const char *, size_t);
void testStrncat(const char *src) {
char dest[10];
@@ -33,3 +34,19 @@
strlcpy(dest + 5, src, 5);
strlcpy(dest + 5, src, 10); // expected-warning {{The third argument is larger than the size of the input buffer.}}
}
+
+void testStrlcat(const char *src) {
+ char dest[10];
+ size_t badlen = 10;
+ size_t ulen;
+ strlcpy(dest, "a", sizeof("a") - 1);
+ strlcat(dest, "", (sizeof("") - 1) - sizeof(dest) - 1);
+ strlcpy(dest, "012345678", sizeof(dest));
+ strlcat(dest, "910", sizeof(dest)); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value- strlen(dest) - 1 or lower}}
+ strlcpy(dest, "0123456789", sizeof(dest));
+ strlcat(dest, "0123456789", badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value 'badlen' - strlen(dest) - 1 or lower}}
+ strlcat(dest, "0123456789", badlen - strlen(dest) - 1);
+ strlcat(dest, src, ulen);
+ strlcpy(dest, src, 5);
+ strlcat(dest + 5, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value 'badlen' - strlen() - 1 or lower}}
+}
Index: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -92,6 +92,17 @@
/// strlcpy(dst, "abcd", cpy);
bool containsBadStrlcpyPattern(const CallExpr *CE);
+ /// Identify erroneous patterns in the last argument to strlcat - the number
+ /// of bytes to copy.
+ /// The bad pattern checked is when the last argument is basically
+ /// pointing to the destination buffer size or argument larger or
+ /// equal to.
+ /// char dst[2];
+ /// strlcat(dst, src2, sizeof(dst));
+ /// strlcat(dst, src2, 2);
+ /// strlcat(dst, src2, 10);
+ bool containsBadStrlcatPattern(const CallExpr *CE);
+
public:
WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
: Checker(Checker), BR(BR), AC(AC) {}
@@ -190,6 +201,57 @@
return false;
}
+bool WalkAST::containsBadStrlcatPattern(const CallExpr *CE) {
+ if (CE->getNumArgs() != 3)
+return false;
+ const Expr *DstArg = CE->getArg(0);
+ const Expr *LenArg = CE->getArg(2);
+
+ const auto *DstArgDecl = dyn_cast(DstArg->IgnoreParenImpCasts());
+ const auto *LenArgDecl = dyn_cast(LenArg->IgnoreParenLValueCasts());
+ uint64_t DstOff = 0;
+ // - sizeof(dst)
+ if (isSizeof(LenArg, DstArg))
+return true;
+ // - size_t dstlen = sizeof(dst)
+ if (LenArgDecl) {
+const auto *LenArgVal = dyn_cast(LenArgDecl->getDecl());
+if (LenArgVal->getInit())
+ LenArg = LenArgVal->getInit();
+ }
+
+ // - integral value
+ // We try to figure out if the last argument is possibly longer or equal
+ // than the destination can possibly handle if its size can be defined.
+ if (const auto *IL = dyn_cast(LenArg->IgnoreParenImpCasts())) {
+uint64_t ILRawVal = IL->getValue().getZExtValue();
+
+// Case when there is pointer arithmetic on the destination buffer
+// especially when we offset from the base decreasing the
+// buffer length accordingly.
+if (!DstArgDecl) {
+ if (const auto *BE = dyn_cast(DstArg->IgnoreParenImpCasts())) {
+DstArgDecl = dyn_cast(BE->getLHS()->IgnoreParenImpCasts());
+if (BE->getOpcode() == BO_Add) {
+ if ((IL = dyn_cast(BE->getRHS()->IgnoreParenImpCasts( {
+DstOff = IL->getValue().getZExtValue();
+ }
+}
+ }
+}
+if (DstArgDecl) {
+ if (const auto *Buffer = dyn_cast(DstArgDecl->getType())) {
+ASTContext &C = BR.getContext();
+uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
+if ((BufferLen - DstOff) <= ILRawVal)
+ return true;
+ }
+}
+ }
+
+ return false;
+}
+
void WalkAST::VisitCallExpr(CallExpr *CE) {
const FunctionDecl *FD = CE->getDirectCallee();
if (!FD)
@@ -234,6 +296,34 @@
if (!DstName.empty())
