[PATCH 0/4] MR6964: win32u: Hold the display_lock when checking the cache update time.

List overview All Threads

newer

older

[PATCH v8 0/3] MR5924:...

Re: [PATCH v3 0/1] MR6918: ntdll:...

Rémi Bernon (＠rbernon)

4 Dec 2024 4 Dec '24

9:28 p.m.

Fix a race condition in lock_display_devices if the cache is updated right after it has checked the registry update time and before it has entered the display_lock.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6964

Show replies by date

Rémi Bernon

4 Dec 4 Dec

9:28 p.m.

New subject: [PATCH 1/4] win32u: Add a force parameter to lock_display_devices.

From: Rémi Bernon rbernon@codeweavers.com

--- dlls/win32u/sysparams.c | 56 ++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index 01342cd4c9f..3e321a166f0 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -2287,9 +2287,9 @@ BOOL update_display_cache( BOOL force ) return TRUE; }

-static BOOL lock_display_devices(void) +static BOOL lock_display_devices( BOOL force ) { - if (!update_display_cache( FALSE )) return FALSE; + if (!update_display_cache( force )) return FALSE; pthread_mutex_lock( &display_lock ); return TRUE; } @@ -2486,7 +2486,7 @@ RECT map_rect_raw_to_virt( RECT rect, UINT dpi_to ) RECT pos = {rect.left, rect.top, rect.left, rect.top}; struct monitor *monitor;

- if (!lock_display_devices()) return rect; + if (!lock_display_devices( FALSE )) return rect; if ((monitor = get_monitor_from_rect( pos, MONITOR_DEFAULTTONEAREST, 0, MDT_RAW_DPI ))) rect = map_monitor_rect( monitor, rect, 0, MDT_RAW_DPI, dpi_to, MDT_DEFAULT ); unlock_display_devices(); @@ -2500,7 +2500,7 @@ RECT map_rect_virt_to_raw( RECT rect, UINT dpi_from ) RECT pos = {rect.left, rect.top, rect.left, rect.top}; struct monitor *monitor;

