[PATCH 0/1] MR8078: windows.ui: Fix a double free.
The object reference is actually held by ref_weak. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8078
From: Zhiyi Zhang <zzhang(a)codeweavers.com> The object reference is actually held by ref_weak. --- dlls/windows.ui/uisettings.c | 1 - 1 file changed, 1 deletion(-) diff --git a/dlls/windows.ui/uisettings.c b/dlls/windows.ui/uisettings.c index a2b59c46cdf..05de9679a0b 100644 --- a/dlls/windows.ui/uisettings.c +++ b/dlls/windows.ui/uisettings.c @@ -108,7 +108,6 @@ static ULONG WINAPI uisettings_Release( IUISettings *iface ) TRACE( "iface %p, ref %lu.\n", iface, ref ); IWeakReference_Release( &impl->IWeakReference_iface ); - if (!ref) free( impl ); return ref; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8078
I think it still makes no sense. If it does support weak references, you should be able to release main object while weak reference is still alive. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8078#note_103803
Yeah, it's not really weak references. It just has the same interfaces. The main object doesn't really use that much memory, and IGeolocator has the same construct. So I was hoping it would be ok. Anyway, I guess I should do it your way. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8078#note_103804
It should be possible to test? Also, there it's very generic and could be reused, even if only at source level. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8078#note_103805
On Mon May 19 09:52:32 2025 +0000, Nikolay Sivov wrote:
It should be possible to test? Also, there it's very generic and could be reused, even if only at source level. Tests do show it's the weak reference object should be a separate object.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8078#note_103808
On Mon May 19 09:52:32 2025 +0000, Nikolay Sivov wrote:
It should be possible to test? Also, there it's very generic and could be reused, even if only at source level. The attached tests show that releasing the weak ref interface releases the main object as well. So the weak ref interface resides in the main object. As for releasing the main object while the weak ref is still alive. I guess you can release most of the allocated memory in uisettings_Release() when ref_strong is 0? It's just the wrapper object, for example `struct uisettings`, for the interfaces and refcount is still active.
[weakref-tests.diff](/uploads/805f58bc9206095968936f38039f9605/weakref-tests.diff) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8078#note_103819
On Mon May 19 10:35:52 2025 +0000, Zhiyi Zhang wrote:
The attached tests show that releasing the weak ref interface releases the main object as well. So the weak ref interface resides in the main object. As for releasing the main object while the weak ref is still alive. I guess you can release most of the allocated memory in uisettings_Release() when ref_strong is 0? It's just the wrapper object, for example `struct uisettings`, for the interfaces and refcount is still active. [weakref-tests.diff](/uploads/805f58bc9206095968936f38039f9605/weakref-tests.diff)
+ /* Test that weakref interface resides in the same object */
+ hr = RoActivateInstance( str, &inspectable );
+ ok( hr == S_OK, "Got unexpected hr %#lx.\n", hr );
+ ref = get_refcount(inspectable);
+ ok( ref == 1, "got ref %ld.\n", ref );
+
+ hr = IInspectable_QueryInterface( inspectable, &IID_IWeakReferenceSource, (void **)&weak_reference_source );
+ ok( hr == S_OK, "got hr %#lx.\n", hr );
+ hr = IWeakReferenceSource_GetWeakReference( weak_reference_source, &weak_reference );
+ ok( hr == S_OK, "got hr %#lx.\n", hr );
+ IWeakReferenceSource_Release( weak_reference_source );
+ ref = get_refcount(weak_reference);
+ ok( ref == 2, "got ref %ld.\n", ref );
+ ref = IWeakReference_Release( weak_reference );
+ ok( ref == 1, "got ref %ld.\n", ref );
+ ref = get_refcount(inspectable);
+ ok( ref == 1, "got ref %ld.\n", ref );
+
+ ref = IWeakReference_Release( weak_reference );
+ ok( ref == 0, "got ref %ld.\n", ref );
You can't do second release on something you don't own. You can only release as many times as you called GetWeakReference(). ``` + ref = get_refcount(inspectable); + /* Now ref for the main object is non-sense, meaning releasing weakref ends up releasing the main object as well */ + ok( ref == 1, "got ref %ld.\n", ref ); + ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8078#note_103824
Having the weak ref on the same object requires to split object destruction (which needs to happen in last strong ref release) from memory deallocation (which needs to happen in last weak ref release), but I think it should work otherwise (with this fix for the double free). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8078#note_103832
This object makes me nervous. To begin with, it looks like ref_weak counts both strong and weak references. Feels to me like ref_total would be a better name. Second, it looks to me like there's a race condition. The object starts with a ref_strong of 1 (ref_weak is irrelevant). Thread 1 enters weak_reference_Resolve() and IUISettings_QueryInterface(). ref_strong is now 2. Thread 2 releases the strong reference. ref_strong is now 1. Thread 1 finishes in IUISettings_QueryInterface(). It did nothing because the IID was bad; ref_strong is still 1. Thread 1 decrements ref_strong. It's now 0, but the InterlockedDecrement return value was discarded, and nobody noticed that that was the last strong reference. Doesn't matter yet, because the object doesn't own any resources of its own, but could easily start mattering; I think that decrement should be changed to uisettings_Release(). Looks like that weak_reference_Resolve() impl is copied from dlls/geolocation/main.c; that object too makes me nervous, for the same reasons. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8078#note_103838
To begin with, it looks like ref_weak counts both strong and weak references. Feels to me like ref_total would be a better name.
The weak/strong names are how it's often called and it works the same way: strong ref have an implicit weak ref too. [^1] Then yes, Resolve seems to be bogus and it is as well in IGeolocation, as it takes a strong reference without the implicit weak one and as it then releases its strong reference without going through `Release`, missing the eventual object destruction. [^1]: For instance taking https://doc.rust-lang.org/std/rc/struct.Weak.html as a reference. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8078#note_103839
participants (5)
-
Alfred Agrell (＠Alcaro) -
Nikolay Sivov (＠nsivov)
-
Rémi Bernon
-
Zhiyi Zhang
-
Zhiyi Zhang (＠zhiyi)