Re: [PATCH] Do without union of variable length arrays.

[Chih-Hung Hsieh <chh at google dot com>]

I will prepare a new patch with the suggested changes:

(a) more efficient overflow check like
        if (num > SIZE_MAX / elem_size)

(b) replacing union of VLA with pointers to arrays like
         void *data = malloc (...);
         T32 (*a32)[n] = data;
         T64 (*a64)[n] = data;

Please feel free to suggest better solutions.
Thanks.


On Mon, Oct 5, 2015 at 11:45 AM, Mark Wielaard <mjw@redhat.com> wrote:

> On Fri, Oct 02, 2015 at 12:10:47AM +0300, Alexander Cherepanov wrote:
> > On 2015-09-16 18:25, Mark Wielaard wrote:
> > >On Fri, 2015-09-11 at 12:22 -0700, Roland McGrath wrote:
> > >>It looks fine to me from a quick skim, but Mark should review and test
> it too.
> > >
> > >I am not super enthusiastic about this change, it seems to just take
> > >away type/size information that the compiler/bounds checking tools can
> > >use.
> >
> > I'm not sure I fully understand the problem. As I understand it unions of
> > VLAs are not Ok while VLAs are Ok and even desirable due to bounds
> checking,
> > right?
>
> There are two issues. First with GCC VLA types are OK and desired compared
> to types not using bounds. But if such a VLAs might be unbounded then they
> should not be stack allocated (this is enforced by building with
> -Wstack-usage except for a couple of files that haven't been converted).
> Secondly there are people who want to use clang to build elfutils. clang
> doesn't support various GNU extensions used in the code like VLAs. So
> for those people any VLA type seems problematic.
>
> > Why not just use VLAs of unions? Cold memory?
>
> I am not sure clang supports such VLA types. If it does, then I would say
> VLAs of unions are preferred above a type that doesn't include bounds.
>
> > Given that the current approach (before the patch) already required to
> write
> > superfluous "->" perhaps an approach requiring a superfluous "*" will
> fit?
> > Like this:
> >
> >   void *data = malloc (...);
> >   T32 (*a32)[n] = data;
> >   T64 (*a64)[n] = data;
> >
> > Then the use looks like "(*a32)[i].member". Clang seems to be happy and
> its
> > UBSAN works fine.
>
> If that works that would probably be preferred since then ubsan can see
> the array bounds and help catch issues. You can build and run elfutils
> and the tests with configure --enable-sanitize-undefined to use ubsan
> checking.
>
> Cheers,
>
> Mark
>

I will prepare a new patch with the suggested changes:

(a) more efficient overflow check like

        if (num > SIZE_MAX / elem_size)

(b) replacing union of VLA with pointers to arrays like

         void *data = "" (...);

         T32 (*a32)[n] = data;

         T64 (*a64)[n] = data;

Please feel free to suggest better solutions.

Thanks.

On Mon, Oct 5, 2015 at 11:45 AM, Mark Wielaard <mjw@redhat.com> wrote:

On Fri, Oct 02, 2015 at 12:10:47AM +0300, Alexander Cherepanov wrote:
> On 2015-09-16 18:25, Mark Wielaard wrote:
> >On Fri, 2015-09-11 at 12:22 -0700, Roland McGrath wrote:
> >>It looks fine to me from a quick skim, but Mark should review and test it too.
> >
> >I am not super enthusiastic about this change, it seems to just take
> >away type/size information that the compiler/bounds checking tools can
> >use.
>
> I'm not sure I fully understand the problem. As I understand it unions of
> VLAs are not Ok while VLAs are Ok and even desirable due to bounds checking,
> right?

There are two issues. First with GCC VLA types are OK and desired compared
to types not using bounds. But if such a VLAs might be unbounded then they
should not be stack allocated (this is enforced by building with
-Wstack-usage except for a couple of files that haven't been converted).
Secondly there are people who want to use clang to build elfutils. clang
doesn't support various GNU extensions used in the code like VLAs. So
for those people any VLA type seems problematic.

> Why not just use VLAs of unions? Cold memory?

I am not sure clang supports such VLA types. If it does, then I would say
VLAs of unions are preferred above a type that doesn't include bounds.

> Given that the current approach (before the patch) already required to write
> superfluous "->" perhaps an approach requiring a superfluous "*" will fit?
> Like this:
>
>   void *data = "" (...);
>   T32 (*a32)[n] = data;
>   T64 (*a64)[n] = data;
>
> Then the use looks like "(*a32)[i].member". Clang seems to be happy and its
> UBSAN works fine.

If that works that would probably be preferred since then ubsan can see
the array bounds and help catch issues. You can build and run elfutils
and the tests with configure --enable-sanitize-undefined to use ubsan
checking.

Cheers,

Mark