[cfe-dev] Suggested starter bug on clang analyzer codebase
Malhar Thakkar via cfe-dev
cfe-dev at lists.llvm.org
Sat Apr 15 03:52:31 PDT 2017
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.
Regards,
Malhar
ᐧ
On Thu, Apr 13, 2017 at 11:48 AM, Devin Coughlin <dcoughlin at apple.com>
wrote:
>
> On Apr 12, 2017, at 10:47 PM, Malhar Thakkar <cs13b1031 at iith.ac.in> wrote:
>
> Dear Dr. Devin,
>
> Thank you for your response.
> Just to be clear, I think I'll have to query what is known about the
> symbol in PthreadLockChecker::ReleaseLock() instead of
> PthreadLockChecker::AcquireLock() as pthread_mutex_destroy() returns a
> non-zero value when a mutex is locked by another thread.
> So, in order to destroy that mutex, one would first need to unlock it and
> then destroy it.
>
>
> In general, unlocking a mutex that is held by another thread is undefined
> behavior, so the snippet below is not safe. It would be great to improve
> the diagnostic emitted for the snippet below explaining why it is not safe
> — but for now let’s focus on fixing the false positive I mentioned before.
>
> The code snippet which produces a false positive in the current checker:
>
> if(pthread_mutex_destroy(&(o->lock_mutex))!=0) {
> pthread_mutex_unlock(&(o->lock_mutex)); // It raises a warning here.
> pthread_mutex_destroy(&(o->lock_mutex));
> }
>
>
> Another example (in addition to the one I gave before) that it would be
> good to treat as safe is similar to what you have above, but without the
> unlock:
>
> if (pthread_mutex_destroy(&m) != 0) {
> pthread_mutex_destroy(&m); // Should not warn here
> }
>
> The fix for that would be exactly analogous to what I outlined before but
> you would query that symbol that is the result of the previous call to
> pthread_mutex_destroy() when deciding whether to diagnose on the second
> call to pthread_mutex_destroy()
>
> Devin
>
>
>
> Thank you.
>
>
> Regards,
> Malhar
> ᐧ
>
> On Wed, Apr 12, 2017 at 11:13 PM, Devin Coughlin <dcoughlin at apple.com>
> wrote:
>
>>
>> On Apr 12, 2017, at 3:32 AM, Malhar Thakkar <cs13b1031 at iith.ac.in> wrote:
>>
>> Hello Dr. Devin,
>>
>> I have the following approach in mind.
>> Just like the Unix API stream checker (where the return value of fopen is
>> checked), we can use the constraint manager to check if the return value
>> of any function (for example: pthread_mutex_destroy) is zero or not.
>> If it is non-zero (which indicates failure), then we don't update the set
>> & map which are used to keep track of the states all mutex variables.
>>
>> Let me know your thoughts on this.
>>
>>
>> Malhar,
>>
>> These sounds like the right approach to me, although I would focus only
>> on pthread_mutex_destroy()’s return value for now.
>>
>> One thing that makes this slightly tricky is that at the point of the
>> return from the call to pthread_mutex_destroy() we don’t know anything
>> about the return value — it is only later, when the programmer checks that
>> value (in a if statement, for example) that the analyzer can discover
>> whether the value returned was zero or not.
>>
>> Here is an example:
>> pthread_mutex_t m;
>> pthread_mutex_init(&m, NULL);
>>
>> // …
>>
>> int result = pthread_mutex_destroy(&m);
>> // (1) At this point the analyzer doesn’t know value of result.
>> // It tracks it as a symbol with no constraints
>>
>> if (result != 0) { // (2) At this point the analyzer constrains the
>> symbol from (1) to be non-zero
>>
>> // (3) At this point the checker should look at the at the symbol.
>> // If it is unknown or is definitely zero (meaning the destroy succeeded)
>> // it should emit a diagnostic. But if it is definitely non-zero then
>> // the analyzer should not report the diagnostic.
>> pthread_mutex_lock(&m);
>> }
>>
>> What this means is that you’ll need to modify
>> PthreadLockChecker::DestroyLock() to keep track of the returned symbol
>> from pthread_mutex_destroy() in the LockState and then query what is known
>> about that symbol in PthreadLockChecker::AcquireLock().
>>
>> Please don’t hesitate to ask if you more questions!
>>
>> Devin
>>
>> P.S. In the future you should CC the cfe-dev mailing on these kinds of
>> questions so that the community can offer their own input and get a sense
>> of what you are working on!
>>
>>
>>
>>
>> Thank you.
>>
>>
>> Regards,
>> Malhar
>> ᐧ
>>
>> On Fri, Apr 7, 2017 at 2:26 AM, Devin Coughlin <dcoughlin at apple.com>
>> wrote:
>>
>>> Hi Malhar (+Michael, cc’d),
>>>
>>> Here is a suggested starter bug on the clang static analyzer code base:
>>>
>>> “False Positive PthreadLockChecker Use destroyed lock” <
>>> https://bugs.llvm.org/show_bug.cgi?id=32455>
>>>
>>> The analyzer has a checker (PthreadLockChecker.cpp) that emits a
>>> diagnostic when the programmer tries to acquire a mutex twice or lock a
>>> mutex that has already been destroyed. Unfortunately, it has a false
>>> positive: if the programmer calls pthread_destroy_lock() and that call
>>> fails (returns a non-zero value) and then later tries to lock then the
>>> analyzer incorrectly warns that the mutex has been destroyed. There is an
>>> example of the false positive in the linked bugzilla URL above.
>>>
>>> Fixing this false positive is a good starter bug to get you familiar
>>> with the checker side of the analyzer. If you haven’t seen it already, a
>>> good place to start would be the Checker Development Manual <
>>> https://clang-analyzer.llvm.org/checker_dev_manual.html>.
>>>
>>> And please don’t hesitate to email if you have any questions or want to
>>> discuss approaches to fixing the bug!
>>>
>>> Devin
>>>
>>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170415/c1aa8a74/attachment.html>
More information about the cfe-dev
mailing list