- if (!lock_display_devices()) return rect; + if (!lock_display_devices( FALSE )) return rect; if ((monitor = get_monitor_from_rect( pos, MONITOR_DEFAULTTONEAREST, dpi_from, MDT_DEFAULT ))) rect = map_monitor_rect( monitor, rect, dpi_from, MDT_DEFAULT, 0, MDT_RAW_DPI ); unlock_display_devices(); @@ -2515,7 +2515,7 @@ struct window_rects map_window_rects_virt_to_raw( struct window_rects rects, UIN RECT rect, monitor_rect; BOOL is_fullscreen;

- if (!lock_display_devices()) return rects; + if (!lock_display_devices( FALSE )) return rects; if ((monitor = get_monitor_from_rect( rects.window, MONITOR_DEFAULTTONEAREST, dpi_from, MDT_DEFAULT ))) { /* if the visible rect is fullscreen, make it cover the full raw monitor, regardless of aspect ratio */ @@ -2538,7 +2538,7 @@ static UINT get_monitor_dpi( HMONITOR handle, UINT type, UINT *x, UINT *y ) struct monitor *monitor; UINT dpi = system_dpi;

- if (!lock_display_devices()) return 0; + if (!lock_display_devices( FALSE )) return 0; if ((monitor = get_monitor_from_handle( handle ))) dpi = monitor_get_dpi( monitor, type, x, y ); unlock_display_devices();

@@ -2781,7 +2781,7 @@ RECT get_virtual_screen_rect( UINT dpi, MONITOR_DPI_TYPE type ) { RECT rect = {0};

- if (!lock_display_devices()) return rect; + if (!lock_display_devices( FALSE )) return rect; rect = monitors_get_union_rect( dpi, type ); unlock_display_devices();

@@ -2793,7 +2793,7 @@ BOOL is_window_rect_full_screen( const RECT *rect, UINT dpi ) struct monitor *monitor; BOOL ret = FALSE;

- if (!lock_display_devices()) return FALSE; + if (!lock_display_devices( FALSE )) return FALSE;

LIST_FOR_EACH_ENTRY( monitor, &monitors, struct monitor, entry ) { @@ -2835,7 +2835,7 @@ RECT get_display_rect( const WCHAR *display )

RtlInitUnicodeString( &name, display ); if (!(index = get_display_index( &name ))) return rect; - if (!lock_display_devices()) return rect; + if (!lock_display_devices( FALSE )) return rect;

LIST_FOR_EACH_ENTRY( monitor, &monitors, struct monitor, entry ) { @@ -2853,7 +2853,7 @@ RECT get_primary_monitor_rect( UINT dpi ) struct monitor *monitor; RECT rect = {0};

- if (!lock_display_devices()) return rect; + if (!lock_display_devices( FALSE )) return rect;

LIST_FOR_EACH_ENTRY( monitor, &monitors, struct monitor, entry ) { @@ -2902,7 +2902,7 @@ LONG WINAPI NtUserGetDisplayConfigBufferSizes( UINT32 flags, UINT32 *num_path_in if ((flags & qdc_retrieve_flags_mask) != QDC_ONLY_ACTIVE_PATHS) FIXME( "only returning active paths\n" );

- if (lock_display_devices()) + if (lock_display_devices( FALSE )) { LIST_FOR_EACH_ENTRY( monitor, &monitors, struct monitor, entry ) { @@ -3119,7 +3119,7 @@ LONG WINAPI NtUserQueryDisplayConfig( UINT32 flags, UINT32 *paths_count, DISPLAY *topology_id = DISPLAYCONFIG_TOPOLOGY_INTERNAL; }

- if (!lock_display_devices()) + if (!lock_display_devices( FALSE )) return ERROR_GEN_FAILURE;

ret = ERROR_GEN_FAILURE; @@ -3251,7 +3251,7 @@ static struct source *find_source( UNICODE_STRING *name ) { struct source *source;

- if (!lock_display_devices()) return NULL; + if (!lock_display_devices( FALSE )) return NULL;

if (name && name->Length) source = find_source_by_name( name ); else source = find_primary_source(); @@ -3322,7 +3322,7 @@ NTSTATUS WINAPI NtUserEnumDisplayDevices( UNICODE_STRING *device, DWORD index,

if (!info || !info->cb) return STATUS_UNSUCCESSFUL;

- if (!lock_display_devices()) return STATUS_UNSUCCESSFUL; + if (!lock_display_devices( FALSE )) return STATUS_UNSUCCESSFUL;

if (!device || !device->Length) { @@ -3840,7 +3840,7 @@ static LONG apply_display_settings( struct source *target, const DEVMODEW *devmo HWND restorer_window; LONG ret;

- if (!lock_display_devices()) return DISP_CHANGE_FAILED; + if (!lock_display_devices( FALSE )) return DISP_CHANGE_FAILED; if (!(displays = get_display_settings( target, devmode ))) { unlock_display_devices(); @@ -4016,7 +4016,7 @@ INT get_display_depth( UNICODE_STRING *name ) struct source *source; INT depth;

- if (!lock_display_devices()) + if (!lock_display_devices( FALSE )) return 32;

if (name && name->Length) source = find_source_by_name( name ); @@ -4076,7 +4076,7 @@ BOOL WINAPI NtUserEnumDisplayMonitors( HDC hdc, RECT *rect, MONITORENUMPROC proc } if (rect && !intersect_rect( &limit, &limit, rect )) return TRUE;

- if (!lock_display_devices()) return FALSE; + if (!lock_display_devices( FALSE )) return FALSE;

count = list_count( &monitors ); if (!count || (count > ARRAYSIZE(enum_buf) && @@ -4134,7 +4134,7 @@ static BOOL get_monitor_info( HMONITOR handle, MONITORINFO *info, UINT dpi )

if (info->cbSize != sizeof(MONITORINFOEXW) && info->cbSize != sizeof(MONITORINFO)) return FALSE;

- if (!lock_display_devices()) return FALSE; + if (!lock_display_devices( FALSE )) return FALSE;

if ((monitor = get_monitor_from_handle( handle ))) { @@ -4160,7 +4160,7 @@ static HMONITOR monitor_from_rect( const RECT *rect, UINT flags, UINT dpi )

r = map_dpi_rect( *rect, dpi, system_dpi );

- if (!lock_display_devices()) return 0; + if (!lock_display_devices( FALSE )) return 0; if ((monitor = get_monitor_from_rect( r, flags, system_dpi, MDT_DEFAULT ))) ret = monitor->handle; unlock_display_devices();

@@ -4173,7 +4173,7 @@ MONITORINFO monitor_info_from_rect( RECT rect, UINT dpi ) MONITORINFO info = {.cbSize = sizeof(info)}; struct monitor *monitor;

- if (!lock_display_devices()) return info; + if (!lock_display_devices( FALSE )) return info; if ((monitor = get_monitor_from_rect( rect, MONITOR_DEFAULTTONEAREST, dpi, MDT_DEFAULT ))) monitor_get_info( monitor, &info, dpi ); unlock_display_devices(); @@ -4186,7 +4186,7 @@ UINT monitor_dpi_from_rect( RECT rect, UINT dpi, UINT *raw_dpi ) struct monitor *monitor; UINT ret = system_dpi, x, y;

- if (!lock_display_devices()) return 0; + if (!lock_display_devices( FALSE )) return 0; if ((monitor = get_monitor_from_rect( rect, MONITOR_DEFAULTTONEAREST, dpi, MDT_DEFAULT ))) { *raw_dpi = monitor_get_dpi( monitor, MDT_RAW_DPI, &x, &y ); @@ -5817,7 +5817,7 @@ BOOL WINAPI NtUserSystemParametersInfo( UINT action, UINT val, void *ptr, UINT w { struct monitor *monitor;

- if (!lock_display_devices()) return FALSE; + if (!lock_display_devices( FALSE )) return FALSE;

LIST_FOR_EACH_ENTRY( monitor, &monitors, struct monitor, entry ) { @@ -6605,7 +6605,7 @@ int get_system_metrics( int index ) rect = get_virtual_screen_rect( get_thread_dpi(), MDT_DEFAULT ); return rect.bottom - rect.top; case SM_CMONITORS: - if (!lock_display_devices()) return FALSE; + if (!lock_display_devices( FALSE )) return FALSE; ret = active_unique_monitor_count(); unlock_display_devices(); return ret; @@ -7132,7 +7132,7 @@ NTSTATUS WINAPI NtUserDisplayConfigGetDeviceInfo( DISPLAYCONFIG_DEVICE_INFO_HEAD if (packet->size < sizeof(*source_name)) return STATUS_INVALID_PARAMETER;

- if (!lock_display_devices()) return STATUS_UNSUCCESSFUL; + if (!lock_display_devices( FALSE )) return STATUS_UNSUCCESSFUL;

LIST_FOR_EACH_ENTRY(source, &sources, struct source, entry) { @@ -7159,7 +7159,7 @@ NTSTATUS WINAPI NtUserDisplayConfigGetDeviceInfo( DISPLAYCONFIG_DEVICE_INFO_HEAD if (packet->size < sizeof(*target_name)) return STATUS_INVALID_PARAMETER;

- if (!lock_display_devices()) return STATUS_UNSUCCESSFUL; + if (!lock_display_devices( FALSE )) return STATUS_UNSUCCESSFUL;

memset( &target_name->flags, 0, sizeof(*target_name) - offsetof(DISPLAYCONFIG_TARGET_DEVICE_NAME, flags) );

@@ -7206,7 +7206,7 @@ NTSTATUS WINAPI NtUserDisplayConfigGetDeviceInfo( DISPLAYCONFIG_DEVICE_INFO_HEAD if (packet->size < sizeof(*preferred_mode)) return STATUS_INVALID_PARAMETER;

- if (!lock_display_devices()) return STATUS_UNSUCCESSFUL; + if (!lock_display_devices( FALSE )) return STATUS_UNSUCCESSFUL;

memset( &preferred_mode->width, 0, sizeof(*preferred_mode) - offsetof(DISPLAYCONFIG_TARGET_PREFERRED_MODE, width) );

@@ -7317,7 +7317,7 @@ NTSTATUS WINAPI NtGdiDdDDIEnumAdapters2( D3DKMT_ENUMADAPTERS2 *desc ) return STATUS_SUCCESS; }

- if (!lock_display_devices()) return STATUS_UNSUCCESSFUL; + if (!lock_display_devices( FALSE )) return STATUS_UNSUCCESSFUL; LIST_FOR_EACH_ENTRY( gpu, &gpus, struct gpu, entry ) { if (count >= ARRAY_SIZE(current_gpus)) @@ -7408,7 +7408,7 @@ BOOL get_vulkan_uuid_from_luid( const LUID *luid, GUID *uuid ) BOOL found = FALSE; struct gpu *gpu;

- if (!lock_display_devices()) return FALSE; + if (!lock_display_devices( FALSE )) return FALSE;

LIST_FOR_EACH_ENTRY( gpu, &gpus, struct gpu, entry ) {

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6964

Rémi Bernon

9:28 p.m.

New subject: [PATCH 2/4] win32u: Implement update_display_cache with lock_display_devices.

From: Rémi Bernon rbernon@codeweavers.com

--- dlls/win32u/sysparams.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index 3e321a166f0..4f6eb6c8eba 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -2237,7 +2237,7 @@ static void commit_display_devices( struct device_manager_ctx *ctx ) set_winstation_monitors(); }

-BOOL update_display_cache( BOOL force ) +static BOOL lock_display_devices( BOOL force ) { static const WCHAR wine_service_station_name[] = {'_','_','w','i','n','e','s','e','r','v','i','c','e','_','w','i','n','s','t','a','t','i','o','n',0}; @@ -2254,7 +2254,6 @@ BOOL update_display_cache( BOOL force ) clear_display_devices(); list_add_tail( &monitors, &virtual_monitor.entry ); set_winstation_monitors(); - pthread_mutex_unlock( &display_lock ); return TRUE; }

@@ -2281,15 +2280,9 @@ BOOL update_display_cache( BOOL force ) return FALSE; }

- return update_display_cache( TRUE ); + if (!lock_display_devices( TRUE )) return FALSE; }

- return TRUE; -} - -static BOOL lock_display_devices( BOOL force ) -{ - if (!update_display_cache( force )) return FALSE; pthread_mutex_lock( &display_lock ); return TRUE; } @@ -2299,6 +2292,13 @@ static void unlock_display_devices(void) pthread_mutex_unlock( &display_lock ); }

+BOOL update_display_cache( BOOL force ) +{ + if (!lock_display_devices( force )) return FALSE; + unlock_display_devices(); + return TRUE; +} + static HDC get_display_dc(void) { pthread_mutex_lock( &display_dc_lock );

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6964

Rémi Bernon

9:28 p.m.

New subject: [PATCH 3/4] win32u: Remove recursive lock_display_devices calls.

From: Rémi Bernon rbernon@codeweavers.com

--- dlls/win32u/sysparams.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index 4f6eb6c8eba..89bbe5d933a 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -2245,6 +2245,7 @@ static BOOL lock_display_devices( BOOL force ) struct device_manager_ctx ctx = {.vulkan_gpus = LIST_INIT(ctx.vulkan_gpus)}; UINT status; WCHAR name[MAX_PATH]; + BOOL ret = TRUE;

/* services do not have any adapters, only a virtual monitor */ if (NtUserGetObjectInformation( winstation, UOI_NAME, name, sizeof(name), NULL ) @@ -2258,33 +2259,21 @@ static BOOL lock_display_devices( BOOL force ) }

if (!force) get_display_driver(); /* make sure at least to load the user driver */ - else + + if (!force && !update_display_cache_from_registry()) force = TRUE; + if (force) { if (!get_vulkan_gpus( &ctx.vulkan_gpus )) WARN( "Failed to find any vulkan GPU\n" ); if (!(status = update_display_devices( &ctx ))) commit_display_devices( &ctx ); else WARN( "Failed to update display devices, status %#x\n", status ); release_display_manager_ctx( &ctx ); - } - - if (!update_display_cache_from_registry()) - { - if (force) - { - ERR( "Failed to read display config.\n" ); - return FALSE; - } - - if (ctx.gpu_count) - { - ERR( "Driver reported devices, but we failed to read them.\n" ); - return FALSE; - }

- if (!lock_display_devices( TRUE )) return FALSE; + ret = update_display_cache_from_registry(); }

- pthread_mutex_lock( &display_lock ); - return TRUE; + if (!ret) ERR( "Failed to read display config.\n" ); + else pthread_mutex_lock( &display_lock ); + return ret; }

static void unlock_display_devices(void)

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6964

Rémi Bernon

9:28 p.m.

New subject: [PATCH 4/4] win32u: Hold the display_lock when checking the cache update time.

From: Rémi Bernon rbernon@codeweavers.com

Fix a race condition in lock_display_devices if the cache is updated right after it has checked the registry update time and before it has entered the display_lock. --- dlls/win32u/sysparams.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index 89bbe5d933a..230c3faa262 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -1273,7 +1273,6 @@ static void add_gpu( const char *name, const struct pci_id *pci_id, const GUID *

if (!ctx->mutex) { - pthread_mutex_lock( &display_lock ); ctx->mutex = get_display_device_init_mutex(); prepare_devices(); } @@ -1724,7 +1723,6 @@ static void release_display_manager_ctx( struct device_manager_ctx *ctx ) { if (ctx->mutex) { - pthread_mutex_unlock( &display_lock ); release_display_device_init_mutex( ctx->mutex ); ctx->mutex = 0; } @@ -1993,7 +1991,6 @@ static BOOL update_display_cache_from_registry(void)

if (key.LastWriteTime.QuadPart <= last_query_display_time) return TRUE;

- pthread_mutex_lock( &display_lock ); mutex = get_display_device_init_mutex();

clear_display_devices(); @@ -2050,7 +2047,6 @@ static BOOL update_display_cache_from_registry(void) last_query_display_time = key.LastWriteTime.QuadPart;

set_winstation_monitors(); - pthread_mutex_unlock( &display_lock ); release_display_device_init_mutex( mutex ); return ret; } @@ -2260,6 +2256,8 @@ static BOOL lock_display_devices( BOOL force )

if (!force) get_display_driver(); /* make sure at least to load the user driver */

+ pthread_mutex_lock( &display_lock ); + if (!force && !update_display_cache_from_registry()) force = TRUE; if (force) { @@ -2271,8 +2269,11 @@ static BOOL lock_display_devices( BOOL force ) ret = update_display_cache_from_registry(); }

- if (!ret) ERR( "Failed to read display config.\n" ); - else pthread_mutex_lock( &display_lock ); + if (!ret) + { + ERR( "Failed to read display config.\n" ); + pthread_mutex_unlock( &display_lock ); + } return ret; }

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6964

Tim Clem (＠tclem)

9:41 p.m.

New subject: [PATCH 0/4] MR6964: win32u: Hold the display_lock when checking the cache update time. - approved

This merge request was approved by Tim Clem.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6964

Age (days ago)

Last active (days ago)

wine-gitlab@winehq.org

5 comments

3 participants

tags (0)

participants (3)

Rémi Bernon
Rémi Bernon (＠rbernon)
Tim Clem (＠tclem)