[PATCH] Teach the analyzer about multi-dimensional VLAs

[Daniel Fahlgren]

Hi Jordan,

On ons, 2014-10-08 at 20:05 -0700, Jordan Rose wrote:

> +  // 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?

Good point. I'm not sure I solved this the correct way. It seems like
it only is possible to create one sink node so I had to call
getPredecessor(). Is that the right way or how should I do to emit
multiple errors? 

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

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

Ok.
 
> +  // 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.

I've broken out the calculation and added the FIXME, but that got me
thinking. What is a too large size? If the size is larger than a size_t
can represent we surely got a problem. We will most likely run out of
stack space way before that, however.

> 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.

That turned out to be easier said than done. The analyzer doesn't seem
to handle sizeof on things that can't be resolved compile time (like
VLAs)? I've also added some FIXMEs for ArrayBoundV2. Casting to char *
works as expected.

Attached is a new version of the patch.

Cheers,
Daniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vla.patch
Type: text/x-patch
Size: 11604 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141015/207e7389/attachment.bin>