[PATCH 0/1] MR7668: wbemprox: Copy table in exec_select_view() instead of using per-table CS.

List overview All Threads

newer

older

[PATCH 0/2] MR7618: comctl32:...

[PATCH v2 0/5] MR7667:...

Paul Gofman (＠gofman)

26 Mar 2025 26 Mar '25

6:57 p.m.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7668

Show replies by date

Paul Gofman

26 Mar 26 Mar

6:57 p.m.

New subject: [PATCH 1/1] wbemprox: Copy table in exec_select_view() instead of using per-table CS.

From: Paul Gofman pgofman@codeweavers.com

diff --git a/dlls/wbemprox/builtin.c b/dlls/wbemprox/builtin.c index 646d93e3978..968d18061b3 100644 --- a/dlls/wbemprox/builtin.c +++ b/dlls/wbemprox/builtin.c @@ -4637,12 +4637,7 @@ void init_table_list( void ) { list_init( &tables[ns] ); for (i = 0; i < builtin_namespaces[ns].table_count; i++) - { - struct table *table = &builtin_namespaces[ns].tables[i]; - InitializeCriticalSectionEx( &table->cs, 0, RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO ); - table->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": table.cs" ); - list_add_tail( &tables[ns], &table->entry ); - } + list_add_tail( &tables[ns], &builtin_namespaces[ns].tables[i].entry ); table_list[ns] = &tables[ns]; } } diff --git a/dlls/wbemprox/class.c b/dlls/wbemprox/class.c index 8e585c2f360..53822372bb7 100644 --- a/dlls/wbemprox/class.c +++ b/dlls/wbemprox/class.c @@ -114,7 +114,7 @@ static HRESULT WINAPI enum_class_object_Next( { struct enum_class_object *ec = impl_from_IEnumWbemClassObject( iface ); struct view *view = ec->query->view; - struct table *table; + const struct table *table; static int once = 0; HRESULT hr; ULONG i, j; @@ -219,7 +219,7 @@ HRESULT EnumWbemClassObject_create( struct query *query, LPVOID *ppObj ) return S_OK; }

-static struct record *create_record( struct table *table ) +static struct record *create_record( const struct table *table ) { UINT i; struct record *record; @@ -536,7 +536,7 @@ static HRESULT WINAPI class_object_Next( struct class_object *obj = impl_from_IWbemClassObject( iface ); struct enum_class_object *iter = impl_from_IEnumWbemClassObject( obj->iter ); struct view *view = iter->query->view; - struct table *table = get_view_table( view, obj->index ); + const struct table *table = get_view_table( view, obj->index ); BSTR prop; HRESULT hr; UINT i; @@ -637,7 +637,7 @@ static BSTR get_body_text( const struct table *table, UINT row, UINT *len ) static BSTR get_object_text( const struct view *view, UINT index ) { UINT len, len_body, row = view->result[index]; - struct table *table = get_view_table( view, index ); + const struct table *table = get_view_table( view, index ); BSTR ret, body;

len = ARRAY_SIZE( L"\ninstance of %s\n{%s\n};" ); @@ -686,7 +686,7 @@ static HRESULT WINAPI class_object_SpawnInstance( { struct class_object *co = impl_from_IWbemClassObject( iface ); struct enum_class_object *ec = impl_from_IEnumWbemClassObject( co->iter ); - struct table *table = get_view_table( ec->query->view, co->index ); + const struct table *table = get_view_table( ec->query->view, co->index ); IEnumWbemClassObject *iter; struct record *record; HRESULT hr; @@ -888,7 +888,7 @@ static HRESULT WINAPI class_object_GetMethod( { struct class_object *co = impl_from_IWbemClassObject( iface ); IWbemClassObject *in, *out; - struct table *table; + const struct table *table; unsigned int i; HRESULT hr;

diff --git a/dlls/wbemprox/query.c b/dlls/wbemprox/query.c index a6599334f58..ef975990f0e 100644 --- a/dlls/wbemprox/query.c +++ b/dlls/wbemprox/query.c @@ -29,9 +29,9 @@

WINE_DEFAULT_DEBUG_CHANNEL(wbemprox);

-static HRESULT append_table( struct view *view, struct table *table ) +static HRESULT append_table( struct view *view, const struct table *table ) { - struct table **tmp; + const struct table **tmp; if (!(tmp = realloc( view->table, (view->table_count + 1) * sizeof(*tmp) ))) return E_OUTOFMEMORY; view->table = tmp; view->table[view->table_count++] = table; @@ -54,7 +54,7 @@ HRESULT create_view( enum view_type type, enum wbm_namespace ns, const WCHAR *pa

case VIEW_TYPE_SELECT: { - struct table *table = find_table( ns, class ); + const struct table *table = find_table( ns, class ); HRESULT hr;

if (table && (hr = append_table( view, table )) != S_OK) @@ -610,7 +610,7 @@ static HRESULT do_query( enum wbm_namespace ns, const WCHAR *str, struct query * }

static HRESULT get_antecedent_table( enum wbm_namespace ns, const WCHAR *assocclass, const WCHAR *dependent, - struct table **table ) + const struct table **table ) { BSTR antecedent = NULL; struct path *path = NULL; @@ -657,7 +657,7 @@ static HRESULT exec_assoc_view( struct view *view ) { ULONG count; IWbemClassObject *obj; - struct table *table; + const struct table *table; VARIANT var;

IEnumWbemClassObject_Next( iter, WBEM_INFINITE, 1, &obj, &count ); @@ -697,15 +697,20 @@ static HRESULT exec_select_view( struct view *view ) { UINT i, j = 0, len; enum fill_status status = FILL_STATUS_UNFILTERED; - struct table *table; + const struct table *table;

if (!view->table_count) return S_OK;

table = view->table[0]; if (table->fill) { - clear_table( table ); - status = table->fill( table, view->cond ); + struct table *table_copy; + + if (!(table_copy = get_table_writeable_copy( table ))) return E_OUTOFMEMORY; + clear_table( table_copy ); + status = table_copy->fill( table_copy, view->cond ); + release_table( table ); + table = view->table[0] = table_copy; } if (status == FILL_STATUS_FAILED) return WBEM_E_FAILED; if (!table->num_rows) return S_OK; @@ -869,7 +874,7 @@ static UINT count_key_columns( const struct table *table ) static BSTR build_relpath( const struct view *view, UINT table_index, UINT result_index, const WCHAR *name ) { BSTR class, proplist, ret = NULL; - struct table *table = view->table[table_index]; + const struct table *table = view->table[table_index]; UINT row = view->result[result_index]; UINT num_keys, len;

@@ -1164,7 +1169,7 @@ static HRESULT map_view_index( const struct view *view, UINT index, UINT *table_ return S_OK; }

-struct table *get_view_table( const struct view *view, UINT index ) +const struct table *get_view_table( const struct view *view, UINT index ) { switch (view->type) { @@ -1185,7 +1190,7 @@ HRESULT get_propval( const struct view *view, UINT index, const WCHAR *name, VAR { HRESULT hr; UINT column, row, table_index, result_index; - struct table *table; + const struct table *table; VARTYPE vartype; void *val_ptr = NULL; LONGLONG val; @@ -1382,7 +1387,7 @@ HRESULT put_propval( const struct view *view, UINT index, const WCHAR *name, VAR { HRESULT hr; UINT row, column, table_index, result_index; - struct table *table; + const struct table *table; LONGLONG val;

if ((hr = map_view_index( view, index, &table_index, &result_index )) != S_OK) return hr; @@ -1412,7 +1417,7 @@ HRESULT get_properties( const struct view *view, UINT index, LONG flags, SAFEARR SAFEARRAY *sa; BSTR str; UINT i, table_index, result_index, count = 0; - struct table *table; + const struct table *table; HRESULT hr; LONG j = 0;

diff --git a/dlls/wbemprox/services.c b/dlls/wbemprox/services.c index 3b55a534de9..29d2d576614 100644 --- a/dlls/wbemprox/services.c +++ b/dlls/wbemprox/services.c @@ -886,7 +886,7 @@ static HRESULT WINAPI wbem_services_ExecMethod( struct path *path; WCHAR *str; class_method *func; - struct table *table; + const struct table *table; HRESULT hr;

TRACE( "%p, %s, %s, %#lx, %p, %p, %p, %p\n", iface, debugstr_w(strObjectPath), diff --git a/dlls/wbemprox/table.c b/dlls/wbemprox/table.c index 37aa546ad2b..0258a94cdda 100644 --- a/dlls/wbemprox/table.c +++ b/dlls/wbemprox/table.c @@ -346,45 +346,31 @@ void free_table( struct table *table ) free( (WCHAR *)table->name ); free_columns( (struct column *)table->columns, table->num_cols ); free( table->data ); - - table->cs.DebugInfo->Spare[0] = 0; - DeleteCriticalSection( &table->cs ); free( table ); }

-void release_table( struct table *table ) +void release_table( const struct table *ctable ) { - if (!--table->refs) - { - clear_table( table ); - if (table->flags & TABLE_FLAG_DYNAMIC) - { - EnterCriticalSection( &table_list_cs ); - list_remove( &table->entry ); - table->removed = TRUE; - LeaveCriticalSection( &table_list_cs ); + struct table *table = (struct table *)ctable;

- LeaveCriticalSection( &table->cs ); - free_table( table ); - return; - } - } - LeaveCriticalSection( &table->cs ); + if (InterlockedDecrement( &table->refs )) return; + clear_table( table ); + if (!(table->flags & TABLE_FLAG_DYNAMIC)) return; + EnterCriticalSection( &table_list_cs ); + list_remove( &table->entry ); + table->removed = TRUE; + LeaveCriticalSection( &table_list_cs ); + free_table( table ); }

-struct table *grab_table( struct table *table ) +const struct table *grab_table( const struct table *table ) { - EnterCriticalSection( &table->cs ); - if (table->removed) - { - LeaveCriticalSection( &table->cs ); - return NULL; - } - table->refs++; + if (table->removed) return NULL; + InterlockedIncrement( (LONG *)&table->refs ); return table; }

-struct table *find_table( enum wbm_namespace ns, const WCHAR *name ) +const struct table *find_table( enum wbm_namespace ns, const WCHAR *name ) { struct table *table;

@@ -419,11 +405,41 @@ struct table *create_table( const WCHAR *name, UINT num_cols, const struct colum table->refs = 0; table->removed = FALSE; list_init( &table->entry ); - InitializeCriticalSectionEx( &table->cs, 0, RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO ); - table->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": table.cs"); return table; }

+struct table *get_table_writeable_copy( const struct table *src ) +{ + struct table *table; + BYTE *data = NULL; + ULONG_PTR size; + unsigned int i; + + if (src->flags & TABLE_FLAG_DYNAMIC) + { + TRACE( "Table %p is dynamic.\n", src ); + return (struct table *)grab_table( src ); + } + + size = 0; + for (i = 0; i < src->num_cols; ++i) + size += get_column_size( src, i ); + size *= src->num_rows_allocated; + + if (size && src->data) + { + if (!(data = malloc( size ))) return NULL; + memcpy( data, src->data, size ); + } + if (!(table = create_table( src->name, src->num_cols, src->columns, src->num_rows, src->num_rows_allocated, + data, src->fill ))) + { + free( data ); + return NULL; + } + return (struct table *)grab_table( table ); +} + BOOL add_table( enum wbm_namespace ns, struct table *table ) { struct table *iter; @@ -449,7 +465,7 @@ BOOL add_table( enum wbm_namespace ns, struct table *table )

BSTR get_method_name( enum wbm_namespace ns, const WCHAR *class, UINT index ) { - struct table *table; + const struct table *table; UINT i, count = 0; BSTR ret;

@@ -474,7 +490,7 @@ BSTR get_method_name( enum wbm_namespace ns, const WCHAR *class, UINT index )

WCHAR *get_first_key_property( enum wbm_namespace ns, const WCHAR *class ) { - struct table *table; + const struct table *table; WCHAR *ret = NULL; UINT i;

diff --git a/dlls/wbemprox/wbemprox_private.h b/dlls/wbemprox/wbemprox_private.h index 5e585049aa6..164ba53f3a5 100644 --- a/dlls/wbemprox/wbemprox_private.h +++ b/dlls/wbemprox/wbemprox_private.h @@ -121,7 +121,6 @@ struct table UINT flags; struct list entry; LONG refs; - CRITICAL_SECTION cs; BOOL removed; };

@@ -154,7 +153,7 @@ struct record { UINT count; struct field *fields; - struct table *table; + const struct table *table; };

struct keyword @@ -179,7 +178,7 @@ struct view const struct property *proplist; /* SELECT query */ const struct expr *cond; UINT table_count; - struct table **table; + const struct table **table; UINT result_count; UINT *result; }; @@ -214,12 +213,13 @@ HRESULT create_view( enum view_type, enum wbm_namespace, const WCHAR *, const st const struct property *, const struct expr *, struct view ** ); void destroy_view( struct view * ); HRESULT execute_view( struct view * ); -struct table *get_view_table( const struct view *, UINT ); +const struct table *get_view_table( const struct view *, UINT ); void init_table_list( void ); enum wbm_namespace get_namespace_from_string( const WCHAR *namespace ); -struct table *find_table( enum wbm_namespace, const WCHAR * ); -struct table *grab_table( struct table * ); -void release_table( struct table * ); +const struct table *find_table( enum wbm_namespace, const WCHAR * ); +const struct table *grab_table( const struct table * ); +struct table *get_table_writeable_copy( const struct table * ); +void release_table( const struct table * ); struct table *create_table( const WCHAR *, UINT, const struct column *, UINT, UINT, BYTE *, enum fill_status (*)(struct table *, const struct expr *) ); BOOL add_table( enum wbm_namespace, struct table * );

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7668

Paul Gofman (＠gofman)

7:04 p.m.

This, in particular, fixes Farlight 84 getting locked in a thread performing wbemprox queries which results in voice chat not working.

The game seems to leak EnumClassObject returned from _ExecQuery on a different thread which does some wbemprox queries earlier (and succeeds). Then, doing wbemprox queries from another thread hangs that thread forever because a builtin table has CS locked by the previous thread (with table grabbed in the query's view). Leaking (or intentionally keeping) wbemprox objects shouldn't block different threads.

I think even without leaking any objects the current way of avoiding races on tables is problematic: - the query / table may be released from a thread different from which executed query / take CS; - if two threads are doing queries in parallel and access builtin tables in different order that may deadlock.

The attached patch gets rid of CS in the table and instead copies the table when needed (that seems to be just one place in query.c:exec_select_view(). The rest is boilerplate to make the table 'const', which is not strictly needed but I suppose that would be helpful to avoid accidental racing on table access in the future.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7668#note_99142

Age (days ago)

Last active (days ago)

wine-gitlab@winehq.org

2 comments

2 participants

tags (0)

participants (2)

Paul Gofman
Paul Gofman (＠gofman)