[PATCH 0/2] MR5984: win32u: Use the desktop shared memory for is_virtual_desktop.

[PATCH 1/2] server: Move the desktop flags to the shared memory.

[Rémi Bernon]

From: Rémi Bernon

---
 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)
     {

[PATCH 2/2] win32u: Use the shared memory to read the desktop flags.

[Rémi Bernon]

From: Rémi Bernon

---
 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;
 }

/***********************************************************************

[Grigory Vasilyev (＠h0tc0d3)]

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);
```

[Grigory Vasilyev (＠h0tc0d3)]

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);
```

[Rémi Bernon (＠rbernon)]

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.
> 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.

[Grigory Vasilyev (＠h0tc0d3)]

LGTM.