[PATCH v22 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. -- v22: 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 fa78e0487f8..2a3c25b7892 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -44,6 +44,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 @@ -879,6 +880,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 93d0a78e344..42aef90de84 100644 --- a/include/winnt.h +++ b/include/winnt.h @@ -7082,11 +7082,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 */ @@ -7120,6 +7123,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(); @@ -7306,6 +7315,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 d5f010a8249..f7d25ffc6f3 100644 --- a/dlls/win32u/win32u_private.h +++ b/dlls/win32u/win32u_private.h @@ -362,4 +362,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..3baf0a8a9a8 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; + BOOL valid; + + while (!(status = NtMapViewOfSection( handle, GetCurrentProcess(), (void **)&session->shared, 0, 0, + NULL, &size, ViewUnmap, 0, PAGE_READONLY ))) + { + session->object_capacity = (size - offsetof(session_shm_t, objects[0])) / sizeof(session_obj_t); + SHARED_READ_BEGIN( session->shared, session_shm_t ) + { + if ((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 2a3c25b7892..dcbdf23bb3f 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -889,9 +889,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 @@ -2810,6 +2816,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 b4cf618f87e..599ffd68c19 100644 --- a/server/user.h +++ b/server/user.h @@ -80,6 +80,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 373a1f7ba92..4de5dd69039 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -296,6 +296,13 @@ static struct desktop *create_desktop( const struct unicode_str *name, unsigned memset( desktop->keystate, 0, sizeof(desktop->keystate) ); list_add_tail( &winstation->desktops, &desktop->entry ); list_init( &desktop->hotkeys ); + desktop->session_index = -1; + + if ((desktop->session_index = alloc_shared_object()) == -1) + { + release_object( desktop ); + return NULL; + } } else { @@ -363,6 +370,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 */ @@ -726,10 +734,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 a31d586a5b6..60c9f9d4177 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -6262,6 +6262,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 f7d25ffc6f3..7015cc73a5d 100644 --- a/dlls/win32u/win32u_private.h +++ b/dlls/win32u/win32u_private.h @@ -194,6 +194,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 3baf0a8a9a8..434bc9e48b8 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 (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 | 73 ++++++++++++++++++++++++++++----------------- server/user.h | 2 -- 3 files changed, 52 insertions(+), 30 deletions(-) diff --git a/server/protocol.def b/server/protocol.def index dcbdf23bb3f..f95515ca154 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -883,6 +883,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 */ @@ -892,6 +898,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 4373bd71254..cf23628e77d 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 ); @@ -1666,6 +1676,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; @@ -1698,8 +1709,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 ))) { @@ -1981,6 +1992,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; @@ -2019,19 +2031,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 ))) @@ -2042,7 +2054,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 ); @@ -2220,6 +2232,7 @@ static int queue_keyboard_message( struct desktop *desktop, user_handle_t win, c 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 message *msg; @@ -2252,8 +2265,8 @@ static void queue_custom_hardware_message( struct desktop *desktop, user_handle_ msg->msg = input->hw.msg; msg->wparam = 0; 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 ); } @@ -2789,6 +2802,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 = get_current_queue(); + const desktop_shm_t *desktop_shm; if (!(desktop = get_hardware_input_desktop( req->win ))) return; if ((origin == IMO_INJECTED && desktop != current->queue->input->desktop) || @@ -2798,9 +2812,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) { @@ -2817,8 +2832,8 @@ DECL_HANDLER(send_hardware_message) set_error( STATUS_INVALID_PARAMETER ); } - 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 ); } @@ -3503,15 +3518,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) { @@ -3534,8 +3551,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 599ffd68c19..dd29b1b1be5 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 f95515ca154..5701aa3cc91 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -887,6 +887,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 cf23628e77d..d829e97b420 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; @@ -1998,7 +1998,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; @@ -2019,10 +2019,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) { @@ -3554,7 +3559,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 dd29b1b1be5..dc64b9c6003 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 | 23 +++++++++++++++-------- dlls/win32u/winstation.c | 2 -- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/dlls/win32u/input.c b/dlls/win32u/input.c index ef8d564c264..7047aa34384 100644 --- a/dlls/win32u/input.c +++ b/dlls/win32u/input.c @@ -749,22 +749,29 @@ BOOL WINAPI NtUserSetCursorPos( INT x, INT y ) */ BOOL get_cursor_pos( POINT *pt ) { - BOOL ret; - DWORD last_change; + struct object_lock lock = {0}; + BOOL valid, 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 ))) + SHARED_READ_BEGIN( &lock.shared->desktop, desktop_shm_t ) { - pt->x = reply->new_x; - pt->y = reply->new_y; - last_change = reply->last_change; + if ((valid = lock.id == shared->obj.id)) + { + pt->x = shared->cursor.x; + pt->y = shared->cursor.y; + last_change = shared->cursor.last_change; + ret = TRUE; + } } + SHARED_READ_END; + object_lock_release( &lock ); + if (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 434bc9e48b8..09185586bdc 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
On Fri Mar 1 13:46:47 2024 +0000, Jinoh Kang wrote:
I didn't see why this would be necessary as the mapping is created during server init as a permanent object. Not a bug, but conceptually confusing. Thanks anyway :3 There are other cases like that, such as the root directory, `root_directory` assignment is in the caller side of `create_directory`; the function itself doesn't keep a reference. Meanwhile we both assign the pointer *and* return the pointer, hence two references. If we want to go `root_directory` way, it should have been:
session_mapping = create_session_mapping( ... );the user shared mapping In this case, we only store the `mmap` address (which is managed independently), not the object pointer itself. So I don't think this is applicable as a precedent either. I changed this to add a separate set_session_mapping, though I don't think it changes much. I'm still not keeping a reference because it messes up with the leak checker which reports a leaked reference.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_63280
On Sat Mar 2 16:25:42 2024 +0000, Rémi Bernon wrote:
changed this line in [version 22 of the diff](/wine/wine/-/merge_requests/3103/diffs?diff_id=102762&start_sha=9d6950a2219773c11210af961c8de9827ae32dab#35a0b3c4868b938764504464c7e3891ad9581e77_71_71) Used a different and simpler locking pattern instead like we discussed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_63286
On Sat Mar 2 16:25:45 2024 +0000, Rémi Bernon wrote:
changed this line in [version 22 of the diff](/wine/wine/-/merge_requests/3103/diffs?diff_id=102762&start_sha=9d6950a2219773c11210af961c8de9827ae32dab#35a0b3c4868b938764504464c7e3891ad9581e77_248_234) Done.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_63287
Addressed the last round of comments I think. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_63289
On Sat Mar 2 03:49:53 2024 +0000, Jinoh Kang wrote:
1. Ensure `session_mapping->size == session.shared.size` at all times, since we rely on it in the `map_shared_session_section` loop. - Note that we don't have to ensure `backing_file[st_size] == session_mapping->size`. It's normal for a section to only have a portion of the backing file. 2. Consistently use `mapping->size` as we've done in `create_session_mapping`. ```suggestion:-4+0 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; ``` Done.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_63290
On Sat Mar 2 16:26:14 2024 +0000, Rémi Bernon wrote:
changed this line in [version 22 of the diff](/wine/wine/-/merge_requests/3103/diffs?diff_id=102762&start_sha=9d6950a2219773c11210af961c8de9827ae32dab#35a0b3c4868b938764504464c7e3891ad9581e77_106_101) Done.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_63291
On Sat Mar 2 16:26:04 2024 +0000, Rémi Bernon wrote:
changed this line in [version 22 of the diff](/wine/wine/-/merge_requests/3103/diffs?diff_id=102762&start_sha=9d6950a2219773c11210af961c8de9827ae32dab#2b37d5dc1c3bb6a637f043480c038caea64531bb_1346_1338) Done.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_63292
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
+{ + 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 ); + } We have an out-of-bounds `info->index` left here. If the next iteration's `get_shared_session()` fails, we won't be able to detect this condition; instead, we will return the invalid index (as well as a NULL session) as-is to the caller.
To avoid this, ensure `info->index` is set to the sentinel when the while condition fails... ```suggestion:-1+0 shared_session_release( session ); info->index = -1; } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_63374
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 (info->index == -1) ...or alternatively, use `valid` here:
```suggestion:-0+0 if (valid && info->index == -1) ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_63375
Jinoh Kang (@iamahuman) commented about dlls/win32u/input.c:
- SERVER_START_REQ( set_cursor ) + while (get_shared_desktop( &lock )) { - if ((ret = !wine_server_call( req ))) + SHARED_READ_BEGIN( &lock.shared->desktop, desktop_shm_t ) { - pt->x = reply->new_x; - pt->y = reply->new_y; - last_change = reply->last_change; + if ((valid = lock.id == shared->obj.id)) + { + pt->x = shared->cursor.x; + pt->y = shared->cursor.y; + last_change = shared->cursor.last_change; + ret = TRUE;
If the current shared-read iteration fails due to destroyed object, `ret = TRUE` will live on even if the next iteration sets `valid = FALSE`. This is kept as-is if `get_shared_desktop()` returned NULL. In general, the number of shared read retries shouldn't affect the outcome of the loop. This means that any variables *conditionally*[^1] written by the read critical section should be considered unknown / garbage.[^2] [^1]: `valid` is unconditionally written. [^2]: Rule of thumb: a read section should *always* write to a variable or not at all. If a variable is ever written, it should be read *only after* it has been written in the current iteration. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_63376
Jinoh Kang (@iamahuman) commented about dlls/win32u/input.c:
- SERVER_START_REQ( set_cursor ) + while (get_shared_desktop( &lock )) { - if ((ret = !wine_server_call( req ))) + SHARED_READ_BEGIN( &lock.shared->desktop, desktop_shm_t ) { - pt->x = reply->new_x; - pt->y = reply->new_y; - last_change = reply->last_change; + if ((valid = lock.id == shared->obj.id)) + { + pt->x = shared->cursor.x; + pt->y = shared->cursor.y; + last_change = shared->cursor.last_change; + ret = TRUE; + } Solution: For the reasons above, I think we should just abolish `if()`s that depend on shared memory value.[^1]
```suggestion:-6+0 valid = lock.id == shared->obj.id; pt->x = shared->cursor.x; pt->y = shared->cursor.y; last_change = shared->cursor.last_change; ``` [^1]: Previously, I thought `if()`s were more or less harmless in seqlocks if used carefully. Now that I think about it, the shared object can get invalid *at any time* and turn into another object type (e.g., `valid` might be set to TRUE but then subsequent reads like `shared->cursor.x` might return inconsistent value, before the shared read loop detects seq change and retries). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_63377
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
+} + +static NTSTATUS map_shared_session_section( struct shared_session *session, HANDLE handle ) +{ + NTSTATUS status; + SIZE_T size = 0; + BOOL valid; + + while (!(status = NtMapViewOfSection( handle, GetCurrentProcess(), (void **)&session->shared, 0, 0, + NULL, &size, ViewUnmap, 0, PAGE_READONLY ))) + { + session->object_capacity = (size - offsetof(session_shm_t, objects[0])) / sizeof(session_obj_t); + SHARED_READ_BEGIN( session->shared, session_shm_t ) + { + if ((valid = session->object_capacity == shared->object_capacity)) + session->id = shared->obj.id; Ditto
```suggestion:-1+0 valid = session->object_capacity == shared->object_capacity; session->id = shared->obj.id; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_63378
Interim review (not fully reviewed yet; more comments expected) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_63379
participants (2)
-
Jinoh Kang (＠iamahuman) -
Rémi Bernon