[PATCH] D25876: [analyzer] Report CFNumberGetValue API misuse
This revision was automatically updated to reflect the committed changes.
Closed by commit rL285253: [analyzer] Report CFNumberGetValue API misuse
(authored by zaks).
Changed prior to commit:
https://reviews.llvm.org/D25876?vs=75488&id=75961#toc
Repository:
rL LLVM
https://reviews.llvm.org/D25876
Files:
cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
cfe/trunk/test/Analysis/CFNumber.c
Index: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -586,8 +586,8 @@
let ParentPackage = CoreFoundation in {
-def CFNumberCreateChecker : Checker<"CFNumber">,
- HelpText<"Check for proper uses of CFNumberCreate">,
+def CFNumberChecker : Checker<"CFNumber">,
+ HelpText<"Check for proper uses of CFNumber APIs">,
DescFile<"BasicObjCFoundationChecks.cpp">;
def CFRetainReleaseChecker : Checker<"CFRetainRelease">,
Index: cfe/trunk/test/Analysis/CFNumber.c
===
--- cfe/trunk/test/Analysis/CFNumber.c
+++ cfe/trunk/test/Analysis/CFNumber.c
@@ -13,21 +13,35 @@
kCFNumberMaxType = 16 };
typedef CFIndex CFNumberType;
typedef const struct __CFNumber * CFNumberRef;
+typedef unsigned char Boolean;
extern CFNumberRef CFNumberCreate(CFAllocatorRef allocator, CFNumberType theType, const void *valuePtr);
+Boolean CFNumberGetValue(CFNumberRef number, CFNumberType theType, void *valuePtr);
-CFNumberRef f1(unsigned char x) {
- return CFNumberCreate(0, kCFNumberSInt16Type, &x); // expected-warning{{An 8 bit integer is used to initialize a CFNumber object that represents a 16 bit integer. 8 bits of the CFNumber value will be garbage}}
+__attribute__((cf_returns_retained)) CFNumberRef f1(unsigned char x) {
+ return CFNumberCreate(0, kCFNumberSInt16Type, &x); // expected-warning{{An 8-bit integer is used to initialize a CFNumber object that represents a 16-bit integer; 8 bits of the CFNumber value will be garbage}}
}
__attribute__((cf_returns_retained)) CFNumberRef f2(unsigned short x) {
- return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{A 16 bit integer is used to initialize a CFNumber object that represents an 8 bit integer. 8 bits of the input integer will be lost}}
+ return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{A 16-bit integer is used to initialize a CFNumber object that represents an 8-bit integer; 8 bits of the integer value will be lost}}
}
// test that the attribute overrides the naming convention.
__attribute__((cf_returns_not_retained)) CFNumberRef CreateNum(unsigned char x) {
return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{leak}}
}
-CFNumberRef f3(unsigned i) {
- return CFNumberCreate(0, kCFNumberLongType, &i); // expected-warning{{A 32 bit integer is used to initialize a CFNumber object that represents a 64 bit integer}}
+__attribute__((cf_returns_retained)) CFNumberRef f3(unsigned i) {
+ return CFNumberCreate(0, kCFNumberLongType, &i); // expected-warning{{A 32-bit integer is used to initialize a CFNumber object that represents a 64-bit integer}}
+}
+
+unsigned char getValueTest1(CFNumberRef x) {
+ unsigned char scalar = 0;
+ CFNumberGetValue(x, kCFNumberSInt16Type, &scalar); // expected-warning{{A CFNumber object that represents a 16-bit integer is used to initialize an 8-bit integer; 8 bits of the CFNumber value will overwrite adjacent storage}}
+ return scalar;
+}
+
+unsigned char getValueTest2(CFNumberRef x) {
+ unsigned short scalar = 0;
+ CFNumberGetValue(x, kCFNumberSInt8Type, &scalar); // expected-warning{{A CFNumber object that represents an 8-bit integer is used to initialize a 16-bit integer; 8 bits of the integer value will be garbage}}
+ return scalar;
}
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
@@ -336,15 +336,15 @@
}
//===--===//
-// Error reporting.
+// Checking for mismatched types passed to CFNumberCreate/CFNumberGetValue.
//===--===//
namespace {
-class CFNumberCreateChecker : public Checker< check::PreStmt > {
+class CFNumberChecker : public Checker< check::PreStmt > {
mutable std::unique_ptr BT;
- mutable IdentifierInfo* II;
+ mutable IdentifierInfo *ICreate, *IGetValue;
public:
- CFNumberCreateChecker() : II(nullptr) {}
+ CFNumberChecker() : ICreate(nullptr), IGetValue(nullptr) {}
void checkPreStmt(const CallExpr *CE, CheckerConte
[PATCH] D25876: [analyzer] Report CFNumberGetValue API misuse
zaks.anna added inline comments.
Comment at: test/Analysis/CFNumber.c:39
+ unsigned char scalar = 0;
+ CFNumberGetValue(x, kCFNumberSInt16Type, &scalar); // expected-warning{{A
CFNumber object that represents a 16-bit integer is used to initialize an 8-bit
integer; 8 bits of the CFNumber value will overwrite adjacent storage}}
+ return scalar;
NoQ wrote:
> We're not sure from this code if the `CFNumber` object `x` actually
> represents a 16-bit integer, or somebody just misplaced the
> `kCFNumberSInt16Type` thing. I think the warning message could be made more
> precise in this sence, but i'm not good at coming up with warning messages.
>
> Hmm, there could actually be a separate check for detecting inconsistent type
> specifiers used for accessing the same CFNumber object.
I see your point. Looks like we'd need to modify both first part of the
sentence and the second to address this concern. We could do something like "A
CFNumber object treated as if it represents a 16-bit integer is used to
initialize an 8-bit integer; 8 bits of the CFNumber value or the adjacent
storage will overwrite adjacent storage of the integer".
Though this is more correct, I do not think it's worth the new language
complexity. Also, the warning message is already quite long.
https://reviews.llvm.org/D25876
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25876: [analyzer] Report CFNumberGetValue API misuse
NoQ added inline comments.
Comment at: test/Analysis/CFNumber.c:39
+ unsigned char scalar = 0;
+ CFNumberGetValue(x, kCFNumberSInt16Type, &scalar); // expected-warning{{A
CFNumber object that represents a 16-bit integer is used to initialize an 8-bit
integer; 8 bits of the CFNumber value will overwrite adjacent storage}}
+ return scalar;
We're not sure from this code if the `CFNumber` object `x` actually represents
a 16-bit integer, or somebody just misplaced the `kCFNumberSInt16Type` thing. I
think the warning message could be made more precise in this sence, but i'm not
good at coming up with warning messages.
Hmm, there could actually be a separate check for detecting inconsistent type
specifiers used for accessing the same CFNumber object.
https://reviews.llvm.org/D25876
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25876: [analyzer] Report CFNumberGetValue API misuse
zaks.anna updated this revision to Diff 75488.
zaks.anna added a comment.
Address comments from Devin.
https://reviews.llvm.org/D25876
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
test/Analysis/CFNumber.c
Index: test/Analysis/CFNumber.c
===
--- test/Analysis/CFNumber.c
+++ test/Analysis/CFNumber.c
@@ -13,21 +13,35 @@
kCFNumberMaxType = 16 };
typedef CFIndex CFNumberType;
typedef const struct __CFNumber * CFNumberRef;
+typedef unsigned char Boolean;
extern CFNumberRef CFNumberCreate(CFAllocatorRef allocator, CFNumberType theType, const void *valuePtr);
+Boolean CFNumberGetValue(CFNumberRef number, CFNumberType theType, void *valuePtr);
-CFNumberRef f1(unsigned char x) {
- return CFNumberCreate(0, kCFNumberSInt16Type, &x); // expected-warning{{An 8 bit integer is used to initialize a CFNumber object that represents a 16 bit integer. 8 bits of the CFNumber value will be garbage}}
+__attribute__((cf_returns_retained)) CFNumberRef f1(unsigned char x) {
+ return CFNumberCreate(0, kCFNumberSInt16Type, &x); // expected-warning{{An 8-bit integer is used to initialize a CFNumber object that represents a 16-bit integer; 8 bits of the CFNumber value will be garbage}}
}
__attribute__((cf_returns_retained)) CFNumberRef f2(unsigned short x) {
- return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{A 16 bit integer is used to initialize a CFNumber object that represents an 8 bit integer. 8 bits of the input integer will be lost}}
+ return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{A 16-bit integer is used to initialize a CFNumber object that represents an 8-bit integer; 8 bits of the integer value will be lost}}
}
// test that the attribute overrides the naming convention.
__attribute__((cf_returns_not_retained)) CFNumberRef CreateNum(unsigned char x) {
return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{leak}}
}
-CFNumberRef f3(unsigned i) {
- return CFNumberCreate(0, kCFNumberLongType, &i); // expected-warning{{A 32 bit integer is used to initialize a CFNumber object that represents a 64 bit integer}}
+__attribute__((cf_returns_retained)) CFNumberRef f3(unsigned i) {
+ return CFNumberCreate(0, kCFNumberLongType, &i); // expected-warning{{A 32-bit integer is used to initialize a CFNumber object that represents a 64-bit integer}}
+}
+
+unsigned char getValueTest1(CFNumberRef x) {
+ unsigned char scalar = 0;
+ CFNumberGetValue(x, kCFNumberSInt16Type, &scalar); // expected-warning{{A CFNumber object that represents a 16-bit integer is used to initialize an 8-bit integer; 8 bits of the CFNumber value will overwrite adjacent storage}}
+ return scalar;
+}
+
+unsigned char getValueTest2(CFNumberRef x) {
+ unsigned short scalar = 0;
+ CFNumberGetValue(x, kCFNumberSInt8Type, &scalar); // expected-warning{{A CFNumber object that represents an 8-bit integer is used to initialize a 16-bit integer; 8 bits of the integer value will be garbage}}
+ return scalar;
}
Index: lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
===
--- lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
+++ lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
@@ -336,15 +336,15 @@
}
//===--===//
-// Error reporting.
+// Checking for mismatched types passed to CFNumberCreate/CFNumberGetValue.
//===--===//
namespace {
-class CFNumberCreateChecker : public Checker< check::PreStmt > {
+class CFNumberChecker : public Checker< check::PreStmt > {
mutable std::unique_ptr BT;
- mutable IdentifierInfo* II;
+ mutable IdentifierInfo *ICreate, *IGetValue;
public:
- CFNumberCreateChecker() : II(nullptr) {}
+ CFNumberChecker() : ICreate(nullptr), IGetValue(nullptr) {}
void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
@@ -425,18 +425,20 @@
}
#endif
-void CFNumberCreateChecker::checkPreStmt(const CallExpr *CE,
+void CFNumberChecker::checkPreStmt(const CallExpr *CE,
CheckerContext &C) const {
ProgramStateRef state = C.getState();
const FunctionDecl *FD = C.getCalleeDecl(CE);
if (!FD)
return;
ASTContext &Ctx = C.getASTContext();
- if (!II)
-II = &Ctx.Idents.get("CFNumberCreate");
-
- if (FD->getIdentifier() != II || CE->getNumArgs() != 3)
+ if (!ICreate) {
+ICreate = &Ctx.Idents.get("CFNumberCreate");
+IGetValue = &Ctx.Idents.get("CFNumberGetValue");
+ }
+ if (!(FD->getIdentifier() == ICreate || FD->getIdentifier() == IGetValue) ||
+ CE->getNumArgs() != 3)
return;
// Get the value of the "theType" argument.
@@ -450,13 +452,13 @@
return;
uint64_t NumberKind = V->getValue().getLimit
[PATCH] D25876: [analyzer] Report CFNumberGetValue API misuse
dcoughlin added a comment. LGTM! https://reviews.llvm.org/D25876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25876: [analyzer] Report CFNumberGetValue API misuse
dcoughlin added inline comments.
Comment at: test/Analysis/CFNumber.c:39
+ unsigned char scalar = 0;
+ CFNumberGetValue(x, kCFNumberSInt16Type, &scalar); // expected-warning{{A
CFNumber object that represents a 16 bit integer is used to initialize an 8 bit
integer. 8 bits of the CFNumber value will be lost}}
+ return scalar;
I feel like this diagnostic is not dire enough. The problem is not that the
bits will be lost -- but rather that they will be overwritten on top of
something else!
How about something like "The remaining 8 bits will overwrite adjacent
storage."?
Comment at: test/Analysis/CFNumber.c:45
+ unsigned short scalar = 0;
+ CFNumberGetValue(x, kCFNumberSInt8Type, &scalar); // expected-warning{{A
CFNumber object that represents an 8 bit integer is used to initialize a 16 bit
integer. 8 bits of the integer value will be garbage}}
+ return scalar;
Some grammar/style nits. (I realize these also hold for the pre-existing
checks):
- There should be a dash between the number and 'bit'. That is, it should be
"16-bit" and "8-bit".
- It is generally considered bad style to start a sentence with a number that
is not written out (except for dates). How about starting the second sentence
with "The remaining 8 bits ..."?
https://reviews.llvm.org/D25876
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25876: [analyzer] Report CFNumberGetValue API misuse
zaks.anna created this revision.
zaks.anna added reviewers: dcoughlin, NoQ.
zaks.anna added subscribers: cfe-commits, rgov.
This patch contains 2 improvements to the CFNumber checker:
- Checking of CFNumberGetValue misuse.
- Treating all CFNumber API misuse errors as non-fatal. (Previously we treated
errors that could cause uninitialized memory as syncs and the truncation errors
as non-fatal.)
This implements a subset of functionality from https://reviews.llvm.org/D17954.
https://reviews.llvm.org/D25876
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
test/Analysis/CFNumber.c
Index: test/Analysis/CFNumber.c
===
--- test/Analysis/CFNumber.c
+++ test/Analysis/CFNumber.c
@@ -13,21 +13,35 @@
kCFNumberMaxType = 16 };
typedef CFIndex CFNumberType;
typedef const struct __CFNumber * CFNumberRef;
+typedef unsigned char Boolean;
extern CFNumberRef CFNumberCreate(CFAllocatorRef allocator, CFNumberType theType, const void *valuePtr);
+Boolean CFNumberGetValue(CFNumberRef number, CFNumberType theType, void *valuePtr);
-CFNumberRef f1(unsigned char x) {
+__attribute__((cf_returns_retained)) CFNumberRef f1(unsigned char x) {
return CFNumberCreate(0, kCFNumberSInt16Type, &x); // expected-warning{{An 8 bit integer is used to initialize a CFNumber object that represents a 16 bit integer. 8 bits of the CFNumber value will be garbage}}
}
__attribute__((cf_returns_retained)) CFNumberRef f2(unsigned short x) {
- return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{A 16 bit integer is used to initialize a CFNumber object that represents an 8 bit integer. 8 bits of the input integer will be lost}}
+ return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{A 16 bit integer is used to initialize a CFNumber object that represents an 8 bit integer. 8 bits of the integer value will be lost}}
}
// test that the attribute overrides the naming convention.
__attribute__((cf_returns_not_retained)) CFNumberRef CreateNum(unsigned char x) {
return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{leak}}
}
-CFNumberRef f3(unsigned i) {
+__attribute__((cf_returns_retained)) CFNumberRef f3(unsigned i) {
return CFNumberCreate(0, kCFNumberLongType, &i); // expected-warning{{A 32 bit integer is used to initialize a CFNumber object that represents a 64 bit integer}}
}
+
+unsigned char getValueTest1(CFNumberRef x) {
+ unsigned char scalar = 0;
+ CFNumberGetValue(x, kCFNumberSInt16Type, &scalar); // expected-warning{{A CFNumber object that represents a 16 bit integer is used to initialize an 8 bit integer. 8 bits of the CFNumber value will be lost}}
+ return scalar;
+}
+
+unsigned char getValueTest2(CFNumberRef x) {
+ unsigned short scalar = 0;
+ CFNumberGetValue(x, kCFNumberSInt8Type, &scalar); // expected-warning{{A CFNumber object that represents an 8 bit integer is used to initialize a 16 bit integer. 8 bits of the integer value will be garbage}}
+ return scalar;
+}
Index: lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
===
--- lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
+++ lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
@@ -336,15 +336,15 @@
}
//===--===//
-// Error reporting.
+// Checking for mismatched types passed to CFNumberCreate/CFNumberGetValue.
//===--===//
namespace {
-class CFNumberCreateChecker : public Checker< check::PreStmt > {
+class CFNumberChecker : public Checker< check::PreStmt > {
mutable std::unique_ptr BT;
- mutable IdentifierInfo* II;
+ mutable IdentifierInfo *ICreate, *IGetValue;
public:
- CFNumberCreateChecker() : II(nullptr) {}
+ CFNumberChecker() : ICreate(nullptr), IGetValue(nullptr) {}
void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
@@ -425,18 +425,20 @@
}
#endif
-void CFNumberCreateChecker::checkPreStmt(const CallExpr *CE,
+void CFNumberChecker::checkPreStmt(const CallExpr *CE,
CheckerContext &C) const {
ProgramStateRef state = C.getState();
const FunctionDecl *FD = C.getCalleeDecl(CE);
if (!FD)
return;
ASTContext &Ctx = C.getASTContext();
- if (!II)
-II = &Ctx.Idents.get("CFNumberCreate");
-
- if (FD->getIdentifier() != II || CE->getNumArgs() != 3)
+ if (!ICreate) {
+ICreate = &Ctx.Idents.get("CFNumberCreate");
+IGetValue = &Ctx.Idents.get("CFNumberGetValue");
+ }
+ if (!(FD->getIdentifier() == ICreate || FD->getIdentifier() == IGetValue) ||
+ CE->getNumArgs() != 3)
return;
// Get the value of the "theType" argument.
@@ -450,13 +452,13 @@
return;
uint64_t NumberKind = V->getValue().getLimitedValue();
- Option
