[PATCH] Fix bug 14526 - Make malloc() static analysis M_ZERO-aware

[Meyer, Conrad]

From: Jordan Rose [jordan_rose at apple.com]
Sent: Thursday, March 13, 2014 8:46 PM
To: Meyer, Conrad
Cc: cfe-commits at cs.uiuc.edu
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 <conrad.meyer at isilon.com> 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