<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Oct 2, 2014, at 2:15 , Daniel Fahlgren <<a href="mailto:daniel@fahlgren.se">daniel@fahlgren.se</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">On Thu, 2014-09-04 at 16:21 +0200, Daniel Fahlgren wrote:<br><blockquote type="cite">Hi,<br><br>This patch handles the fixme about multi-dimensional VLAs in<br>VLASizeChecker. I tried to keep the changeset as small as possible by<br>cutting the checkPreStmt method into two parts. The checker first<br>iterates over all dimensions and reports if a bug is found. It later<br>iterates again, calculating the size. By scanning the dimensions twice<br>it can report bugs even if an unknown size is found.<br><br>Feedback and comments are, as always, welcome.<br></blockquote><br>Ping?<br></blockquote></div><br><div>Good ping. Sorry for the delay. Some comments:</div><div><br></div><div>+ // First only check for errors. That way we will find problems even if one of<br>+ // the dimensions is unknown.<br>+ const Expr *SE;<br>+ QualType QT;<br>+ do {<br>+ // Check size expression.<br>+ SE = VLA->getSizeExpr();<br>+ State = checkSizeExpr(SE, State, C);<br>+ if (!State)<br>+ return;</div><div><br></div><div>Seems like a worthy goal. If we're finding problems even if one dimension is unknown, though, is it worth finding problems in every size expression, rather than exiting early when we find a problem in the first one?</div><div><br>+<br>+ // Check next dimension.<br>+ QT = VLA->getElementType();<br>+ VLA = Ctx.getAsVariableArrayType(QT);<br>+ } while(VLA);</div><div><br></div><div>Nitpick: can you put a space after the "while"? (here and below)</div><div><br></div><div>+ // Record the state so far, in case one of the lengths is unknown.<br>+ C.addTransition(State);<br><br></div><div>Unfortunately we can't actually do this—you end up recording <i>two</i> transitions here, rather than replacing the first one. Maybe the size calculation should be factored out into a helper function as well?</div><div><br></div><div>+ // Multiply the current size by the element size.<br>+ ArraySizeVal = SvalBuilder.evalBinOpNN(State, BO_Mul, ArrayLength,<br>+ ArraySizeVal.castAs<NonLoc>(), SizeTy);<br><br></div><div>Can you add a FIXME in here to check for overflow? That's a security risk, really.</div><div><br></div><div>As for the tests, can you add some assertions that we really do multiply the size out correctly? For example, using a too-large index should warn, casting to char * and assigning outside the buffer should warn, and sizeof should give the right answer.</div><div><br></div><div>Thanks for fixing this up,</div><div>Jordan</div></body></html>