Re: [PATCH] flex-array: allow arrays of unions with flexible members.

[Luc Van Oostenryck <luc.vanoostenryck@gmail.com>]

On Thu, Oct 08, 2020 at 08:36:02AM +0200, Ilya Maximets wrote:
> On 10/8/20 1:09 AM, Luc Van Oostenryck wrote:
> > On Wed, Oct 07, 2020 at 01:52:34PM +0200, Ilya Maximets wrote:
> >> There is a common pattern on how to allocate memory for a flexible-size
> >> structure, e.g.
> >>
> >>   union {
> >>       struct flex f;  /* Structure that contains a flexible array. */
> >>       char buf[MAX_SIZE];  /* Memeory buffer for structure 'flex' and
> >>                               its flexible array. */
> >>   };
> >>
> >> There is another exmaple of such thing in CMSG manpage with the
> >> alignment purposes:
> >>
> >>   union {         /* Ancillary data buffer, wrapped in a union
> >>                      in order to ensure it is suitably aligned */
> >>       char buf[CMSG_SPACE(sizeof(myfds))];
> >>       struct cmsghdr align;
> >>   } u;
> >>
> >> Such unions could form an array in case user wants multiple
> >> instances of them.  For example, if you want receive up to
> >> 32 network packets via recvmmsg(), you will need 32 unions like 'u'.
> >> Open vSwitch does exactly that and fails the check.
> >>
> >> Disabling this check by default for unions.  Adding new option
> >> -Wflex-union-array to enable it back.  This option works only
> >> if -Wflex-array-array enabled (which is default behavior).
> >>
> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> >> ---
> >>
> >> Not sure if this is a best way to fix the issue, but it works fine for
> >> openvswitch project. The actual code in question that makes sparse fail
> >> OVS build could be found here:
> >>   https://github.com/openvswitch/ovs/blob/39fbd2c3f0392811689ec780f09baf90faceb877/lib/netdev-linux.c#L1238
> > 
> > This fixes your problem for -Wflexible-array-array but the same
> > will happen with -Wflexible-array-sizeof (and you're using sizeof()
> > on such flexible unions) and -Wflexible-array-nested.
> 
> I thought that it will fail some other checks too, but for some reason
> it doesn't.  But, yes, you're right, It sounds safer to disable all
> of them to avoid possible issues in the future since we're actually
> using these unions.

Thanks for giving it a try.

> >  options.c                         |  2 ++
> >  options.h                         |  1 +
> >  sparse.1                          |  9 +++++++++
> >  symbol.c                          |  2 +-
> >  validation/flex-union-array-no.c  |  9 +++++++++
> >  validation/flex-union-array-yes.c | 11 +++++++++++
> >  validation/flex-union-array.h     | 11 +++++++++++
> >  7 files changed, 44 insertions(+), 1 deletion(-)
> >  create mode 100644 validation/flex-union-array-no.c
> >  create mode 100644 validation/flex-union-array-yes.c
> >  create mode 100644 validation/flex-union-array.h
> 
> Since you renamed the option, it might make sense to rename
> files to 'flex-array-union...'.

Well yes, I left them more or less on purpose but renamed them now.

-- Luc