[PATCH v16 0/6] MR7610: win32u: Create a shared object for user handle entries.
For https://gitlab.winehq.org/wine/wine/-/merge_requests/7512 to use it to locate window shared objects. -- v16: win32u: Use the session shared object for user handle entries. win32u: Use the session shared object to implement is_window. server: Move the user object handle table to the shared memory. server: Allocate a session shared memory header structure. server: Use NTUSER_OBJ constants for user object types. include: Implement ReadAcquire64. https://gitlab.winehq.org/wine/wine/-/merge_requests/7610
From: Rémi Bernon <rbernon(a)codeweavers.com> --- include/winnt.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/include/winnt.h b/include/winnt.h index 722d2c3a542..0f4243ab75f 100644 --- a/include/winnt.h +++ b/include/winnt.h @@ -7117,6 +7117,22 @@ static FORCEINLINE LONG ReadAcquire( LONG const volatile *src ) return value; } +static FORCEINLINE LONG64 ReadAcquire64( LONG64 const volatile *src ) +{ + LONG64 value; +#if defined(__i386__) && _MSC_VER < 1700 + __asm { + mov eax, src + fild qword ptr [eax] + fistp value + } +#else + value = __WINE_LOAD64_NO_FENCE( (__int64 const volatile *)src ); + __wine_memory_barrier_acq_rel(); +#endif + return value; +} + static FORCEINLINE LONG ReadNoFence( LONG const volatile *src ) { LONG value = __WINE_LOAD32_NO_FENCE( (int const volatile *)src ); @@ -7331,6 +7347,17 @@ static FORCEINLINE LONG ReadAcquire( LONG const volatile *src ) return value; } +static FORCEINLINE LONG64 ReadAcquire64( LONG64 const volatile *src ) +{ + LONG64 value; +#ifdef __i386__ + __asm__ __volatile__( "fildq %1\n\tfistpq %0" : "=m" (value) : "m" (*src) : "memory", "st" ); +#else + __WINE_ATOMIC_LOAD_ACQUIRE( src, &value ); +#endif + return value; +} + static FORCEINLINE LONG ReadNoFence( LONG const volatile *src ) { LONG value; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7610
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/win32u/ntuser_private.h | 8 ++++---- dlls/win32u/window.c | 10 ++++++---- server/hook.c | 6 +++--- server/protocol.def | 2 ++ server/queue.c | 4 ++-- server/user.c | 30 ++++++++++++++++++++++-------- server/user.h | 17 +++++------------ server/window.c | 24 ++++++++++++------------ 8 files changed, 56 insertions(+), 45 deletions(-) diff --git a/dlls/win32u/ntuser_private.h b/dlls/win32u/ntuser_private.h index 2f3bbdf91c0..4dd7d49cb22 100644 --- a/dlls/win32u/ntuser_private.h +++ b/dlls/win32u/ntuser_private.h @@ -230,11 +230,11 @@ extern BOOL vulkan_init(void); extern void vulkan_detach_surfaces( struct list *surfaces ); /* window.c */ -HANDLE alloc_user_handle( struct user_object *ptr, unsigned int type ); -void *free_user_handle( HANDLE handle, unsigned int type ); -void *get_user_handle_ptr( HANDLE handle, unsigned int type ); +HANDLE alloc_user_handle( struct user_object *ptr, unsigned short type ); +void *free_user_handle( HANDLE handle, unsigned short type ); +void *get_user_handle_ptr( HANDLE handle, unsigned short type ); void release_user_handle_ptr( void *ptr ); -void *next_process_user_handle_ptr( HANDLE *handle, unsigned int type ); +void *next_process_user_handle_ptr( HANDLE *handle, unsigned short type ); UINT win_set_flags( HWND hwnd, UINT set_mask, UINT clear_mask ); static inline UINT win_get_flags( HWND hwnd ) diff --git a/dlls/win32u/window.c b/dlls/win32u/window.c index 3fb64755ba0..72f241a753f 100644 --- a/dlls/win32u/window.c +++ b/dlls/win32u/window.c @@ -55,12 +55,13 @@ static void *user_handles[NB_USER_HANDLES]; /*********************************************************************** * alloc_user_handle */ -HANDLE alloc_user_handle( struct user_object *ptr, unsigned int type ) +HANDLE alloc_user_handle( struct user_object *ptr, unsigned short type ) { HANDLE handle = 0; SERVER_START_REQ( alloc_user_handle ) { + req->type = type; if (!wine_server_call_err( req )) handle = wine_server_ptr_handle( reply->handle ); } SERVER_END_REQ; @@ -80,7 +81,7 @@ HANDLE alloc_user_handle( struct user_object *ptr, unsigned int type ) /*********************************************************************** * get_user_handle_ptr */ -void *get_user_handle_ptr( HANDLE handle, unsigned int type ) +void *get_user_handle_ptr( HANDLE handle, unsigned short type ) { struct user_object *ptr; WORD index = USER_HANDLE_TO_INDEX( handle ); @@ -106,7 +107,7 @@ void *get_user_handle_ptr( HANDLE handle, unsigned int type ) * * user_lock must be held by caller. */ -void *next_process_user_handle_ptr( HANDLE *handle, unsigned int type ) +void *next_process_user_handle_ptr( HANDLE *handle, unsigned short type ) { struct user_object *ptr; WORD index = *handle ? USER_HANDLE_TO_INDEX( *handle ) + 1 : 0; @@ -143,7 +144,7 @@ void release_user_handle_ptr( void *ptr ) /*********************************************************************** * free_user_handle */ -void *free_user_handle( HANDLE handle, unsigned int type ) +void *free_user_handle( HANDLE handle, unsigned short type ) { struct user_object *ptr; WORD index = USER_HANDLE_TO_INDEX( handle ); @@ -152,6 +153,7 @@ void *free_user_handle( HANDLE handle, unsigned int type ) { SERVER_START_REQ( free_user_handle ) { + req->type = type; req->handle = wine_server_user_handle( handle ); if (wine_server_call( req )) ptr = NULL; else InterlockedCompareExchangePointer( &user_handles[index], NULL, ptr ); diff --git a/server/hook.c b/server/hook.c index ffe7206369e..bf2ad266e76 100644 --- a/server/hook.c +++ b/server/hook.c @@ -153,7 +153,7 @@ static struct hook *add_hook( struct desktop *desktop, struct process *process, } if (!(hook = mem_alloc( sizeof(*hook) ))) return NULL; - if (!(hook->handle = alloc_user_handle( hook, USER_HOOK ))) + if (!(hook->handle = alloc_user_handle( hook, NTUSER_OBJ_HOOK ))) { free( hook ); return NULL; @@ -502,7 +502,7 @@ DECL_HANDLER(remove_hook) if (req->handle) { - if (!(hook = get_user_object( req->handle, USER_HOOK ))) + if (!(hook = get_user_object( req->handle, NTUSER_OBJ_HOOK ))) { set_error( STATUS_INVALID_HANDLE ); return; @@ -589,7 +589,7 @@ DECL_HANDLER(get_hook_info) { struct hook *hook; - if (!(hook = get_user_object( req->handle, USER_HOOK ))) return; + if (!(hook = get_user_object( req->handle, NTUSER_OBJ_HOOK ))) return; if (hook->thread && (hook->thread != current)) { set_error( STATUS_INVALID_HANDLE ); diff --git a/server/protocol.def b/server/protocol.def index 63bb0111473..1a2808d24a6 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3928,6 +3928,7 @@ struct handle_info /* Allocate an arbitrary user handle */ @REQ(alloc_user_handle) + unsigned short type; /* user object type */ @REPLY user_handle_t handle; /* allocated handle */ @END @@ -3935,6 +3936,7 @@ struct handle_info /* Free an arbitrary user handle */ @REQ(free_user_handle) + unsigned short type; /* user object type */ user_handle_t handle; /* handle to free*/ @END diff --git a/server/queue.c b/server/queue.c index 520659d377c..df3258a5158 100644 --- a/server/queue.c +++ b/server/queue.c @@ -3331,7 +3331,7 @@ DECL_HANDLER(get_message) const queue_shm_t *queue_shm; unsigned int filter; - if (get_win && get_win != 1 && get_win != -1 && !get_user_object( get_win, USER_WINDOW )) + if (get_win && get_win != 1 && get_win != -1 && !get_user_object( get_win, NTUSER_OBJ_WINDOW )) { set_win32_error( ERROR_INVALID_WINDOW_HANDLE ); return; @@ -4047,7 +4047,7 @@ DECL_HANDLER(set_cursor) reply->prev_y = desktop_shm->cursor.y; if ((req->flags & SET_CURSOR_HANDLE) && req->handle && - !get_user_object( req->handle, USER_CLIENT )) + !get_user_object( req->handle, NTUSER_OBJ_ICON )) { set_win32_error( ERROR_INVALID_CURSOR_HANDLE ); return; diff --git a/server/user.c b/server/user.c index 2d038a6ddbf..22d72c669e7 100644 --- a/server/user.c +++ b/server/user.c @@ -90,7 +90,7 @@ static inline void *free_user_entry( struct user_handle *ptr ) } /* allocate a user handle for a given object */ -user_handle_t alloc_user_handle( void *ptr, enum user_object type ) +user_handle_t alloc_user_handle( void *ptr, unsigned short type ) { struct user_handle *entry = alloc_user_entry(); if (!entry) return 0; @@ -101,7 +101,7 @@ user_handle_t alloc_user_handle( void *ptr, enum user_object type ) } /* return a pointer to a user object from its handle */ -void *get_user_object( user_handle_t handle, enum user_object type ) +void *get_user_object( user_handle_t handle, unsigned short type ) { struct user_handle *entry; @@ -120,7 +120,7 @@ user_handle_t get_user_full_handle( user_handle_t handle ) } /* same as get_user_object plus set the handle to the full 32-bit value */ -void *get_user_object_handle( user_handle_t *handle, enum user_object type ) +void *get_user_object_handle( user_handle_t *handle, unsigned short type ) { struct user_handle *entry; @@ -143,7 +143,7 @@ void *free_user_handle( user_handle_t handle ) } /* return the next user handle after 'handle' that is of a given type */ -void *next_user_handle( user_handle_t *handle, enum user_object type ) +void *next_user_handle( user_handle_t *handle, unsigned short type ) { struct user_handle *entry; @@ -172,14 +172,28 @@ void free_process_user_handles( struct process *process ) unsigned int i; for (i = 0; i < nb_handles; i++) - if (handles[i].type == USER_CLIENT && handles[i].ptr == process) - free_user_entry( &handles[i] ); + { + switch (handles[i].type) + { + case NTUSER_OBJ_MENU: + case NTUSER_OBJ_ICON: + case NTUSER_OBJ_WINPOS: + case NTUSER_OBJ_ACCEL: + case NTUSER_OBJ_IMC: + if (handles[i].ptr == process) free_user_entry( &handles[i] ); + break; + case NTUSER_OBJ_HOOK: + case NTUSER_OBJ_WINDOW: + default: + continue; + } + } } /* allocate an arbitrary user handle */ DECL_HANDLER(alloc_user_handle) { - reply->handle = alloc_user_handle( current->process, USER_CLIENT ); + reply->handle = alloc_user_handle( current->process, req->type ); } @@ -188,7 +202,7 @@ DECL_HANDLER(free_user_handle) { struct user_handle *entry; - if ((entry = handle_to_entry( req->handle )) && entry->type == USER_CLIENT) + if ((entry = handle_to_entry( req->handle )) && entry->type == req->type) free_user_entry( entry ); else set_error( STATUS_INVALID_HANDLE ); diff --git a/server/user.h b/server/user.h index ce463b9395d..b103fcd5652 100644 --- a/server/user.h +++ b/server/user.h @@ -34,13 +34,6 @@ struct window_class; struct atom_table; struct clipboard; -enum user_object -{ - USER_WINDOW = 1, - USER_HOOK, - USER_CLIENT /* arbitrary client handle */ -}; - #define DESKTOP_ATOM ((atom_t)32769) #define MAX_USER_HANDLES ((LAST_USER_HANDLE - FIRST_USER_HANDLE + 1) >> 1) @@ -99,12 +92,12 @@ struct desktop /* user handles functions */ -extern user_handle_t alloc_user_handle( void *ptr, enum user_object type ); -extern void *get_user_object( user_handle_t handle, enum user_object type ); -extern void *get_user_object_handle( user_handle_t *handle, enum user_object type ); +extern user_handle_t alloc_user_handle( void *ptr, unsigned short type ); +extern void *get_user_object( user_handle_t handle, unsigned short type ); +extern void *get_user_object_handle( user_handle_t *handle, unsigned short type ); extern user_handle_t get_user_full_handle( user_handle_t handle ); extern void *free_user_handle( user_handle_t handle ); -extern void *next_user_handle( user_handle_t *handle, enum user_object type ); +extern void *next_user_handle( user_handle_t *handle, unsigned short type ); extern void free_process_user_handles( struct process *process ); /* clipboard functions */ @@ -314,7 +307,7 @@ static inline void union_rect( struct rectangle *dest, const struct rectangle *s /* validate a window handle and return the full handle */ static inline user_handle_t get_valid_window_handle( user_handle_t win ) { - if (get_user_object_handle( &win, USER_WINDOW )) return win; + if (get_user_object_handle( &win, NTUSER_OBJ_WINDOW )) return win; set_win32_error( ERROR_INVALID_WINDOW_HANDLE ); return 0; } diff --git a/server/window.c b/server/window.c index f7f9d5e517f..1eb815dfe3e 100644 --- a/server/window.c +++ b/server/window.c @@ -185,7 +185,7 @@ static void window_destroy( struct object *obj ) /* retrieve a pointer to a window from its handle */ static inline struct window *get_window( user_handle_t handle ) { - struct window *ret = get_user_object( handle, USER_WINDOW ); + struct window *ret = get_user_object( handle, NTUSER_OBJ_WINDOW ); if (!ret) set_win32_error( ERROR_INVALID_WINDOW_HANDLE ); return ret; } @@ -672,7 +672,7 @@ static struct window *create_window( struct window *parent, struct window *owner memset( win->extra_bytes, 0, extra_bytes ); win->nb_extra_bytes = extra_bytes; } - if (!(win->handle = alloc_user_handle( win, USER_WINDOW ))) goto failed; + if (!(win->handle = alloc_user_handle( win, NTUSER_OBJ_WINDOW ))) goto failed; win->last_active = win->handle; /* if parent belongs to a different thread and the window isn't */ @@ -728,7 +728,7 @@ void destroy_thread_windows( struct thread *thread ) user_handle_t handle = 0; struct window *win; - while ((win = next_user_handle( &handle, USER_WINDOW ))) + while ((win = next_user_handle( &handle, NTUSER_OBJ_WINDOW ))) { if (win->thread != thread) continue; if (is_desktop_window( win )) detach_window_thread( win ); @@ -751,8 +751,8 @@ static struct window *get_desktop_window( struct thread *thread ) /* check whether child is a descendant of parent */ int is_child_window( user_handle_t parent, user_handle_t child ) { - struct window *child_ptr = get_user_object( child, USER_WINDOW ); - struct window *parent_ptr = get_user_object( parent, USER_WINDOW ); + struct window *child_ptr = get_user_object( child, NTUSER_OBJ_WINDOW ); + struct window *parent_ptr = get_user_object( parent, NTUSER_OBJ_WINDOW ); if (!child_ptr || !parent_ptr) return 0; while (child_ptr->parent) @@ -766,7 +766,7 @@ int is_child_window( user_handle_t parent, user_handle_t child ) /* check if window can be set as foreground window */ int is_valid_foreground_window( user_handle_t window ) { - struct window *win = get_user_object( window, USER_WINDOW ); + struct window *win = get_user_object( window, NTUSER_OBJ_WINDOW ); return win && (win->style & (WS_POPUP|WS_CHILD)) != WS_CHILD; } @@ -782,7 +782,7 @@ int make_window_active( user_handle_t window ) while (owner) { owner->last_active = win->handle; - owner = get_user_object( owner->owner, USER_WINDOW ); + owner = get_user_object( owner->owner, NTUSER_OBJ_WINDOW ); } return 1; } @@ -860,14 +860,14 @@ static int is_visible( const struct window *win ) /* same as is_visible but takes a window handle */ int is_window_visible( user_handle_t window ) { - struct window *win = get_user_object( window, USER_WINDOW ); + struct window *win = get_user_object( window, NTUSER_OBJ_WINDOW ); if (!win) return 0; return is_visible( win ); } int is_window_transparent( user_handle_t window ) { - struct window *win = get_user_object( window, USER_WINDOW ); + struct window *win = get_user_object( window, NTUSER_OBJ_WINDOW ); if (!win) return 0; return (win->ex_style & (WS_EX_LAYERED|WS_EX_TRANSPARENT)) == (WS_EX_LAYERED|WS_EX_TRANSPARENT); } @@ -1026,7 +1026,7 @@ user_handle_t shallow_window_from_point( struct desktop *desktop, int x, int y ) /* return thread of top-most window containing point (in absolute raw coords) */ struct thread *window_thread_from_point( user_handle_t scope, int x, int y ) { - struct window *win = get_user_object( scope, USER_WINDOW ); + struct window *win = get_user_object( scope, NTUSER_OBJ_WINDOW ); if (!win) return NULL; @@ -1068,7 +1068,7 @@ static int all_windows_from_point( struct window *top, int x, int y, unsigned in /* return the thread owning a window */ struct thread *get_window_thread( user_handle_t handle ) { - struct window *win = get_user_object( handle, USER_WINDOW ); + struct window *win = get_user_object( handle, NTUSER_OBJ_WINDOW ); if (!win || !win->thread) return NULL; return (struct thread *)grab_object( win->thread ); } @@ -2326,7 +2326,7 @@ DECL_HANDLER(get_window_info) reply->is_unicode = win->is_unicode; reply->dpi_context = win->dpi_context; - if (get_user_object( win->last_active, USER_WINDOW )) reply->last_active = win->last_active; + if (get_user_object( win->last_active, NTUSER_OBJ_WINDOW )) reply->last_active = win->last_active; if (win->thread) { reply->tid = get_thread_id( win->thread ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7610
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/win32u/class.c | 5 +++++ dlls/win32u/gdiobj.c | 2 -- dlls/win32u/ntgdi_private.h | 2 -- dlls/win32u/win32u_private.h | 3 +++ dlls/win32u/winstation.c | 12 ++++++++++++ server/file.h | 1 + server/mapping.c | 6 ++++-- server/protocol.def | 5 +++++ 8 files changed, 30 insertions(+), 6 deletions(-) diff --git a/dlls/win32u/class.c b/dlls/win32u/class.c index ea0d7efe74b..8c5f2819a67 100644 --- a/dlls/win32u/class.c +++ b/dlls/win32u/class.c @@ -36,6 +36,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(class); WINE_DECLARE_DEBUG_CHANNEL(win); +SYSTEM_BASIC_INFORMATION system_info; + #define MAX_WINPROCS 4096 #define WINPROC_PROC16 ((void *)1) /* placeholder for 16-bit window procs */ @@ -248,6 +250,9 @@ DLGPROC get_dialog_proc( DLGPROC ret, BOOL ansi ) static void init_user(void) { + NtQuerySystemInformation( SystemBasicInformation, &system_info, sizeof(system_info), NULL ); + + shared_session_init(); gdi_init(); sysparams_init(); winstation_init(); diff --git a/dlls/win32u/gdiobj.c b/dlls/win32u/gdiobj.c index fec99243aa2..f49b1c49464 100644 --- a/dlls/win32u/gdiobj.c +++ b/dlls/win32u/gdiobj.c @@ -48,7 +48,6 @@ static GDI_SHARED_MEMORY *gdi_shared; static GDI_HANDLE_ENTRY *next_free; static GDI_HANDLE_ENTRY *next_unused; static LONG debug_count; -SYSTEM_BASIC_INFORMATION system_info; static inline HGDIOBJ entry_to_handle( GDI_HANDLE_ENTRY *entry ) { @@ -1040,7 +1039,6 @@ void gdi_init(void) pthread_mutex_init( &gdi_lock, &attr ); pthread_mutexattr_destroy( &attr ); - NtQuerySystemInformation( SystemBasicInformation, &system_info, sizeof(system_info), NULL ); init_gdi_shared(); if (!gdi_shared) return; diff --git a/dlls/win32u/ntgdi_private.h b/dlls/win32u/ntgdi_private.h index 99ebb127d68..136921a211c 100644 --- a/dlls/win32u/ntgdi_private.h +++ b/dlls/win32u/ntgdi_private.h @@ -641,6 +641,4 @@ extern void free_heap_bits( struct gdi_image_bits *bits ); void set_gdi_client_ptr( HGDIOBJ handle, void *ptr ); -extern SYSTEM_BASIC_INFORMATION system_info; - #endif /* __WINE_NTGDI_PRIVATE_H */ diff --git a/dlls/win32u/win32u_private.h b/dlls/win32u/win32u_private.h index b4738ed6b16..2530e68793f 100644 --- a/dlls/win32u/win32u_private.h +++ b/dlls/win32u/win32u_private.h @@ -219,6 +219,7 @@ struct object_lock * 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 const session_shm_t *shared_session; extern NTSTATUS get_shared_desktop( struct object_lock *lock, const desktop_shm_t **desktop_shm ); extern NTSTATUS get_shared_queue( struct object_lock *lock, const queue_shm_t **queue_shm ); extern NTSTATUS get_shared_input( UINT tid, struct object_lock *lock, const input_shm_t **input_shm ); @@ -279,6 +280,8 @@ static inline void release_win_ptr( struct tagWND *ptr ) user_unlock(); } +extern SYSTEM_BASIC_INFORMATION system_info; +extern void shared_session_init(void); extern void gdi_init(void); extern void winstation_init(void); extern void sysparams_init(void); diff --git a/dlls/win32u/winstation.c b/dlls/win32u/winstation.c index b350b706454..14472b85bce 100644 --- a/dlls/win32u/winstation.c +++ b/dlls/win32u/winstation.c @@ -71,6 +71,7 @@ struct session_block static pthread_mutex_t session_lock = PTHREAD_MUTEX_INITIALIZER; static struct list session_blocks = LIST_INIT(session_blocks); +const session_shm_t *shared_session; static struct session_thread_data *get_session_thread_data(void) { @@ -195,6 +196,17 @@ static const shared_object_t *find_shared_session_object( struct obj_locator loc return NULL; } +void shared_session_init(void) +{ + struct session_block *block; + UINT status; + + if ((status = find_shared_session_block( 0, sizeof(*shared_session), &block ))) + ERR( "Failed to map initial shared session block, status %#x\n", status ); + else + shared_session = (const session_shm_t *)block->data; +} + NTSTATUS get_shared_desktop( struct object_lock *lock, const desktop_shm_t **desktop_shm ) { struct session_thread_data *data = get_session_thread_data(); diff --git a/server/file.h b/server/file.h index 59b73c0245c..64ca61e155f 100644 --- a/server/file.h +++ b/server/file.h @@ -193,6 +193,7 @@ 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 const session_shm_t *shared_session; extern const volatile void *alloc_shared_object(void); extern void free_shared_object( const volatile void *object_shm ); extern void invalidate_shared_object( const volatile void *object_shm ); diff --git a/server/mapping.c b/server/mapping.c index 247b28cf6f5..dd01b72eb5d 100644 --- a/server/mapping.c +++ b/server/mapping.c @@ -249,6 +249,7 @@ struct session object_id_t last_object_id; }; +const session_shm_t *shared_session; static struct mapping *session_mapping; static struct session session = { @@ -1303,7 +1304,7 @@ struct mapping *create_session_mapping( struct object *root, const struct unicod unsigned int attr, const struct security_descriptor *sd ) { static const unsigned int access = FILE_READ_DATA | FILE_WRITE_DATA; - size_t size = max( sizeof(shared_object_t) * 512, 0x10000 ); + size_t size = max( sizeof(*shared_session) + sizeof(object_shm_t) * 512, 0x10000 ); size = round_size( size, host_page_mask ); return create_mapping( root, name, attr, size, SEC_COMMIT, 0, access, sd ); @@ -1325,9 +1326,10 @@ void set_session_mapping( struct mapping *mapping ) block->data = tmp; block->offset = 0; - block->used_size = 0; + block->used_size = sizeof(*shared_session); block->block_size = size; + shared_session = tmp; session_mapping = mapping; list_add_tail( &session.blocks, &block->entry ); } diff --git a/server/protocol.def b/server/protocol.def index 1a2808d24a6..0e1e530327f 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1023,6 +1023,11 @@ typedef volatile struct object_shm_t shm; /* object shared data */ } shared_object_t; +typedef volatile struct +{ + unsigned __int64 placeholder; +} session_shm_t; + struct obj_locator { object_id_t id; /* object unique id, object data is valid if != 0 */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7610
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/win32u/window.c | 11 ++- include/ntuser.h | 19 ++++++ server/protocol.def | 2 +- server/user.c | 158 +++++++++++++++++++++++++------------------ server/user.h | 2 - 5 files changed, 119 insertions(+), 73 deletions(-) diff --git a/dlls/win32u/window.c b/dlls/win32u/window.c index 72f241a753f..4d011639a92 100644 --- a/dlls/win32u/window.c +++ b/dlls/win32u/window.c @@ -34,10 +34,9 @@ WINE_DEFAULT_DEBUG_CHANNEL(win); -#define NB_USER_HANDLES ((LAST_USER_HANDLE - FIRST_USER_HANDLE + 1) >> 1) #define USER_HANDLE_TO_INDEX(hwnd) ((LOWORD(hwnd) - FIRST_USER_HANDLE) >> 1) -static void *user_handles[NB_USER_HANDLES]; +static void *user_handles[MAX_USER_HANDLES]; #define SWP_AGG_NOGEOMETRYCHANGE \ (SWP_NOSIZE | SWP_NOCLIENTSIZE | SWP_NOZORDER) @@ -70,7 +69,7 @@ HANDLE alloc_user_handle( struct user_object *ptr, unsigned short type ) { UINT index = USER_HANDLE_TO_INDEX( handle ); - assert( index < NB_USER_HANDLES ); + assert( index < MAX_USER_HANDLES ); ptr->handle = handle; ptr->type = type; InterlockedExchangePointer( &user_handles[index], ptr ); @@ -86,7 +85,7 @@ void *get_user_handle_ptr( HANDLE handle, unsigned short type ) struct user_object *ptr; WORD index = USER_HANDLE_TO_INDEX( handle ); - if (index >= NB_USER_HANDLES) return NULL; + if (index >= MAX_USER_HANDLES) return NULL; user_lock(); if ((ptr = user_handles[index])) @@ -112,7 +111,7 @@ void *next_process_user_handle_ptr( HANDLE *handle, unsigned short type ) struct user_object *ptr; WORD index = *handle ? USER_HANDLE_TO_INDEX( *handle ) + 1 : 0; - while (index < NB_USER_HANDLES) + while (index < MAX_USER_HANDLES) { if (!(ptr = user_handles[index++])) continue; /* OBJ_OTHER_PROCESS */ if (ptr->type != type) continue; @@ -128,7 +127,7 @@ void *next_process_user_handle_ptr( HANDLE *handle, unsigned short type ) static void set_user_handle_ptr( HANDLE handle, struct user_object *ptr ) { WORD index = USER_HANDLE_TO_INDEX(handle); - assert( index < NB_USER_HANDLES ); + assert( index < MAX_USER_HANDLES ); InterlockedExchangePointer( &user_handles[index], ptr ); } diff --git a/include/ntuser.h b/include/ntuser.h index 848e5a4a651..dd8f6059d35 100644 --- a/include/ntuser.h +++ b/include/ntuser.h @@ -46,6 +46,25 @@ typedef enum MONITOR_DPI_TYPE typedef NTSTATUS (WINAPI *ntuser_callback)( void *args, ULONG len ); NTSYSAPI NTSTATUS KeUserModeCallback( ULONG id, const void *args, ULONG len, void **ret_ptr, ULONG *ret_len ); +struct user_entry +{ + ULONG64 offset; /* shared user object offset */ + ULONG tid; /* owner thread id */ + ULONG pid; /* owner process id */ + ULONG64 padding; + union + { + struct + { + USHORT type; /* object type (0 if free) */ + USHORT generation; /* generation counter */ + }; + LONG64 uniq; + }; +}; + +#define MAX_USER_HANDLES ((LAST_USER_HANDLE - FIRST_USER_HANDLE + 1) >> 1) + /* KernelCallbackTable codes, not compatible with Windows. All of these functions must live inside user32.dll. Overwatch 2's KiUserCallbackDispatcher hook verifies this and prevents the callback from diff --git a/server/protocol.def b/server/protocol.def index 0e1e530327f..c2a97125464 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1025,7 +1025,7 @@ typedef volatile struct typedef volatile struct { - unsigned __int64 placeholder; + struct user_entry user_entries[MAX_USER_HANDLES]; } session_shm_t; struct obj_locator diff --git a/server/user.c b/server/user.c index 22d72c669e7..a2f2a302551 100644 --- a/server/user.c +++ b/server/user.c @@ -18,101 +18,125 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */ +#include "ntstatus.h" +#define WIN32_NO_STATUS +#include "windef.h" +#include "winternl.h" + #include "thread.h" +#include "file.h" #include "user.h" +#include "file.h" +#include "process.h" #include "request.h" -struct user_handle -{ - void *ptr; /* pointer to object */ - unsigned short type; /* object type (0 if free) */ - unsigned short generation; /* generation counter */ -}; +typedef volatile struct user_entry user_entry_t; -static struct user_handle *handles; -static struct user_handle *freelist; +static void *server_objects[MAX_USER_HANDLES]; +static mem_size_t freelist = -1; static int nb_handles; -static int allocated_handles; -static struct user_handle *handle_to_entry( user_handle_t handle ) +static void *get_server_object( const user_entry_t *entry ) { - unsigned short generation; - int index = ((handle & 0xffff) - FIRST_USER_HANDLE) >> 1; + const user_entry_t *handles = shared_session->user_entries; + return server_objects[entry - handles]; +} + +static void *set_server_object( const user_entry_t *entry, void *ptr ) +{ + const user_entry_t *handles = shared_session->user_entries; + void *prev = server_objects[entry - handles]; + server_objects[entry - handles] = ptr; + return prev; +} + +static const user_entry_t *handle_to_entry( user_handle_t handle ) +{ + const user_entry_t *handles = shared_session->user_entries; + unsigned short generation = handle >> 16; + const user_entry_t *entry; + int index; + + index = ((handle & 0xffff) - FIRST_USER_HANDLE) >> 1; if (index < 0 || index >= nb_handles) return NULL; - if (!handles[index].type) return NULL; - generation = handle >> 16; - if (generation == handles[index].generation || !generation || generation == 0xffff) - return &handles[index]; + entry = handles + index; + + if (!entry->type) return NULL; + if (!generation || generation == 0xffff) return entry; + if (generation == entry->generation) return entry; return NULL; } -static inline user_handle_t entry_to_handle( struct user_handle *ptr ) +static user_handle_t entry_to_handle( const user_entry_t *entry ) { - unsigned int index = ptr - handles; - return (index << 1) + FIRST_USER_HANDLE + (ptr->generation << 16); + const user_entry_t *handles = shared_session->user_entries; + unsigned int index = entry - handles; + return (index << 1) + FIRST_USER_HANDLE + (entry->generation << 16); } -static inline struct user_handle *alloc_user_entry(void) +static const user_entry_t *alloc_user_entry( unsigned short type ) { - struct user_handle *handle; + const user_entry_t *handles = shared_session->user_entries; + unsigned short generation; + user_entry_t *entry; - if (freelist) + if (freelist != -1) { - handle = freelist; - freelist = handle->ptr; - return handle; + entry = (user_entry_t *)handles + freelist; + generation = entry->generation + 1; + freelist = entry->offset; } - if (nb_handles >= allocated_handles) /* need to grow the array */ + else { - struct user_handle *new_handles; - /* grow array by 50% (but at minimum 32 entries) */ - int growth = max( 32, allocated_handles / 2 ); - int new_size = min( allocated_handles + growth, MAX_USER_HANDLES ); - if (new_size <= allocated_handles) return NULL; - if (!(new_handles = realloc( handles, new_size * sizeof(*handles) ))) - return NULL; - handles = new_handles; - allocated_handles = new_size; + entry = (user_entry_t *)handles + nb_handles; + generation = 1; + nb_handles++; } - handle = &handles[nb_handles++]; - handle->generation = 0; - return handle; + + if (generation == 0 || generation == 0xffff) generation = 1; + + entry->offset = -1; + entry->tid = get_thread_id( current ); + entry->pid = get_process_id( current->process ); + entry->padding = -1; + WriteRelease64( &entry->uniq, MAKELONG(type, generation) ); + return entry; } -static inline void *free_user_entry( struct user_handle *ptr ) +static void free_user_entry( const user_entry_t *entry_const ) { - void *ret; - ret = ptr->ptr; - ptr->ptr = freelist; - ptr->type = 0; - freelist = ptr; - return ret; + const user_entry_t *handles = shared_session->user_entries; + user_entry_t *entry = (user_entry_t *)entry_const; + size_t index = entry - handles; + + WriteRelease64( &entry->uniq, MAKELONG(0, entry->generation) ); + entry->offset = freelist; + freelist = index; } /* allocate a user handle for a given object */ user_handle_t alloc_user_handle( void *ptr, unsigned short type ) { - struct user_handle *entry = alloc_user_entry(); - if (!entry) return 0; - entry->ptr = ptr; - entry->type = type; - if (++entry->generation >= 0xffff) entry->generation = 1; + const user_entry_t *entry; + + if (!(entry = alloc_user_entry( type ))) return 0; + set_server_object( entry, ptr ); return entry_to_handle( entry ); } /* return a pointer to a user object from its handle */ void *get_user_object( user_handle_t handle, unsigned short type ) { - struct user_handle *entry; + const user_entry_t *entry; if (!(entry = handle_to_entry( handle )) || entry->type != type) return NULL; - return entry->ptr; + return get_server_object( entry ); } /* get the full handle for a possibly truncated handle */ user_handle_t get_user_full_handle( user_handle_t handle ) { - struct user_handle *entry; + const user_entry_t *entry; if (handle >> 16) return handle; if (!(entry = handle_to_entry( handle ))) return handle; @@ -122,30 +146,34 @@ user_handle_t get_user_full_handle( user_handle_t handle ) /* same as get_user_object plus set the handle to the full 32-bit value */ void *get_user_object_handle( user_handle_t *handle, unsigned short type ) { - struct user_handle *entry; + const user_entry_t *entry; if (!(entry = handle_to_entry( *handle )) || entry->type != type) return NULL; *handle = entry_to_handle( entry ); - return entry->ptr; + return get_server_object( entry ); } /* free a user handle and return a pointer to the object */ void *free_user_handle( user_handle_t handle ) { - struct user_handle *entry; + const user_entry_t *entry; + void *ret; if (!(entry = handle_to_entry( handle ))) { set_error( STATUS_INVALID_HANDLE ); return NULL; } - return free_user_entry( entry ); + + ret = set_server_object( entry, NULL ); + free_user_entry( entry ); + return ret; } /* return the next user handle after 'handle' that is of a given type */ void *next_user_handle( user_handle_t *handle, unsigned short type ) { - struct user_handle *entry; + const user_entry_t *entry, *handles = shared_session->user_entries; if (!*handle) entry = handles; else @@ -159,7 +187,7 @@ void *next_user_handle( user_handle_t *handle, unsigned short type ) if (!type || entry->type == type) { *handle = entry_to_handle( entry ); - return entry->ptr; + return get_server_object( entry ); } entry++; } @@ -169,18 +197,20 @@ void *next_user_handle( user_handle_t *handle, unsigned short type ) /* free client-side user handles managed by the process */ void free_process_user_handles( struct process *process ) { - unsigned int i; + const user_entry_t *handles = shared_session->user_entries; + unsigned int i, pid = get_process_id( process ); for (i = 0; i < nb_handles; i++) { - switch (handles[i].type) + const user_entry_t *entry = handles + i; + switch (entry->type) { case NTUSER_OBJ_MENU: case NTUSER_OBJ_ICON: case NTUSER_OBJ_WINPOS: case NTUSER_OBJ_ACCEL: case NTUSER_OBJ_IMC: - if (handles[i].ptr == process) free_user_entry( &handles[i] ); + if (entry->pid == pid) free_user_entry( entry ); break; case NTUSER_OBJ_HOOK: case NTUSER_OBJ_WINDOW: @@ -193,14 +223,14 @@ void free_process_user_handles( struct process *process ) /* allocate an arbitrary user handle */ DECL_HANDLER(alloc_user_handle) { - reply->handle = alloc_user_handle( current->process, req->type ); + reply->handle = alloc_user_handle( (void *)-1 /* never used */, req->type ); } /* free an arbitrary user handle */ DECL_HANDLER(free_user_handle) { - struct user_handle *entry; + const user_entry_t *entry; if ((entry = handle_to_entry( req->handle )) && entry->type == req->type) free_user_entry( entry ); diff --git a/server/user.h b/server/user.h index b103fcd5652..df74b792af8 100644 --- a/server/user.h +++ b/server/user.h @@ -36,8 +36,6 @@ struct clipboard; #define DESKTOP_ATOM ((atom_t)32769) -#define MAX_USER_HANDLES ((LAST_USER_HANDLE - FIRST_USER_HANDLE + 1) >> 1) - struct winstation { struct object obj; /* object header */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7610
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/win32u/window.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/dlls/win32u/window.c b/dlls/win32u/window.c index 4d011639a92..8ca673f7e62 100644 --- a/dlls/win32u/window.c +++ b/dlls/win32u/window.c @@ -77,6 +77,20 @@ HANDLE alloc_user_handle( struct user_object *ptr, unsigned short type ) return handle; } +static BOOL is_valid_entry_uniq( HANDLE handle, unsigned short type, UINT64 uniq ) +{ + if (!HIWORD(handle) || HIWORD(handle) == 0xffff) return LOWORD(uniq) == type; + return uniq == MAKELONG(type, HIWORD(handle)); +} + +static BOOL is_valid_entry( HANDLE handle, unsigned short type ) +{ + const volatile struct user_entry *entries = shared_session->user_entries; + WORD index = USER_HANDLE_TO_INDEX( handle ); + if (index >= MAX_USER_HANDLES) return FALSE; + return is_valid_entry_uniq( handle, type, ReadAcquire64( (LONG64 *)&entries[index].uniq ) ); +} + /*********************************************************************** * get_user_handle_ptr */ @@ -290,26 +304,13 @@ HWND is_current_process_window( HWND hwnd ) /* see IsWindow */ BOOL is_window( HWND hwnd ) { - WND *win; - BOOL ret; - - if (!(win = get_win_ptr( hwnd ))) return FALSE; - if (win == WND_DESKTOP) return TRUE; - - if (win != WND_OTHER_PROCESS) - { - release_win_ptr( win ); - return TRUE; - } - - /* check other processes */ - SERVER_START_REQ( get_window_info ) + if (!hwnd) return FALSE; + if (!is_valid_entry( hwnd, NTUSER_OBJ_WINDOW )) { - req->handle = wine_server_user_handle( hwnd ); - ret = !wine_server_call_err( req ); + RtlSetLastWin32Error( ERROR_INVALID_WINDOW_HANDLE ); + return FALSE; } - SERVER_END_REQ; - return ret; + return TRUE; } /* see GetWindowThreadProcessId */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7610
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/win32u/win32u_private.h | 7 ++++ dlls/win32u/window.c | 74 ++++++++++++++++++++++++++---------- dlls/win32u/winstation.c | 7 ---- 3 files changed, 60 insertions(+), 28 deletions(-) diff --git a/dlls/win32u/win32u_private.h b/dlls/win32u/win32u_private.h index 2530e68793f..7ccd2067dbb 100644 --- a/dlls/win32u/win32u_private.h +++ b/dlls/win32u/win32u_private.h @@ -213,6 +213,13 @@ struct object_lock }; #define OBJECT_LOCK_INIT {0} +#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 + /* Get shared session object's data pointer, must be called in a loop while STATUS_PENDING * is returned, lock must be initialized with OBJECT_LOCK_INIT. * diff --git a/dlls/win32u/window.c b/dlls/win32u/window.c index 8ca673f7e62..7cfe43bec90 100644 --- a/dlls/win32u/window.c +++ b/dlls/win32u/window.c @@ -35,8 +35,9 @@ WINE_DEFAULT_DEBUG_CHANNEL(win); #define USER_HANDLE_TO_INDEX(hwnd) ((LOWORD(hwnd) - FIRST_USER_HANDLE) >> 1) +#define USER_HANDLE_FROM_INDEX(index, generation) UlongToHandle( (index << 1) + FIRST_USER_HANDLE + (generation << 16) ) -static void *user_handles[MAX_USER_HANDLES]; +static void *client_objects[MAX_USER_HANDLES]; #define SWP_AGG_NOGEOMETRYCHANGE \ (SWP_NOSIZE | SWP_NOCLIENTSIZE | SWP_NOZORDER) @@ -72,7 +73,7 @@ HANDLE alloc_user_handle( struct user_object *ptr, unsigned short type ) assert( index < MAX_USER_HANDLES ); ptr->handle = handle; ptr->type = type; - InterlockedExchangePointer( &user_handles[index], ptr ); + InterlockedExchangePointer( &client_objects[index], ptr ); } return handle; } @@ -91,27 +92,57 @@ static BOOL is_valid_entry( HANDLE handle, unsigned short type ) return is_valid_entry_uniq( handle, type, ReadAcquire64( (LONG64 *)&entries[index].uniq ) ); } +static BOOL read_acquire_user_entry( HANDLE handle, unsigned short type, const volatile struct user_entry *src, struct user_entry *dst ) +{ + UINT64 uniq = ReadAcquire64( (LONG64 *)&src->uniq ); + if (!is_valid_entry_uniq( handle, type, uniq )) return FALSE; + dst->offset = src->offset; + dst->tid = src->tid; + dst->pid = src->pid; + dst->padding = src->padding; + __SHARED_READ_FENCE; + dst->uniq = ReadNoFence64( &src->uniq ); + return dst->uniq == uniq; +} + +static BOOL get_user_entry( HANDLE handle, unsigned short type, struct user_entry *entry, HANDLE *full ) +{ + const volatile struct user_entry *entries = shared_session->user_entries; + WORD index = USER_HANDLE_TO_INDEX( handle ); + + if (index >= MAX_USER_HANDLES) return FALSE; + if (!read_acquire_user_entry( handle, type, entries + index, entry )) return FALSE; + *full = USER_HANDLE_FROM_INDEX( index, entry->generation ); + return TRUE; +} + +static BOOL get_user_entry_at( WORD index, unsigned short type, struct user_entry *entry, HANDLE *full ) +{ + const volatile struct user_entry *entries = shared_session->user_entries; + if (index >= MAX_USER_HANDLES) return FALSE; + if (!read_acquire_user_entry( 0, type, entries + index, entry )) return FALSE; + *full = USER_HANDLE_FROM_INDEX( index, entry->generation ); + return TRUE; +} + /*********************************************************************** * get_user_handle_ptr */ void *get_user_handle_ptr( HANDLE handle, unsigned short type ) { - struct user_object *ptr; WORD index = USER_HANDLE_TO_INDEX( handle ); + struct user_object *ptr = NULL; + struct user_entry entry; if (index >= MAX_USER_HANDLES) return NULL; user_lock(); - if ((ptr = user_handles[index])) - { - if (ptr->type == type && - ((UINT)(UINT_PTR)ptr->handle == (UINT)(UINT_PTR)handle || - !HIWORD(handle) || HIWORD(handle) == 0xffff)) - return ptr; - ptr = NULL; - } - else ptr = OBJ_OTHER_PROCESS; - user_unlock(); + + if (!get_user_entry( handle, type, &entry, &handle )) ptr = NULL; + else if (entry.pid != GetCurrentProcessId()) ptr = OBJ_OTHER_PROCESS; + else ptr = client_objects[index]; + + if (!ptr || ptr == OBJ_OTHER_PROCESS) user_unlock(); return ptr; } @@ -122,16 +153,17 @@ void *get_user_handle_ptr( HANDLE handle, unsigned short type ) */ void *next_process_user_handle_ptr( HANDLE *handle, unsigned short type ) { - struct user_object *ptr; WORD index = *handle ? USER_HANDLE_TO_INDEX( *handle ) + 1 : 0; + struct user_entry entry; + UINT i; - while (index < MAX_USER_HANDLES) + for (i = index; i < MAX_USER_HANDLES; i++) { - if (!(ptr = user_handles[index++])) continue; /* OBJ_OTHER_PROCESS */ - if (ptr->type != type) continue; - *handle = ptr->handle; - return ptr; + if (!get_user_entry_at( i, type, &entry, handle )) continue; + if (entry.pid != GetCurrentProcessId()) continue; + return client_objects[i]; } + return NULL; } @@ -142,7 +174,7 @@ static void set_user_handle_ptr( HANDLE handle, struct user_object *ptr ) { WORD index = USER_HANDLE_TO_INDEX(handle); assert( index < MAX_USER_HANDLES ); - InterlockedExchangePointer( &user_handles[index], ptr ); + InterlockedExchangePointer( &client_objects[index], ptr ); } /*********************************************************************** @@ -169,7 +201,7 @@ void *free_user_handle( HANDLE handle, unsigned short type ) req->type = type; req->handle = wine_server_user_handle( handle ); if (wine_server_call( req )) ptr = NULL; - else InterlockedCompareExchangePointer( &user_handles[index], NULL, ptr ); + else InterlockedCompareExchangePointer( &client_objects[index], NULL, ptr ); } SERVER_END_REQ; user_unlock(); diff --git a/dlls/win32u/winstation.c b/dlls/win32u/winstation.c index 14472b85bce..73a2c82e1fd 100644 --- a/dlls/win32u/winstation.c +++ b/dlls/win32u/winstation.c @@ -80,13 +80,6 @@ static struct session_thread_data *get_session_thread_data(void) 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) -#else -#define __SHARED_READ_FENCE __atomic_thread_fence( __ATOMIC_ACQUIRE ) -#endif - static void shared_object_acquire_seqlock( const shared_object_t *object, UINT64 *seq ) { while ((*seq = ReadNoFence64( &object->seq )) & 1) YieldProcessor(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7610
v5: Compare uniq values for equality. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7610#note_99840
Other than const misuse, LGTM. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7610#note_99845
Fwiw I claim that there's no const misuse here. The const keyword in C/C++ never means that a value, possibly through a pointer to it, is constant. It always only means that the reference or pointer cannot be used directly to mutate the value. This is exactly how it has been used in the shared object infrastructure, and how it is being used here. The shared objects are not constant, neither on the wineserver nor on the client side, they are still mostly always kept as const pointers to avoid accidental modification and to convey information that additional steps need to be used to mutate the values: a) From the client side, call a wineserver request. b) From wineserver side, use the SHARED_WRITE macros. To the contrary to other shared objects which have specific and stronger data consistency mechanism and can use the SHARED_WRITE macros more freely to mutate the values, the shared handle table uses a different and less robust mechanism as has been specifically requested during review. This other mechanism makes many more assumptions, and the client/server side only work with some very specific sequence of modification. This means that the shared handles can safely be mutated only during allocation / free of their corresponding handles, and nowhere else. Therefore, I believe it is even more critical to keep the const qualifier, on the wineserver side, in this case that it would even be for other shared objects. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7610#note_100258
So, anything I should do here? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7610#note_101014
I would like to keep working on moving window properties to the shared memory and it is blocked by this, is there something I must do here? A debatable use of const in two places doesn't seem like a good blocking reason to me, when it's been used similarly previously in many other places. Or maybe it's not the reason this is stalled? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7610#note_102125
@jacek Would you please reconsider approving this? Fwiw the use of const here is exactly the same as what has been done so far with shared memory objects. See their declaration in https://gitlab.winehq.org/wine/wine/-/blob/master/server/queue.c?ref_type=he..., https://gitlab.winehq.org/wine/wine/-/blob/master/server/queue.c?ref_type=he..., etc... and the cast to get a writable pointer only where allowed in https://gitlab.winehq.org/wine/wine/-/blob/master/server/file.h?ref_type=hea... If you don't agree with that, please could you detail exactly why? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7610#note_102637
I don’t agree with your arguments. Sure, you can call const in C a “hint,” but a misleading hint is worse than none. If you need to cast away const to work with your own internal data structures, that’s a strong sign the structure doesn’t really match how it’s being used. The `SHARED_WRITE` macros don’t feel relevant here. They deal with shared memory for Windows handles (something those handles weren’t designed for) so they need extra complexity and supporting infrastructure. Their use is also more widespread, so various parts of code need to deal with them. I get why const might be helpful in that case. But the user handle table is much simpler. Its access is well-contained and clear, and I don’t see the point in trying to be “protective” against ourselves. Take your `handle_to_entry` function: it has just five callers, and two already treat the result as mutable (even if indirectly). So why not just return a non-const pointer? The others can still store its result in a const variable. And all of them are already closely tied to the table’s internals, so whoever touches them is going to understand the consequences anyway. (Also, on top of that series, it might be a good idea to move `get_user_full_handle` out of the server, which would leave only half of its callers treating the result as immutable.) That said, you seem to feel strongly about it, and I wouldn't feel honest approving it as-is. My approval isn't required anyway. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7610#note_102647
I don’t agree with your arguments. Sure, you can call const in C a “hint,” but a misleading hint is worse than none. If you need to cast away const to work with your own internal data structures, that’s a strong sign the structure doesn’t really match how it’s being used.
How is the hint misleading? If you consider it as a hint that something is constant, then yes, because it's not what it means and we're then using it wrong everywhere. It's not a hint but a guard to make sure a pointer cannot directly be used to mutate what it's being pointed to.
The `SHARED_WRITE` macros don’t feel relevant here. They deal with shared memory for Windows handles (something those handles weren’t designed for) so they need extra complexity and supporting infrastructure. Their use is also more widespread, so various parts of code need to deal with them. I get why const might be helpful in that case.
I don't see any difference with them, and like I described above I think it's even more important to keep it for user handle entries which lack proper data consistency outside of the alloc/free calls. For the sake of consistency I would rather remove const everywhere or keep it everywhere. Otherwise, having some of the use case non-const cascades up to the shared_session pointer, and raises genuine interrogation and suspicion over why some pointers are deemed to be const qualified and some aren't.
That said, you seem to feel strongly about it, and I wouldn't feel honest approving it as-is. My approval isn't required anyway.
Well I don't know what's required, it would be much easier with some explicit feedback but as usual my only option here is to take guesses. I have the strong feeling that even if not a requirement, an approval from a trusted developer is a strong incentive to get things merged. Similarly, a comment from a trusted developer questioning part of the code, is a strong incentive not to get things merged until things get settled. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7610#note_102649
How is the hint misleading?
It's a hint that the code accessing this pointer doesn't modify its value.
If you consider it as a hint that something is constant, then yes, because it's not what it means and we're then using it wrong everywhere.
That depends on what you mean by constant in this context. You seem to suggest it only applies to things with enforced read-only memory protection. I meant exactly what I said above. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7610#note_102657
participants (2)
-
Jacek Caban (＠jacek) -
Rémi Bernon