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

[Devin Coughlin via cfe-dev]

> 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 <mailto:dcoughlin at apple.com>> wrote:
> 
>> On Apr 12, 2017, at 3:32 AM, Malhar Thakkar <cs13b1031 at iith.ac.in <mailto: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 <mailto: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 <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 <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/20170412/fd74fa52/attachment.html>