[cfe-dev] Suggested starter bug on clang analyzer codebase
Malhar Thakkar via cfe-dev
cfe-dev at lists.llvm.org
Mon Apr 17 04:55:52 PDT 2017
Consider the following test-case:
int retval = pthread_mutex_destroy(&mtx1);
if(retval == 0){
pthread_mutex_lock(&mtx1);
}
REGISTER_MAP_WITH_PROGRAMSTATE(RetValConstraint, const MemRegion *,
SymbolRef)
I have added the following snippet for checkDeadSymbols:
void PthreadLockChecker::checkDeadSymbols(SymbolReaper &SymReaper,
CheckerContext &C) const {
// std::cout<<"Checking dead symbols\n";
ProgramStateRef State = C.getState();
ConstraintManager &CMgr = State->getConstraintManager();
RetValConstraintTy TrackedSymbols = State->get<RetValConstraint>();
// int counter = 0;
for (RetValConstraintTy::iterator I = TrackedSymbols.begin(),
E = TrackedSymbols.end(); I != E; ++I) {
// counter++;
// std::cout << "Counter: "<<counter<<std::endl;
SymbolRef Sym = I->second;
const MemRegion* lockR = I->first;
bool IsSymDead = SymReaper.isDead(Sym);
// Remove the dead symbol from the return value symbols map.
if (IsSymDead){
ConditionTruthVal retZero = CMgr.isNull(State, Sym);
if(retZero.isConstrainedFalse()){
std::cout<<"False\n";
// Update LockMap
}
else if(retZero.isConstrainedTrue()){
std::cout<<"True\n";
}
State = State->remove<RetValConstraint>(lockR);
std::cout<<"Removed\n";
}
}
}
Now, after the execution of PthreadLockChecker::DestroyLock(),
checkDeadSymbols() is executed twice before
PthreadLockChecker::AcquireLock() is executed.
When checkDeadSymbols is first executed, there is just one symbol in
RetValConstraint(trivial). But, this symbol is constrained *not *to be
NULL. Then, the symbol is removed. During the execution of checkDeadSymbols
for the second time, there is again one symbol in RetValConstraint(I don't
understand this. Shouldn't this symbol have been destroyed during the
previous execution?). Nevertheless, this time, this symbol is constrained
to be NULL.
Finally, PthreadLockChecker::AcquireLock() is executed.
Also, I have one more query. After fixing the above mentioned issue, how do
I update LockMap in the sense that I'll need to revert the change made to
it if the symbol is constrained not to be NULL. Apart from keeping track of
the previous LockState, is there any other way to do it?
Thank you.
Regards,
Malhar
On Sat, Apr 15, 2017 at 11:34 PM, Artem Dergachev <noqnoqneo at gmail.com>
wrote:
>
>
> 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
>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170417/fcd88fdd/attachment.html>
More information about the cfe-dev
mailing list