From: Jordan Rose [[email protected]] Sent: Thursday, March 13, 2014 8:46 PM To: Meyer, Conrad Cc: [email protected] Subject: Re: [PATCH] Fix bug 14526 - Make malloc() static analysis M_ZERO-aware
> Hi, Conrad. Thanks for picking this up. I have a few comments on the patch, > and of course you'll need to include a regression test too before it can be > committed. You can see how these work in test/Analysis/malloc.c; feel free to > make a new test file so that you can declare the kernel version of malloc > without conflicting with the normal one. Cool, will do. > On Mar 13, 2014, at 10:28 , Meyer, Conrad <[email protected]> wrote: > > @@ -586,7 +588,55 @@ void MallocChecker::checkPostStmt(const CallExpr *CE, > > CheckerContext &C) const { > > initIdentifierInfo(C.getASTContext()); > > IdentifierInfo *FunI = FD->getIdentifier(); > > > > - if (FunI == II_malloc || FunI == II_valloc) { > > + if (FunI == II_malloc) { > > + if (CE->getNumArgs() < 1) > > + return; > > + if (CE->getNumArgs() < 2) { > > + State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State); > > + } else if (CE->getNumArgs() == 3) { > > + // This logic is largely cloned from O_CREAT in UnixAPIChecker, > > maybe some > > + // code could be shared. > Most people have probably never seen a three-argument malloc. Mentioning that > it's from the FreeBSD kernel (and possibly other kernels?) and possibly > including the declaration in a comment would go a long way towards explaining > why this code is here at all. Can do. Looks like NetBSD also has it, although there it is 0x0002. And on OpenBSD it is 0x0008. > > + > > + if (!Val_M_ZERO.hasValue()) { > > + if (C.getASTContext().getTargetInfo().getTriple().getOS() > > + == > > llvm::Triple::FreeBSD) > > + Val_M_ZERO = 0x00100; > > + else > > + // FIXME: We need a more general way of getting the M_ZERO > > value. > > + // See also: O_CREAT in UnixAPIChecker.cpp. > > + return; > > + } > I don't think "return" is appropriate here. Isn't it better to fall back to > regular malloc() if we don't know what M_ZERO is on this platform? (In the > O_CREAT case, the warning was only about misusing O_CREAT, so it didn't > matter that we were bailing out early.) Sure, that seems reasonable. > > + > > + ProgramStateRef state = C.getState(); > > + const Expr *flagsEx = CE->getArg(2); > Please follow the LLVM naming conventions: capital letters for all variables > as well as types. (This applies further down as well.) Okay, will fix. > > + const SVal V = state->getSVal(flagsEx, C.getLocationContext()); > > + if (!V.getAs<NonLoc>()) { > > + // The case where 'V' can be a location can only be due to a bad > > header, > > + // so in this case bail out. > > + return; > > + } > > + > > + NonLoc flags = V.castAs<NonLoc>(); > > + NonLoc zeroFlag = C.getSValBuilder() > > + .makeIntVal(Val_M_ZERO.getValue(), > > flagsEx->getType()).castAs<NonLoc>(); > > + SVal maskedFlagsUC = C.getSValBuilder().evalBinOpNN(state, BO_And, > > + flags, > > zeroFlag, > > + > > flagsEx->getType()); > > + if (maskedFlagsUC.isUnknownOrUndef()) > > + return; > > + DefinedSVal maskedFlags = maskedFlagsUC.castAs<DefinedSVal>(); > > + > > + // Check if maskedFlags is non-zero. > > + ProgramStateRef trueState, falseState; > > + std::tie(trueState, falseState) = state->assume(maskedFlags); > > + > > + // If M_ZERO is set, treat this like calloc (initialized). > > + if (!(trueState && !falseState)) > > + State = CallocMem(C, CE); > > + else > > + State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), > > State); > You need to be a bit more careful here...the if-check is exactly backwards > from what you want. In more detail, there are three possible cases: > > - M_ZERO is known to be set: trueState exists, but falseState doesn't. > - M_ZERO is known not to be set: falseState exists, but trueState doesn't > - The analyzer isn't sure: both states exist Ah, you're right. > In the latter two states, we should probably assume the existing behavior > (malloc returns undefined). Agreed. > Thanks for working on this! > Jordan Thanks for reviewing. I'll try and clean this up and get a fresh patch submitted tomorrow. Conrad _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
