[PATCH] Teach the analyzer about multi-dimensional VLAs

[Daniel Fahlgren]

Hi Jordan,

On tor, 2014-10-16 at 09:21 -0700, Jordan Rose wrote:
> On Oct 14, 2014, at 15:12 , Daniel Fahlgren <daniel at fahlgren.se> wrote:
> 
> > 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? 
> 
> It's more just the normal uniquing of identical nodes kicking in—if
> we've somehow already gotten to this state, that's supposed to mean
> we've already emitted any errors. I think that means you should collect
> all the errors up front and then emit them all on the same sink node.
> Emitting them on the predecessor would be valid but not really what we
> want, because it could have the wrong location.


Ah, thanks for the explanation. I've updated the patch to store all
errors in a vector, allowing to emit all of them on the same node.


> >> +
> >> +    // 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.
> 
> On platforms where RSIZE_MAX exists and is smaller than SIZE_MAX the
> "most correct" answer is probably RSIZE_MAX. If not that, then we could
> perhaps make a policy of using SSIZE_MAX (or SIZE_MAX >> 1, which is
> how RSIZE_MAX is defined on my system anyway). But even just the simple
> case of overflowing a size_t would probably be good enough.
> 
> Again, though, let's worry about this later. :-)
> 
> 
> >> 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.
> 
> Huh, I thought we handled sizeof for a VLA by using the extent, but I
> guess not. Out-of-bounds checks it is.
> 
> Jordan

Best regards,
Daniel Fahlgren
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vla.patch
Type: text/x-patch
Size: 13486 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141020/6648be22/attachment.bin>