[PATCH v30 0/9] 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. -- v30: 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: Return the desktop object info in get_thread_desktop. 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 | 30 ++++++++++++++++++++++++++++++ server/protocol.def | 24 ++++++++++++++++++++++++ tools/make_requests | 1 + 5 files changed, 81 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..3a288f0a212 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..011bccf478e 100644 --- a/server/mapping.c +++ b/server/mapping.c @@ -225,6 +225,13 @@ 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; +}; +static struct mapping *session_mapping; +static struct session session; + #define ROUND_SIZE(size) (((size) + page_mask) & ~page_mask) void init_memory(void) @@ -1256,6 +1263,29 @@ 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; + mem_size_t size = max( offsetof( session_shm_t, objects[512] ), 0x10000 ); + + return create_mapping( root, name, attr, size, 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 = -1; /* use the last valid object id for the session */ + 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 55dda2d7636..3b44ef2af08 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 e7c322fd127..5ea908c97ad 100644 --- a/include/winnt.h +++ b/include/winnt.h @@ -7059,11 +7059,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 */ @@ -7097,6 +7100,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(); @@ -7283,6 +7292,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/winstation.c | 149 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 148 insertions(+), 1 deletion(-) diff --git a/dlls/win32u/winstation.c b/dlls/win32u/winstation.c index 6ddd5411f94..202bdbfe7df 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,146 @@ WINE_DECLARE_DEBUG_CHANNEL(win); #define DESKTOP_ALL_ACCESS 0x01ff +struct shared_session +{ + LONG ref; + UINT object_capacity; + const session_shm_t *shared; +}; + +static pthread_mutex_t session_lock = PTHREAD_MUTEX_INITIALIZER; +static struct shared_session *shared_session; + +#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 + +static void object_shm_acquire_seqlock( const object_shm_t *shared, UINT64 *seq ) +{ + while ((*seq = ReadNoFence64( &shared->seq )) & 1) YieldProcessor(); + __SHARED_READ_FENCE; +} + +static BOOL object_shm_release_seqlock( const object_shm_t *shared, UINT64 seq ) +{ + __SHARED_READ_FENCE; + return ReadNoFence64( &shared->seq ) == seq; +} + +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 BOOL shared_session_has_capacity( struct shared_session *session, SIZE_T capacity ) +{ + TRACE( "session %p, capacity 0x%s\n", session, wine_dbgstr_longlong(capacity) ); + + for (;;) + { + const session_shm_t *session_shm = session->shared; + UINT64 seq; + BOOL ret; + + object_shm_acquire_seqlock( &session_shm->obj, &seq ); + ret = session_shm->object_capacity == capacity; + if (object_shm_release_seqlock( &session_shm->obj, seq )) return ret; + } +} + +static NTSTATUS map_shared_session_section( struct shared_session *session, HANDLE handle ) +{ + NTSTATUS status; + SIZE_T size = 0; + + TRACE( "session %p, handle %p\n", session, handle ); + + while (!(status = NtMapViewOfSection( handle, GetCurrentProcess(), (void **)&session->shared, 0, 0, + NULL, &size, ViewUnmap, 0, PAGE_READONLY ))) + { + SIZE_T capacity; + + capacity = (size - offsetof(session_shm_t, objects[0])) / sizeof(session_obj_t); + if (shared_session_has_capacity( session, capacity )) + { + session->object_capacity = capacity; + 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; + + TRACE( "force %u\n", force ); + + 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) ))) goto done; + session->ref = 1; + + InitializeObjectAttributes( &attr, &name, 0, NULL, NULL ); + if (!(status = NtOpenSection( &handle, SECTION_MAP_READ, &attr ))) + { + status = map_shared_session_section( session, handle ); + NtClose( handle ); + } + + if (status) + { + ERR( "Failed to map session mapping, status %#x\n", status ); + free( session ); + session = NULL; + goto done; + } + + if (shared_session) shared_session_release( shared_session ); + shared_session = session; + } + + session = shared_session_acquire( shared_session ); + +done: + pthread_mutex_unlock( &session_lock ); + + return session; +} + BOOL is_virtual_desktop(void) { HANDLE desktop = NtUserGetThreadDesktop( GetCurrentThreadId() ); @@ -630,12 +773,16 @@ void winstation_init(void) { RTL_USER_PROCESS_PARAMETERS *params = NtCurrentTeb()->Peb->ProcessParameters; WCHAR *winstation = NULL, *desktop = NULL, *buffer = NULL; + struct shared_session *session; HANDLE handle, dir = NULL; OBJECT_ATTRIBUTES attr; UNICODE_STRING str; static const WCHAR winsta0[] = {'W','i','n','S','t','a','0',0}; + if ((session = get_shared_session( FALSE ))) + shared_session_release( session ); + 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 | 80 +++++++++++++++++++++++++++++++++++++++++++++ server/object.c | 13 +++++++- server/object.h | 2 ++ server/protocol.def | 6 ++++ server/user.h | 1 + server/winstation.c | 8 +++++ 7 files changed, 113 insertions(+), 1 deletion(-) diff --git a/server/file.h b/server/file.h index 3a288f0a212..5380e1a6430 100644 --- a/server/file.h +++ b/server/file.h @@ -192,6 +192,10 @@ extern struct mapping *create_session_mapping( struct object *root, const struct unsigned int attr, const struct security_descriptor *sd ); 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 { \ diff --git a/server/mapping.c b/server/mapping.c index 011bccf478e..dbaa0ec5a20 100644 --- a/server/mapping.c +++ b/server/mapping.c @@ -228,6 +228,8 @@ static struct addr_range ranges64; struct session { const session_shm_t *shared; + mem_size_t next_object_index; /* next index to try, may be greater than capacity or already occupied. */ + object_id_t last_object_id; }; static struct mapping *session_mapping; static struct session session; @@ -1286,6 +1288,84 @@ 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; + assert( capacity > session.shared->object_capacity ); + 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) + { + file_set_error(); + 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->object_capacity = (size - offsetof(session_shm_t, objects[0])) / sizeof(session_obj_t); + } + SHARED_WRITE_END; + + return 0; +} + +int alloc_shared_object(void) +{ + mem_size_t index, offset = session.next_object_index, capacity = session.shared->object_capacity; + + for (index = offset; index != offset + capacity; index++) + if (!session.shared->objects[index % capacity].obj.id) + break; + if (index != offset + capacity) index %= capacity; + else + { + if (grow_session_mapping()) return -1; + index = capacity; + } + + SHARED_WRITE_BEGIN( &session.shared->objects[index], session_obj_t ) + { + /* mark the object data as uninitialized */ + mark_block_uninitialized( (void *)(&shared->obj + 1), sizeof(*shared) - sizeof(object_shm_t) ); + shared->obj.id = ++session.last_object_id; + } + SHARED_WRITE_END; + session.next_object_index = index + 1; + + return index; +} + +void free_shared_object( int index ) +{ + if (index < 0) return; + + SHARED_WRITE_BEGIN( &session.shared->objects[index], session_obj_t ) + { + /* mark the object data as innaccessible */ + mark_block_noaccess( (void *)(&shared->obj + 1), sizeof(*shared) - sizeof(object_shm_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/object.c b/server/object.c index 89e541ffb6b..9204593cb6c 100644 --- a/server/object.c +++ b/server/object.c @@ -102,8 +102,19 @@ void close_objects(void) /*****************************************************************/ +/* mark a block of memory as not accessible for debugging purposes */ +void mark_block_noaccess( void *ptr, size_t size ) +{ + memset( ptr, 0xfe, size ); +#if defined(VALGRIND_MAKE_MEM_NOACCESS) + VALGRIND_DISCARD( VALGRIND_MAKE_MEM_NOACCESS( ptr, size ) ); +#elif defined(VALGRIND_MAKE_NOACCESS) + VALGRIND_DISCARD( VALGRIND_MAKE_NOACCESS( ptr, size ) ); +#endif +} + /* mark a block of memory as uninitialized for debugging purposes */ -static inline void mark_block_uninitialized( void *ptr, size_t size ) +void mark_block_uninitialized( void *ptr, size_t size ) { memset( ptr, 0x55, size ); #if defined(VALGRIND_MAKE_MEM_UNDEFINED) diff --git a/server/object.h b/server/object.h index d4d66536b81..2337ee88231 100644 --- a/server/object.h +++ b/server/object.h @@ -139,6 +139,8 @@ struct wait_queue_entry struct thread_wait *wait; }; +extern void mark_block_noaccess( void *ptr, size_t size ); +extern void mark_block_uninitialized( void *ptr, size_t size ); extern void *mem_alloc( size_t size ) __WINE_ALLOC_SIZE(1) __WINE_DEALLOC(free) __WINE_MALLOC; extern void *memdup( const void *data, size_t len ) __WINE_ALLOC_SIZE(2) __WINE_DEALLOC(free); extern void *alloc_object( const struct object_ops *ops ); diff --git a/server/protocol.def b/server/protocol.def index 3b44ef2af08..edfdd9621ff 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 diff --git a/server/user.h b/server/user.h index d805a179d16..f1652faec9a 100644 --- a/server/user.h +++ b/server/user.h @@ -82,6 +82,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 80126ad5d60..167ac8aeb62 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 = alloc_shared_object(); + + if (!get_shared_desktop( desktop->session_index )) + { + release_object( desktop ); + return NULL; + } } else { @@ -366,6 +373,7 @@ static void desktop_destroy( struct object *obj ) if (desktop->global_hooks) release_object( desktop->global_hooks ); if (desktop->close_timeout) remove_timeout_user( desktop->close_timeout ); release_object( desktop->winstation ); + free_shared_object( desktop->session_index ); } /* retrieve the thread desktop, checking the handle access rights */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
From: Rémi Bernon <rbernon(a)codeweavers.com> --- server/protocol.def | 2 ++ server/winstation.c | 12 ++++++++++++ 2 files changed, 14 insertions(+) diff --git a/server/protocol.def b/server/protocol.def index edfdd9621ff..509d0c95aee 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -2815,6 +2815,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/winstation.c b/server/winstation.c index 167ac8aeb62..df87de758c2 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -741,10 +741,22 @@ 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 ))) clear_error(); + else + { + 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 | 18 +++++ dlls/win32u/winstation.c | 133 +++++++++++++++++++++++++++++++++-- 4 files changed, 149 insertions(+), 4 deletions(-) diff --git a/dlls/win32u/ntuser_private.h b/dlls/win32u/ntuser_private.h index bb2169998b6..c0027f44c2f 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 d5ad97a2c33..411fbb4a381 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -6228,6 +6228,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 0af6960a3eb..e72a68230ed 100644 --- a/dlls/win32u/win32u_private.h +++ b/dlls/win32u/win32u_private.h @@ -193,6 +193,24 @@ extern void user_unlock(void); extern void user_check_not_lock(void); /* winstation.c */ + +struct shared_session; + +struct object_lock +{ + UINT64 id; + UINT64 seq; + struct shared_session *session; /* only valid when locked */ +}; + +/* Get shared session objects data pointer, must be called in a loop while STATUS_PENDING + * is returned, lock must be zero initialized. + * + * The data read from the objects may be transient and no logic should be executed based + * on it, within the loop, or after, unless the function has returned STATUS_SUCCESS. + */ +extern NTSTATUS get_shared_desktop( struct object_lock *lock, const desktop_shm_t **desktop_shm ); + extern BOOL is_virtual_desktop(void); /* window.c */ diff --git a/dlls/win32u/winstation.c b/dlls/win32u/winstation.c index 202bdbfe7df..2a41692751a 100644 --- a/dlls/win32u/winstation.c +++ b/dlls/win32u/winstation.c @@ -24,6 +24,8 @@ #include <stdarg.h> #include <stddef.h> + +#include <assert.h> #include <pthread.h> #include "ntstatus.h" @@ -43,6 +45,17 @@ WINE_DECLARE_DEBUG_CHANNEL(win); #define DESKTOP_ALL_ACCESS 0x01ff +struct object_info +{ + UINT64 id; + UINT index; +}; + +struct session_thread_data +{ + struct object_info shared_desktop; /* thread desktop shared session object info */ +}; + struct shared_session { LONG ref; @@ -53,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(*thread_info->session_data)); + return thread_info->session_data; +} + #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) @@ -183,6 +203,106 @@ done: return session; } +enum object_type +{ + OBJECT_TYPE_DESKTOP = 1, +}; + +static NTSTATUS get_thread_session_object_info( UINT tid, enum object_type type, + struct object_info *info ) +{ + NTSTATUS status; + + TRACE( "tid %04x, type %u, info %p\n", tid, type, info ); + + switch (type) + { + case OBJECT_TYPE_DESKTOP: + SERVER_START_REQ( get_thread_desktop ) + { + req->tid = tid; + if (!(status = wine_server_call( req ))) + { + info->id = reply->object_id; + info->index = reply->index; + } + if (info->index == -1) status = STATUS_INVALID_HANDLE; + } + SERVER_END_REQ; + break; + default: + ERR( "Invalid session object type %u\n", type ); + return STATUS_INVALID_PARAMETER; + } + + return status; +} + +/* return a locked session object for a thread id and type */ +static NTSTATUS get_thread_session_object( UINT tid, enum object_type type, struct object_info *info, + struct object_lock *lock, const object_shm_t **object_shm ) +{ + struct shared_session *session; + NTSTATUS status; + BOOL valid; + + assert( !lock->id || *object_shm ); + + TRACE( "tid %04x, type %u, info %p, lock %p, shared %p\n", tid, type, info, lock, object_shm ); + + if (lock->id) + { + /* lock was previously acquired, finish reading and check data consistency */ + const object_shm_t *object = *object_shm; + valid = lock->id == object->id; + + if (!object_shm_release_seqlock( object, lock->seq )) + { + /* retry if the seqlock doesn't match, wineserver has written to it */ + object_shm_acquire_seqlock( object, &lock->seq ); + return STATUS_PENDING; + } + + shared_session_release( lock->session ); + memset( lock, 0, sizeof(*lock) ); + + if (valid) return STATUS_SUCCESS; + info->id = 0; /* invalidate info if the lock was abandoned due to id mismatch */ + } + + valid = TRUE; /* assume the session is valid at first */ + while ((session = get_shared_session( !valid ))) + { + if (!info->id && (status = get_thread_session_object_info( tid, type, info ))) break; + if ((valid = info->index < session->object_capacity)) + { + lock->id = info->id; + lock->session = session; + *object_shm = &session->shared->objects[info->index].obj; + object_shm_acquire_seqlock( *object_shm, &lock->seq ); + return STATUS_PENDING; + } + shared_session_release( session ); + memset( info, 0, sizeof(*info) ); + } + + WARN( "Failed to find object type %u for thread %04x\n", type, tid ); + if (session) shared_session_release( session ); + memset( info, 0, sizeof(*info) ); + return status; +} + +NTSTATUS get_shared_desktop( struct object_lock *lock, const desktop_shm_t **desktop_shm ) +{ + struct session_thread_data *data = get_session_thread_data(); + struct object_info *info = &data->shared_desktop; + + TRACE( "lock %p, desktop_shm %p\n", lock, desktop_shm ); + + return get_thread_session_object( GetCurrentThreadId(), OBJECT_TYPE_DESKTOP, info, + lock, (const object_shm_t **)desktop_shm ); +} + BOOL is_virtual_desktop(void) { HANDLE desktop = NtUserGetThreadDesktop( GetCurrentThreadId() ); @@ -404,9 +524,18 @@ 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_info *desktop_info = &get_session_thread_data()->shared_desktop; + const desktop_shm_t *desktop_shm; + 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( desktop_info, 0, sizeof(*desktop_info) ); + + while (get_shared_desktop( &lock, &desktop_shm ) == STATUS_PENDING) + /* nothing */; + if (was_virtual_desktop != is_virtual_desktop()) update_display_cache( TRUE ); } return ret; @@ -773,16 +902,12 @@ void winstation_init(void) { RTL_USER_PROCESS_PARAMETERS *params = NtCurrentTeb()->Peb->ProcessParameters; WCHAR *winstation = NULL, *desktop = NULL, *buffer = NULL; - struct shared_session *session; HANDLE handle, dir = NULL; OBJECT_ATTRIBUTES attr; UNICODE_STRING str; static const WCHAR winsta0[] = {'W','i','n','S','t','a','0',0}; - if ((session = get_shared_session( FALSE ))) - shared_session_release( session ); - 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> Based on a patch by Huw Davies <huw(a)codeweavers.com>. --- server/protocol.def | 7 ++++ server/queue.c | 78 ++++++++++++++++++++++++++++----------------- server/user.h | 2 -- server/winstation.c | 11 ++++++- 4 files changed, 65 insertions(+), 33 deletions(-) diff --git a/server/protocol.def b/server/protocol.def index 509d0c95aee..e6a770d8c6d 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 ed099b3b989..3780e35d83e 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; unsigned int old_flags; int x, y; @@ -547,9 +557,9 @@ void set_clip_rectangle( struct desktop *desktop, const rectangle_t *rect, unsig desktop->cursor.clip_flags = flags; /* 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 ); @@ -1673,6 +1683,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; @@ -1705,8 +1716,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 ))) { @@ -1988,6 +1999,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; @@ -2026,19 +2038,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 ))) @@ -2049,7 +2061,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 ); @@ -2251,6 +2263,7 @@ static void queue_pointer_message( struct pointer *pointer, int repeated ) }; struct hw_msg_source source = { IMDT_UNAVAILABLE, IMDT_TOUCH }; struct desktop *desktop = pointer->desktop; + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); const hw_input_t *input = &pointer->input; unsigned int i, wparam = input->hw.wparam; timeout_t time = get_tick_count(); @@ -2273,8 +2286,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 ); } @@ -2329,6 +2342,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; @@ -2373,8 +2387,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 ); } @@ -2922,6 +2936,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; @@ -2932,9 +2947,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) { @@ -2952,8 +2968,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 ); } @@ -3646,16 +3662,18 @@ DECL_HANDLER(set_cursor) user_handle_t prev_cursor, new_cursor; 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 ); prev_cursor = input->cursor_count < 0 ? 0 : input->cursor; 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) { @@ -3678,8 +3696,8 @@ DECL_HANDLER(set_cursor) new_cursor = input->cursor_count < 0 ? 0 : input->cursor; if (prev_cursor != new_cursor) update_desktop_cursor_handle( desktop, input, new_cursor ); - 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 f1652faec9a..af39cc4055e 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 clip_flags; /* last cursor clip flags */ unsigned int last_change; /* time of last position change */ diff --git a/server/winstation.c b/server/winstation.c index df87de758c2..b0387bf227f 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -272,6 +272,8 @@ static struct desktop *create_desktop( const struct unicode_str *name, unsigned { if (get_error() != STATUS_OBJECT_NAME_EXISTS) { + const desktop_shm_t *desktop_shm; + /* initialize it if it didn't already exist */ desktop->flags = flags; @@ -299,11 +301,18 @@ static struct desktop *create_desktop( const struct unicode_str *name, unsigned list_init( &desktop->pointers ); desktop->session_index = alloc_shared_object(); - if (!get_shared_desktop( desktop->session_index )) + if (!(desktop_shm = get_shared_desktop( desktop->session_index ))) { release_object( desktop ); return NULL; } + + SHARED_WRITE_BEGIN( desktop_shm, desktop_shm_t ) + { + shared->cursor.x = 0; + shared->cursor.y = 0; + } + SHARED_WRITE_END; } else { -- 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 - server/winstation.c | 1 + 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/server/protocol.def b/server/protocol.def index e6a770d8c6d..3a431961073 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 3780e35d83e..3ebf5e18de1 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; @@ -2005,7 +2005,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; @@ -2026,10 +2026,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) { @@ -3699,7 +3704,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 af39cc4055e..5654a76d3b5 100644 --- a/server/user.h +++ b/server/user.h @@ -58,7 +58,6 @@ struct global_cursor { rectangle_t clip; /* cursor clip rectangle */ unsigned int clip_flags; /* last cursor clip flags */ - unsigned int last_change; /* time of last position change */ user_handle_t win; /* window that contains the cursor */ }; diff --git a/server/winstation.c b/server/winstation.c index b0387bf227f..8ceb2f504ee 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -311,6 +311,7 @@ static struct desktop *create_desktop( const struct unicode_str *name, unsigned { shared->cursor.x = 0; shared->cursor.y = 0; + shared->cursor.last_change = 0; } SHARED_WRITE_END; } -- 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 | 20 ++++++++++---------- dlls/win32u/winstation.c | 7 ------- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/dlls/win32u/input.c b/dlls/win32u/input.c index 1886ff979d7..1bab15a1cc4 100644 --- a/dlls/win32u/input.c +++ b/dlls/win32u/input.c @@ -739,22 +739,22 @@ BOOL WINAPI NtUserSetCursorPos( INT x, INT y ) */ BOOL get_cursor_pos( POINT *pt ) { - BOOL ret; - DWORD last_change; + const desktop_shm_t *desktop_shm; + struct object_lock lock = {0}; + BOOL ret = FALSE; + DWORD last_change = 0; + NTSTATUS status; UINT dpi; if (!pt) return FALSE; - SERVER_START_REQ( set_cursor ) + while ((status = get_shared_desktop( &lock, &desktop_shm )) == STATUS_PENDING) { - if ((ret = !wine_server_call( req ))) - { - pt->x = reply->new_x; - pt->y = reply->new_y; - last_change = reply->last_change; - } + pt->x = desktop_shm->cursor.x; + pt->y = desktop_shm->cursor.y; + last_change = desktop_shm->cursor.last_change; } - SERVER_END_REQ; + if (!status) ret = TRUE; /* 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 2a41692751a..519ba373769 100644 --- a/dlls/win32u/winstation.c +++ b/dlls/win32u/winstation.c @@ -525,17 +525,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_info *desktop_info = &get_session_thread_data()->shared_desktop; - const desktop_shm_t *desktop_shm; - 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( desktop_info, 0, sizeof(*desktop_info) ); - - while (get_shared_desktop( &lock, &desktop_shm ) == STATUS_PENDING) - /* nothing */; - if (was_virtual_desktop != is_virtual_desktop()) update_display_cache( TRUE ); } return ret; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=144760 Your paranoid android. === debian11 (build log) === error: patch failed: dlls/win32u/win32u_private.h:193 Task: Patch failed to apply === debian11b (build log) === error: patch failed: dlls/win32u/win32u_private.h:193 Task: Patch failed to apply
(question) I don't really care about this a bit, but my anecdotal experience of getting reviewed tells us that we want to either make this consistent with `mark_block_uninitialized`... ```suggestion:-0+0 VALGRIND_DISCARD( VALGRIND_MAKE_MEM_NOACCESS( ptr, size )); ``` ...or the other way around. Is this something you've missed, or am I overly cautious? (Wine codebase, for some reason, likes unbalanced parenthesis spacing as in `foo( bar( baz ))`, although I've seen both balanced `foo( bar( baz ) )` and unbalanced parenthesis spacing get committed recently in server/, so I'm not even sure what's going on. Feel free to use your own judgment.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_67357
(nit) typo ```suggestion:-0+0 /* mark the object data as not accessible */ ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_67358
After `SHARED_READ_{BEGIN,END}` retirement as well as introduction of uninitialized memory marking, I think this field no longer belongs to `desktop_shm_t`. 1. Callers of `get_shared_desktop` have no business accessing the `seq` and `id`—they are mere internals. Such internals should be private to winstation machinery in order to avoid "leaky abstraction." 2. It's not possible to access the body part of the object directly, which leads to somewhat ugly pointer arith for `mark_block_uninitialized()` call in `alloc_shared_object`, etc. Specifically, we're doing some `CONTAINING_RECORD`-like pointer hack based on the assumption of a specific data layout that will break silently (i.e. no compile-time errors) when we decide we want to change `session_obj_t`. 3. Finally, an incorrect `object_shm_t obj;` header for a new object type isn't reliably caught by the compiler, making it easier to introduce a mistake. Instead, I'd propose something like this (see the MR-form suggestion): ```c 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 */ union { // this can be a separate union definition instead desktop_shm_t desktop; /* ... */ } body; } object_shm_t; ``` Also, we'll need a helper for `DECL_HANDLER(get_thread_desktop)` to retrieve object id and index. This gives us a clean-up opportunity: we can just move `struct object_info` into protocol.def, and move it around via server requests (which eliminates the need for separate `id` and `index` assignment. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_67359
(nit) vocabulary: "seqlock" isn't something we match, we match sequence (number) instead. ```suggestion:-0+0 /* retry if seq doesn't match, wineserver has written to it */ ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_67360
`status` is uninitialized if `get_shared_session()` returns NULL. As a quick fix we can do `return session ? status : ...;`, but I think we should rather make `get_shared_session()` return NTSTATUS. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_67361
(nit) you probably mean `object_shm`? ```suggestion:-0+0 TRACE( "tid %04x, type %u, info %p, lock %p, object_shm %p\n", tid, type, info, lock, object_shm ); ``` (note: conflicts with the MR-form suggestion) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_67362
(nit) For consistency with other `ret` assignments, `ret` should only go from TRUE to FALSE. This invariant arguably makes understanding logic easier. ```suggestion:-0+0 ret = !status; ``` (and change `BOOL ret = FALSE;` -> `BOOL ret;`) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_67363
(nit) I'm aware that Wine likes to reuse variables, but I'd prefer that we avoid overloading the meaning of variables (valid_object vs. valid_session). Instead, just save the id... ```suggestion:-0+0 object_id_t latest_id = object->id; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_67364
...and check it later: ```suggestion:-0+0 if (latest_id == lock->id) return STATUS_SUCCESS; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_67365
(nit) we only have one session object of interest (desktop), so it's singular. ```suggestion:-0+0 /* Get shared session object data pointer, must be called in a loop while STATUS_PENDING ``` ```suggestion:-0+0 /* Get shared session object's data pointer, must be called in a loop while STATUS_PENDING ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_67366
(nit) It's more beneficial to check `*object_shm` instead of `lock->id` for previous acquisition, and require the caller to initilaize `object_shm` to NULL instead: ```suggestion:-0+0 if (*object_shm) ``` ...and change the assert above to `assert( !*object_shm || lock->id );`. This has several advantages: 1. Easier typing. `{0}` is hard to type: <kbd>Shift/AltGr</kbd>-<kbd>{</kbd>, <kbd>0</kbd>, <kbd>Shift/AltGr</kbd>-<kbd>}</kbd>. In comparison, `NULL` is <kbd>Shift</kbd>-<kbd>N</kbd><kbd>U</kbd><kbd>L</kbd><kbd>L</kbd>. 2. No constraint on lock's representation. We currently require `(struct object_lock){0}` to be an invalid lock state. Using `object_shm` removes this restriction and allows us to change its fields freely. 3. Minimized heap corruption. Forgetting to zero-initialize `object` causes outright crash or infinite retry in the usual case. In comparison, forgetting to zero-initialize `lock` (`lock->session` in particular) leads to invalid pointer free() which is sometimes hard to debug. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_67367
(nit) Ensure that the caller doesn't access stale pointer. (also needed by the `if (*object_shm)` suggestion) ```suggestion:-0+0 *object_shm = NULL; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_67368
I've submitted the rest of the review as an MR form: <https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/15/diffs>. 1. server: Ensure that object capacity does not overflow "int". [(commit)](https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/15/diffs?commit_id...) - Winstations are not subject to USER object count limit, and each winstation can have multiple desktops. Explicitly guard against a faulty application that tries to create winstations and desktops indefinitely instead of crashing in a subtle failure mode (due to int overflow wraparound or negative index). 1. win32u: No longer retry until the session has enough capacity in map_shared_session_section. [(commit)](https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/15/diffs?commit_id...) - The retry-until-capacity-logic was necessary because the shared object acquisition logic didn't check for out-of-bounds index (it checked the shared mapping ID instead.) Now that get_thread_session_object() explicitly checks for OOB index, this is now redundant. 1. server: Remove object_capacity from session_shm_t. [(commit)](https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/15/diffs?commit_id...) - Move it back to server session structure. The client no longer needs this information; a conservative approximation (which is good enough) can always be inferred from the mapping size, and if the client side view is too small it will attempt to remap anyway. 1. server: Remove session_shm_t, replace with simple session_obj_t array. [(commit)](https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/15/diffs?commit_id...) - We removed object_capacity from session_shm_t. We won't need any kind of other metadata for session mapping in the future, either. (In case we want metadata for session *itself*, it can be its own session object type). 1. server: Ensure all newly mapped blocks are marked as "free." [(commit)](https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/15/diffs?commit_id...) - This allows catching accidental access to unallocated object slots. Also, this makes "never-allocated" object slots (currently zero-filled) consistent with "allocated-and-then-freed" object slots (0xFE-filled). 1. server: Make `struct object_info` a protocol type (objshm_loc_t). [(commit)](https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/15/diffs?commit_id...) - This simplifies object info transport for future object types (queue and input). Note that UINT64 is required so that PE and Unix agrees on its alignment consistency. 1. server: Remove object header from desktop_shm_t. [(commit)](https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/15/diffs?commit_id...) - Make object_shm_t hold the object payload instead (replacing session_obj_t). 1. server: Compute shared object capacity independent of mapping size. [(commit)](https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/15/diffs?commit_id...) - Also, calculate the mmap() size solely based on the object capacity, not the mapping size. This decouples session.objects mmap size from the shared mapping size, which simplifies the case when mmap() fails and the VMA size is decoupled from the actual mapping (due to the old mmap being retained). This prepares refactoring with simplified mmap() error handling. 1. server: Always set session mapping size even on mmap failure. [(commit)](https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/15/diffs?commit_id...) - Prepare for grow_session_mapping() refactoring. 1. server: Factor out resize_anonymous_mapping() from grow_session_mapping(). [(commit)](https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/15/diffs?commit_id...) - Prepare for grow_session_mapping() refactoring. 1. server: Deduplicate shared session mmap()-ing logic. [(commit)](https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/15/diffs?commit_id...) - update_session_mapping_capacity() contains logic that set_session_mapping() and grow_session_mapping() have in common. 1. server: Make next_object_index more intuitive. [(commit)](https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/15/diffs?commit_id...) - next_object_index doesn't actually mean the next index. last_object_index actually means the last index. While we're at it, introduce an assert() that the newly allocated index is indeed less than the (potentially just grown) capacity. Also, make sure to keep "index < capacity" hold at all times. This not only prevents potential integer overflows that happens before modulo operation, but makes the meaning of "index" straightforward by not overloading it to mean "index plus some multiple of capacity." -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_67369
If the status was nonzero, we are reading unassigned variable `info->index`, *and* we're masking server error. ```suggestion:-0+0 if (!status && info->index == -1) status = STATUS_INVALID_HANDLE; ``` ```suggestion:-2+0 info->index = reply->index; if (info->index == -1) status = STATUS_INVALID_HANDLE; } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_67453
participants (3)
-
Jinoh Kang (＠iamahuman) -
Marvin -
Rémi Bernon