[PATCH] diagnosing flexible array assignments

[Eli Friedman]

On Tue, Sep 10, 2013 at 2:50 PM, Aaron Ballman <aaron at aaronballman.com>wrote:

> On Tue, Sep 10, 2013 at 5:40 PM, Eli Friedman <eli.friedman at gmail.com>
> wrote:
> > On Tue, Sep 10, 2013 at 2:33 PM, Aaron Ballman <aaron at aaronballman.com>
> > wrote:
> >>
> >> On Tue, Sep 10, 2013 at 5:21 PM, Eli Friedman <eli.friedman at gmail.com>
> >> wrote:
> >> > On Tue, Sep 10, 2013 at 2:03 PM, Aaron Ballman <
> aaron at aaronballman.com>
> >> > wrote:
> >> >>
> >> >> Currently, if you have a value assignment involving structures
> >> >> containing a flexible array, no diagnostic is emitted.  However,
> since
> >> >> the array members won't be copied as part of the assignment, a
> >> >> diagnostic will help programmers avoid bugs.  This patch emits said
> >> >> diagnostic.
> >> >>
> >> >
> >> > +def warn_flexible_array_assignment : ExtWarn<
> >> > +  "assignment of flexible arrays does not copy the array members">,
> >> > +  InGroup<FlexibleArrayExtensions>;
> >> >
> >> > I assume this is supposed to be a Warning, not an ExtWarn?
> >>
> >> Yes, thanks for pointing that out.
> >>
> >> > Also, I'm not sure putting this into the FlexibleArrayExtensions
> >> > diagnostic
> >> > group is the best idea.
> >>
> >> I wasn't certain either, but it was the closest existing group.  Would
> >> it make more sense to add a new group for it?
> >
> >
> > I think so.
>
> Any complaints with naming the group flexible-arrays?
>

That's sort of vague; maybe flexible-array-slicing?


>
> >
> >>
> >> > What sort of mistakes do you expect this to catch in practice?
> >>
> >> Simple programmer mistakes, mostly.  It also nicely covers a CERT
> >> secure C coding rule:
> >>
> >>
> >>
> https://www.securecoding.cert.org/confluence/display/seccode/MEM33-C.++Allocate+and+copy+structures+containing+flexible+array+members+dynamically
> >
> >
> > Okay.
> >
> > Do you have any idea what the false positive rate is?
>
> I would expect it to be quite low, but have nothing concrete to back
> that up.  If the warning turns out to be chatty for some reason, it
> could be tweaked easily enough (esp with a separate diagnostic group).
>

Okay.


> >
> >>
> >>
> >>
> >> > Have you considered warning about the variable declaration rather than
> >> > the
> >> > assignment?  In your testcase, it's impossible to call foo() without
> >> > triggering the warning, so it seems better to warn about foo() itself
> >> > rather
> >> > than the call.
> >>
> >> Hmmm, would there ever be a case where it would make sense to declare
> >> a structure with a flexible array member as a value type?The only
> >>
> >> situation I could think of would be overlaying the value type with
> >> stack-allocated memory in some sort of bizarre union type punning
> >> scenario.  So I'm thinking that may be a better approach than checking
> >> on assignment, unless there are intermediary ways you could get this
> >> assignment to happen in C.
> >>
> >
> > Well, you can write something like "*a = *b;", which doesn't involve
> > declaring a variable...
>
> True.  Assignment would be the catch-all way to do it, since that's
> what causes the slicing behavior.  Perhaps both declaration and
> assignment diagnostics would make sense?  Declarations like that are
> definitively bizarre, but assignment still misbehaves.
>
>
The CERT page classifies the mistakes into three categories; covering all
of them seems reasonable.

-Eli
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130910/da0aec42/attachment.html>