<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Jan 17, 2013, at 8:40 PM, Branden Archer <<a href="mailto:b.m.archer4@gmail.com">b.m.archer4@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Anna,<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>The naming of the kinds is not very consistent, so I suggest renaming:</div><div>
<span style="font-family:Monaco;font-size:11px"> </span><span style="font-family:Monaco;font-size:11px">PSK_InvalidatedRegionDirectly, -> </span><span style="font-family:Monaco;font-size:11px">PSK_DirectEscapeOnCall,</span></div>
<div><span style="font-family:Monaco;font-size:11px"> </span><span style="font-family:Monaco;font-size:11px">PSK_InvalidatedRegionIndirectly</span><span style="font-family:Monaco;font-size:11px">, -> </span><span style="font-family:Monaco;font-size:11px">PSK_DirectEscapeOnCall</span><span style="font-family:Monaco;font-size:11px">,</span></div>
<div><div style="margin:0px;font-size:11px;font-family:Monaco"> PSK_InvalidatedRegionOther -> PSKEscapeOther</div></div></blockquote><div><br>I agree. Your proposed names are much more elegant and clear!<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 style="margin:0px">In the checkers, I'd use the Kind instead of the Call parameter to be more explicit:</div><div style="margin:0px;font-size:11px;font-family:Monaco"> if (Call &&</div><div style="margin:0px">
with<span style="font-size:11px;font-family:Monaco"> </span></div><div style="margin:0px;font-size:11px;font-family:Monaco"> if ((Kind == PSK_DirectEscapeOnCall || Kind == PSK_DirectEscapeOnCall) &&</div></blockquote>
</div><br>I also agree. What is the point of passing more information if the checker does not take advantage of it. Besides, the behavior of Call being NULL is changed, so using Kind makes it more clear.<br><br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">
In<span style="font-size:11px;font-family:Monaco"> CheckerManager::runCheckersForPointerEscape</span>, we could assert that <font style="font-size:11px" face="Monaco">(Kind == </font><span style="font-size:11px;font-family:Monaco">PSK_DirectEscapeOnCall || Kind == </span><span style="font-size:11px;font-family:Monaco">PSK_DirectEscapeOnCall</span><font style="font-size:11px" face="Monaco">) </font><font>implies</font><font style="font-size:11px" face="Monaco"> (Call != 0).</font><br>
</blockquote> <br>I have put an assert for this. Not sure how you feel about ternary operators in an assert though... Is it clear?<br></div></blockquote><div><br></div><div>It's not necessary to use the ternary operator, you can rewrite implication in terms of or:</div><div>(a->b) is the same as (!a || b)</div><div><br></div><div>Anna.</div><blockquote type="cite"><div><br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">
Jordan has mentioned that he'd like to review this as well.<br></blockquote><div><br>Bring it on! (: <br></div><br>- Branden<br><br></div><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">
Hi Branden,<div><br></div><div>Sorry, I now realize that I've missed some issues the first time around:</div><div><br></div><div>patch 01:</div><div>The naming of the kinds is not very consistent, so I suggest renaming:</div>
<div><span style="font-family:Monaco;font-size:11px"> </span><span style="font-family:Monaco;font-size:11px">PSK_InvalidatedRegionDirectly, -> </span><span style="font-family:Monaco;font-size:11px">PSK_DirectEscapeOnCall,</span></div>
<div><span style="font-family:Monaco;font-size:11px"> </span><span style="font-family:Monaco;font-size:11px">PSK_InvalidatedRegionIndirectly</span><span style="font-family:Monaco;font-size:11px">, -> </span><span style="font-family:Monaco;font-size:11px">PSK_DirectEscapeOnCall</span><span style="font-family:Monaco;font-size:11px">,</span></div>
<div><div style="margin:0px;font-size:11px;font-family:Monaco"> PSK_InvalidatedRegionOther -> PSKEscapeOther</div></div><div style="margin:0px;font-size:11px;font-family:Monaco"><br></div><div style="margin:0px">In the checkers, I'd use the Kind instead of the Call parameter to be more explicit:</div>
<div style="margin:0px;font-size:11px;font-family:Monaco"> if (Call &&</div><div style="margin:0px">with<span style="font-size:11px;font-family:Monaco"> </span></div><div style="margin:0px;font-size:11px;font-family:Monaco">
if ((Kind == PSK_DirectEscapeOnCall || Kind == PSK_DirectEscapeOnCall) &&</div><div style="margin:0px;font-size:11px;font-family:Monaco"><br></div><div style="margin:0px">In<span style="font-size:11px;font-family:Monaco"> CheckerManager::runCheckersForPointerEscape</span>, we could assert that <font style="font-size:11px" face="Monaco">(Kind == </font><span style="font-size:11px;font-family:Monaco">PSK_DirectEscapeOnCall || Kind == </span><span style="font-size:11px;font-family:Monaco">PSK_DirectEscapeOnCall</span><font style="font-size:11px" face="Monaco">) </font><font>implies</font><font style="font-size:11px" face="Monaco"> (Call != 0).</font></div>
<div style="margin:0px;font-size:11px;font-family:Monaco"> </div><div style="margin:0px">Jordan has mentioned that he'd like to review this as well.</div><div style="margin:0px"><br></div><div style="margin:0px"><div style="margin:0px">
Thanks for working on this!</div><div>Anna.</div></div><br><br><div class="gmail_quote">On Wed, Jan 16, 2013 at 11:17 PM, Branden Archer <span dir="ltr"><<a href="mailto:b.m.archer4@gmail.com" target="_blank">b.m.archer4@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Anna,<br><br>Thanks for the feedback.<br><br>I think I fixed all the coding style issues you pointed out. Let me know if I missed any.<div class="im">
<br><br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">
4) I think it does not make sense to peel the super region in
ReportBadFree before reporting the new warning. The MR region is used
for naming whatever is passed to free. It does not currently trigger
since we are unable to print/summarize the region. Also, the loop can be
replaced with MR = MR->getBaseRegion().<br></blockquote><br></div>Do you mean that you do not see a reason to find the base region at all? I think that the base region is found and used, as there are some decision made about the type of bug which require it. I attempted to remove the loop just to see what would happen, and I noticed that a few of the older unit tests failed. I do agree with replacing the loop with the getBaseRegion call to simplify things. As that change is unrelated to the other changes, I have attached a third patch which handles it.<br>
<br>Also, thanks for the additional test case. <br><span class="HOEnZb"><font color="#888888"><br>- Branden</font></span><div class="HOEnZb"><div class="h5"><br><br><div class="gmail_quote">On Wed, Jan 16, 2013 at 3:20 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"><div>Branden,</div><div><br></div><div>Thanks for working on this! Everything looks good in general. Here are some minor issues.</div>
<div><br></div><div>Patch 01:</div><div>1) There are several occurrences of 80 column rule violations. For example, the definition of "<span style="font-family:Monaco;font-size:11px;color:rgb(0,97,65)">ProgramStateRef</span><span style="font-family:Monaco;font-size:11px"> MallocChecker::checkPointerEscape</span>".</div>
<div>2) Make the style of malloc test cases consistent :</div><div><div style="margin:0px;font-size:11px;font-family:Monaco"><span style="color:#931a68">int</span> *testOffsetAllocate(size_t size)</div><div style="margin:0px;font-size:11px;font-family:Monaco">
{</div></div><div style="margin:0px;font-size:11px;font-family:Monaco">-></div><div style="margin:0px;font-size:11px;font-family:Monaco"><div style="margin:0px"><span style="color:#931a68">int</span> *testOffsetAllocate(size_t size) {</div>
<div style="margin:0px"><br></div><div style="margin:0px">3) The spacing in the following if statement looks odd (should be no space between '(' and 'offset':</div><div style="margin:0px"><div style="margin:0px">
<span style="color:#931a68">if</span> ( offset.<span style="text-decoration:underline">isValid</span>()</div><div><br></div></div></div><div>4) I think it does not make sense to peel the super region in ReportBadFree before reporting the new warning. The MR region is used for naming whatever is passed to free. It does not currently trigger since we are unable to print/summarize the region. Also, the loop can be replaced with MR = MR->getBaseRegion().</div>
<div> </div><div>5)Modify the following comment to start with "The reason for pointer escape is unknown. For example,...".</div><div> /// A checker invalidates a region and intends the invalidation</div><div> /// to cause a pointer escape event.</div>
<div> PSK_InvalidatedRegionOther</div><div><div><br></div><div>6) Additional test case.</div><div><div style="margin:0px;font-size:11px;font-family:Monaco"><span style="color:rgb(147,26,104)">void</span> testOffsetZeroDoubleFree() {</div>
<div style="margin:0px;font-size:11px;font-family:Monaco"> <span style="color:rgb(147,26,104)">int</span> *array = malloc(<span style="color:rgb(147,26,104)">sizeof</span>(<span style="color:rgb(147,26,104)">int</span>)*2);</div>
<div style="margin:0px;font-size:11px;font-family:Monaco"> <span style="color:rgb(147,26,104)">int</span> *p = &array[0];</div><div style="margin:0px;font-size:11px;font-family:Monaco"> free(p);</div><div style="margin:0px;font-size:11px;font-family:Monaco;color:rgb(78,144,114)">
<span style=""> free(&array[0]); </span>// expected-warning{{Attempt to free released memory}}</div><div style="margin:0px;font-size:11px;font-family:Monaco">}</div></div><div><br></div></div><div><div style="margin:0px">
Cheers,</div></div><div style="margin:0px">Anna.</div><div style="margin:0px"><br></div><div><div><div>On Jan 15, 2013, at 6:32 PM, Branden Archer <<a href="mailto:b.m.archer4@gmail.com" target="_blank">b.m.archer4@gmail.com</a>> wrote:</div>
<br></div><blockquote type="cite"><div>I agree with keeping the third option open in case a checker in the future needs to invalidate a region. <br><br>Attached are two patches: one to add an enum to the checkPointerEscape callback describing the type of pointer escape, and another to update the malloc checker to catch the case when a malloc'ed pointer is free'd with a pointer with an offset.<br>
<br><span>Please take a look at the patches and let me know if they are acceptable and properly update the malloc checker to catch freeing pointers with an offset. I have included several test cases in test/Analysis/malloc.c, but if you can think of additional situations that I did not catch, let me know.<br>
<br>- Branden</span><br><br><div class="gmail_quote">On Fri, Jan 11, 2013 at 4:04 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>On Jan 10, 2013, at 8:57 PM, Branden Archer <<a href="mailto:b.m.archer4@gmail.com" target="_blank">b.m.archer4@gmail.com</a>> wrote:</div><br><blockquote type="cite">
Anna,<br><br>I am looking in ExprEngine.cpp in the methods that call runCheckersForPointerEscape. Looking into adding an enum to the checkPointerEscape callbacks, there appears to be four cases:<br> - in processPointerEscapedOnBind there is only one: pointer is lost on a bind.<br>
- in processPointerEscapedOnInvalidateRegions there are three. The last two are for direct and indirect invalidating due to some function call. The first one I do not quite get. Can you give an example of a region being invalidated that does not involve a function (e.g. Call is NULL). I am not clear what to call this case.<br>
<br></blockquote><div><br></div></div>Looks like the third case is never triggered. Thanks for finding this.</div><div><br></div><div>The ProgramState API allows checkers to trigger such events. (For example, if a checker invalidates a region and intends the invalidation to cause pointer escape event, like in <span style="font-family:Monaco;font-size:11px">CStringChecker::InvalidateBuffer,</span>but with <span style="color:rgb(78,144,114);font-family:Monaco;font-size:11px">/*CausedPointerEscape*/</span><span style="font-family:Monaco;font-size:11px"> <font color="#931a68">true</font></span>). However, none of the checkers currently do this and I do not have a specific case where it would be used. </div>
<div><br></div><div>Our options are : </div><div> - restrict the <span style="font-family:Monaco;font-size:11px">ProgramState::invalidateRegions</span> not to allow such combination</div><div> - leave the handling of the third case and allow the callers of <span style="font-family:Monaco;font-size:11px">ProgramState::invalidateRegions</span> to set the reason for invalidation. We could add Unknown into enum.</div>
<div><br></div><div>I am leaning toward the second option.</div><span><font color="#888888">Anna.</font></span><div><div><br></div><div><blockquote type="cite">- Branden<br><br><div class="gmail_quote">
On Mon, Jan 7, 2013 at 9:08 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>On Jan 7, 2013, at 5:51 PM, Branden Archer <<a href="mailto:b.m.archer4@gmail.com" target="_blank">b.m.archer4@gmail.com</a>> wrote:</div><br><blockquote type="cite">
Anna & Jordan,<br><br>I have a better idea on what is being the checkPointerEscape callback. Thanks for your description.<br><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div>The discussion below is about how we could special case it. Ex: check the callback to actually pass the Call parameter and use another argument to specify if the invalidation is direct (a pointer is an argument) or not. I am not sure how generic this solution is and how important it is to handle this. </div>
</div></blockquote><div><br>I cannot think of another case where knowing if an invalidation is direct or indirect would be important. I imagine that any checker monitoring a function that takes a pointer to ensure that the passed pointer was valid could make use of the information. This would beg the question of would it be worth writing such checkers, but that would be a separate issue. The callback could be made even more general, for example by passing an enum that represented the type of invalidation that has occurred. However, I do not know how much use something like that would be, especially if nothing else may use it.<br>
<br>For the purpose of modifying the malloc checker to detect pointers with non-zero offsets, would modifying the checkPointerEscape callback to pass a boolean such as isDirectInvalidation be acceptable? </div></div></blockquote>
<div><br></div></div><div>Absolutely. An enum with more options would also be fine. </div><div><br></div><div>What I meant by a generic solution is that it might be possible and better not to perform the indirect invalidation in cases we know it will not happen. For example, many calls to system APIs ('free' including) only invalidate the pointers that are being directly passed in. If we go with the proposed solution, the pointer will get invalidated, but we will keep tracking it in the malloc checker. However, it is possible not to invalidate the pointer altogether, which is the most precise. This would be a more intrusive solution and I am not 100% sure how it should be. One option would be to model all such functions. Another just add the special casing in the core as we do for pointer to const arguments. This solution is better and more generic but possibly more difficult to implement.</div>
<div><div><br></div><blockquote type="cite"><div class="gmail_quote">There are only two checkers using the callback currently, so making the change would not be that extensive. I could prepare a patch with that change and another where the free error I am interested in is detecting, and submit both for review.<br>
<br>- Branden<br></div>
</blockquote></div></div><br></div></blockquote></div><br>
</blockquote></div><br></div></div></blockquote></div><br>
</div><span><01-modify-checkPointerEscape.patch></span><span><02-update-malloc-checker.patch></span></blockquote></div><br></div></blockquote></div><br>
</div></div></blockquote></div><br></blockquote>
<span><01-modify-checkPointerEscape.patch></span><span><02-update-malloc-checker.patch></span><span><03-use-getbaseregion.patch></span></blockquote></div><br></body></html>