[PATCH v2] Bug 14526 - Make malloc() static analysis M_ZERO-aware

[Meyer, Conrad]

________________________________________
From: Jordan Rose [jordan_rose at apple.com]

> Seeing this code is really making me think we need to expose a way to
> look up macros in the analyzer, even if it only handles simple ones...

Agreed. I have one quick follow-up question, inline below.

> On Mar 14, 2014, at 9:53 , Meyer, Conrad <conrad.meyer at isilon.com> wrote:
>> +        if (!Val_M_ZERO.hasValue()) {
>> +          if (C.getASTContext().getTargetInfo().getTriple().getOS()
>> +                                                            == llvm::Triple::FreeBSD)
>> +            Val_M_ZERO = 0x0100;
>> +          else if (C.getASTContext().getTargetInfo().getTriple().getOS()
>> +                                                                 == llvm::Triple::NetBSD)
>> +            Val_M_ZERO = 0x0002;
>> +          else if (C.getASTContext().getTargetInfo().getTriple().getOS()
>> +                                                                 == llvm::Triple::OpenBSD)
>> +            Val_M_ZERO = 0x0008;
>
> Let's factor that common call out of the if-statement, so we don't
> have to wrap it. (The LLVM way to wrap it would be to put the == at
> the end of the line, then break, then indent to just past the open
> paren.)


Sure, I can fix that.

>
>> +          else
>> +            // FIXME: We need a more general way of getting the M_ZERO value.
>> +            // See also: O_CREAT in UnixAPIChecker.cpp.
>> +
>> +            // Fall back to normal malloc behavior on platforms where we don't
>> +            // know M_ZERO.
>> +            State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);
>> +        }
>> +
>> +        if (Val_M_ZERO.hasValue()) {
>> +          const Expr *FlagsEx = CE->getArg(2);
>> +          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;
>
> This could be unknown if the argument value is unknown (i.e. the
> analyzer has failed to symbolize it for some reason). In that case, we
> should probably still treat this as a regular malloc.


Agreed. It's probably obvious, but I'm not super familiar with Clang internals — should we also consider the previous case (!V.getAs<NonLoc>()) as regular malloc as well?

>
>> +          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);
>> +        }
>> +      }
>
> The "fall-back" case has been duplicated twice already, and I just
> suggested adding it a third time. How about adding a helper function
> for three-argument malloc that returns an Optional<ProgramStateRef>,
> and if it returns None, we just do the normal malloc thing? That way
> we can use early returns to simplify all this.
>
> (Why Optional<ProgramStateRef>? In case there's ever a reason for
> three-argument malloc to return a null state, i.e. it detects some
> state that shouldn't be reached.)

This sounds like the right approach. Okay.

>
>> +    } else if (FunI == II_valloc) {
>>       if (CE->getNumArgs() < 1)
>>         return;
>>       State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);
>
> This is generally looking good. Thanks for bearing with me on these
> change requests!
>
> Jordan

Thanks! I appreciate your patience with me — it is looking a lot better than when we started.

Conrad