Re: RFA: new pass to warn on questionable uses of alloca() and VLAs

[Jeff Law <law at redhat dot com>]

On 07/08/2016 05:48 AM, Aldy Hernandez wrote:
> [New thread now that I actually have a tested patch :)].
>
> > I think detecting potentially problematic uses of alloca would
> > be useful, especially when done in an intelligent way like in
> > your patch (as opposed to simply diagnosing every call to
> > the function regardless of the value of its argument).  At
> > the same time, it seems that an even more reliable solution
> > than pointing out potentially unsafe calls to the function
> > and relying on users to modify their code to use malloc for
> > large/unbounded allocations would be to let GCC do it for
> > them automatically (i.e., in response to some other option,
> > emit a call to malloc instead, and insert a call to free when
> > appropriate).
>
> As Jeff said, we were thinking the other way around: notice a malloced
> area that doesn't escape and replace it with a call to alloca.  But all
> this is beyond the scope of this patch.
Definitely out of scope for this set of changes.    BUt it's part of my 
evil plan to squash explicit alloca calls while still allowing its use 
when it's provably safe.  But we're not in that world yet.

> > I found the "warning: unbounded use of alloca" misleading when
> > a call to the function was, in fact, bounded but to a limit
> > that's greater than alloca-max-size as in the program below:
>
> I have added various levels of granularity for the warning, along with
> appropriately different messages:
>
> // Possible problematic uses of alloca.
> enum alloca_type {
>   // Alloca argument is within known bounds that are appropriate.
>   ALLOCA_OK,
>
>   // Alloca argument is KNOWN to have a value that is too large.
>   ALLOCA_BOUND_DEFINITELY_LARGE,
>
>   // Alloca argument may be too large.
>   ALLOCA_BOUND_MAYBE_LARGE,
>
>   // Alloca argument is bounded but of an indeterminate size.
>   ALLOCA_BOUND_UNKNOWN,
>
>   // Alloca argument was casted from a signed integer.
>   ALLOCA_CAST_FROM_SIGNED,
>
>   // Alloca appears in a loop.
>   ALLOCA_IN_LOOP,
>
>   // Alloca call is unbounded.  That is, there is no controlling
>   // predicate for its argument.
>   ALLOCA_UNBOUNDED
> };
>
> Of course, there are plenty of cases where we can't get the exact
> diagnosis (due to the limitations on our range info) and we fall back to
> ALLOCA_UNBOUNDED or ALLOCA_BOUND_MAYBE_LARGE.  In practice, I'm
> wondering whether we should lump everything into 2-3 warnings instead of
> trying so hard to get the exact reason for the problematic use of
> alloca.  (More details on upcoming changes to range info further down.)
I'll trust your judgment on this -- I kind of like the more precise 
warnings -- if for no other reason than it makes it easier for someone 
looking at legacy code (think glibc) to see why GCC determined their 
alloca call was unsafe.

> >   void f (void*);
> >
> >   void g (int n)
> >   {
> >     void *p;
> >     if (n < 4096)
> >       p = __builtin_alloca (n);
> >     else
> >       p = __builtin_malloc (n);
> >     f (p);
> >   }
> >   t.C: In function ‘g’:
> >   t.C:7:7: warning: unbounded use of alloca [-Walloca]
> >        p = __builtin_alloca (n);
>
> Well, in this particular case you are using a signed int, so n < 4096
> can cause the value passed to alloca  to be rather large in the case of
> n < 0.
Right.  And if the bad guys can get control of "N" in this case, they 
can pass in a negative value and perform a "stack shift" -- essentially 
moving the stack so that it points somewhere else  more "interesting" in 
memory.


> This patch depends on range information, which is less than stellar past
> the VRP pass.  To get better range information, we'd have to incorporate
> this somehow into the VRP pass and use the ASSERT_EXPRs therein.  And
> still, VRP gets awfully confused with PHI merge points.  Andrew Macleod
> is working on a new fancy range info infrastructure that should
> eliminate a lot of the false positives we may get and/or help us
> diagnose a wider range of cases.  Hence the reason that I'm not bending
> over backwards to incorporate this pass into tree-vrp.c.
Right.  Getting better range information is important in so many ways.

> FYI, I have a few XFAILed tests in this patch, to make sure we don't
> lose track of possible improvements (which in this case should be
> handled by better range info).  What's the blessed way of doing this?
> Are adding new XFAILed tests acceptable?
I'd go with a new XFAIL'd test.

Jeff