Re: [PATCH 4/6] dlls/quartz: use VIDEOINFOHEADER instead of partially allocated VIDEOINFO structure

[Zebediah Figura]

On 2/14/22 12:27, Eric Pouech wrote:
> Le 14/02/2022 à 19:07, Zebediah Figura a écrit :
> > On 2/14/22 03:28, Eric Pouech wrote:
> > > this fixes a couple of GCC11 warnings (-Wbounds)
> > >
> > > Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com>
> > >
> > > ---
> > >    dlls/quartz/avidec.c |    9 +++++----
> > >    1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/dlls/quartz/avidec.c b/dlls/quartz/avidec.c
> > > index f8660890273..1295e8704df 100644
> > > --- a/dlls/quartz/avidec.c
> > > +++ b/dlls/quartz/avidec.c
> > > @@ -331,7 +331,7 @@ static HRESULT
> > > avi_decompressor_source_get_media_type(struct strmbase_pin *iface
> > >          struct avi_decompressor *filter =
> > > impl_from_strmbase_filter(iface->filter);
> > >        const VIDEOINFOHEADER *sink_format;
> > > -    VIDEOINFO *format;
> > > +    VIDEOINFOHEADER *format;
> > >          if (!filter->sink.pin.peer)
> > >            return VFW_S_NO_MORE_ITEMS;
> > > @@ -363,9 +363,10 @@ static HRESULT
> > > avi_decompressor_source_get_media_type(struct strmbase_pin *iface
> > >              if (IsEqualGUID(formats[index].subtype,
> > > &MEDIASUBTYPE_RGB565))
> > >            {
> > > -            format->dwBitMasks[iRED] = 0xf800;
> > > -            format->dwBitMasks[iGREEN] = 0x07e0;
> > > -            format->dwBitMasks[iBLUE] = 0x001f;
> > > +            DWORD *dwBitMasks = (DWORD *)(format + 1);
> > > +            dwBitMasks[iRED] = 0xf800;
> > > +            dwBitMasks[iGREEN] = 0x07e0;
> > > +            dwBitMasks[iBLUE] = 0x001f;
> > >                mt->cbFormat = offsetof(VIDEOINFO, dwBitMasks[3]);
> > >            }
> > >            else
> >
> > I don't think this is an improvement.
> >
> > I also don't get the warning with gcc 11.2.0. How exactly are you
> > compiling and with which version?
>
> Hi Zebdiah
>
> compiling with stock gcc version from fedora and regular wine's
> configure (no parameter added)
>
> this is what I get with compiling a 64bit tree, configured with
>
> ./configure --enable-wine64
>
> make[1]: Entering directory '/home/eric/work/wine-format/build64'
> x86_64-w64-mingw32-gcc -c -o dlls/quartz/avidec.cross.o
> /home/eric/work/wine-format/dlls/quartz/avidec.c -Idlls/quartz \
>     -I/home/eric/work/wine-format/dlls/quartz -Iinclude
> -I/home/eric/work/wine-format/include \
>     -I/home/eric/work/wine-format/include/msvcrt -D__WINESRC__ -D_UCRT
> -D__WINE_PE_BUILD -Wall \
>     -fno-strict-aliasing -Wdeclaration-after-statement -Wempty-body
> -Wignored-qualifiers -Winit-self \
>     -Wno-packed-not-aligned -Wshift-overflow=2 -Wstrict-prototypes
> -Wtype-limits \
>     -Wunused-but-set-parameter -Wvla -Wwrite-strings -Wpointer-arith
> -Wlogical-op -Wabsolute-value \
>     -Wno-format -Wformat-overflow -Wnonnull -mcx16 -gdwarf-4 -Wformat -g -O2
> /home/eric/work/wine-format/dlls/quartz/avidec.c: In function
> 'avi_decompressor_source_get_media_type':
> /home/eric/work/wine-format/dlls/quartz/avidec.c:349:15: warning: array
> subscript 'VIDEOINFO {aka struct tagVIDEOINFO}[0]' is partly outside
> array bounds of 'unsigned char[100]' [-Warray-bounds]
>     349 |         format->rcSource = sink_format->rcSource;
>         |               ^~
> /home/eric/work/wine-format/dlls/quartz/avidec.c:345:24: note:
> referencing an object of size 100 allocated by 'CoTaskMemAlloc'
>     345 |         if (!(format = CoTaskMemAlloc(offsetof(VIDEOINFO,
> dwBitMasks[3]))))
>         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Okay, I was wrong about my gcc version; for some reason Debian ships gcc 
and mingw-w64 gcc versions that are different.

I think this is a gcc bug. It now warns about *any* access to a 
structure which was allocated with anything but sizeof(). E.g. the 
following will generate warnings with -O2 -Warray-bounds=1:

--

#include <stddef.h>
#include <stdlib.h>

struct apple
{
     unsigned int u;
     char v[10];
};

struct apple *func(void)
{
     struct apple *a = malloc(offsetof(struct apple, v[10]));

     a->u = 1;
     a->v[9] = 3;
     return a;
}

--

Same with malloc(14). Only malloc(sizeof(struct apple)) works, even 
though the three expressions are obviously identical.

If we must work around this in Wine, I'd rather either disable 
-Warray-bounds or else just use sizeof() and over-allocate.