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

[Zebediah Figura]

On 2/14/22 13:32, Eric Pouech wrote:
> > 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.
>
> compiling here your snipnet gives 16 bytes for apple not 14
>
> using malloc(16) instead of malloc(offsetof(struct apple, v[10]))
> doesn't generate warning
>
> my understanding if that gcc generates a warning when accessing a
> structure which allocation is strictly smaller than the structure size
>
> even if all required bits are inside the allocated part

Right, I managed to forget how struct alignment works.

[1] claims the following:

"It's not valid to access an object of one type using an lvalue of 
another.  In simple cases GCC diagnoses violations of this requirement 
by -Wstrict-aliasing.  -Warray-bounds doesn't detect aliasing violations 
but it does detect a subset of the problem that's apparent when the size 
of the lvalue's type is greater than the size of the object.  The object 
must be big enough for the whole lvalue, even if the accessed member is 
within the bounds of the smaller object."

I can't easily find this in the C specification, but it's also not the 
easiest thing to search for, and it wouldn't surprise me if it's true.

Still, I'm not convinced we really want to change our usage accordingly. 
I'm guessing this is one of those things that may be undefined behaviour 
but always works in practice, and from the looks of things we have more 
false positives from -Warray-bounds than true positives.

But if we really do want -Warray-bounds, I'd rather just allocate the 
whole VIDEOINFO structure here.


[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103292

P.S. can you please remember to keep wine-devel in CC?

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

[Eric Pouech]

Le 15/02/2022 à 18:54, Zebediah Figura a écrit :
> [1] claims the following:
>
> "It's not valid to access an object of one type using an lvalue of 
> another.  In simple cases GCC diagnoses violations of this requirement 
> by -Wstrict-aliasing.  -Warray-bounds doesn't detect aliasing 
> violations but it does detect a subset of the problem that's apparent 
> when the size of the lvalue's type is greater than the size of the 
> object.  The object must be big enough for the whole lvalue, even if 
> the accessed member is within the bounds of the smaller object."
>
> I can't easily find this in the C specification, but it's also not the 
> easiest thing to search for, and it wouldn't surprise me if it's true.

I think you mean this (from C89/90 ; 6.3 expressions)

Anobjectshallhaveitsstoredvalueaccessedonlybyanlvaluethathasoneofthefollowing
types?”
-thedeclaredtypeoftheobject,
-a qualifiedversionofthedeclaredtypeoftheobject.
-a typethatis 
thesignedorunsignedtypecorrespondingtothedeclaredtypeoftheobject,
-a typethatisthesignedorunsignedtypecorrespondingtoa 
qualifiedversionofthedeclared
typeoftheobject.
-anaggregateoruniontypethatincludesoneoftheaforementionedtypesamongitsmembers
(including.recursively.a memberofa hubaggregateorcontainedunion).or
-a charactertype
> Still, I'm not convinced we really want to change our usage 
> accordingly. I'm guessing this is one of those things that may be 
> undefined behaviour but always works in practice, and from the looks 
> of things we have more false positives from -Warray-bounds than true 
> positives.

I'm not proposing to find and fix all UB in Wine code [1]; I'm just 
trying to reduce the warnigs generated by our default settings : roughly 
200 warnings for a full Wine compilation for gcc (mingw 11.2 cross 
compiler) [2]

very few true positive indeed (IMO the years of coverity and other tools 
did clean up a lot of them) (only one found [3])
> But if we really do want -Warray-bounds, I'd rather just allocate the 
> whole VIDEOINFO structure here.

I think it's still useful for detecting bad allocations in future code

I'll resubmit with the whole allocation then

thanks for looking into it
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103292
>
> P.S. can you please remember to keep wine-devel in CC?

sorry, my bad

[1] even though having ubsan and asan running on Wine's PE code would be 
nice (mingw clang may provide it)

[2] it maybe I get nauseous about those warnings because of all the full 
recompilations for the 'long types' thingie

[3] the funny story is that the first code committed was correct but it 
has been (badly) changed to fix a Coverity report <g>

https://source.winehq.org/git/wine.git/?a=commit;h=14f8089dc1793a6c7d91d3dfe...