Oops, forgot to reply-all...<br><br><div class="gmail_quote">---------- Forwarded message ----------<br>From: <b class="gmail_sendername">Andrew McGregor</b> <span dir="ltr"><<a href="mailto:andrewmcgr@gmail.com">andrewmcgr@gmail.com</a>></span><br>
Date: Mon, Jun 28, 2010 at 12:04 PM<br>Subject: Re: Ownership attribute for malloc etc. checking<br>To: Jordy Rose <<a href="mailto:jediknil@belkadan.com">jediknil@belkadan.com</a>><br><br><br>Hi,<div><br></div><div>
Thanks for the review. Comments inline, new patch attached.</div><div><br></div><div>Andrew<br><br><div class="gmail_quote"><div class="im">On Sun, Jun 27, 2010 at 9:33 AM, Jordy Rose <span dir="ltr"><<a href="mailto:jediknil@belkadan.com" target="_blank">jediknil@belkadan.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
Looking good! Some remaining comments below.<br>
<br>
I'm also a reasonably new committer, so someone else should probably look<br>
over this as well (particularly the Attr hunks, which I'm not very familiar<br>
with).<br>
<br>
Jordy<br>
<br>
<br>
General:<br>
- It would be nice to make sure one argument isn't both ownership_takes<br>
and ownership_holds.<br></blockquote><div><br></div></div><div>True, will do.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- LLVM's convention is to use spaces rather than tabs.<br>
- There's a hunk where the only thing that changed is indentation; it'd be<br>
nice to take that out.<br></blockquote><div><br></div></div><div>Oops, neither of these were intentional.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- Two more useful test would be free(p); my_free(p) and free(p);<br>
my_hold(p). Both of which should warn, of course.<br></blockquote><div><br></div></div><div>Instead of crashing, yes. Well spotted.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
(MallocChecker::EvalCallExpr)<br>
+ for (const Attr *attr = FD->getAttrs(); attr; attr = attr->getNext())<br>
{<br>
+ if (const OwnershipReturnsAttr* Att =<br>
dyn_cast<OwnershipReturnsAttr>(attr)) {<br>
+ MallocMemReturnsAttr(C, CE, Att);<br>
+ rv = true;<br>
+ }<br>
+ if (const OwnershipTakesAttr* Att = dyn_cast<OwnershipTakesAttr><br>
(attr)) {<br>
+ FreeMemTakesAttr(C, CE, Att);<br>
+ rv = true;<br>
+ }<br>
+ if (const OwnershipHoldsAttr* Att = dyn_cast<OwnershipHoldsAttr><br>
(attr)) {<br>
+ FreeMemHoldsAttr(C, CE, Att);<br>
+ rv = true;<br>
+ }<br>
+ }<br>
<br>
Here's the dispatch from EvalCallExpr(). I think even though it's<br>
technically a little less efficient, you should just simplify this and use<br>
FD->getAttr<OwnershipTakesAttr>(), etc. It's a little simpler, and it lets<br>
you order the attributes.<br></blockquote><div><br></div></div><div>However, I did it this way because I wanted to allow two or more attributes of the same kind (for the benefit of macros).</div><div><br></div><div>In other words, __attribute((ownership_holds, 1)) __attribute((ownership_holds, 2)) should be the same as __attribute((ownership_holds, 1, 2)).</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Why does that matter? Because an ownership_takes() or ownership_holds()<br>
function might have a return value! If it's also ownership_returns(), then<br>
you're fine, but otherwise you need to set a symbolic return value. (You<br>
can see how this is done in MallocMemAux(), or in StreamChecker.cpp with<br>
fopen.) There should probably be a test for this as well.</blockquote><div><br></div></div><div>It isn't necessarily the case that an ownership_takes() function that returns a pointer generated that pointer from an ownership_takes argument, or allocated anything, so what would you set the region to, in the absence of other information? They default to Unknown, yes?</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Actually, because these attributes don't affect the return value, they<br>
should probably go in a PreVisitCallExpr() callback, rather than<br>
EvalCallExpr(). But for now it's probably okay, as long as you handle that<br>
return value.<br>
<br>
<br>
(MallocChecker::MallocMemReturnsAttr)<br>
+ for (const unsigned *I = Att->begin(), *E = Att->end(); I != E; ++I,<br>
++count) {<br>
+ const GRState *state =<br>
+ MallocMemAux(C, CE, CE->getArg(*I), UndefinedVal(),<br>
C.getState());<br>
+ C.addTransition(state);<br>
+ }<br>
+ if (count == 0) {<br>
+ const GRState *state = MallocMemAux(C, CE, UndefinedVal(),<br>
UndefinedVal(),<br>
+ C.getState());<br>
+ C.addTransition(state);<br>
+ }<br>
<br>
What does it mean to have multiple size fields? There can still only be<br>
one return value. Maybe you should only allow zero or one positions for<br>
ownership_returns (which would make this section simpler).<br></blockquote><div><br></div></div><div>The intention here was to implement something for calloc-like functions and matrix allocators; one argument is a special case, more than one the size of the region is the product of the arguments times sizeof(pointee of the return value), taking sizeof(void) as 1. If it isn't worth doing that, fair enough, but that's what I was thinking this could lead to.</div>
<div><br></div><div>However, the parser only allows the one argument at the moment, so this is redundant.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
(MallocChecker::PreVisitReturnStmt)<br>
+ if (RS->isReleased()) {<br>
+ ExplodedNode *N = C.GenerateSink();<br>
+ if (!BT_UseFree)<br>
+ BT_UseFree = new BuiltinBug("Use dynamically allocated memory<br>
after"<br>
+ " it is freed.");<br>
<br>
+ BugReport *R = new BugReport(*BT_UseFree,<br>
BT_UseFree->getDescription(),<br>
+ N);<br>
+ C.EmitReport(R);<br>
+ }<br>
<br>
This section checks whether the return value is a released address.<br>
Unfortunately this isn't a 100% unambiguous case; consider the following<br>
(contrived) example:<br>
<br>
struct Node *freeSmallest () {<br>
struct Node *min;<br>
// find the minimum node<br>
free(min);<br>
return min; // so the caller knows the ID of the deleted node<br>
}<br>
<br>
As long as the caller doesn't dereference the returned pointer, it could<br>
still be a useful value. 95% of the time, this check would be useful, but<br>
since a false positive is worse than a false negative for the analyzer, we<br>
should probably leave it out.<br>
</blockquote></div></div><br></div><div>Ok, I see. Out it is, then.</div><div><br></div><font color="#888888"><div>Andrew</div>
</font></div><br>