[PATCH] dlls/windows.globalization: don't allocate a partial structure
GCC 11 complains about accessing struct hstring_vector (-Warray-bounds) when the allocation is made for a 0-sized vector so ensure that we always allocate a memory block to fit a whole structure Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/windows.globalization/main.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/dlls/windows.globalization/main.c b/dlls/windows.globalization/main.c index 3e5a59bde14..363e0150af1 100644 --- a/dlls/windows.globalization/main.c +++ b/dlls/windows.globalization/main.c @@ -213,8 +213,10 @@ static const struct IVectorView_HSTRINGVtbl hstring_vector_vtbl = static HRESULT hstring_vector_create(HSTRING *values, SIZE_T count, IVectorView_HSTRING **out) { struct hstring_vector *impl; - - if (!(impl = malloc(offsetof(struct hstring_vector, values[count])))) return E_OUTOFMEMORY; + /* always allocate at least the full structure to avoid GCC11 warnings */ + if (!(impl = malloc(max(offsetof(struct hstring_vector, values[count]), + sizeof(struct hstring_vector))))) + return E_OUTOFMEMORY; impl->ref = 1; impl->IVectorView_HSTRING_iface.lpVtbl = &hstring_vector_vtbl;
On 3/23/22 08:19, Eric Pouech wrote:
GCC 11 complains about accessing struct hstring_vector (-Warray-bounds) when the allocation is made for a 0-sized vector
so ensure that we always allocate a memory block to fit a whole structure
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com>
--- dlls/windows.globalization/main.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/dlls/windows.globalization/main.c b/dlls/windows.globalization/main.c index 3e5a59bde14..363e0150af1 100644 --- a/dlls/windows.globalization/main.c +++ b/dlls/windows.globalization/main.c @@ -213,8 +213,10 @@ static const struct IVectorView_HSTRINGVtbl hstring_vector_vtbl = static HRESULT hstring_vector_create(HSTRING *values, SIZE_T count, IVectorView_HSTRING **out) { struct hstring_vector *impl; - - if (!(impl = malloc(offsetof(struct hstring_vector, values[count])))) return E_OUTOFMEMORY; + /* always allocate at least the full structure to avoid GCC11 warnings */ + if (!(impl = malloc(max(offsetof(struct hstring_vector, values[count]), + sizeof(struct hstring_vector))))) + return E_OUTOFMEMORY; impl->ref = 1;
impl->IVectorView_HSTRING_iface.lpVtbl = &hstring_vector_vtbl;
IMHO GCC should fix its warning instead, we do that in many places and I think it's completely valid. -- Rémi Bernon <rbernon(a)codeweavers.com>
Le 23/03/2022 à 11:15, Rémi Bernon a écrit :
On 3/23/22 08:19, Eric Pouech wrote:
GCC 11 complains about accessing struct hstring_vector (-Warray-bounds) when the allocation is made for a 0-sized vector
so ensure that we always allocate a memory block to fit a whole structure
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com>
--- dlls/windows.globalization/main.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/dlls/windows.globalization/main.c b/dlls/windows.globalization/main.c index 3e5a59bde14..363e0150af1 100644 --- a/dlls/windows.globalization/main.c +++ b/dlls/windows.globalization/main.c @@ -213,8 +213,10 @@ static const struct IVectorView_HSTRINGVtbl hstring_vector_vtbl = static HRESULT hstring_vector_create(HSTRING *values, SIZE_T count, IVectorView_HSTRING **out) { struct hstring_vector *impl; - - if (!(impl = malloc(offsetof(struct hstring_vector, values[count])))) return E_OUTOFMEMORY; + /* always allocate at least the full structure to avoid GCC11 warnings */ + if (!(impl = malloc(max(offsetof(struct hstring_vector, values[count]), + sizeof(struct hstring_vector))))) + return E_OUTOFMEMORY; impl->ref = 1; impl->IVectorView_HSTRING_iface.lpVtbl = &hstring_vector_vtbl;
IMHO GCC should fix its warning instead, we do that in many places and I think it's completely valid.
Hi Rémi see https://www.winehq.org/pipermail/wine-devel/2022-February/thread.html#207795 for a previous discussion on a similar issue (and the final decision of over-allocating) to my understanding, accessing through a pointer of type mytype* a memory block which storage is strictly smaller to sizeo(mytype) is clearly undefined behavior (I'm not stating that it does in fact generate wrong results) (even if the accessed field is inside the allocated memory size) in this precise case, defining the structure with 0 length array would be another option, yet non portable and for the record, gcc12 (even if non yet released) generates a few more warnings about this subject on wine code (and I'm not even talking of mingw port of gcc12) A+
On 3/23/22 12:11, Eric Pouech wrote:
Le 23/03/2022 à 11:15, Rémi Bernon a écrit :
On 3/23/22 08:19, Eric Pouech wrote:
GCC 11 complains about accessing struct hstring_vector (-Warray-bounds) when the allocation is made for a 0-sized vector
so ensure that we always allocate a memory block to fit a whole structure
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com>
--- dlls/windows.globalization/main.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/dlls/windows.globalization/main.c b/dlls/windows.globalization/main.c index 3e5a59bde14..363e0150af1 100644 --- a/dlls/windows.globalization/main.c +++ b/dlls/windows.globalization/main.c @@ -213,8 +213,10 @@ static const struct IVectorView_HSTRINGVtbl hstring_vector_vtbl = static HRESULT hstring_vector_create(HSTRING *values, SIZE_T count, IVectorView_HSTRING **out) { struct hstring_vector *impl; - - if (!(impl = malloc(offsetof(struct hstring_vector, values[count])))) return E_OUTOFMEMORY; + /* always allocate at least the full structure to avoid GCC11 warnings */ + if (!(impl = malloc(max(offsetof(struct hstring_vector, values[count]), + sizeof(struct hstring_vector))))) + return E_OUTOFMEMORY; impl->ref = 1; impl->IVectorView_HSTRING_iface.lpVtbl = &hstring_vector_vtbl;
IMHO GCC should fix its warning instead, we do that in many places and I think it's completely valid.
Hi Rémi
see https://www.winehq.org/pipermail/wine-devel/2022-February/thread.html#207795 for a previous discussion on a similar issue (and the final decision of over-allocating)
to my understanding, accessing through a pointer of type mytype* a memory block which storage is strictly smaller to sizeo(mytype) is clearly undefined behavior (I'm not stating that it does in fact generate wrong results)
(even if the accessed field is inside the allocated memory size)
in this precise case, defining the structure with 0 length array would be another option, yet non portable
and for the record, gcc12 (even if non yet released) generates a few more warnings about this subject on wine code (and I'm not even talking of mingw port of gcc12)
A+
Well I think this makes the code very bloated, and if it is truly UB then we should instead find a way to use 0-sized arrays in a portable way. I believe 0 sized array for the last struct member is standard C99, so is there any compiler not supporting it yet (or some variation of it like with empty size)? Of course this doesn't apply to structs which need a specific size with 1-sized array for Win32 ABI compat, but it's probably not that many and not the case here. -- Rémi Bernon <rbernon(a)codeweavers.com>
Well I think this makes the code very bloated, and if it is truly UB then we should instead find a way to use 0-sized arrays in a portable way.
I believe 0 sized array for the last struct member is standard C99, so is there any compiler not supporting it yet (or some variation of it like with empty size)?
yes the so called "flexible array member" is c99 and seems to be supported by gcc, clang, msvc but they don't seem to be accepted directly into wine (for now) (for example include/winnt.h see https://source.winehq.org/source/include/winnt.h#5815) (there are a few others instances in the same way) there are a couple of 0-length array instances (mosts of them protected by #ifdef _GNUC and fallback with 1 length array) (and wine/msvcpdb.h uses them directly w/o fallbacks...) I did a previous attempt with 0 length array for this but it never landed (https://www.winehq.org/pipermail/wine-devel/2022-February/207793.html) so I'd wait for Alexandre's guidance on this A+
On 3/23/22 14:05, Eric Pouech wrote:
Well I think this makes the code very bloated, and if it is truly UB then we should instead find a way to use 0-sized arrays in a portable way.
I believe 0 sized array for the last struct member is standard C99, so is there any compiler not supporting it yet (or some variation of it like with empty size)?
yes the so called "flexible array member" is c99 and seems to be supported by gcc, clang, msvc
but they don't seem to be accepted directly into wine (for now) (for example include/winnt.h see https://source.winehq.org/source/include/winnt.h#5815)
(there are a few others instances in the same way)
there are a couple of 0-length array instances (mosts of them protected by #ifdef _GNUC and fallback with 1 length array)
(and wine/msvcpdb.h uses them directly w/o fallbacks...)
I did a previous attempt with 0 length array for this but it never landed (https://www.winehq.org/pipermail/wine-devel/2022-February/207793.html)
so I'd wait for Alexandre's guidance on this
A+
C99 flexible array is with empty size, not 0. From https://gcc.godbolt.org/z/nc6xGM5nh I don't see any compiler not supporting even both methods, though -pedantic emits a warning with 0 size (empty size is fine). It only tests some old GCC / Clang version as they don't provide old MSVC but maybe it's okay? -- Rémi Bernon <rbernon(a)codeweavers.com>
On 3/23/22 19:40, Rémi Bernon wrote:
On 3/23/22 14:05, Eric Pouech wrote:
Well I think this makes the code very bloated, and if it is truly UB then we should instead find a way to use 0-sized arrays in a portable way.
I believe 0 sized array for the last struct member is standard C99, so is there any compiler not supporting it yet (or some variation of it like with empty size)?
yes the so called "flexible array member" is c99 and seems to be supported by gcc, clang, msvc
but they don't seem to be accepted directly into wine (for now) (for example include/winnt.h see https://source.winehq.org/source/include/winnt.h#5815)
(there are a few others instances in the same way)
there are a couple of 0-length array instances (mosts of them protected by #ifdef _GNUC and fallback with 1 length array)
(and wine/msvcpdb.h uses them directly w/o fallbacks...)
I did a previous attempt with 0 length array for this but it never landed (https://www.winehq.org/pipermail/wine-devel/2022-February/207793.html)
so I'd wait for Alexandre's guidance on this
A+
C99 flexible array is with empty size, not 0.
From https://gcc.godbolt.org/z/nc6xGM5nh I don't see any compiler not supporting even both methods, though -pedantic emits a warning with 0 size (empty size is fine).
It only tests some old GCC / Clang version as they don't provide old MSVC but maybe it's okay?
Note that it is not valid C++, so I don't think we can safely use it in public headers. -- Rémi Bernon <rbernon(a)codeweavers.com>
Rémi Bernon <rbernon(a)codeweavers.com> writes:
On 3/23/22 14:05, Eric Pouech wrote:
Well I think this makes the code very bloated, and if it is truly UB then we should instead find a way to use 0-sized arrays in a portable way.
I believe 0 sized array for the last struct member is standard C99, so is there any compiler not supporting it yet (or some variation of it like with empty size)? yes the so called "flexible array member" is c99 and seems to be supported by gcc, clang, msvc but they don't seem to be accepted directly into wine (for now) (for example include/winnt.h see https://source.winehq.org/source/include/winnt.h#5815) (there are a few others instances in the same way)
there are a couple of 0-length array instances (mosts of them protected by #ifdef _GNUC and fallback with 1 length array) (and wine/msvcpdb.h uses them directly w/o fallbacks...)
I did a previous attempt with 0 length array for this but it never landed (https://www.winehq.org/pipermail/wine-devel/2022-February/207793.html)
so I'd wait for Alexandre's guidance on this A+
C99 flexible array is with empty size, not 0.
From https://gcc.godbolt.org/z/nc6xGM5nh I don't see any compiler not supporting even both methods, though -pedantic emits a warning with 0 size (empty size is fine).
It only tests some old GCC / Clang version as they don't provide old MSVC but maybe it's okay?
An empty size is probably OK to use at this point. It's not clear how much benefit it brings though, because obviously 1-size arrays in public structures can't be changed. -- Alexandre Julliard julliard(a)winehq.org
On Wednesday, March 23, 2022 12:32:52 PM PDT Alexandre Julliard wrote:
An empty size is probably OK to use at this point. It's not clear how much benefit it brings though, because obviously 1-size arrays in public structures can't be changed.
And I'm not sure it would change anything regarding this patch and ensuring the object is properly allocated. A flexible array member isn't guaranteed to be at the very end of the struct, it can overlap with some padding: struct Foo { int a; char b; char c[]; }; On most systems, sizeof(struct Foo) will be 8 bytes, but 'c' would immediately follow 'b' causing offsetof(struct Foo, c[0]) to be 5. So if you use offsetof(struct Foo, c[count]) for the allocation size, anything less than c[3] would be under-allocating and create potential implicit overruns just like before. so you still need to do something like max(sizeof(struct Foo), offsetof(struct Foo, c[count]) to ensure a proper minimum allocation size.
On 3/23/22 22:54, Chris Robinson wrote:
On Wednesday, March 23, 2022 12:32:52 PM PDT Alexandre Julliard wrote:
An empty size is probably OK to use at this point. It's not clear how much benefit it brings though, because obviously 1-size arrays in public structures can't be changed.
And I'm not sure it would change anything regarding this patch and ensuring the object is properly allocated. A flexible array member isn't guaranteed to be at the very end of the struct, it can overlap with some padding:
struct Foo { int a; char b; char c[]; };
On most systems, sizeof(struct Foo) will be 8 bytes, but 'c' would immediately follow 'b' causing offsetof(struct Foo, c[0]) to be 5. So if you use offsetof(struct Foo, c[count]) for the allocation size, anything less than c[3] would be under-allocating and create potential implicit overruns just like before. so you still need to do something like
max(sizeof(struct Foo), offsetof(struct Foo, c[count])
to ensure a proper minimum allocation size.
Sounds like an unfortunate and an aweful case, but GCC would probably emit the warning and we can make sure the flexible array is aligned with the struct. Or we can (and probably should) also add a C_ASSERT to validate that sizeof(struct) == offsetof(struct, array[0]) for structs with flexible arrays. That would make the expectations much more explicit. In any case, I think we should avoid max(sizeof, offsetof), it makes the code less readable imho. Unless required by the Win32 ABI compatibility and public header definitions, of course. In this particular case here the array is of pointers, and there's no alignment issues. -- Rémi Bernon <rbernon(a)codeweavers.com>
Le 23/03/2022 à 23:20, Rémi Bernon a écrit :
On 3/23/22 22:54, Chris Robinson wrote:
On Wednesday, March 23, 2022 12:32:52 PM PDT Alexandre Julliard wrote:
An empty size is probably OK to use at this point. It's not clear how much benefit it brings though, because obviously 1-size arrays in public structures can't be changed.
And I'm not sure it would change anything regarding this patch and ensuring the object is properly allocated. A flexible array member isn't guaranteed to be at the very end of the struct, it can overlap with some padding:
struct Foo { int a; char b; char c[]; };
On most systems, sizeof(struct Foo) will be 8 bytes, but 'c' would immediately follow 'b' causing offsetof(struct Foo, c[0]) to be 5. So if you use offsetof(struct Foo, c[count]) for the allocation size, anything less than c[3] would be under-allocating and create potential implicit overruns just like before. so you still need to do something like
max(sizeof(struct Foo), offsetof(struct Foo, c[count])
to ensure a proper minimum allocation size.
Sounds like an unfortunate and an aweful case, but GCC would probably emit the warning and we can make sure the flexible array is aligned with the struct.
Or we can (and probably should) also add a C_ASSERT to validate that sizeof(struct) == offsetof(struct, array[0]) for structs with flexible arrays. That would make the expectations much more explicit.
In any case, I think we should avoid max(sizeof, offsetof), it makes the code less readable imho. Unless required by the Win32 ABI compatibility and public header definitions, of course.
In this particular case here the array is of pointers, and there's no alignment issues.
so to summarize: - -Warray-bounds from GCC >= 11 need to be taken care of - when possible, use flexible array member (*) - if not, allocate the at least the whole struct I'll resubmit this patch with FAM instead (*) note that msvc supports them even in C++. And they are some instances of FAM present in MS SDK headers; MSVC generate warnings, but those are silenced in those headers with pragmas warning(disable: 4200)). g++ supports them as well (with a warning in -pedantic mode)
On Wed, 23 Mar 2022 at 22:55, Chris Robinson <chris.kcat(a)gmail.com> wrote:
On Wednesday, March 23, 2022 12:32:52 PM PDT Alexandre Julliard wrote:
An empty size is probably OK to use at this point. It's not clear how much benefit it brings though, because obviously 1-size arrays in public structures can't be changed.
And I'm not sure it would change anything regarding this patch and ensuring the object is properly allocated. A flexible array member isn't guaranteed to be at the very end of the struct, it can overlap with some padding:
struct Foo { int a; char b; char c[]; };
On most systems, sizeof(struct Foo) will be 8 bytes, but 'c' would immediately follow 'b' causing offsetof(struct Foo, c[0]) to be 5. So if you use offsetof(struct Foo, c[count]) for the allocation size, anything less than c[3] would be under-allocating and create potential implicit overruns just like before. so you still need to do something like
max(sizeof(struct Foo), offsetof(struct Foo, c[count])
to ensure a proper minimum allocation size.
On some level, sure. It's perhaps also worth pointing out that memory allocators tend to have a certain allocation granularity. E.g., asking HeapAlloc() for 5 bytes wouldn't actually get you a 5 byte allocation; you'd get at least 8.
participants (6)
-
Alexandre Julliard -
Chris Robinson -
Eric Pouech -
Eric Pouech -
Henri Verbeet -
Rémi Bernon