Thanks for the comments so far!<br><br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">Both malloc and simple stream checker should be the same - if we pass
p+1 to a function, which is known not to close files / free memory, we
should still track the symbol.<br></blockquote><div><br>Got it. Corrected.<br></div><br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">which is where you pass an <i>offset</i> of a local variable:<div>
<br></div><div>char c;</div><div>char *r = &c;</div><div>free(r + 1);</div></blockquote><div><br>Oh, my mistake. For whatever reason I though testFreeNonMallocPointerWithOffset was testing that. I was not looking carefully enough. I added your test and gave it a whirl. Indeed, you are correct; the wrong message is being printed. The attached patch has a separate ReportOffsetFree function for reporting this error, and I have added your test case.<br>
<br>Side question. I noticed that the tests do not always strictly obey the expected-warning tags. For example, when I was running the test case above, I tried the following for the free() line:<br><div style="margin-left:40px">
free(r+1); // expected-warning {{Argument to free() is the address of the local variable 'c', which is not memory allocated by malloc()}}<br></div>or<br><div style="margin-left:40px">free(r+1); // expected-warning {{Argument to free() is the address of the local variable 'c', which is not memory allocated by malloc(); the memory location passed is an offset of an allocated location}}<br>
</div>The unit tests will pass both of these, which does not make any sense. I finally tried the following, which also passed:<br><div style="margin-left:40px">free(r+1); // expected-warning {{}}<br></div>However, no expected-warning comment resulted in a failure. Is there a special format for the expected-warning comment that I am missing, or is the comparison that is taking place not quite right?<br>
<br>The message emitted from ReportOffsetFree is a little different from the one that was emitted in ReportBadFree. Take a look and see if you agree with its phrasing and the information it is providing.<br><br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">
<div>A couple of style things are still a bit off from LLVM conventions:
some &&s are still leading instead of trailing, and some nested
conditions are not properly lined up with parentheses. (If you have to
wrap, the next line should be indented to one character past the open
paren.) </div></blockquote><div><br>I think I have all the && and spacing issues now. If I am missing them now, I probably do not see them.<br> </div><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">
<div>Also, I thought of yet another way to write the implication
assertion:</div><div><br></div><div>if (Kind == PSK_DirectEscapeOnCall || Kind == PSK_IndirectEscapeOnCall)</div><div> assert(Call);</div></blockquote><div><br> <blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">
<div style="margin:0px;font-size:11px;font-family:Monaco"> assert((!(Kind == PSK_DirectEscapeOnCall || </div><div style="margin:0px;font-size:11px;font-family:Monaco"> Kind == PSK_IndirectEscapeOnCall) ||</div>
<div style="margin:0px;font-size:11px;font-family:Monaco;color:rgb(57,51,255)"><span style="color:#000000"> Call) && </span>"Call must not be NULL when escaping on call"<span style="color:#000000">);</span></div>
</blockquote><div><br> I sort of agree with part of both of the possibilities. I would rather the assert be its own contained unit, but I do not like inverting the logic in the assert, as it may make it a little harder to parse. How about this?<br>
<br><span style="font-family:courier new,monospace"> assert((Call != NULL ||<br> (Kind != PSK_DirectEscapeOnCall &&<br> Kind != PSK_IndirectEscapeOnCall)) &&<br> "Call must not be NULL when escaping on call");</span><br>
</div></div><br>I will probably be called on spacing rules though, as I tried to indent pass the ( only if within the same set of (). That is, the Kinds should line up, but the Call and "Call should not, as the first Call is within an extra ().<br>
<br>- Branden<br></div><br><div class="gmail_quote">On Mon, Jan 21, 2013 at 1:31 PM, Anna Zaks <span dir="ltr"><<a href="mailto:ganna@apple.com" target="_blank">ganna@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><br><div><div><div><div>On Jan 21, 2013, at 9:40 AM, Jordan Rose <<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>> wrote:</div><br><blockquote type="cite">
<div style="word-wrap:break-word"><br><div><div>On Jan 19, 2013, at 21:09 , Branden Archer <<a href="mailto:b.m.archer4@gmail.com" target="_blank">b.m.archer4@gmail.com</a>> wrote:</div><br><blockquote type="cite">
Attached are the most recent version of the two patches. (By the way, is there a website set up for conducting code reviews for clang? If so, maybe in the future that would be more convenient and easier to use that instead of many emails).<br>
<br>Jordan, I think you were looking at an older patch. Some of your comments are probably already resolved. However, just in case...<br><br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">
<div><div>+</div><div>+ if ( offset.isValid()</div><div>+ && !offset.hasSymbolicOffset()</div><div>+ && offset.getOffset() != 0) {</div><div>+ os << "; the memory location passed is an offset of an allocated location";</div>
<div>+ }</div><div>+</div></div></blockquote><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote"><br></blockquote><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">
<div>...this test is not correct; it will fire for free(&local[1]).<br></div></blockquote><div> </div>I do not agree. My understanding is that local stack variables would fail the following test in ReportBadFree:<br>
<br>
<div style="margin-left:40px">const MemRegion *MR = ArgVal.getAsRegion();<br> if (MR) {<br></div></blockquote><div><br></div><div>All memory is represented by MemRegions. '&local[1]' translates to an ElementRegion whose super-region is a VarRegion.</div>
<div><br></div><div><br></div><br><blockquote type="cite">which protects the code you mention above. In FreeMemAux, before ReportBadFree is called the symbol is confirmed to be known malloc'ed memory (which did not appear in earlier patches). Just to be sure, I do have a unit test which tests just this:<br>
<br><div style="margin-left:40px">void testFreeNonMallocPointerWithOffset() {<br> char c;<br> char * r = &c;<br> r = r + 10;<br> free(r-10); // expected-warning {{Argument to free() is the address of the local variable 'c', which is not memory allocated by malloc()}}<br>
}</div></blockquote><div><br></div><div>This doesn't actually test the case I was talking about, which is where you pass an <i>offset</i> of a local variable:</div><div><br></div><div>char c;</div><div>char *r = &c;</div>
<div>free(r + 1);</div><div><br></div><div>I believe that will fire, but I admit I haven't built clang with your new warning.</div><div><br></div><br><blockquote type="cite"><br><blockquote style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex;font-size:10px" class="gmail_quote">
<font style="font-size:12px" size="4">Given that the message isn't great either, and that
you've already established what kind of value is being freed, I think
you should just make a new helper function to report the error,
ReportOffsetFree or something. Then you can make a nicer error message,
like "Argument to free() is an offset of an allocation". (I don't think
this message is perfect either -- it doesn't mention malloc(), "an
offset" isn't really the correct term -- but at least it's more
concise.)</font><br></blockquote><br>The reason I added to ReportBadFree instead of making another function was I believed that most of the code would be identical. Additionally, ReportBadFree also reports if the free is performed on malloc'ed data or not, which I believed would be useful in a message. If you feel that the message should be more clear, I can add another function, ReportOffsetFree, for it.<font size="4"><br>
</font></blockquote><div><br></div><div>Yeah, I thought the same at the start, but since the "offset" message is only being reported for memory that is already known to be malloc()'d, I think most of the benefits of code reuse are lost.</div>
<div><br></div><div><br></div><br><blockquote type="cite"><blockquote style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex" class="gmail_quote">
<font size="4"><div style="font-size:12px">+void freeOffsetPointerPassedToFunction()</div>
<div style="font-size:12px">+{</div><div style="font-size:12px">+ int *p = malloc(sizeof(int)*2);</div><div style="font-size:12px">+ p[1] = 0;</div><div style="font-size:12px">+ p += 1;</div><div style="font-size:12px">
+ myfooint(*p); // not passing the pointer, only a value pointed by pointer</div><div style="font-size:12px">+
free(p); // expected-warning {{Argument to free() is not memory
allocated by malloc(); the memory location passed is an offset of an
allocated location}}</div><div style="font-size:12px">+}</div></font><br style="font-size:10px"><font style="font-size:12px" size="4"><div>A better test would be to pass 'p' as a <i>const</i> pointer, which means the contents of the region get invalidated but 'p' itself will not.</div>
</font></blockquote><br>I am not clear on what you mean. Pass 'p' as a const pointer to free? The const will be ignored in that case. Or maybe you mean myfooint? That accepts an integer and not a pointer. Maybe you were referring to something different, maybe passing 'p' to a function that takes a const pointer. I have added another test case that tests for this, called testOffsetPassedAsConst.<br>
</blockquote><div><br></div><div>Thanks, the latter is what I meant. The test for passing *p doesn't do much based on how the analyzer core works right now, but it's still a good completeness test.</div><div><br>
</div>
<div><br></div><br><blockquote type="cite"><blockquote style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex" class="gmail_quote"><span style="font-size:15px"><div style="font-size:13px">
- if (Call && doesNotFreeMemory(Call, State))</div>
<div style="font-size:13px">+ if ((Kind == PSK_DirectEscapeOnCall ||</div><div style="font-size:13px">+ Kind == PSK_IndirectEscapeOnCall) &&</div><div style="font-size:13px">+ doesNotFreeMemory(Call, State)) {</div>
</span><br style="font-size:10px"><span style="font-size:13px">
This
is not correct. Before, this branch was only taken if the Kind
is PSK_DirectEscapeOnCall. Indirect escapes can still possibly free
memory (although it's unlikely).</span><br style="font-size:10px"><span style="font-size:13px"><div>- if (Call && guaranteedNotToCloseFile(*Call))</div><div>+ if ((Kind == PSK_DirectEscapeOnCall ||</div><div>
+ Kind == PSK_IndirectEscapeOnCall) &&</div>
<div>+ guaranteedNotToCloseFile(*Call)) {</div></span><br style="font-size:10px"><span style="font-size:13px">Ditto.</span></blockquote><br>Anna mentioned that the behavior change is intended for the malloc checker. However, I did not consider the behavior change for the simple stream checker. In the attached patch, I have changed the condition to only check for direct escape.<br>
</blockquote><div><br></div><div>Whoops, clearly I have trouble reasoning about this part. I think I understand Anna's explanation now: it's unlikely to have a <i>known, system </i>API that will <i>not</i> free (or cause to escape) a top-level argument, but <i>will</i> free (or cause to escape) some indirectly-accessible region, and that applies to stream handles as well.</div>
<div><br></div><div>(The reason I was confused is because of RetainCountChecker, because the Cocoa memory conventions specify that direct arguments will not have their retain counts messed with, but stuff accessed through them may be fair game.)</div>
<div><br></div><div>A couple of style things are still a bit off from LLVM conventions: some &&s are still leading instead of trailing, and some nested conditions are not properly lined up with parentheses. (If you have to wrap, the next line should be indented to one character past the open paren.) Also, I thought of yet another way to write the implication assertion:</div>
<div><br></div><div>if (Kind == PSK_DirectEscapeOnCall || Kind == PSK_IndirectEscapeOnCall)</div><div> assert(Call);</div><div><br></div></div></div></blockquote><div><br></div></div></div><div>I prefer to keep everything inside an assert. For example, the one below might be more readable.</div>
<div><br></div><div style="margin:0px;font-size:11px;font-family:Monaco"> assert((!(Kind == PSK_DirectEscapeOnCall || </div><div style="margin:0px;font-size:11px;font-family:Monaco"> Kind == PSK_IndirectEscapeOnCall) ||</div>
<div style="margin:0px;font-size:11px;font-family:Monaco;color:rgb(57,51,255)"><span style="color:#000000"> Call) && </span>"Call must not be NULL when escaping on call"<span style="color:#000000">);</span></div>
<div><span><br></span></div></div><div><div><br><blockquote type="cite"><div style="word-wrap:break-word"><div><div>Dead code cleanup will take care of this in release builds, and now it reads like a proper implication, too.</div>
<div><br></div><div>Jordan</div></div></div></blockquote></div><br></div></div></blockquote></div><br>