<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Nov 26, 2011, at 8:26 PM, Cyril Roelandt wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; ">On 11/19/2011 05:35 AM, Jordy Rose wrote:<br><blockquote type="cite">I didn't look into the patch in detail, but it's worth noting that realloc can be safe with a 0 size on BSD, at least. ("If size is zero and ptr is not NULL, a new, minimum sized object is allocated and the original object is freed." but "If ptr is NULL, realloc() is identical to a call to malloc() for size bytes.")<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Also, how much overlap is there with MallocChecker? There's a fixme on CheckMallocZero already about having malloc-related checks in two places.<br></blockquote><blockquote type="cite"><br></blockquote><br>So, here is a new version of the patch :<br><br>* Remove all the malloc-related code from the UnixAPIChecker<br>* Remove the malloc-related tests from test/Analysis/unix-fns.c<br>* Check zero byte allocations for the malloc(), calloc() and realloc() functions in the MallocChecker<br>* Add tests in test/Analysis/malloc.c<br><br><br>About the realloc function, the C99 standard specifies that "if ptr is a null pointer, the realloc function behaves like the malloc function for the specified size". I am not sure anything is specified if the ptr is not NULL and the size is 0. With this patch, realloc(ptr, 0) will not raise any warning if ptr is not NULL.<br></span></blockquote></div><br><div><br></div><div>Hi Cyril,</div><div><br></div><div>I hate to give you confusing advice, but I think this should go into the UnixAPIChecker for now. I'm very sorry I didn't have time to review your original patch when you submitted it.</div><div><br></div><div>Jordy is absolutely right that putting this exclusively into MallocChecker is the right long term direction, but I disagree with him that this is what we should do right now. My concern is that the MallocChecker is not on by default, so moving this functionality from UnixAPIChecker to MallocChecker is actually a regression in functionality for most users.</div><div><br></div><div>The reason the MallocChecker is not on by default is because it makes very optimistic assumptions about wrapper functions for free() and malloc(). Basically, if it sees a pointer returned from malloc() passed to another function, it doesn't assume that the function frees that memory unless it is (a) free() or (b) is a function specially annotated with an attribute indicating that it is a free function. While this approach has merit, it will not really fly on most code out of the box, and will cause the checker to produce a bunch of noise (making it not eligible to be turned on by default yet). Unfortunately, this checker is likely one of those that we will need interprocedural analysis in order to work well in practice, or we have an option where we can relax the checker so it conservatively assumes that any function it doesn't understand *may* free its argument (although that would likely make the checker useless in practice).</div><div><br></div><div>I'm very sorry for not responding to your earlier patch. I know you submitted it weeks ago, and I've been massively backlogged on patch review.</div><div><br></div><div>Here's what I think we should do. I think we should put this checking logic into both UnixAPIChecker and MallocChecker. When MallocChecker is enabled by default, we remove this logic from UnixAPIChecker. We could possibly enhance UnixAPIChecker to check if the MallocChecker is enabled, and if so, not warn about these cases (avoiding duplicate checking).</div><div><br></div><div>Cyril/Jordy: what do you think?</div><div><br></div><div>Cheers,</div><div>Ted</div></body></html>