<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div><div>On Dec 21, 2010, at 7:04 AM, Lenny Maiorani wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Is that patch in the queue to be reviewed and committed? I just want to make sure it wasn't dropped...<br><br>-Lenny<br><br><br>On Dec 17, 2010, at 5:22 PM, Lenny Maiorani wrote:<br><br><blockquote type="cite">After taking Argiris's advice, attached is a patch submittal for adding strncpy() buffer overflow checker to CStringChecker.cpp and associated unit tests.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">-Lenny<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><strncpy-checker.diff><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">the definition of open: "mkdir android ; cd android ; repo init -i <a href="git://android.git.kernel.org/platform/manifest.git">git://android.git.kernel.org/platform/manifest.git</a> ; repo sync ; make" - Andy Rubin<br></blockquote></div></blockquote><br></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" color="#000000"> // Get the string length of the source.</font></div><div><font class="Apple-style-span" color="#000000"> SVal strLength = getCStringLength(C, state, srcExpr, srcVal);</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000">+ if (isStrncpy) {</font></div><div><font class="Apple-style-span" color="#000000">+ // Check if the number of bytes to copy is less than the size of the src</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">+ strLength = state->getSVal(lenExpr);</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000"> // If the source isn't a valid C string, give up.</font></div><div><font class="Apple-style-span" color="#000000"> if (strLength.isUndef())</font></div><div><font class="Apple-style-span" color="#000000"> 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></body></html>