[cfe-dev] Suggested starter bug on clang analyzer codebase

[Artem Dergachev via cfe-dev]

On 4/15/17 8:21 PM, Devin Coughlin wrote:
>> On Apr 15, 2017, at 10:15 AM, Artem Dergachev <noqnoqneo at gmail.com> wrote:
>>
>> The other part of the "check if the return value was checked" is the escape problem. For example, if we have:
>>
>>   int result = pthread_mutex_destroy(&m);
>>   if (is_bad_result(result)) {
>>     pthread_mutex_lock(&m);
>>   }
>>
>> and body of is_bad_result() is not modeled, then the return value symbol is dead and unconstrained, but we may still need to suppress the checker's warning. In this case, we say that the symbol "escapes" to an unknown function (it may also escape to a global variable, for example, or be passed into the function as part of a structure or an array)
>>
>> The real problem here is, we have a convenient checkPointerEscape callback that handles situations when a pointer "escapes" in any way, which we use to suppress memory leak reports for escaped pointers, but we don't have a similar callback for non-pointers. So it's not easy to track escapes in this case. I'm all for implementing such callback.
> Artem makes a very good point — but I would suggest not tackling adding this new callback as part of the starter bug.
>
> Devin

Yep, i agree. The example i point out is unlikely; the only reasonable 
case i imagine is something like if (EXPECT(result)), but we model that 
in the builtin function checker. But generally, if we commit more effort 
into "unchecked return value" checks, we'd have to deal with this somehow.

>>
>> On 4/15/17 7:14 PM, Devin Coughlin via cfe-dev wrote:
>>>> On Apr 15, 2017, at 3:52 AM, Malhar Thakkar <cs13b1031 at iith.ac.in <mailto:cs13b1031 at iith.ac.in>> wrote:
>>>>
>>>> Dear Dr. Devin,
>>>>
>>>> I am storing the return value of pthread_mutex_destroy() (as a SymbolRef) and mapping it to the corresponding mutex's MemRegion. While trying to access this SymbolRef and checking for constraints on this SymbolRef, I am unable to produce the desired output.
>>>>
>>>> The changes made are mentioned below.
>>>>
>>>>
>>>> REGISTER_MAP_WITH_PROGRAMSTATE(RetValConstraint, const MemRegion *, SymbolRef) // Map to store SymbolRef                                     corresponding to a mutex's MemRegion
>>>>
>>>> PthreadLockChecker::AcquireLock(CheckerContext &C, const CallExpr *CE,
>>>>                                    SVal lock, bool isTryLock,
>>>>                                    enum LockingSemantics semantics) const {
>>>> const MemRegion *lockR = lock.getAsRegion();
>>>> if (!lockR)
>>>>   return;
>>>>
>>>> ProgramStateRef state = C.getState();
>>>>
>>>> SVal X = state->getSVal(CE, C.getLocationContext());
>>>> if (X.isUnknownOrUndef())
>>>>   return;
>>>>
>>>> DefinedSVal retVal = X.castAs<DefinedSVal>();
>>>>
>>>> ConstraintManager &CMgr = state->getConstraintManager();
>>>> const SymbolRef* sym = state->get<RetValConstraint>(lockR);
>>>> ConditionTruthVal retZero = CMgr.isNull(state, *sym);       // Is this the correct way to check if the return                    value is zero or not? I also tried using                          isZeroConstant but neither seem to work.
>>>> ...
>>>> }
>>>>
>>>> void PthreadLockChecker::DestroyLock(CheckerContext &C, const CallExpr *CE,
>>>> SVal Lock) const {
>>>>   const LockState *LState = State->get<LockMap>(LockR);
>>>> if (!LState || LState->isUnlocked()) {
>>>>   SVal X = State->getSVal(CE, C.getLocationContext());  // Added this
>>>>   if (X.isUnknownOrUndef())     // Added this
>>>>     return;
>>>>
>>>>   DefinedSVal retVal = X.castAs<DefinedSVal>();         // Added this (Is this the correct way to get return  value?)
>>>>   SymbolRef sym = retVal.getAsSymbol();     // Added this
>>>>
>>>>   State = State->set<LockMap>(LockR, LockState::getDestroyed());
>>>>   State = State->set<RetValConstraint>(LockR, sym);            // Added this
>>>>   C.addTransition(State);
>>>>   return;
>>>> }
>>>> ...
>>>> }
>>>>
>>>>
>>>> I have been stuck on this for a while and haven't been able to find my way around it.
>>> This is great progress!
>>>
>>> What is going on here is that the analyzer is very aggressive about pruning constraints on symbols for values that the program can’t refer to again. These constraints are very expensive to keep in the store, so it removes them as quickly as possible.
>>>
>>> So, for example, in
>>>
>>>   int result = pthread_mutex_destroy(&m);
>>>   if (result != 0) { // (1)
>>> pthread_mutex_lock(&m);
>>>   }
>>>
>>> The ‘result’ local variable is never referred in the code to after point (1). The analyzer sees this and removes the constraint on result discovered from the guard condition almost immediately after it is executed. Then, when you query for that symbol when analyzing the call to pthread_mutex_lock(), the analyze doesn’t have any constraints.
>>>
>>> For example, if you were to artificially extend the lifetime of ‘result’ in the above snippet:
>>>   int result = pthread_mutex_destroy(&m);
>>>   if (result != 0) { // (1)
>>>   pthread_mutex_lock(&m);
>>>   }
>>>   (void)result; // This extends the life of 'result'.
>>>
>>> Your isNull() query would work as you expect because the symbol is still alive at the call to pthread_mutex_lock().
>>>
>>> Here is a suggested solution: you can add a checkDeadSymbols callback to the PthreadChecker. The analyzer calls this whenever it sees that a symbol became dead. In this callback you can iterate through the RetValConstraint symbols — if one of them becomes dead, you can query the constraint for the value then and update the LockMap appropriately. Don’t forget to also remove the dead symbol from RetValConstraint when it becomes dead. This way the analyzer won’t keep all those extra dead symbols in memory.
>>>
>>> There is an example of how the use the checkDeadSymbols callback in SimpleStreamChecker.cpp.
>>>
>>> Devin
>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-dev mailing list
>>> cfe-dev at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev