<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi Lenny,<div><br></div><div>This is looking better. The patch doesn't apply cleanly to TOT, so would it be possible to regenerate it? Some of the patch doesn't really match with the current contents of the checker, so it's hard to evaluate.</div><div><br></div><div>A few comments:</div><div><br></div><div>- Could you add comments about what the 'IsPotential' flag is for? This checker is really lacking in comments, and the logic is starting to look really complicated.</div><div><br></div><div>- While I can't quite tell because the patch doesn't apply correctly, the following code bothers me a bit:</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+ NonLoc * lenValNL;</font></div></blockquote></div><div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+ SVal lenVal;</font></div><div><font class="Apple-style-span" color="#000000">+ bool checkPotentialLen = false;</font></div><div><font class="Apple-style-span" color="#000000"> if (isStrncpy) {</font></div><div><font class="Apple-style-span" color="#000000"> // Get the max number of characters to copy</font></div><div><font class="Apple-style-span" color="#000000"> const Expr *lenExpr = CE->getArg(2);</font></div><div><font class="Apple-style-span" color="#000000">- SVal lenVal = state->getSVal(lenExpr);</font></div><div><font class="Apple-style-span" color="#000000">-</font></div><div><font class="Apple-style-span" color="#000000">+ lenVal = state->getSVal(lenExpr);</font></div><div><font class="Apple-style-span" color="#000000">+ </font></div><div><font class="Apple-style-span" color="#000000"> NonLoc * strLengthNL = dyn_cast<NonLoc>(&strLength);</font></div><div><font class="Apple-style-span" color="#000000">- NonLoc * lenValNL = dyn_cast<NonLoc>(&lenVal);</font></div><div><font class="Apple-style-span" color="#000000">+ lenValNL = dyn_cast<NonLoc>(&lenVal);</font></div></blockquote>... <SNIP></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+ // Max number to copy is greater than the length of the src buffer. So</font></div><div><font class="Apple-style-span" color="#000000">+ // also check that it is still <= length of destination buffer.</font></div><div><font class="Apple-style-span" color="#000000">+ if (checkPotentialLen) {</font></div><div><font class="Apple-style-span" color="#000000">+ SVal lastElement =</font></div><div><font class="Apple-style-span" color="#000000">+ C.getSValBuilder().evalBinOpLN(state, BO_Add, *dstRegVal,</font></div><div><font class="Apple-style-span" color="#000000">+ *lenValNL, Dst->getType());</font></div><div><font class="Apple-style-span" color="#000000">+ </font></div><div><font class="Apple-style-span" color="#000000">+ </font></div></blockquote></div><div><br></div></div><div>I'm not a huge fan of declaring variables (e.g., lenValN:) and conditionally initializing them on one branch, and then conditionally using them later on another branch. I often feel that makes the logic of the checker not well-composed, difficult to follow, and error prone. I can't make more specific comments since the patch doesn't apply cleanly, but if the method probably could be further factored into additional methods where the shared logic was composed using calls to sub-functions rather than a bunch of branches it would honestly be much easier to follow.</div><div><br><div><div>On Feb 4, 2011, at 4:39 PM, Lenny Maiorani wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Feb 3, 2011, at 3:13 PM, Lenny Maiorani wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Dec 21, 2010, at 9:52 AM, Ted Kremenek wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div><div>Hi Lenny,</div><div><br></div><div>Thank you for your patience. Overall the patch looks great, but I'm a little confused about the following section:</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span"> // Get the string length of the source.</font></div><div><font class="Apple-style-span"> SVal strLength = getCStringLength(C, state, srcExpr, srcVal);</font></div><div><font class="Apple-style-span"> </font></div><div><font class="Apple-style-span">+ if (isStrncpy) {</font></div><div><font class="Apple-style-span">+ // Check if the number of bytes to copy is less than the size of the src</font></div><div><font class="Apple-style-span">+ const Expr *lenExpr = CE->getArg(2);</font></div><div><font class="Apple-style-span">+ strLength = state->getSVal(lenExpr);</font></div><div><font class="Apple-style-span">+ }</font></div><div><font class="Apple-style-span">+</font></div><div><font class="Apple-style-span"> // If the source isn't a valid C string, give up.</font></div><div><font class="Apple-style-span"> if (strLength.isUndef())</font></div><div><font class="Apple-style-span"> return;</font></div></blockquote><br></div><div>This looks like an intermingling of logic that it's not clear should compose is this way.</div><div><br></div><div>At the beginning we (a) fetch a value for 'strLength', then (b) overwrite that value if 'isStrncpy' is true, and then (c) we check if strLength is undefined. Both (a) and (b) look like competing logic. If they are truly mutually exclusive, I rather have one, but not both, get computed. This logic also looks slightly pessimistic, as the length of the string can be smaller than the max number of bytes specified to strncpy(). If the value retrieved at (a) is less than the value retrieved at (b), should we use the strLength from (a) and not (b)? I can see the argument to always use the most pessimistic value, but then our error reporting should probably reflect that 'size_t n' argument to strncpy() is too large, and not necessarily that we have a buffer overflow. That would make it clearer to the user what they actually need to fix in their code (i.e., while it might not be a buffer overflow, it is one waiting to happen, etc.).</div></div><div><br></div><div>Overall, this looks great. I'd just like to iron these last details out a bit (and document the final design decision in the code itself with comments) so it's clear the checker is always doing what you intend and that the user understands why they are getting a warning for their code.</div><div><br></div><div>Cheers,</div><div>Ted</div></blockquote></div><br><div>
<span class="Apple-style-span" style="font-size: 12px; ">After a hiatus, I am back. Ted, you were correct. My patch was pessimistic. I have modified it to accurately reflect whether or not there is a buffer overflow. Now, it compares the size of the src buffer and the value of the size_t and takes the smaller. See attached patch.</span></div><div><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-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; "><span class="Apple-style-span" style="font-size: 12px; "><br></span></span></div><div><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-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; "><span class="Apple-style-span" style="font-size: 12px; ">Maybe there should be an additional check to see if the size_t (3rd arg) is larger than the size of dst. This would be more of a potential logic error waiting to happen when the code changed sometime in the future. This patch does not contain that.<br class="Apple-interchange-newline"><br></span></span></div><div><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-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; "><span class="Apple-style-span" style="font-size: 12px; ">-Lenny</span></span></div><div><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-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; "><span class="Apple-style-span" style="font-size: 12px; "></span></span></div></div><span><strncpy-checker.diff></span><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><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-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; "><span class="Apple-style-span" style="font-size: 12px; "><br><div><div><font class="Apple-style-span" face="'Courier New'"><font class="Apple-style-span" size="5"><span class="Apple-style-span" style="font-size: 18px; "> __o</span></font></font></div><div><font class="Apple-style-span" face="'Courier New'"><font class="Apple-style-span" size="5"><span class="Apple-style-span" style="font-size: 18px; "> _`\<,_</span></font></font></div><div><font class="Apple-style-span" face="'Courier New'"><font class="Apple-style-span" size="5"><span class="Apple-style-span" style="font-size: 18px; "> (*)/ (*)</span></font></font></div><div><font class="Apple-style-span" face="'Courier New'" size="5"><span class="Apple-style-span" style="font-size: 18px; ">~~~~~~~~~~~~~~~~~~~~</span></font></div></div></span></span>
</div>
<br></div>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br></blockquote></div><br><div>
<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-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; "><span class="Apple-style-span" style="font-size: 12px; ">This patch extends my previous patch to also check for a separate pessimistic case. It ensures that the size_t n (3rd arg to strncpy()) is less than the size of the destination buffer. It contains a different warning message than the other strict buffer overruns since this one is not actually a buffer overrun, only a chance of a buffer overrun in the future.</span></span></div><div><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-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; "><span class="Apple-style-span" style="font-size: 12px; "><br></span></span></div><div><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-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; "><span class="Apple-style-span" style="font-size: 12px; ">-Lenny</span></span></div><div><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-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; "><span class="Apple-style-span" style="font-size: 12px; "><br></span></span></div><div><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-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; "><span class="Apple-style-span" style="font-size: 12px; "></span></span></div></div><span><strncpy-pessimistic-checker.diff></span><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><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-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; "><span class="Apple-style-span" style="font-size: 12px; "><br class="Apple-interchange-newline"><br><div><div><font class="Apple-style-span" face="'Courier New'"><font class="Apple-style-span" size="5"><span class="Apple-style-span" style="font-size: 18px; "> __o</span></font></font></div><div><font class="Apple-style-span" face="'Courier New'"><font class="Apple-style-span" size="5"><span class="Apple-style-span" style="font-size: 18px; "> _`\<,_</span></font></font></div><div><font class="Apple-style-span" face="'Courier New'"><font class="Apple-style-span" size="5"><span class="Apple-style-span" style="font-size: 18px; "> (*)/ (*)</span></font></font></div><div><font class="Apple-style-span" face="'Courier New'" size="5"><span class="Apple-style-span" style="font-size: 18px; ">~~~~~~~~~~~~~~~~~~~~</span></font></div></div></span></span>
</div>
<br></div>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits<br></blockquote></div><br></div></body></html>