[PATCH v24 0/8] MR3103: server: Create and use a desktop shared mapping for GetCursorPos.
First part of Proton shared memory series. The full branch can be seen at https://gitlab.winehq.org/rbernon/wine/-/commits/mr/shared-memories. -- v24: win32u: Use the desktop shared data for GetCursorPos. server: Move the last cursor time to the desktop session object. server: Move the cursor position to the desktop session object. win32u: Open desktop shared objects from session mapping. server: Allocate shared session object for desktops. win32u: Open the global session shared mapping. include: Add ReadNoFence64 inline helpers. server: Create a global session shared mapping. https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
From: Rémi Bernon <rbernon(a)codeweavers.com> --- server/directory.c | 7 +++++++ server/file.h | 19 +++++++++++++++++++ server/mapping.c | 29 +++++++++++++++++++++++++++++ server/protocol.def | 24 ++++++++++++++++++++++++ tools/make_requests | 1 + 5 files changed, 80 insertions(+) diff --git a/server/directory.c b/server/directory.c index 23d7eb0a2b7..1ef645c2c2d 100644 --- a/server/directory.c +++ b/server/directory.c @@ -439,11 +439,14 @@ void init_directories( struct fd *intl_fd ) /* mappings */ static const WCHAR intlW[] = {'N','l','s','S','e','c','t','i','o','n','L','A','N','G','_','I','N','T','L'}; static const WCHAR user_dataW[] = {'_','_','w','i','n','e','_','u','s','e','r','_','s','h','a','r','e','d','_','d','a','t','a'}; + static const WCHAR sessionW[] = {'_','_','w','i','n','e','_','s','e','s','s','i','o','n'}; static const struct unicode_str intl_str = {intlW, sizeof(intlW)}; static const struct unicode_str user_data_str = {user_dataW, sizeof(user_dataW)}; + static const struct unicode_str session_str = {sessionW, sizeof(sessionW)}; struct directory *dir_driver, *dir_device, *dir_global, *dir_kernel, *dir_nls; struct object *named_pipe_device, *mailslot_device, *null_device; + struct mapping *session_mapping; unsigned int i; root_directory = create_directory( NULL, NULL, OBJ_PERMANENT, HASH_SIZE, NULL ); @@ -491,6 +494,10 @@ void init_directories( struct fd *intl_fd ) release_object( create_user_data_mapping( &dir_kernel->obj, &user_data_str, OBJ_PERMANENT, NULL )); release_object( intl_fd ); + session_mapping = create_session_mapping( &dir_kernel->obj, &session_str, OBJ_PERMANENT, NULL ); + set_session_mapping( session_mapping ); + release_object( session_mapping ); + release_object( named_pipe_device ); release_object( mailslot_device ); release_object( null_device ); diff --git a/server/file.h b/server/file.h index 7f2d1637863..ab6e475539b 100644 --- a/server/file.h +++ b/server/file.h @@ -188,6 +188,25 @@ extern struct mapping *create_fd_mapping( struct object *root, const struct unic unsigned int attr, const struct security_descriptor *sd ); extern struct object *create_user_data_mapping( struct object *root, const struct unicode_str *name, unsigned int attr, const struct security_descriptor *sd ); +extern struct mapping *create_session_mapping( struct object *root, const struct unicode_str *name, + unsigned int attr, const struct security_descriptor *sd ); +extern void set_session_mapping( struct mapping *mapping ); + + +#define SHARED_WRITE_BEGIN( object, type ) \ + do { \ + const type *__shared = (object); \ + type *shared = (type *)__shared; \ + LONG64 __seq = shared->obj.seq + 1, __end = __seq + 1; \ + assert( (__seq & 1) != 0 ); \ + __WINE_ATOMIC_STORE_RELEASE( &shared->obj.seq, &__seq ); \ + do + +#define SHARED_WRITE_END \ + while(0); \ + assert( __seq == shared->obj.seq ); \ + __WINE_ATOMIC_STORE_RELEASE( &shared->obj.seq, &__end ); \ + } while(0) /* device functions */ diff --git a/server/mapping.c b/server/mapping.c index 6b0de1b8b94..c540cd5bad9 100644 --- a/server/mapping.c +++ b/server/mapping.c @@ -225,6 +225,14 @@ static const mem_size_t granularity_mask = 0xffff; static struct addr_range ranges32; static struct addr_range ranges64; +struct session +{ + const session_shm_t *shared; + unsigned int object_count; +}; +static struct mapping *session_mapping; +static struct session session; + #define ROUND_SIZE(size) (((size) + page_mask) & ~page_mask) void init_memory(void) @@ -1256,6 +1264,27 @@ int get_page_size(void) return page_mask + 1; } +struct mapping *create_session_mapping( struct object *root, const struct unicode_str *name, + unsigned int attr, const struct security_descriptor *sd ) +{ + static const unsigned int access = FILE_READ_DATA | FILE_WRITE_DATA; + return create_mapping( root, name, attr, 0x10000, SEC_COMMIT, 0, access, sd ); +} + +void set_session_mapping( struct mapping *mapping ) +{ + session.shared = mmap( NULL, mapping->size, PROT_READ | PROT_WRITE, MAP_SHARED, get_unix_fd( mapping->fd ), 0 ); + if (session.shared == MAP_FAILED) return; + + session_mapping = mapping; + SHARED_WRITE_BEGIN( session.shared, session_shm_t ) + { + shared->obj.id++; + shared->object_capacity = (mapping->size - offsetof(session_shm_t, objects[0])) / sizeof(session_obj_t); + } + SHARED_WRITE_END; +} + struct object *create_user_data_mapping( struct object *root, const struct unicode_str *name, unsigned int attr, const struct security_descriptor *sd ) { diff --git a/server/protocol.def b/server/protocol.def index 7b21aa16636..60ae6b1d87a 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -45,6 +45,7 @@ typedef unsigned __int64 mem_size_t; typedef unsigned __int64 file_pos_t; typedef unsigned __int64 client_ptr_t; typedef unsigned __int64 affinity_t; +typedef unsigned __int64 object_id_t; typedef client_ptr_t mod_handle_t; struct request_header @@ -874,6 +875,29 @@ typedef struct lparam_t info; } cursor_pos_t; +/****************************************************************/ +/* shared session mapping structures */ + +typedef volatile struct +{ + LONG64 seq; /* sequence number - server updating if (seq & 1) != 0 */ + object_id_t id; /* object unique id, object data is valid if != 0 */ +} object_shm_t; + +typedef volatile union +{ + object_shm_t obj; +} session_obj_t; + +typedef volatile struct +{ + object_shm_t obj; + mem_size_t object_capacity; + session_obj_t objects[]; +} session_shm_t; + +C_ASSERT(sizeof(session_shm_t) == offsetof(session_shm_t, objects[0])); + /****************************************************************/ /* Request declarations */ diff --git a/tools/make_requests b/tools/make_requests index e3eaaf45b6f..f22ad279b61 100755 --- a/tools/make_requests +++ b/tools/make_requests @@ -42,6 +42,7 @@ my %formats = "file_pos_t" => [ 8, 8, "&dump_uint64" ], "mem_size_t" => [ 8, 8, "&dump_uint64" ], "affinity_t" => [ 8, 8, "&dump_uint64" ], + "object_id_t" => [ 8, 8, "&dump_uint64" ], "timeout_t" => [ 8, 8, "&dump_timeout" ], "abstime_t" => [ 8, 8, "&dump_abstime" ], "rectangle_t" => [ 16, 4, "&dump_rectangle" ], -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
From: Rémi Bernon <rbernon(a)codeweavers.com> --- include/winnt.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/include/winnt.h b/include/winnt.h index 7adbc1d3761..5c93113c7dc 100644 --- a/include/winnt.h +++ b/include/winnt.h @@ -7050,11 +7050,14 @@ static FORCEINLINE void MemoryBarrier(void) */ #if _MSC_VER >= 1700 #pragma intrinsic(__iso_volatile_load32) +#pragma intrinsic(__iso_volatile_load64) #pragma intrinsic(__iso_volatile_store32) #define __WINE_LOAD32_NO_FENCE(src) (__iso_volatile_load32(src)) +#define __WINE_LOAD64_NO_FENCE(src) (__iso_volatile_load64(src)) #define __WINE_STORE32_NO_FENCE(dest, value) (__iso_volatile_store32(dest, value)) #else /* _MSC_VER >= 1700 */ #define __WINE_LOAD32_NO_FENCE(src) (*(src)) +#define __WINE_LOAD64_NO_FENCE(src) (*(src)) #define __WINE_STORE32_NO_FENCE(dest, value) ((void)(*(dest) = (value))) #endif /* _MSC_VER >= 1700 */ @@ -7088,6 +7091,12 @@ static FORCEINLINE LONG ReadNoFence( LONG const volatile *src ) return value; } +static FORCEINLINE LONG64 ReadNoFence64( LONG64 const volatile *src ) +{ + LONG64 value = __WINE_LOAD64_NO_FENCE( (__int64 const volatile *)src ); + return value; +} + static FORCEINLINE void WriteRelease( LONG volatile *dest, LONG value ) { __wine_memory_barrier_acq_rel(); @@ -7274,6 +7283,13 @@ static FORCEINLINE LONG ReadNoFence( LONG const volatile *src ) return value; } +static FORCEINLINE LONG64 ReadNoFence64( LONG64 const volatile *src ) +{ + LONG64 value; + __WINE_ATOMIC_LOAD_RELAXED( src, &value ); + return value; +} + static FORCEINLINE void WriteRelease( LONG volatile *dest, LONG value ) { __WINE_ATOMIC_STORE_RELEASE( dest, &value ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/win32u/win32u_private.h | 23 +++++++ dlls/win32u/winstation.c | 113 ++++++++++++++++++++++++++++++++++- 2 files changed, 135 insertions(+), 1 deletion(-) diff --git a/dlls/win32u/win32u_private.h b/dlls/win32u/win32u_private.h index 0af6960a3eb..582204d9f04 100644 --- a/dlls/win32u/win32u_private.h +++ b/dlls/win32u/win32u_private.h @@ -364,4 +364,27 @@ static inline BOOL intersect_rect( RECT *dst, const RECT *src1, const RECT *src2 return !IsRectEmpty( dst ); } +#if defined(__i386__) || defined(__x86_64__) +/* this prevents compilers from incorrectly reordering non-volatile reads (e.g., memcpy) from shared memory */ +#define __SHARED_READ_FENCE do { __asm__ __volatile__( "" ::: "memory" ); } while (0) +#else +#define __SHARED_READ_FENCE __atomic_thread_fence( __ATOMIC_ACQUIRE ) +#endif + +#define SHARED_READ_BEGIN( ptr, type ) \ + do { \ + const type *shared = (ptr); \ + LONG64 __seq; \ + do { \ + while ((__seq = ReadNoFence64( &shared->obj.seq )) & 1) \ + YieldProcessor(); \ + __SHARED_READ_FENCE; \ + do + +#define SHARED_READ_END \ + while (0); \ + __SHARED_READ_FENCE; \ + } while (ReadNoFence64( &shared->obj.seq ) != __seq); \ + } while(0) + #endif /* __WINE_WIN32U_PRIVATE */ diff --git a/dlls/win32u/winstation.c b/dlls/win32u/winstation.c index 6ddd5411f94..9066a7279b7 100644 --- a/dlls/win32u/winstation.c +++ b/dlls/win32u/winstation.c @@ -22,9 +22,12 @@ #pragma makedep unix #endif +#include <stdarg.h> +#include <stddef.h> +#include <pthread.h> + #include "ntstatus.h" #define WIN32_NO_STATUS -#include <stdarg.h> #include "windef.h" #include "winbase.h" #include "ntuser.h" @@ -40,6 +43,112 @@ WINE_DECLARE_DEBUG_CHANNEL(win); #define DESKTOP_ALL_ACCESS 0x01ff +struct shared_session +{ + LONG ref; + UINT64 id; + UINT object_capacity; + const session_shm_t *shared; +}; + +static pthread_mutex_t session_lock = PTHREAD_MUTEX_INITIALIZER; +static struct shared_session *shared_session; + +static struct shared_session *shared_session_acquire( struct shared_session *session ) +{ + int ref = InterlockedIncrement( &session->ref ); + TRACE( "session %p incrementing ref to %d\n", session, ref ); + return session; +} + +static void shared_session_release( struct shared_session *session ) +{ + int ref = InterlockedDecrement( &session->ref ); + TRACE( "session %p decrementing ref to %d\n", session, ref ); + if (!ref) + { + NtUnmapViewOfSection( GetCurrentProcess(), (void *)session->shared ); + free( session ); + } +} + +static NTSTATUS map_shared_session_section( struct shared_session *session, HANDLE handle ) +{ + NTSTATUS status; + SIZE_T size = 0; + + while (!(status = NtMapViewOfSection( handle, GetCurrentProcess(), (void **)&session->shared, 0, 0, + NULL, &size, ViewUnmap, 0, PAGE_READONLY ))) + { + BOOL valid; + session->object_capacity = (size - offsetof(session_shm_t, objects[0])) / sizeof(session_obj_t); + SHARED_READ_BEGIN( session->shared, session_shm_t ) + { + valid = session->object_capacity == shared->object_capacity; + session->id = shared->obj.id; + } + SHARED_READ_END; + if (valid) break; + + NtUnmapViewOfSection( GetCurrentProcess(), (void *)session->shared ); + session->shared = NULL; + size = 0; + } + + return status; +} + +static struct shared_session *get_shared_session( BOOL force ) +{ + struct shared_session *session; + + pthread_mutex_lock( &session_lock ); + + if (force || !shared_session) + { + static const WCHAR nameW[] = + { + '\\','K','e','r','n','e','l','O','b','j','e','c','t','s','\\', + '_','_','w','i','n','e','_','s','e','s','s','i','o','n',0 + }; + UNICODE_STRING name = RTL_CONSTANT_STRING( nameW ); + OBJECT_ATTRIBUTES attr; + unsigned int status; + HANDLE handle; + + if (!(session = calloc( 1, sizeof(*session) ))) + { + pthread_mutex_unlock( &session_lock ); + return NULL; + } + session->ref = 1; + + InitializeObjectAttributes( &attr, &name, 0, NULL, NULL ); + if (!(status = NtOpenSection( &handle, SECTION_MAP_READ | SECTION_QUERY, &attr ))) + { + status = map_shared_session_section( session, handle ); + NtClose( handle ); + } + + if (status) + { + ERR( "Failed to map session mapping, status %#x\n", status ); + free( session ); + } + else + { + if (shared_session) shared_session_release( shared_session ); + shared_session = session; + } + } + + session = shared_session_acquire( shared_session ); + + pthread_mutex_unlock( &session_lock ); + + return session; +} + BOOL is_virtual_desktop(void) { HANDLE desktop = NtUserGetThreadDesktop( GetCurrentThreadId() ); @@ -636,6 +745,8 @@ void winstation_init(void) static const WCHAR winsta0[] = {'W','i','n','S','t','a','0',0}; + shared_session = get_shared_session( FALSE ); + if (params->Desktop.Length) { buffer = malloc( params->Desktop.Length + sizeof(WCHAR) ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
From: Rémi Bernon <rbernon(a)codeweavers.com> --- server/file.h | 4 +++ server/mapping.c | 74 +++++++++++++++++++++++++++++++++++++++++++++ server/protocol.def | 8 +++++ server/user.h | 1 + server/winstation.c | 19 ++++++++++++ 5 files changed, 106 insertions(+) diff --git a/server/file.h b/server/file.h index ab6e475539b..efacf240cb2 100644 --- a/server/file.h +++ b/server/file.h @@ -193,6 +193,10 @@ extern struct mapping *create_session_mapping( struct object *root, const struct extern void set_session_mapping( struct mapping *mapping ); +extern int alloc_shared_object(void); +extern void free_shared_object( int index ); +extern const desktop_shm_t *get_shared_desktop( int index ); + #define SHARED_WRITE_BEGIN( object, type ) \ do { \ const type *__shared = (object); \ diff --git a/server/mapping.c b/server/mapping.c index c540cd5bad9..8987fb0dd8e 100644 --- a/server/mapping.c +++ b/server/mapping.c @@ -229,6 +229,7 @@ struct session { const session_shm_t *shared; unsigned int object_count; + object_id_t last_object_id; }; static struct mapping *session_mapping; static struct session session; @@ -1285,6 +1286,79 @@ void set_session_mapping( struct mapping *mapping ) SHARED_WRITE_END; } +static int grow_session_mapping(void) +{ + unsigned int capacity; + mem_size_t size; + int unix_fd; + void *tmp; + + capacity = session.shared->object_capacity * 3 / 2; + size = offsetof(session_shm_t, objects[capacity]); + size = (size + page_mask) & ~((mem_size_t)page_mask); + + unix_fd = get_unix_fd( session_mapping->fd ); + if (!grow_file( unix_fd, size )) return -1; + + if ((tmp = mmap( NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, unix_fd, 0 )) == MAP_FAILED) return -1; + munmap( (void *)session.shared, session_mapping->size ); + session.shared = tmp; + session_mapping->size = size; + + SHARED_WRITE_BEGIN( session.shared, session_shm_t ) + { + shared->obj.id++; + shared->object_capacity = (size - offsetof(session_shm_t, objects[0])) / sizeof(session_obj_t); + } + SHARED_WRITE_END; + + return 0; +} + +int alloc_shared_object(void) +{ + int index; + + if (session.object_count < session.shared->object_capacity) + index = session.object_count++; + else + { + for (index = 0; index < session.object_count; index++) + if (!session.shared->objects[index].obj.id) + break; + if (index == session.object_count) + { + if (grow_session_mapping()) return -1; + index = session.object_count++; + } + } + + SHARED_WRITE_BEGIN( &session.shared->objects[index], session_obj_t ) + { + shared->obj.id = ++session.last_object_id; + } + SHARED_WRITE_END; + + return index; +} + +void free_shared_object( int index ) +{ + if (index < 0) return; + + SHARED_WRITE_BEGIN( &session.shared->objects[index], session_obj_t ) + { + shared->obj.id = 0; + } + SHARED_WRITE_END; +} + +const desktop_shm_t *get_shared_desktop( int index ) +{ + if (index < 0) return NULL; + return &session.shared->objects[index].desktop; +} + struct object *create_user_data_mapping( struct object *root, const struct unicode_str *name, unsigned int attr, const struct security_descriptor *sd ) { diff --git a/server/protocol.def b/server/protocol.def index 60ae6b1d87a..f5ba4c1ff9b 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -884,9 +884,15 @@ typedef volatile struct object_id_t id; /* object unique id, object data is valid if != 0 */ } object_shm_t; +typedef volatile struct +{ + object_shm_t obj; +} desktop_shm_t; + typedef volatile union { object_shm_t obj; + desktop_shm_t desktop; } session_obj_t; typedef volatile struct @@ -2805,6 +2811,8 @@ enum coords_relative thread_id_t tid; /* thread id */ @REPLY obj_handle_t handle; /* handle to the desktop */ + int index; /* index of desktop object in session shared memory */ + object_id_t object_id; /* id of the session object */ @END diff --git a/server/user.h b/server/user.h index 309a0966104..21d67656c9c 100644 --- a/server/user.h +++ b/server/user.h @@ -81,6 +81,7 @@ struct desktop unsigned int users; /* processes and threads using this desktop */ struct global_cursor cursor; /* global cursor information */ unsigned char keystate[256]; /* asynchronous key state */ + int session_index; /* desktop index in session shared memory */ }; /* user handles functions */ diff --git a/server/winstation.c b/server/winstation.c index 4672cf65e56..89dee4a3e8e 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -297,6 +297,13 @@ static struct desktop *create_desktop( const struct unicode_str *name, unsigned list_add_tail( &winstation->desktops, &desktop->entry ); list_init( &desktop->hotkeys ); list_init( &desktop->pointers ); + desktop->session_index = -1; + + if ((desktop->session_index = alloc_shared_object()) == -1) + { + release_object( desktop ); + return NULL; + } } else { @@ -365,6 +372,7 @@ static void desktop_destroy( struct object *obj ) if (desktop->close_timeout) remove_timeout_user( desktop->close_timeout ); list_remove( &desktop->entry ); release_object( desktop->winstation ); + free_shared_object( desktop->session_index ); } /* retrieve the thread desktop, checking the handle access rights */ @@ -728,10 +736,21 @@ DECL_HANDLER(close_desktop) /* get the thread current desktop */ DECL_HANDLER(get_thread_desktop) { + struct desktop *desktop; struct thread *thread; if (!(thread = get_thread_from_id( req->tid ))) return; reply->handle = thread->desktop; + reply->index = -1; + + if ((desktop = get_thread_desktop( thread, 0 ))) + { + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); + reply->index = desktop->session_index; + reply->object_id = desktop_shm->obj.id; + release_object( desktop ); + } + release_object( thread ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/win32u/ntuser_private.h | 1 + dlls/win32u/sysparams.c | 1 + dlls/win32u/win32u_private.h | 13 ++++ dlls/win32u/winstation.c | 123 +++++++++++++++++++++++++++++++++++ 4 files changed, 138 insertions(+) diff --git a/dlls/win32u/ntuser_private.h b/dlls/win32u/ntuser_private.h index 3b6cab5bdc9..27430c3478b 100644 --- a/dlls/win32u/ntuser_private.h +++ b/dlls/win32u/ntuser_private.h @@ -127,6 +127,7 @@ struct user_thread_info UINT spy_indent; /* Current spy indent */ BOOL clipping_cursor; /* thread is currently clipping */ DWORD clipping_reset; /* time when clipping was last reset */ + struct session_thread_data *session_data; /* shared session thread data */ }; C_ASSERT( sizeof(struct user_thread_info) <= sizeof(((TEB *)0)->Win32ClientInfo) ); diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index 9457c9010f0..f856c9fe05e 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -6039,6 +6039,7 @@ static void thread_detach(void) destroy_thread_windows(); cleanup_imm_thread(); NtClose( thread_info->server_queue ); + free( thread_info->session_data ); exiting_thread_id = 0; } diff --git a/dlls/win32u/win32u_private.h b/dlls/win32u/win32u_private.h index 582204d9f04..f5088829fb8 100644 --- a/dlls/win32u/win32u_private.h +++ b/dlls/win32u/win32u_private.h @@ -193,6 +193,19 @@ extern void user_unlock(void); extern void user_check_not_lock(void); /* winstation.c */ + +struct shared_session; + +struct object_lock +{ + UINT64 id; + const session_obj_t *shared; /* only valid when locked, read inside SHARED_READ_BEGIN */ + struct shared_session *session; /* only valid when locked */ +}; + +extern void object_lock_release( struct object_lock *lock ); +extern BOOL get_shared_desktop( struct object_lock *lock ); + extern BOOL is_virtual_desktop(void); /* window.c */ diff --git a/dlls/win32u/winstation.c b/dlls/win32u/winstation.c index 9066a7279b7..bf985036fb1 100644 --- a/dlls/win32u/winstation.c +++ b/dlls/win32u/winstation.c @@ -43,6 +43,18 @@ WINE_DECLARE_DEBUG_CHANNEL(win); #define DESKTOP_ALL_ACCESS 0x01ff +struct object_info +{ + UINT64 session_id; + UINT64 id; + UINT index; +}; + +struct session_thread_data +{ + struct object_info shared_desktop; /* thread desktop shared session object info */ +}; + struct shared_session { LONG ref; @@ -54,6 +66,13 @@ struct shared_session static pthread_mutex_t session_lock = PTHREAD_MUTEX_INITIALIZER; static struct shared_session *shared_session; +static struct session_thread_data *get_session_thread_data(void) +{ + struct user_thread_info *thread_info = get_user_thread_info(); + if (!thread_info->session_data) thread_info->session_data = calloc(1, sizeof(struct session_thread_data)); + return thread_info->session_data; +} + static struct shared_session *shared_session_acquire( struct shared_session *session ) { int ref = InterlockedIncrement( &session->ref ); @@ -149,6 +168,107 @@ static struct shared_session *get_shared_session( BOOL force ) return session; } +static struct shared_session *shared_session_try_acquire( UINT64 session_id ) +{ + struct shared_session *session; + + pthread_mutex_lock( &session_lock ); + + if (shared_session && shared_session->id == session_id) + session = shared_session_acquire( shared_session ); + else + { + TRACE( "session %s has been discarded\n", wine_dbgstr_longlong(session_id) ); + session = NULL; + } + + pthread_mutex_unlock( &session_lock ); + + return session; +} + +static BOOL try_lock_session_object( struct object_info *info, struct object_lock *lock ) +{ + if (!(lock->id = info->id) || !(lock->session = shared_session_try_acquire( info->session_id ))) return FALSE; + lock->shared = &lock->session->shared->objects[info->index]; + return TRUE; +} + +void object_lock_release( struct object_lock *lock ) +{ + shared_session_release( lock->session ); + memset( lock, 0, sizeof(*lock) ); + lock->id = -1; /* force object to be refreshed when retrying lock */ +} + +enum object_type +{ + OBJECT_TYPE_DESKTOP = 1, +}; + +static UINT get_thread_session_object_index( UINT tid, enum object_type type, UINT64 *id ) +{ + switch (type) + { + case OBJECT_TYPE_DESKTOP: + SERVER_START_REQ( get_thread_desktop ) + { + req->tid = tid; + if (wine_server_call_err( req )) return -1; + *id = reply->object_id; + return reply->index; + } + SERVER_END_REQ; + break; + } + + return -1; +} + +/* return a locked session object for a thread id and type */ +static BOOL get_thread_session_object( UINT tid, enum object_type type, struct object_info *info, + struct object_lock *lock ) +{ + struct shared_session *session; + BOOL valid = TRUE; + + TRACE( "tid %04x, type %u\n", tid, type ); + + memset( info, 0, sizeof(*info) ); + info->index = -1; + + while ((session = get_shared_session( !valid ))) + { + info->session_id = session->id; + if ((info->index = get_thread_session_object_index( tid, type, &info->id )) == -1) break; + if ((valid = info->index < session->object_capacity)) break; + shared_session_release( session ); + } + + if (valid && info->index == -1) + { + WARN( "Failed to find object type %u for thread %04x\n", type, tid ); + if (session) shared_session_release( session ); + return FALSE; + } + + lock->id = info->id; + lock->session = session; + lock->shared = &session->shared->objects[info->index]; + return TRUE; +} + +BOOL get_shared_desktop( struct object_lock *lock ) +{ + struct session_thread_data *data = get_session_thread_data(); + struct object_info *info = &data->shared_desktop; + + TRACE( "lock %p\n", lock ); + + if (lock->id != -1 && try_lock_session_object( info, lock )) return TRUE; + return get_thread_session_object( GetCurrentThreadId(), OBJECT_TYPE_DESKTOP, info, lock ); +} + BOOL is_virtual_desktop(void) { HANDLE desktop = NtUserGetThreadDesktop( GetCurrentThreadId() ); @@ -370,9 +490,12 @@ BOOL WINAPI NtUserSetThreadDesktop( HDESK handle ) { struct user_thread_info *thread_info = get_user_thread_info(); struct user_key_state_info *key_state_info = thread_info->key_state; + struct object_lock lock = {0}; thread_info->client_info.top_window = 0; thread_info->client_info.msg_window = 0; if (key_state_info) key_state_info->time = 0; + memset( &get_session_thread_data()->shared_desktop, 0, sizeof(struct object_info) ); + if (get_shared_desktop( &lock )) object_lock_release( &lock ); if (was_virtual_desktop != is_virtual_desktop()) update_display_cache( TRUE ); } return ret; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
From: Rémi Bernon <rbernon(a)codeweavers.com> Based on a patch by Huw Davies <huw(a)codeweavers.com>. --- server/protocol.def | 7 ++++ server/queue.c | 79 ++++++++++++++++++++++++++++----------------- server/user.h | 2 -- 3 files changed, 56 insertions(+), 32 deletions(-) diff --git a/server/protocol.def b/server/protocol.def index f5ba4c1ff9b..c02597d872f 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -878,6 +878,12 @@ typedef struct /****************************************************************/ /* shared session mapping structures */ +struct shared_cursor +{ + int x; /* cursor position */ + int y; +}; + typedef volatile struct { LONG64 seq; /* sequence number - server updating if (seq & 1) != 0 */ @@ -887,6 +893,7 @@ typedef volatile struct typedef volatile struct { object_shm_t obj; + struct shared_cursor cursor; /* global cursor information */ } desktop_shm_t; typedef volatile union diff --git a/server/queue.c b/server/queue.c index 0e8653bedf0..fcda0602f57 100644 --- a/server/queue.c +++ b/server/queue.c @@ -418,6 +418,7 @@ static void queue_cursor_message( struct desktop *desktop, user_handle_t win, un lparam_t wparam, lparam_t lparam ) { static const struct hw_msg_source source = { IMDT_UNAVAILABLE, IMO_SYSTEM }; + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); struct thread_input *input; struct message *msg; @@ -426,8 +427,8 @@ static void queue_cursor_message( struct desktop *desktop, user_handle_t win, un msg->msg = message; msg->wparam = wparam; msg->lparam = lparam; - msg->x = desktop->cursor.x; - msg->y = desktop->cursor.y; + msg->x = desktop_shm->cursor.x; + msg->y = desktop_shm->cursor.y; if (!(msg->win = win) && (input = desktop->foreground_input)) msg->win = input->active; queue_hardware_message( desktop, msg, 1 ); } @@ -465,13 +466,20 @@ static int update_desktop_cursor_window( struct desktop *desktop, user_handle_t static int update_desktop_cursor_pos( struct desktop *desktop, user_handle_t win, int x, int y ) { + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); int updated; x = max( min( x, desktop->cursor.clip.right - 1 ), desktop->cursor.clip.left ); y = max( min( y, desktop->cursor.clip.bottom - 1 ), desktop->cursor.clip.top ); - updated = (desktop->cursor.x != x || desktop->cursor.y != y); - desktop->cursor.x = x; - desktop->cursor.y = y; + + SHARED_WRITE_BEGIN( desktop_shm, desktop_shm_t ) + { + updated = shared->cursor.x != x || shared->cursor.y != y; + shared->cursor.x = x; + shared->cursor.y = y; + } + SHARED_WRITE_END; + desktop->cursor.last_change = get_tick_count(); if (!win || !is_window_visible( win ) || is_window_transparent( win )) @@ -517,15 +525,17 @@ static void set_cursor_pos( struct desktop *desktop, int x, int y ) static void get_message_defaults( struct msg_queue *queue, int *x, int *y, unsigned int *time ) { struct desktop *desktop = queue->input->desktop; + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); - *x = desktop->cursor.x; - *y = desktop->cursor.y; + *x = desktop_shm->cursor.x; + *y = desktop_shm->cursor.y; *time = get_tick_count(); } /* set the cursor clip rectangle */ void set_clip_rectangle( struct desktop *desktop, const rectangle_t *rect, unsigned int flags, int reset ) { + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); rectangle_t top_rect; int x, y; @@ -543,9 +553,9 @@ void set_clip_rectangle( struct desktop *desktop, const rectangle_t *rect, unsig else desktop->cursor.clip = top_rect; /* warp the mouse to be inside the clip rect */ - x = max( min( desktop->cursor.x, desktop->cursor.clip.right - 1 ), desktop->cursor.clip.left ); - y = max( min( desktop->cursor.y, desktop->cursor.clip.bottom - 1 ), desktop->cursor.clip.top ); - if (x != desktop->cursor.x || y != desktop->cursor.y) set_cursor_pos( desktop, x, y ); + x = max( min( desktop_shm->cursor.x, desktop->cursor.clip.right - 1 ), desktop->cursor.clip.left ); + y = max( min( desktop_shm->cursor.y, desktop->cursor.clip.bottom - 1 ), desktop->cursor.clip.top ); + if (x != desktop_shm->cursor.x || y != desktop_shm->cursor.y) set_cursor_pos( desktop, x, y ); /* request clip cursor rectangle reset to the desktop thread */ if (reset) post_desktop_message( desktop, WM_WINE_CLIPCURSOR, flags, FALSE ); @@ -1668,6 +1678,7 @@ static void prepend_cursor_history( int x, int y, unsigned int time, lparam_t in /* queue a hardware message into a given thread input */ static void queue_hardware_message( struct desktop *desktop, struct message *msg, int always_queue ) { + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); user_handle_t win; struct thread *thread; struct thread_input *input; @@ -1700,8 +1711,8 @@ static void queue_hardware_message( struct desktop *desktop, struct message *msg if (desktop->keystate[VK_XBUTTON2] & 0x80) msg->wparam |= MK_XBUTTON2; break; } - msg->x = desktop->cursor.x; - msg->y = desktop->cursor.y; + msg->x = desktop_shm->cursor.x; + msg->y = desktop_shm->cursor.y; if (msg->win && (thread = get_window_thread( msg->win ))) { @@ -1983,6 +1994,7 @@ static void dispatch_rawinput_message( struct desktop *desktop, struct rawinput_ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, const hw_input_t *input, unsigned int origin, struct msg_queue *sender ) { + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); const struct rawinput_device *device; struct hardware_msg_data *msg_data; struct rawinput_message raw_msg; @@ -2021,19 +2033,19 @@ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, cons x = input->mouse.x; y = input->mouse.y; if (flags & ~(MOUSEEVENTF_MOVE | MOUSEEVENTF_ABSOLUTE) && - x == desktop->cursor.x && y == desktop->cursor.y) + x == desktop_shm->cursor.x && y == desktop_shm->cursor.y) flags &= ~MOUSEEVENTF_MOVE; } else { - x = desktop->cursor.x + input->mouse.x; - y = desktop->cursor.y + input->mouse.y; + x = desktop_shm->cursor.x + input->mouse.x; + y = desktop_shm->cursor.y + input->mouse.y; } } else { - x = desktop->cursor.x; - y = desktop->cursor.y; + x = desktop_shm->cursor.x; + y = desktop_shm->cursor.y; } if ((foreground = get_foreground_thread( desktop, win ))) @@ -2044,7 +2056,7 @@ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, cons raw_msg.time = time; raw_msg.message = WM_INPUT; raw_msg.flags = flags; - rawmouse_init( &raw_msg.rawinput, &raw_msg.data.mouse, x - desktop->cursor.x, y - desktop->cursor.y, + rawmouse_init( &raw_msg.rawinput, &raw_msg.data.mouse, x - desktop_shm->cursor.x, y - desktop_shm->cursor.y, raw_msg.flags, input->mouse.data, input->mouse.info ); dispatch_rawinput_message( desktop, &raw_msg ); @@ -2249,11 +2261,13 @@ static void queue_pointer_message( struct pointer *pointer, int repeated ) const hw_input_t *input = &pointer->input; unsigned int i, wparam = input->hw.wparam; timeout_t time = get_tick_count(); + const desktop_shm_t *desktop_shm; user_handle_t win = pointer->win; rectangle_t top_rect; struct message *msg; int x, y; + desktop_shm = get_shared_desktop( desktop->session_index ); get_top_window_rectangle( desktop, &top_rect ); x = LOWORD(input->hw.lparam) * (top_rect.right - top_rect.left) / 65535; y = HIWORD(input->hw.lparam) * (top_rect.bottom - top_rect.top) / 65535; @@ -2268,8 +2282,8 @@ static void queue_pointer_message( struct pointer *pointer, int repeated ) msg->msg = messages[input->hw.msg - WM_POINTERUPDATE][i]; msg->wparam = wparam; msg->lparam = MAKELONG(x, y); - msg->x = desktop->cursor.x; - msg->y = desktop->cursor.y; + msg->x = desktop_shm->cursor.x; + msg->y = desktop_shm->cursor.y; queue_hardware_message( desktop, msg, 1 ); } @@ -2324,6 +2338,7 @@ static struct pointer *find_pointer_from_id( struct desktop *desktop, unsigned i static void queue_custom_hardware_message( struct desktop *desktop, user_handle_t win, unsigned int origin, const hw_input_t *input ) { + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); struct hw_msg_source source = { IMDT_UNAVAILABLE, origin }; struct thread *foreground; struct pointer *pointer; @@ -2368,8 +2383,8 @@ static void queue_custom_hardware_message( struct desktop *desktop, user_handle_ msg->msg = input->hw.msg; msg->wparam = input->hw.wparam; msg->lparam = input->hw.lparam; - msg->x = desktop->cursor.x; - msg->y = desktop->cursor.y; + msg->x = desktop_shm->cursor.x; + msg->y = desktop_shm->cursor.y; queue_hardware_message( desktop, msg, 1 ); } @@ -2917,6 +2932,7 @@ DECL_HANDLER(send_hardware_message) struct desktop *desktop; unsigned int origin = (req->flags & SEND_HWMSG_INJECTED ? IMO_INJECTED : IMO_HARDWARE); struct msg_queue *sender = req->flags & SEND_HWMSG_INJECTED ? get_current_queue() : NULL; + const desktop_shm_t *desktop_shm; int wait = 0; if (!(desktop = get_hardware_input_desktop( req->win ))) return; @@ -2927,9 +2943,10 @@ DECL_HANDLER(send_hardware_message) set_error( STATUS_ACCESS_DENIED ); return; } + desktop_shm = get_shared_desktop( desktop->session_index ); - reply->prev_x = desktop->cursor.x; - reply->prev_y = desktop->cursor.y; + reply->prev_x = desktop_shm->cursor.x; + reply->prev_y = desktop_shm->cursor.y; switch (req->input.type) { @@ -2947,8 +2964,8 @@ DECL_HANDLER(send_hardware_message) } reply->wait = sender ? wait : 0; - reply->new_x = desktop->cursor.x; - reply->new_y = desktop->cursor.y; + reply->new_x = desktop_shm->cursor.x; + reply->new_y = desktop_shm->cursor.y; release_object( desktop ); } @@ -3633,15 +3650,17 @@ DECL_HANDLER(set_cursor) struct msg_queue *queue = get_current_queue(); struct thread_input *input; struct desktop *desktop; + const desktop_shm_t *desktop_shm; if (!queue) return; input = queue->input; desktop = input->desktop; + desktop_shm = get_shared_desktop( desktop->session_index ); reply->prev_handle = input->cursor; reply->prev_count = input->cursor_count; - reply->prev_x = desktop->cursor.x; - reply->prev_y = desktop->cursor.y; + reply->prev_x = desktop_shm->cursor.x; + reply->prev_y = desktop_shm->cursor.y; if (req->flags & SET_CURSOR_HANDLE) { @@ -3664,8 +3683,8 @@ DECL_HANDLER(set_cursor) if (req->flags & (SET_CURSOR_HANDLE | SET_CURSOR_COUNT)) update_desktop_cursor_handle( desktop, input ); - reply->new_x = desktop->cursor.x; - reply->new_y = desktop->cursor.y; + reply->new_x = desktop_shm->cursor.x; + reply->new_y = desktop_shm->cursor.y; reply->new_clip = desktop->cursor.clip; reply->last_change = desktop->cursor.last_change; } diff --git a/server/user.h b/server/user.h index 21d67656c9c..94c107116ed 100644 --- a/server/user.h +++ b/server/user.h @@ -56,8 +56,6 @@ struct winstation struct global_cursor { - int x; /* cursor position */ - int y; rectangle_t clip; /* cursor clip rectangle */ unsigned int last_change; /* time of last position change */ user_handle_t win; /* window that contains the cursor */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
From: Rémi Bernon <rbernon(a)codeweavers.com> Based on a patch by Huw Davies <huw(a)codeweavers.com>. --- server/protocol.def | 1 + server/queue.c | 17 +++++++++++------ server/user.h | 1 - 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/server/protocol.def b/server/protocol.def index c02597d872f..c912a538b4d 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -882,6 +882,7 @@ struct shared_cursor { int x; /* cursor position */ int y; + unsigned int last_change; /* time of last position change */ }; typedef volatile struct diff --git a/server/queue.c b/server/queue.c index fcda0602f57..ef0b12a9084 100644 --- a/server/queue.c +++ b/server/queue.c @@ -468,6 +468,7 @@ static int update_desktop_cursor_pos( struct desktop *desktop, user_handle_t win { const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); int updated; + unsigned int time = get_tick_count(); x = max( min( x, desktop->cursor.clip.right - 1 ), desktop->cursor.clip.left ); y = max( min( y, desktop->cursor.clip.bottom - 1 ), desktop->cursor.clip.top ); @@ -477,11 +478,10 @@ static int update_desktop_cursor_pos( struct desktop *desktop, user_handle_t win updated = shared->cursor.x != x || shared->cursor.y != y; shared->cursor.x = x; shared->cursor.y = y; + shared->cursor.last_change = time; } SHARED_WRITE_END; - desktop->cursor.last_change = get_tick_count(); - if (!win || !is_window_visible( win ) || is_window_transparent( win )) win = shallow_window_from_point( desktop, x, y ); if (update_desktop_cursor_window( desktop, win )) updated = 1; @@ -2000,7 +2000,7 @@ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, cons struct rawinput_message raw_msg; struct message *msg; struct thread *foreground; - unsigned int i, time, flags; + unsigned int i, time = get_tick_count(), flags; struct hw_msg_source source = { IMDT_MOUSE, origin }; int wait = 0, x, y; @@ -2021,10 +2021,15 @@ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, cons WM_MOUSEHWHEEL /* 0x1000 = MOUSEEVENTF_HWHEEL */ }; - desktop->cursor.last_change = get_tick_count(); + SHARED_WRITE_BEGIN( desktop_shm, desktop_shm_t ) + { + shared->cursor.last_change = time; + } + SHARED_WRITE_END; + flags = input->mouse.flags; time = input->mouse.time; - if (!time) time = desktop->cursor.last_change; + if (!time) time = desktop_shm->cursor.last_change; if (flags & MOUSEEVENTF_MOVE) { @@ -3686,7 +3691,7 @@ DECL_HANDLER(set_cursor) reply->new_x = desktop_shm->cursor.x; reply->new_y = desktop_shm->cursor.y; reply->new_clip = desktop->cursor.clip; - reply->last_change = desktop->cursor.last_change; + reply->last_change = desktop_shm->cursor.last_change; } /* Get the history of the 64 last cursor positions */ diff --git a/server/user.h b/server/user.h index 94c107116ed..4f98ce46ffa 100644 --- a/server/user.h +++ b/server/user.h @@ -57,7 +57,6 @@ struct winstation struct global_cursor { rectangle_t clip; /* cursor clip rectangle */ - unsigned int last_change; /* time of last position change */ user_handle_t win; /* window that contains the cursor */ }; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
From: Rémi Bernon <rbernon(a)codeweavers.com> Based on a patch by Huw Davies <huw(a)codeweavers.com>. --- dlls/win32u/input.c | 21 +++++++++++++-------- dlls/win32u/winstation.c | 2 -- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/dlls/win32u/input.c b/dlls/win32u/input.c index 0f6b9482942..871912a1454 100644 --- a/dlls/win32u/input.c +++ b/dlls/win32u/input.c @@ -739,22 +739,27 @@ BOOL WINAPI NtUserSetCursorPos( INT x, INT y ) */ BOOL get_cursor_pos( POINT *pt ) { - BOOL ret; - DWORD last_change; + struct object_lock lock = {0}; + BOOL ret = FALSE; + DWORD last_change = 0; UINT dpi; if (!pt) return FALSE; - SERVER_START_REQ( set_cursor ) + while (get_shared_desktop( &lock )) { - if ((ret = !wine_server_call( req ))) + BOOL valid; + SHARED_READ_BEGIN( &lock.shared->desktop, desktop_shm_t ) { - pt->x = reply->new_x; - pt->y = reply->new_y; - last_change = reply->last_change; + valid = lock.id == shared->obj.id; + pt->x = shared->cursor.x; + pt->y = shared->cursor.y; + last_change = shared->cursor.last_change; } + SHARED_READ_END; + object_lock_release( &lock ); + if ((ret = valid)) break; } - SERVER_END_REQ; /* query new position from graphics driver if we haven't updated recently */ if (ret && NtGetTickCount() - last_change > 100) ret = user_driver->pGetCursorPos( pt ); diff --git a/dlls/win32u/winstation.c b/dlls/win32u/winstation.c index bf985036fb1..6b3aabf8cb3 100644 --- a/dlls/win32u/winstation.c +++ b/dlls/win32u/winstation.c @@ -490,12 +490,10 @@ BOOL WINAPI NtUserSetThreadDesktop( HDESK handle ) { struct user_thread_info *thread_info = get_user_thread_info(); struct user_key_state_info *key_state_info = thread_info->key_state; - struct object_lock lock = {0}; thread_info->client_info.top_window = 0; thread_info->client_info.msg_window = 0; if (key_state_info) key_state_info->time = 0; memset( &get_session_thread_data()->shared_desktop, 0, sizeof(struct object_info) ); - if (get_shared_desktop( &lock )) object_lock_release( &lock ); if (was_virtual_desktop != is_virtual_desktop()) update_display_cache( TRUE ); } return ret; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
+ BOOL valid = TRUE; + + TRACE( "tid %04x, type %u\n", tid, type ); + + memset( info, 0, sizeof(*info) ); + info->index = -1; + + while ((session = get_shared_session( !valid ))) + { + info->session_id = session->id; + if ((info->index = get_thread_session_object_index( tid, type, &info->id )) == -1) break; + if ((valid = info->index < session->object_capacity)) break; + shared_session_release( session ); + } + + if (valid && info->index == -1) My previous suggestion was wrong:
```suggestion:-0+0 if (!valid || info->index == -1) ``` But this is hard to understand too. Instead of `break;`-ing out of the loop on failure, I wonder if we can just put success logic (`...; return TRUE`) inside the loop. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64970
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
static const WCHAR winsta0[] = {'W','i','n','S','t','a','0',0};
+ shared_session = get_shared_session( FALSE );
Since `get_shared_session` returns the last `shared_session` value anyway, this is more-or-less equivalent to: ```c shared_session_release( get_shared_session( FALSE ) ); shared_session = shared_session_acquire( shared_session ); ``` which leaks a reference. In principle, you shouldn't overwrite a reference-counted pointer variable before releasing its old object, **even if the old one is the same as the new one.**[^1] Here, `get_shared_session` already writes to `shared_session`. To re-assign `shared_session`, you need to first release it. ```suggestion:-0+0 struct shared_session *tmp = get_shared_session( FALSE ); // acquire reference // get_shared_session() has assigned to the `shared_sesion` variable. // We need to release it first. shared_session_release( shared_session ); // Now, the reference associated with `shared_session` is now gone. We can safely overwrite it without leaking: shared_session = tmp; // transfer ownership to the variable ``` Since `shared_session == tmp`, the above reduces to: ```suggestion:-0+0 shared_session_release( get_shared_session( FALSE ) ); ``` [^1]: Except you probably want to skip the assign-the-same-value noop operation altogether. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64971
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
static const WCHAR winsta0[] = {'W','i','n','S','t','a','0',0};
+ shared_session = get_shared_session( FALSE );
Technically this is not strictly necessary; the shared mapping will be created when we actually need it. If we still need this for other reasons (e.g., catching mapping-related errors early, or avoiding unpredictable latency on next shmem-reading operation), a comment would be nice. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64972
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
+} + +enum object_type +{ + OBJECT_TYPE_DESKTOP = 1, +}; + +static UINT get_thread_session_object_index( UINT tid, enum object_type type, UINT64 *id ) +{ + switch (type) + { + case OBJECT_TYPE_DESKTOP: + SERVER_START_REQ( get_thread_desktop ) + { + req->tid = tid; + if (wine_server_call_err( req )) return -1; We're setting Win32 LastError in GetCursorPos(). This is new (and probably untested) behavior.
```suggestion:-0+0 if (wine_server_call( req )) return -1; ``` --- **Apropos the bigger context of the entire [shared-memories][] branch:** I'm aware that `NtUserGetForegroundWindow()` does set Win32 LastError. In this case, we can either choose `wine_server_call_err` for `OBJECT_TYPE_INPUT` (not in this MR), or (ideally) refactor `get_thread_session_object_index` so that it returns NTSTATUS instead (again out of scope of this MR). [shared-memories]: <https://gitlab.winehq.org/rbernon/wine/-/commits/mr/shared-memories> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64973
Jinoh Kang (@iamahuman) commented about server/mapping.c:
+ +static int grow_session_mapping(void) +{ + unsigned int capacity; + mem_size_t size; + int unix_fd; + void *tmp; + + capacity = session.shared->object_capacity * 3 / 2; + size = offsetof(session_shm_t, objects[capacity]); + size = (size + page_mask) & ~((mem_size_t)page_mask); + + unix_fd = get_unix_fd( session_mapping->fd ); + if (!grow_file( unix_fd, size )) return -1; + + if ((tmp = mmap( NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, unix_fd, 0 )) == MAP_FAILED) return -1; We're failing without setting error. This will make create\_desktop server call fail with `STATUS_SUCCESS`.
```suggestion:-0+0 if ((tmp = mmap( NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, unix_fd, 0 )) == MAP_FAILED) { file_set_error(); return -1; } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64974
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
+/* return a locked session object for a thread id and type */ +static BOOL get_thread_session_object( UINT tid, enum object_type type, struct object_info *info, + struct object_lock *lock ) +{ + struct shared_session *session; + BOOL valid = TRUE; + + TRACE( "tid %04x, type %u\n", tid, type ); + + memset( info, 0, sizeof(*info) ); + info->index = -1; + + while ((session = get_shared_session( !valid ))) + { + info->session_id = session->id; + if ((info->index = get_thread_session_object_index( tid, type, &info->id )) == -1) break; Suppose that:
1. `get_thread_session_object_index` succeeded in the previous iteration, but 2. `get_thread_session_object_index` failed in the current iteration. Then, `info->id` has been set to nonzero value due to (1), but `info->index` is set to `-1` due to (2). On the next `get_shared_desktop()` call, `try_lock_session_object()` will notice the nonzero id, and take the address of `objects[-1]` which is out-of-bounds. --- You have two ways to address this, depending on how you want to treat the `info` parameter: | Postcondition on failure | Changes to callee (`get_thread_session_object`) | Changes to caller (`get_shared_desktop`) | |--------------------------|----------------------------------------|----------| | `info` is clobbered | (No changes) | **Don't pass a persistent structure via the `info` parameter.** | | `info` is zeroed | **Zero `info` before returning `FALSE`.** | (No changes) | In general, if you want to handle errors gracefully, you have to do it seriously or not at all. It's essential to define a clear code contract[^1][^2] even if you're not putting it into words. If this seems like too much work, you should just crash on failure instead of introducing subtle failure modes. [^1]: [Design by contract - Wikipedia](<https://en.wikipedia.org/wiki/Design_by_contract>) [^2]: C# example: [Code Contracts - .NET Framework | Microsoft Learn](<https://learn.microsoft.com/en-us/dotnet/framework/debug-trace-profile/code-contracts>) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64975
Jinoh Kang (@iamahuman) commented about server/file.h:
+extern void free_shared_object( int index ); +extern const desktop_shm_t *get_shared_desktop( int index ); + +#define SHARED_WRITE_BEGIN( object, type ) \ + do { \ + const type *__shared = (object); \ + type *shared = (type *)__shared; \ + LONG64 __seq = shared->obj.seq + 1, __end = __seq + 1; \ + assert( (__seq & 1) != 0 ); \ + __WINE_ATOMIC_STORE_RELEASE( &shared->obj.seq, &__seq ); \ + do + +#define SHARED_WRITE_END \ + while(0); \ + assert( __seq == shared->obj.seq ); \ + __WINE_ATOMIC_STORE_RELEASE( &shared->obj.seq, &__end ); \ Minor nit, but is there any reason why the `\`s are not aligned at 4-space tabulation boundary? This padding has 2 extra spaces (mod 4). I think it's quite hard to align the `\`s with <kbd>Tab</kbd>.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64976
Jinoh Kang (@iamahuman) commented about dlls/win32u/win32u_private.h:
return !IsRectEmpty( dst ); }
+#if defined(__i386__) || defined(__x86_64__) +/* this prevents compilers from incorrectly reordering non-volatile reads (e.g., memcpy) from shared memory */ +#define __SHARED_READ_FENCE do { __asm__ __volatile__( "" ::: "memory" ); } while (0) +#else +#define __SHARED_READ_FENCE __atomic_thread_fence( __ATOMIC_ACQUIRE )
(nit) I think we no longer need spaces here. Also it's already 89 columns long. ```suggestion:-2+0 #define __SHARED_READ_FENCE do { __asm__ __volatile__( "" ::: "memory" ); } while (0) #else #define __SHARED_READ_FENCE __atomic_thread_fence( __ATOMIC_ACQUIRE ) ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64977
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
#pragma makedep unix #endif
+#include <stdarg.h> +#include <stddef.h> Is this needed?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64978
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
+ +struct shared_session +{ + LONG ref; + UINT64 id; + UINT object_capacity; + const session_shm_t *shared; +}; + +static pthread_mutex_t session_lock = PTHREAD_MUTEX_INITIALIZER; +static struct shared_session *shared_session; + +static struct session_thread_data *get_session_thread_data(void) +{ + struct user_thread_info *thread_info = get_user_thread_info(); + if (!thread_info->session_data) thread_info->session_data = calloc(1, sizeof(struct session_thread_data)); (nit) Repeated type not kept in sync: changing type name of `session_data` leads to allocation of different types (and possibly size).
```suggestion:-0+0 if (!thread_info->session_data) thread_info->session_data = calloc(1, sizeof(*thread_info->session_data)); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64979
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
+ return TRUE; +} + +void object_lock_release( struct object_lock *lock ) +{ + shared_session_release( lock->session ); + memset( lock, 0, sizeof(*lock) ); + lock->id = -1; /* force object to be refreshed when retrying lock */ +} + +enum object_type +{ + OBJECT_TYPE_DESKTOP = 1, +}; + +static UINT get_thread_session_object_index( UINT tid, enum object_type type, UINT64 *id ) (nit) `get index` is misleading since it returns the id as well.
```suggestion:-0+0 static UINT get_thread_session_object_location( UINT tid, enum object_type type, UINT64 *id ) ``` ```suggestion:-0+0 static UINT get_thread_session_object_loc( UINT tid, enum object_type type, UINT64 *id ) ``` ```suggestion:-0+0 static UINT get_thread_session_object_index_and_id( UINT tid, enum object_type type, UINT64 *id ) ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64980
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
+ '_','_','w','i','n','e','_','s','e','s','s','i','o','n',0 + }; + UNICODE_STRING name = RTL_CONSTANT_STRING( nameW ); + OBJECT_ATTRIBUTES attr; + unsigned int status; + HANDLE handle; + + if (!(session = calloc( 1, sizeof(*session) ))) + { + pthread_mutex_unlock( &session_lock ); + return NULL; + } + session->ref = 1; + + InitializeObjectAttributes( &attr, &name, 0, NULL, NULL ); + if (!(status = NtOpenSection( &handle, SECTION_MAP_READ | SECTION_QUERY, &attr ))) (nit) I don't think `SECTION_QUERY` is required for NtMapViewOfSection() to automatically infer the full size.
```suggestion:-0+0 if (!(status = NtOpenSection( &handle, SECTION_MAP_READ, &attr ))) ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64981
Jinoh Kang (@iamahuman) commented about server/winstation.c:
/* get the thread current desktop */ DECL_HANDLER(get_thread_desktop) { + struct desktop *desktop; struct thread *thread;
if (!(thread = get_thread_from_id( req->tid ))) return; reply->handle = thread->desktop; + reply->index = -1; + + if ((desktop = get_thread_desktop( thread, 0 ))) + { + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); + reply->index = desktop->session_index; + reply->object_id = desktop_shm->obj.id; (nit) extra spaces before `=`
```suggestion:-1+0 reply->index = desktop->session_index; reply->object_id = desktop_shm->obj.id; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64982
Jinoh Kang (@iamahuman) commented about server/queue.c:
const hw_input_t *input = &pointer->input; unsigned int i, wparam = input->hw.wparam; timeout_t time = get_tick_count(); + const desktop_shm_t *desktop_shm; user_handle_t win = pointer->win; rectangle_t top_rect; struct message *msg; int x, y;
+ desktop_shm = get_shared_desktop( desktop->session_index );
(minor nit) consistency ```suggestion:-6+0 const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); user_handle_t win = pointer->win; rectangle_t top_rect; struct message *msg; int x, y; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64983
Jinoh Kang (@iamahuman) commented about server/mapping.c:
static struct addr_range ranges32; static struct addr_range ranges64;
+struct session +{ + const session_shm_t *shared; + unsigned int object_count; (commits) This field wouldn't be used until commit 6f49aa2506688b901913f24a65adb2bd85bab264 "server: Allocate shared session object for desktops."
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64984
Jinoh Kang (@iamahuman) commented about server/winstation.c:
/* get the thread current desktop */ DECL_HANDLER(get_thread_desktop) { + struct desktop *desktop; struct thread *thread;
if (!(thread = get_thread_from_id( req->tid ))) return; reply->handle = thread->desktop; + reply->index = -1; + + if ((desktop = get_thread_desktop( thread, 0 ))) (commits) This function call sets error, which introduces behavioral change (and thus possibility of regression) of NtUserGetThreadDesktop().
The `get_thread_desktop()` call should thus be split into its own commit. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64985
Jinoh Kang (@iamahuman) commented about server/mapping.c:
SHARED_WRITE_END; }
+static int grow_session_mapping(void) +{ + unsigned int capacity; + mem_size_t size; + int unix_fd; + void *tmp; + + capacity = session.shared->object_capacity * 3 / 2; + size = offsetof(session_shm_t, objects[capacity]); + size = (size + page_mask) & ~((mem_size_t)page_mask); +
`grow_file` corrupts the file if we the size decreases, so other callers of `grow_file()` explicitly guards against this condition. I think we should do it here too: ```suggestion:-0+0 size = max(size, session_mapping->size); ``` Note that the "size ↔ capacity" translation is not a round-trip, and going from capacity back to size may actually result in the size decreasing if the element size becomes sufficiently greater than page size (plus certain overhead accounting for existing mapping size): $$ \newcommand{\NP}{\mathsf{NewPages}} \newcommand{\OP}{\mathsf{OldPages}} \newcommand{\Ps}{\mathsf{PageSize}} \newcommand{\Hs}{\mathsf{HeadSize}} \newcommand{\Es}{\mathsf{EltSize}} \newcommand{\ceil}[1]{\mathopen{}\left\lceil#1\right\rceil\mathclose{}} \newcommand{\floor}[1]{\mathopen{}\left\lfloor#1\right\rfloor\mathclose{}} \newcommand{\paren}[1]{\mathopen{}\left(#1\right)\mathclose{}} \displaystyle \\[1ex] \begin{align*} \NP&=\ceil{\paren{\tfrac32\floor{{\paren{\OP\cdot\Ps - \Hs}}/{\Es}}\cdot\Es + \Hs}/{\Ps}}\\ &=\ceil{\paren{\tfrac32\paren{{\paren{\OP\cdot\Ps - \Hs}}/{\Es} - k}\cdot\Es + \Hs}/{\Ps}}\\ &=\ceil{\OP - \paren{k\cdot\Es - \tfrac13\paren{\OP\cdot\Ps - \Hs}}/\paren{\tfrac23\cdot\Ps}}\\ &<\OP \end{align*} $$ where $$ \newcommand{\NP}{\mathsf{NewPages}} \newcommand{\OP}{\mathsf{OldPages}} \newcommand{\Ps}{\mathsf{PageSize}} \newcommand{\Hs}{\mathsf{HeadSize}} \newcommand{\Es}{\mathsf{EltSize}} \begin{align*} \mathbb{N}\ni\NP&=\text{New \texttt{session\_mapping->size / page\_size}}\\ \mathbb{N}\ni\OP&=\text{Old \texttt{session\_mapping->size / page\_size}}\\ \mathbb{N}\ni\Ps&=\text{\texttt{page\_size}}>0\\ \mathbb{N}\ni\Hs&=\text{\texttt{offsetof(session\_shm\_t, objects[0])}}>0\\ \mathbb{N}\ni\Es&=\text{\texttt{sizeof(session\_obj\_t)}}>0\\ 0&\leq k< 1&\text{(truncated fractional part)}\\ k\cdot\Es &> \tfrac23\cdot\Ps + \tfrac13\left(\OP\cdot\Ps - \Hs\right) \\ \left\lfloor\cdot\right\rfloor&=\mathrm{floor}\!\left(\cdot\right)\\ \left\lceil\cdot\right\rceil&=\mathrm{ceil}\!\left(\cdot\right)\\ \end{align*} $$ -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64986
Jinoh Kang (@iamahuman) commented about server/mapping.c:
+ { + for (index = 0; index < session.object_count; index++) + if (!session.shared->objects[index].obj.id) + break; + if (index == session.object_count) + { + if (grow_session_mapping()) return -1; + index = session.object_count++; + } + } + + SHARED_WRITE_BEGIN( &session.shared->objects[index], session_obj_t ) + { + shared->obj.id = ++session.last_object_id; + } + SHARED_WRITE_END; This will scan the entire objects array once the last index has been occupied (object\_count full).
Instead, how about replacing `object_count` with `last_object_index`? This field doesn't actually count valid objects anyway, since some objects in the middle could be freed later. Instead, I would keep track of the *last* object index and start from there. ```suggestion:-20+0 unsigned int index = session.last_object_index; for (;;) { index = (index + 1) % session.shared->object_capacity; if (!session.shared->objects[index].obj.id) { /* found a free index */ break; } /* check if we're back to the starting point */ if (index == session.last_object_index) { /* all slots exhausted; skip to first index to be allocated */ index = session.shared->object_capacity; break; } } while (index >= session.shared->object_capacity) { if (grow_session_mapping()) return -1; } session.last_object_index = index; SHARED_WRITE_BEGIN( &session.shared->objects[index], session_obj_t ) { assert(!shared->obj.id); shared->obj.id = ++session.last_object_id; } SHARED_WRITE_END; ``` Note that the following initialization is also required: ```c session.last_object_index = -1; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64987
Additionally, I think the current session tracking logic can be greatly simplified by removing the id: see https://gitlab.winehq.org/iamahuman/wine/-/commit/f6697367fb1447c5508b6ec990... (untested). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64988
Remaining nits (incomplete) are at https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/9. I might eventually submit them here (or not). I thought it would be nice to let you know. You don't need to fix them right now though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64991
On Sat Mar 16 09:57:58 2024 +0000, Jinoh Kang wrote:
`grow_file` corrupts the file if we the size decreases, so other callers of `grow_file()` explicitly guards against this condition. I think we should do it here too: ```suggestion:-0+0 assert(size > session_mapping->size); ``` Note that the "size ↔ capacity" translation is not a round-trip, and going from capacity back to size may actually result in the size decreasing if the element size becomes sufficiently greater than page size (plus certain overhead accounting for existing mapping size): $$ \newcommand{\NP}{\mathsf{NewPages}} \newcommand{\OP}{\mathsf{OldPages}} \newcommand{\Ps}{\mathsf{PageSize}} \newcommand{\Hs}{\mathsf{HeadSize}} \newcommand{\Es}{\mathsf{EltSize}} \newcommand{\ceil}[1]{\mathopen{}\left\lceil#1\right\rceil\mathclose{}} \newcommand{\floor}[1]{\mathopen{}\left\lfloor#1\right\rfloor\mathclose{}} \newcommand{\paren}[1]{\mathopen{}\left(#1\right)\mathclose{}} \displaystyle \\[1ex] \begin{align*} \NP&=\ceil{\paren{\tfrac32\floor{{\paren{\OP\cdot\Ps - \Hs}}/{\Es}}\cdot\Es + \Hs}/{\Ps}}\\ &=\ceil{\paren{\tfrac32\paren{{\paren{\OP\cdot\Ps - \Hs}}/{\Es} - k}\cdot\Es + \Hs}/{\Ps}}\\ &=\ceil{\OP - \paren{k\cdot\Es - \tfrac13\paren{\OP\cdot\Ps - \Hs}}/\paren{\tfrac23\cdot\Ps}}\\ &<\OP \end{align*} $$ where $$ \newcommand{\NP}{\mathsf{NewPages}} \newcommand{\OP}{\mathsf{OldPages}} \newcommand{\Ps}{\mathsf{PageSize}} \newcommand{\Hs}{\mathsf{HeadSize}} \newcommand{\Es}{\mathsf{EltSize}} \begin{align*} \mathbb{N}\ni\NP&=\text{New \texttt{session\_mapping->size / page\_size}}\\ \mathbb{N}\ni\OP&=\text{Old \texttt{session\_mapping->size / page\_size}}\\ \mathbb{N}\ni\Ps&=\text{\texttt{page\_size}}>0\\ \mathbb{N}\ni\Hs&=\text{\texttt{offsetof(session\_shm\_t, objects[0])}}>0\\ \mathbb{N}\ni\Es&=\text{\texttt{sizeof(session\_obj\_t)}}>0\\ 0&\leq k< 1&\text{(truncated fractional part)}\\ k\cdot\Es &> \tfrac23\cdot\Ps + \tfrac13\left(\OP\cdot\Ps - \Hs\right) \\ \left\lfloor\cdot\right\rfloor&=\mathrm{floor}\!\left(\cdot\right)\\ \left\lceil\cdot\right\rceil&=\mathrm{ceil}\!\left(\cdot\right)\\ \end{align*} $$ I don't think this holds if we can guarantee capacity ≥ 3:
*E = EltSize, P = PageSize, O = OldPages, H = HeadSize, C = (Old)Capacity* ``` C > 0 in N O = ⌈(C⋅E + H) / P⌉ ≥ C⋅E + H / P ``` Back to your condition for this: ``` k⋅E > 2/3⋅P + 1/3⋅(O⋅P - H) ⇔ 3⋅k⋅E > 2⋅P + O⋅P - H ⇒ 3⋅k⋅E > 2⋅P + C⋅E ⇒ 3⋅k > C ⇒ 3 > C ``` Which means that `C ≥ 3 ⇒ k⋅E ≤ 2/3⋅P + 1/3⋅(O⋅P - H)`, and then we're safe. I think it's unlikely to ever not be the case and we would then have bigger issues, but I'll change the initial size to depend on the element size, and enforce at least 3 elements.
so other callers of `grow_file()` explicitly guards against this condition.
I don't see what you mean by that, where is this guarded exactly? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_64997
participants (2)
-
Jinoh Kang (＠iamahuman) -
Rémi Bernon