[cfe-commits] [PATCH 1/1] Checking zero byte allocation for realloc() and calloc().
Ted Kremenek
kremenek at apple.com
Mon Dec 5 22:33:19 PST 2011
On Nov 19, 2011, at 9:07 PM, Cyril Roelandt wrote:
> The code should look a little bit better now. Three different functions are used. The 0-checking and report emission are now two different functions, for I thought they served a different purpose, and that the code would be easier to understand that way.
Hi Cyril,
I like this patch. I have a few specific comments inline:
> diff --git a/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp b/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
> index e955f9e..6a4d63f 100644
> --- a/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
> +++ b/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
> @@ -36,10 +36,17 @@ public:
>
> void CheckOpen(CheckerContext &C, const CallExpr *CE) const;
> void CheckPthreadOnce(CheckerContext &C, const CallExpr *CE) const;
> + void CheckCallocZero(CheckerContext &C, const CallExpr *CE) const;
> void CheckMallocZero(CheckerContext &C, const CallExpr *CE) const;
> + void CheckReallocZero(CheckerContext &C, const CallExpr *CE) const;
>
> typedef void (UnixAPIChecker::*SubChecker)(CheckerContext &,
> const CallExpr *) const;
> +private:
> + void ReportZeroByteAllocation(CheckerContext &C,
> + const ExplodedNode *N,
> + const Expr *arg,
> + const char *fn_name) const;
> };
> } //end anonymous namespace
>
> @@ -170,48 +177,138 @@ void UnixAPIChecker::CheckPthreadOnce(CheckerContext &C,
> }
>
> //===----------------------------------------------------------------------===//
> -// "malloc" with allocation size 0
> +//"calloc", "malloc" and "realloc" with allocation size 0
Very minor: please maintain the space here between the '//' and the first letter.
> //===----------------------------------------------------------------------===//
>
> +// Fills in trueState and falseState
Minor: please follow the LLVM convention of ending comments with a '.'
> +static int IsZeroByteAllocation(const ProgramState *state,
> + const SVal argVal,
> + const ProgramState **trueState,
> + const ProgramState **falseState) {
> + llvm::tie(*trueState, *falseState) = state->assume(cast<DefinedSVal>(argVal));
> + return (*falseState && !*trueState);
> +}
Please have this method return 'bool'. This is C++. :)
Also, please have a comment indicate what the return value indicates.
> +
> +// N should be non-null here.
Please expand the comments, indicating what this function does.
Also, instead of having a comment that says "N should be non-null", how about
using __attribute__((nonnull))?
> +void UnixAPIChecker::ReportZeroByteAllocation(CheckerContext &C,
> + const ExplodedNode *N,
> + const Expr *arg,
> + const char *fn_name) const {
> + // FIXME: Add reference to CERT advisory, and/or C99 standard in bug
> + // output.
> + LazyInitialize(BT_mallocZero, "Undefined allocation of 0 bytes");
> +
> + llvm::SmallString<256> S;
> + llvm::raw_svector_ostream os(S);
> + os << "Call to '" << fn_name << "' has an allocation size of 0 bytes";
> + BugReport *report = new BugReport(*BT_mallocZero, os.str(), N);
> +
> + report->addRange(arg->getSourceRange());
> + report->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, arg));
> + C.EmitReport(report);
> +}
Looks good.
> +
> +void UnixAPIChecker::CheckCallocZero(CheckerContext &C,
> + const CallExpr *CE) const {
> + if (CE->getNumArgs() != 2)
> + return;
> +
> + SVal argVal;
> + const Expr *arg;
> + const ProgramState *state = C.getState();
> + const ProgramState *trueState = NULL, *falseState = NULL;
> +
> + /* Check if one of the arguments is equal to 0 */
> + arg = CE->getArg(0);
> + argVal = state->getSVal(arg);
This is unnecessarily verbose. You can just tie declaration and initialization on one line. All, please use '//' style comments, not /* *..
For example:
const ProgramState *state = C.getState();
const ProgramState *trueState = 0, *falseState = 0;
// Check if one of the arguments is equal to 0.
const Expr *arg = CE->getArg(0);
SVal argVal = state->getSVal(arg);
> + if (argVal.isUnknownOrUndef())
> + goto check_snd_arg;
> +
> + if (IsZeroByteAllocation(state, argVal, &trueState, &falseState)) {
> + ExplodedNode *N = C.generateSink(falseState);
> + if (!N)
> + goto check_snd_arg;
> +
> + UnixAPIChecker::ReportZeroByteAllocation(C, N, arg, "calloc");
> + return;
> + }
I'm not a huge fan of gotos. They stylistically are problematic in C++. It's okay here, but I still think it is a little gross. You could alternatively have a "do ... while(0)" and use "break" statements instead. It's a little gross as well (to some people), but it is more structured.
> +
> +check_snd_arg:
> + arg = CE->getArg(1);
> + argVal = state->getSVal(arg);
> + if (argVal.isUnknownOrUndef())
> + return;
> +
> + if (IsZeroByteAllocation(state, argVal, &trueState, &falseState)) {
> + ExplodedNode *N = C.generateSink(falseState);
> + if (!N)
> + return;
> +
> + UnixAPIChecker::ReportZeroByteAllocation(C, N, arg, "calloc");
Why do we need this fully qualified name? Why not just 'ReportZeroByteAllocation()'?
> + return;
> + }
If we have 'ReportZeroByteAllocation() generate the sink node and do the null check, you could just do:
if (IsZeroByteAllocation(state, argVal, &trueState, &falseState) {
UnixAPIChecker::ReportZeroByteAllocation(C, falseState, arg, "calloc");
return;
}
Same thing for the other callsites.
> +
> + // Assume the the value is non-zero going forward.
> + assert(trueState);
> + if (trueState != state) {
> + C.addTransition(trueState);
> + }
Can just be made:
if (trueState != state)
C.addTransition(trueState);
No need for the extra braces.
> +}
> +
> // FIXME: Eventually this should be rolled into the MallocChecker, but this
> // check is more basic and is valuable for widespread use.
> void UnixAPIChecker::CheckMallocZero(CheckerContext &C,
> const CallExpr *CE) const {
> -
> // Sanity check that malloc takes one argument.
> if (CE->getNumArgs() != 1)
> return;
>
> // Check if the allocation size is 0.
> const ProgramState *state = C.getState();
> - SVal argVal = state->getSVal(CE->getArg(0));
> + const ProgramState *trueState = NULL, *falseState = NULL;
> + const Expr *arg = CE->getArg(0);
> + SVal argVal = state->getSVal(arg);
>
> if (argVal.isUnknownOrUndef())
> return;
> -
> - const ProgramState *trueState, *falseState;
> - llvm::tie(trueState, falseState) = state->assume(cast<DefinedSVal>(argVal));
> -
> +
> // Is the value perfectly constrained to zero?
> - if (falseState && !trueState) {
> + if (IsZeroByteAllocation(state, argVal, &trueState, &falseState)) {
> ExplodedNode *N = C.generateSink(falseState);
> - if (!N)
> - return;
> -
> - // FIXME: Add reference to CERT advisory, and/or C99 standard in bug
> - // output.
> + if (N) {
> + UnixAPIChecker::ReportZeroByteAllocation(C, N, arg, "malloc");
Why do we need this fully qualified name? Why not just 'ReportZeroByteAllocation()'?
> + }
> + return;
Same comment as before. If we have ReportZeroByteAllocation() do the generation of the node, you can compress this code.
> + }
> + // Assume the the value is non-zero going forward.
> + assert(trueState);
> + if (trueState != state) {
> + C.addTransition(trueState);
> + }
Ditch the extra braces.
> +}
>
> - LazyInitialize(BT_mallocZero, "Undefined allocation of 0 bytes");
> -
> - BugReport *report =
> - new BugReport(*BT_mallocZero, "Call to 'malloc' has an allocation"
> - " size of 0 bytes", N);
> - report->addRange(CE->getArg(0)->getSourceRange());
> - report->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N,
> - CE->getArg(0)));
> - C.EmitReport(report);
> +void UnixAPIChecker::CheckReallocZero(CheckerContext &C,
> + const CallExpr *CE) const {
> + if (CE->getNumArgs() != 2)
> + return;
> +
> + const ProgramState *state = C.getState();
> + const ProgramState *trueState = NULL, *falseState = NULL;
> + const Expr *arg = CE->getArg(1);
> + SVal argVal = state->getSVal(arg);
> +
> + if (argVal.isUnknownOrUndef())
> + return;
> +
> +
We don't need two blank lines.
> + if (IsZeroByteAllocation(state, argVal, &trueState, &falseState)) {
> + ExplodedNode *N = C.generateSink(falseState);
> + if (N) {
> + UnixAPIChecker::ReportZeroByteAllocation(C, N, arg, "realloc");
> + }
> return;
Per comment above, this can be made more condensed by sinking the node generation into ReportZeroByteAllocation().
> }
> +
> // Assume the the value is non-zero going forward.
> assert(trueState);
> if (trueState != state) {
> @@ -232,7 +329,9 @@ void UnixAPIChecker::checkPreStmt(const CallExpr *CE, CheckerContext &C) const {
> llvm::StringSwitch<SubChecker>(FName)
> .Case("open", &UnixAPIChecker::CheckOpen)
> .Case("pthread_once", &UnixAPIChecker::CheckPthreadOnce)
> + .Case("calloc", &UnixAPIChecker::CheckCallocZero)
> .Case("malloc", &UnixAPIChecker::CheckMallocZero)
> + .Case("realloc", &UnixAPIChecker::CheckReallocZero)
> .Default(NULL);
>
> if (SC)
> diff --git a/test/Analysis/unix-fns.c b/test/Analysis/unix-fns.c
> index cf5ca81..86b95c6 100644
> --- a/test/Analysis/unix-fns.c
> +++ b/test/Analysis/unix-fns.c
> @@ -9,7 +9,9 @@ typedef __darwin_pthread_once_t pthread_once_t;
> int pthread_once(pthread_once_t *, void (*)(void));
> typedef long unsigned int __darwin_size_t;
> typedef __darwin_size_t size_t;
> +void *calloc(size_t, size_t);
> void *malloc(size_t);
> +void *realloc(void *, size_t);
>
> typedef void (^dispatch_block_t)(void);
> typedef long dispatch_once_t;
> @@ -66,3 +68,33 @@ void pr2899_nowarn(size_t size) {
> foo[i] = 0;
> }
> }
> +void test_calloc(void) {
> + char *foo = calloc(0, 42); // expected-warning{{Call to 'calloc' has an allocation size of 0 bytes}}
> + for (unsigned i = 0; i < 100; i++) {
> + foo[i] = 0;
> + }
> +}
> +void test_calloc2(void) {
> + char *foo = calloc(42, 0); // expected-warning{{Call to 'calloc' has an allocation size of 0 bytes}}
> + for (unsigned i = 0; i < 100; i++) {
> + foo[i] = 0;
> + }
> +}
> +void test_calloc_nowarn(size_t nmemb, size_t size) {
> + char *foo = calloc(nmemb, size); // no-warning
> + for (unsigned i = 0; i < 100; i++) {
> + foo[i] = 0;
> + }
> +}
> +void test_realloc(char *ptr) {
> + char *foo = realloc(ptr, 0); // expected-warning{{Call to 'realloc' has an allocation size of 0 bytes}}
> + for (unsigned i = 0; i < 100; i++) {
> + foo[i] = 0;
> + }
> +}
> +void test_realloc_nowarn(char *ptr, size_t size) {
> + char *foo = realloc(ptr, size); // no-warning
> + for (unsigned i = 0; i < 100; i++) {
> + foo[i] = 0;
> + }
> +}
Test cases look good.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111205/5777541d/attachment.html>
More information about the cfe-commits
mailing list