[PATCH 1/5] kernelbase: Fix incorrect pseudo console allocation size (GCC 11).
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- GCC 11 is much more verbose with its array-bounds and stringop-overread warnings, and they are enabled by default. Some of these lead to some genuine fixes like the first two patches here while some are a bit more dubious but easy to fix, like the last three patches. Then there's a lot more additional array-bounds warnings emitted, caused by flexible array sizes which we define to 1 by default, in most cases. When a variable is allocated with the array size set to 0, GCC emits a warning each time the variable is used, as it assumes any of its members may be accessed (although the first array member may not be accessed at all). A possible fix for this would be to use true C99 flexible array sizes instead, but it then breaks a bunch of type size checks instead. dlls/kernelbase/console.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/kernelbase/console.c b/dlls/kernelbase/console.c index c38e8e91955..a7eeb439232 100644 --- a/dlls/kernelbase/console.c +++ b/dlls/kernelbase/console.c @@ -1728,7 +1728,7 @@ HRESULT WINAPI CreatePseudoConsole( COORD size, HANDLE input, HANDLE output, DWO if (!size.X || !size.Y || !ret) return E_INVALIDARG; - if (!(pseudo_console = HeapAlloc( GetProcessHeap(), 0, HEAP_ZERO_MEMORY ))) return E_OUTOFMEMORY; + if (!(pseudo_console = HeapAlloc( GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*pseudo_console) ))) return E_OUTOFMEMORY; swprintf( pipe_name, ARRAY_SIZE(pipe_name), L"\\\\.\\pipe\\wine_pty_signal_pipe%x", GetCurrentThreadId() ); -- 2.33.0
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/msacm32/driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/msacm32/driver.c b/dlls/msacm32/driver.c index a8d6f3cf6d7..1be3b522241 100644 --- a/dlls/msacm32/driver.c +++ b/dlls/msacm32/driver.c @@ -429,7 +429,7 @@ LRESULT WINAPI acmDriverMessage(HACMDRIVER had, UINT uMsg, LPARAM lParam1, LPARA * reports a 16-byte structure to codecs, so allocate 16 bytes, * just to be on the safe side. */ - const unsigned int iStructSize = 16; + const unsigned int iStructSize = max(16, sizeof(*pConfigInfo)); pConfigInfo = HeapAlloc(MSACM_hHeap, 0, iStructSize); if (!pConfigInfo) { ERR("OOM while supplying DRVCONFIGINFO for DRV_CONFIGURE, using NULL\n"); -- 2.33.0
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ddraw/device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/ddraw/device.c b/dlls/ddraw/device.c index 8afbab0e58d..486ae092be3 100644 --- a/dlls/ddraw/device.c +++ b/dlls/ddraw/device.c @@ -4678,7 +4678,7 @@ static HRESULT WINAPI d3d_device3_ComputeSphereVisibility(IDirect3DDevice3 *ifac D3DVECTOR *centers, D3DVALUE *radii, DWORD sphere_count, DWORD flags, DWORD *return_values) { static const DWORD enabled_planes = 0x3f; - struct wined3d_vec4 plane[6]; + struct wined3d_vec4 plane[12]; unsigned int i, j; TRACE("iface %p, centers %p, radii %p, sphere_count %u, flags %#x, return_values %p.\n", -- 2.33.0
On Mon, 27 Sept 2021 at 10:58, Rémi Bernon <rbernon(a)codeweavers.com> wrote:
@@ -4678,7 +4678,7 @@ static HRESULT WINAPI d3d_device3_ComputeSphereVisibility(IDirect3DDevice3 *ifac D3DVECTOR *centers, D3DVALUE *radii, DWORD sphere_count, DWORD flags, DWORD *return_values) { static const DWORD enabled_planes = 0x3f; - struct wined3d_vec4 plane[6]; + struct wined3d_vec4 plane[12]; unsigned int i, j;
It seems tempting to rewrite compute_sphere_visibility() to use wined3d_bit_scan() and get rid of both instances of "12" in there.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/user32/win.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/dlls/user32/win.c b/dlls/user32/win.c index 7c2471d2746..ca758a535b1 100644 --- a/dlls/user32/win.c +++ b/dlls/user32/win.c @@ -614,10 +614,15 @@ static inline void reset_bounds( RECT *bounds ) static struct offscreen_window_surface *impl_from_window_surface( struct window_surface *base ) { - if (!base || base->funcs != &offscreen_window_surface_funcs) return NULL; return CONTAINING_RECORD( base, struct offscreen_window_surface, header ); } +static struct offscreen_window_surface *impl_from_window_surface_or_null( struct window_surface *base ) +{ + if (!base || base->funcs != &offscreen_window_surface_funcs) return NULL; + return impl_from_window_surface( base ); +} + static void CDECL offscreen_window_surface_lock( struct window_surface *base ) { struct offscreen_window_surface *impl = impl_from_window_surface( base ); @@ -687,7 +692,7 @@ void create_offscreen_window_surface( const RECT *visible_rect, struct window_su surface_rect.bottom = (surface_rect.bottom + 0x1f) & ~0x1f; /* check that old surface is an offscreen_window_surface, or release it */ - if ((impl = impl_from_window_surface( *surface ))) + if ((impl = impl_from_window_surface_or_null( *surface ))) { /* if the rect didn't change, keep the same surface */ if (EqualRect( &surface_rect, &impl->header.rect )) return; -- 2.33.0
On 9/27/21 3:58 AM, Rémi Bernon wrote:
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/user32/win.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/dlls/user32/win.c b/dlls/user32/win.c index 7c2471d2746..ca758a535b1 100644 --- a/dlls/user32/win.c +++ b/dlls/user32/win.c @@ -614,10 +614,15 @@ static inline void reset_bounds( RECT *bounds )
static struct offscreen_window_surface *impl_from_window_surface( struct window_surface *base ) { - if (!base || base->funcs != &offscreen_window_surface_funcs) return NULL; return CONTAINING_RECORD( base, struct offscreen_window_surface, header ); }
+static struct offscreen_window_surface *impl_from_window_surface_or_null( struct window_surface *base ) +{ + if (!base || base->funcs != &offscreen_window_surface_funcs) return NULL; + return impl_from_window_surface( base ); +} +
FWIW, in d3d code that's called "unsafe_impl_from_window_surface".
static void CDECL offscreen_window_surface_lock( struct window_surface *base ) { struct offscreen_window_surface *impl = impl_from_window_surface( base ); @@ -687,7 +692,7 @@ void create_offscreen_window_surface( const RECT *visible_rect, struct window_su surface_rect.bottom = (surface_rect.bottom + 0x1f) & ~0x1f;
/* check that old surface is an offscreen_window_surface, or release it */ - if ((impl = impl_from_window_surface( *surface ))) + if ((impl = impl_from_window_surface_or_null( *surface ))) { /* if the rect didn't change, keep the same surface */ if (EqualRect( &surface_rect, &impl->header.rect )) return;
So that GCC 11 stops warning about reading from a 0-size memory region. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/instr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index f197570db0c..fbcd376dbc1 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -498,8 +498,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(int); #define SIB_BASE( sib, rex ) (((sib) & 7) | (((rex) & REX_B) ? 8 : 0)) /* keep in sync with dlls/ntdll/thread.c:thread_init */ -static const BYTE *wine_user_shared_data = (BYTE *)0x7ffe0000; -static const BYTE *user_shared_data = (BYTE *)0xfffff78000000000; +static const BYTE *volatile wine_user_shared_data = (BYTE *)0x7ffe0000; +static const BYTE *volatile user_shared_data = (BYTE *)0xfffff78000000000; static inline DWORD64 *get_int_reg( CONTEXT *context, int index ) { -- 2.33.0
On 9/27/21 3:58 AM, Rémi Bernon wrote:
So that GCC 11 stops warning about reading from a 0-size memory region.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/instr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index f197570db0c..fbcd376dbc1 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -498,8 +498,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(int); #define SIB_BASE( sib, rex ) (((sib) & 7) | (((rex) & REX_B) ? 8 : 0))
/* keep in sync with dlls/ntdll/thread.c:thread_init */ -static const BYTE *wine_user_shared_data = (BYTE *)0x7ffe0000; -static const BYTE *user_shared_data = (BYTE *)0xfffff78000000000; +static const BYTE *volatile wine_user_shared_data = (BYTE *)0x7ffe0000; +static const BYTE *volatile user_shared_data = (BYTE *)0xfffff78000000000;
static inline DWORD64 *get_int_reg( CONTEXT *context, int index ) {
This looks wrong. It should presumably be "const volatile BYTE *" (actually: "const volatile BYTE *const"), but I'm guessing that doesn't actually fix the warning. Granted, there's an open GCC bug for this [1], and marking the variable volatile is suggested as a workaround... Perhaps at least we should mark that we're working around a GCC bug in the code, since otherwise it looks like "volatile" is in the wrong place. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
On 9/27/21 6:33 PM, Zebediah Figura wrote:
On 9/27/21 3:58 AM, Rémi Bernon wrote:
So that GCC 11 stops warning about reading from a 0-size memory region.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/instr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index f197570db0c..fbcd376dbc1 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -498,8 +498,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(int); #define SIB_BASE( sib, rex ) (((sib) & 7) | (((rex) & REX_B) ? 8 : 0)) /* keep in sync with dlls/ntdll/thread.c:thread_init */ -static const BYTE *wine_user_shared_data = (BYTE *)0x7ffe0000; -static const BYTE *user_shared_data = (BYTE *)0xfffff78000000000; +static const BYTE *volatile wine_user_shared_data = (BYTE *)0x7ffe0000; +static const BYTE *volatile user_shared_data = (BYTE *)0xfffff78000000000; static inline DWORD64 *get_int_reg( CONTEXT *context, int index ) {
This looks wrong. It should presumably be "const volatile BYTE *" (actually: "const volatile BYTE *const"), but I'm guessing that doesn't actually fix the warning. Granted, there's an open GCC bug for this [1], and marking the variable volatile is suggested as a workaround...
Perhaps at least we should mark that we're working around a GCC bug in the code, since otherwise it looks like "volatile" is in the wrong place.
No, the contents of the memory don't need to be volatile, the pointer does. This way GCC cannot assume its fixed value (the warning triggers when accessing non-NULL pointers). -- Rémi Bernon <rbernon(a)codeweavers.com>
On 9/27/21 11:43 AM, Rémi Bernon wrote:
On 9/27/21 6:33 PM, Zebediah Figura wrote:
On 9/27/21 3:58 AM, Rémi Bernon wrote:
So that GCC 11 stops warning about reading from a 0-size memory region.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/instr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index f197570db0c..fbcd376dbc1 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -498,8 +498,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(int); #define SIB_BASE( sib, rex ) (((sib) & 7) | (((rex) & REX_B) ? 8 : 0)) /* keep in sync with dlls/ntdll/thread.c:thread_init */ -static const BYTE *wine_user_shared_data = (BYTE *)0x7ffe0000; -static const BYTE *user_shared_data = (BYTE *)0xfffff78000000000; +static const BYTE *volatile wine_user_shared_data = (BYTE *)0x7ffe0000; +static const BYTE *volatile user_shared_data = (BYTE *)0xfffff78000000000; static inline DWORD64 *get_int_reg( CONTEXT *context, int index ) {
This looks wrong. It should presumably be "const volatile BYTE *" (actually: "const volatile BYTE *const"), but I'm guessing that doesn't actually fix the warning. Granted, there's an open GCC bug for this [1], and marking the variable volatile is suggested as a workaround...
Perhaps at least we should mark that we're working around a GCC bug in the code, since otherwise it looks like "volatile" is in the wrong place.
No, the contents of the memory don't need to be volatile, the pointer does. This way GCC cannot assume its fixed value (the warning triggers when accessing non-NULL pointers).
Well, right, to hide the GCC warning, that's true. But the contents of the memory *should* be volatile (they're arbitrarily rewritten by another process), and in theory the value of the "user_shared_data" pointer should never change. Knowing that, "const BYTE *volatile" looks like a mistake.
On 9/27/21 6:45 PM, Zebediah Figura wrote:
On 9/27/21 11:43 AM, Rémi Bernon wrote:
On 9/27/21 6:33 PM, Zebediah Figura wrote:
On 9/27/21 3:58 AM, Rémi Bernon wrote:
So that GCC 11 stops warning about reading from a 0-size memory region.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/instr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index f197570db0c..fbcd376dbc1 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -498,8 +498,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(int); #define SIB_BASE( sib, rex ) (((sib) & 7) | (((rex) & REX_B) ? 8 : 0)) /* keep in sync with dlls/ntdll/thread.c:thread_init */ -static const BYTE *wine_user_shared_data = (BYTE *)0x7ffe0000; -static const BYTE *user_shared_data = (BYTE *)0xfffff78000000000; +static const BYTE *volatile wine_user_shared_data = (BYTE *)0x7ffe0000; +static const BYTE *volatile user_shared_data = (BYTE *)0xfffff78000000000; static inline DWORD64 *get_int_reg( CONTEXT *context, int index ) {
This looks wrong. It should presumably be "const volatile BYTE *" (actually: "const volatile BYTE *const"), but I'm guessing that doesn't actually fix the warning. Granted, there's an open GCC bug for this [1], and marking the variable volatile is suggested as a workaround...
Perhaps at least we should mark that we're working around a GCC bug in the code, since otherwise it looks like "volatile" is in the wrong place.
No, the contents of the memory don't need to be volatile, the pointer does. This way GCC cannot assume its fixed value (the warning triggers when accessing non-NULL pointers).
Well, right, to hide the GCC warning, that's true. But the contents of the memory *should* be volatile (they're arbitrarily rewritten by another process), and in theory the value of the "user_shared_data" pointer should never change. Knowing that, "const BYTE *volatile" looks like a mistake.
Sure, it's a bit confusing. FWIW it could be static "const volatile BYTE* const volatile"... -- Rémi Bernon <rbernon(a)codeweavers.com>
On 9/27/21 11:50 AM, Rémi Bernon wrote:
On 9/27/21 6:45 PM, Zebediah Figura wrote:
On 9/27/21 11:43 AM, Rémi Bernon wrote:
On 9/27/21 6:33 PM, Zebediah Figura wrote:
On 9/27/21 3:58 AM, Rémi Bernon wrote:
So that GCC 11 stops warning about reading from a 0-size memory region.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/instr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index f197570db0c..fbcd376dbc1 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -498,8 +498,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(int); #define SIB_BASE( sib, rex ) (((sib) & 7) | (((rex) & REX_B) ? 8 : 0)) /* keep in sync with dlls/ntdll/thread.c:thread_init */ -static const BYTE *wine_user_shared_data = (BYTE *)0x7ffe0000; -static const BYTE *user_shared_data = (BYTE *)0xfffff78000000000; +static const BYTE *volatile wine_user_shared_data = (BYTE *)0x7ffe0000; +static const BYTE *volatile user_shared_data = (BYTE *)0xfffff78000000000; static inline DWORD64 *get_int_reg( CONTEXT *context, int index ) {
This looks wrong. It should presumably be "const volatile BYTE *" (actually: "const volatile BYTE *const"), but I'm guessing that doesn't actually fix the warning. Granted, there's an open GCC bug for this [1], and marking the variable volatile is suggested as a workaround...
Perhaps at least we should mark that we're working around a GCC bug in the code, since otherwise it looks like "volatile" is in the wrong place.
No, the contents of the memory don't need to be volatile, the pointer does. This way GCC cannot assume its fixed value (the warning triggers when accessing non-NULL pointers).
Well, right, to hide the GCC warning, that's true. But the contents of the memory *should* be volatile (they're arbitrarily rewritten by another process), and in theory the value of the "user_shared_data" pointer should never change. Knowing that, "const BYTE *volatile" looks like a mistake.
Sure, it's a bit confusing.
FWIW it could be static "const volatile BYTE* const volatile"...
That seems like the ideal declaration, yes :-) I think a comment mentioning the GCC bug wouldn't hurt regardless, though.
participants (4)
-
Henri Verbeet -
Jacek Caban -
Rémi Bernon -
Zebediah Figura