[PATCH 0/2] MR5984: win32u: Use the desktop shared memory for is_virtual_desktop.
From: Rémi Bernon <rbernon(a)codeweavers.com> --- server/protocol.def | 1 + server/user.h | 1 - server/winstation.c | 25 ++++++++++++++++++------- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/server/protocol.def b/server/protocol.def index 21e35204ed0..39c8de7ef69 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -909,6 +909,7 @@ struct shared_cursor typedef volatile struct { + unsigned int flags; /* desktop flags */ struct shared_cursor cursor; /* global cursor information */ unsigned char keystate[256]; /* asynchronous key state */ } desktop_shm_t; diff --git a/server/user.h b/server/user.h index 4633c7dae7f..6f10f296200 100644 --- a/server/user.h +++ b/server/user.h @@ -67,7 +67,6 @@ struct key_repeat struct desktop { struct object obj; /* object header */ - unsigned int flags; /* desktop flags */ struct winstation *winstation; /* winstation this desktop belongs to */ timeout_t input_time; /* last time this desktop had the input */ struct list entry; /* entry in winstation list of desktops */ diff --git a/server/winstation.c b/server/winstation.c index 75b604ae1c3..76a23b197a4 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -274,13 +274,11 @@ static struct desktop *create_desktop( const struct unicode_str *name, unsigned { /* initialize it if it didn't already exist */ - desktop->flags = flags; - /* inherit DF_WINE_*_DESKTOP flags if none of them are specified */ if (!(flags & (DF_WINE_ROOT_DESKTOP | DF_WINE_VIRTUAL_DESKTOP)) && (current_desktop = get_thread_desktop( current, 0 ))) { - desktop->flags |= current_desktop->flags & (DF_WINE_VIRTUAL_DESKTOP | DF_WINE_ROOT_DESKTOP); + flags |= current_desktop->shared->flags & (DF_WINE_VIRTUAL_DESKTOP | DF_WINE_ROOT_DESKTOP); release_object( current_desktop ); } @@ -308,6 +306,7 @@ static struct desktop *create_desktop( const struct unicode_str *name, unsigned SHARED_WRITE_BEGIN( desktop->shared, desktop_shm_t ) { + shared->flags = flags; shared->cursor.x = 0; shared->cursor.y = 0; shared->cursor.last_change = 0; @@ -321,7 +320,12 @@ static struct desktop *create_desktop( const struct unicode_str *name, unsigned } else { - desktop->flags |= flags & (DF_WINE_VIRTUAL_DESKTOP | DF_WINE_ROOT_DESKTOP); + SHARED_WRITE_BEGIN( desktop->shared, desktop_shm_t ) + { + shared->flags |= flags & (DF_WINE_VIRTUAL_DESKTOP | DF_WINE_ROOT_DESKTOP); + } + SHARED_WRITE_END; + clear_error(); } } @@ -333,7 +337,7 @@ static void desktop_dump( struct object *obj, int verbose ) struct desktop *desktop = (struct desktop *)obj; fprintf( stderr, "Desktop flags=%x winstation=%p top_win=%p hooks=%p\n", - desktop->flags, desktop->winstation, desktop->top_window, desktop->global_hooks ); + desktop->shared->flags, desktop->winstation, desktop->top_window, desktop->global_hooks ); } static int desktop_link_name( struct object *obj, struct object_name *name, struct object *parent ) @@ -839,8 +843,15 @@ DECL_HANDLER(set_user_object_info) { struct desktop *desktop = (struct desktop *)obj; reply->is_desktop = 1; - reply->old_obj_flags = desktop->flags; - if (req->flags & SET_USER_OBJECT_SET_FLAGS) desktop->flags = req->obj_flags; + reply->old_obj_flags = desktop->shared->flags; + if (req->flags & SET_USER_OBJECT_SET_FLAGS) + { + SHARED_WRITE_BEGIN( desktop->shared, desktop_shm_t ) + { + shared->flags = req->obj_flags; + } + SHARED_WRITE_END; + } } else if (obj->ops == &winstation_ops) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5984
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/win32u/winstation.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/dlls/win32u/winstation.c b/dlls/win32u/winstation.c index 1a1b37f6eda..0500d510fd4 100644 --- a/dlls/win32u/winstation.c +++ b/dlls/win32u/winstation.c @@ -256,12 +256,16 @@ NTSTATUS get_shared_queue( struct object_lock *lock, const queue_shm_t **queue_s BOOL is_virtual_desktop(void) { - HANDLE desktop = NtUserGetThreadDesktop( GetCurrentThreadId() ); - USEROBJECTFLAGS flags = {0}; - DWORD len; + struct object_lock lock = OBJECT_LOCK_INIT; + const desktop_shm_t *desktop_shm; + BOOL ret = FALSE; + UINT status; - if (!NtUserGetObjectInformation( desktop, UOI_FLAGS, &flags, sizeof(flags), &len )) return FALSE; - return !!(flags.dwFlags & DF_WINE_VIRTUAL_DESKTOP); + while ((status = get_shared_desktop( &lock, &desktop_shm )) == STATUS_PENDING) + ret = !!(desktop_shm->flags & DF_WINE_VIRTUAL_DESKTOP); + if (status) ret = FALSE; + + return ret; } /*********************************************************************** -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5984
Grigory Vasilyev (@h0tc0d3) commented about dlls/win32u/winstation.c:
BOOL is_virtual_desktop(void) { - HANDLE desktop = NtUserGetThreadDesktop( GetCurrentThreadId() ); - USEROBJECTFLAGS flags = {0}; - DWORD len; + struct object_lock lock = OBJECT_LOCK_INIT; + const desktop_shm_t *desktop_shm; + BOOL ret = FALSE; + UINT status;
- if (!NtUserGetObjectInformation( desktop, UOI_FLAGS, &flags, sizeof(flags), &len )) return FALSE; - return !!(flags.dwFlags & DF_WINE_VIRTUAL_DESKTOP); + while ((status = get_shared_desktop( &lock, &desktop_shm )) == STATUS_PENDING) + ret = !!(desktop_shm->flags & DF_WINE_VIRTUAL_DESKTOP);
Can we get an incorrect pointer in desktop_shm, for example NULL, which will lead to null pointer dereference? It's probably better to move to ELSE after checking the status. ```C if (status) ret = FALSE; else ret = !!(desktop_shm->flags & DF_WINE_VIRTUAL_DESKTOP); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5984#note_75018
Can we get an incorrect pointer in desktop_shm, for example NULL, which will lead to null pointer dereference? It's probably better to move to ELSE after checking the status. ```C if (status) ret = FALSE; else ret = !!(desktop_shm->flags & DF_WINE_VIRTUAL_DESKTOP); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5984#note_75019
On Wed Jul 3 14:23:23 2024 +0000, Grigory Vasilyev wrote:
Can we get an incorrect pointer in desktop_shm, for example NULL, which will lead to null pointer dereference? It's probably better to move to ELSE after checking the status. ```C ret = status ? FALSE : !!(desktop_shm->flags & DF_WINE_VIRTUAL_DESKTOP); ``` No, desktop_shm will always be a valid pointer, although it has to be used within the loop and only within it. This is how the shared memory objects have to be accessed to make sure the data we read is valid.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5984#note_75020
participants (2)
-
Grigory Vasilyev (＠h0tc0d3) -
Rémi Bernon