[PATCH] Teach the analyzer about multi-dimensional VLAs

[Jordan Rose]

On Oct 2, 2014, at 2:15 , Daniel Fahlgren <daniel at fahlgren.se> wrote:

> On Thu, 2014-09-04 at 16:21 +0200, Daniel Fahlgren wrote:
>> Hi,
>> 
>> This patch handles the fixme about multi-dimensional VLAs in
>> VLASizeChecker. I tried to keep the changeset as small as possible by
>> cutting the checkPreStmt method into two parts. The checker first
>> iterates over all dimensions and reports if a bug is found. It later
>> iterates again, calculating the size. By scanning the dimensions twice
>> it can report bugs even if an unknown size is found.
>> 
>> Feedback and comments are, as always, welcome.
> 
> Ping?

Good ping. Sorry for the delay. Some comments:

+  // First only check for errors. That way we will find problems even if one of
+  // the dimensions is unknown.
+  const Expr *SE;
+  QualType QT;
+  do {
+    // Check size expression.
+    SE = VLA->getSizeExpr();
+    State = checkSizeExpr(SE, State, C);
+    if (!State)
+      return;

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?

+
+    // Check next dimension.
+    QT = VLA->getElementType();
+    VLA = Ctx.getAsVariableArrayType(QT);
+  } while(VLA);

Nitpick: can you put a space after the "while"? (here and below)

+  // Record the state so far, in case one of the lengths is unknown.
+  C.addTransition(State);

Unfortunately we can't actually do this—you end up recording two transitions here, rather than replacing the first one. Maybe the size calculation should be factored out into a helper function as well?

+    // Multiply the current size by the element size.
+    ArraySizeVal = SvalBuilder.evalBinOpNN(State, BO_Mul, ArrayLength,
+      ArraySizeVal.castAs<NonLoc>(), SizeTy);

Can you add a FIXME in here to check for overflow? That's a security risk, really.

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.

Thanks for fixing this up,
Jordan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141008/24fd0ace/attachment.html>