[objcruntime] Reduce duplication for GetCheckedHandle and fix it/them - #14084
Merged
Conversation
Not sure why it was added to `DisposableObject` but it seems the compiler is not happy to use the extension method for the `NativeObject` subclass even if it implement `INativeObject` (that might be the reason why?) So remove it from `NativeObjectExtensions` for .net - as I guess it's too late to remove from the recent `DisposableObject` in legacy. The platform assemblies only use it for `NativeObject` subclass so it's not an big issue... as I assume there's not many `INativeObject` user code that does not use (or should be using) `NativeObject`. Fix #1: avoid using `IntPtr` when `NativeHandle` is defined since the compiler will add (not really needed) implicit conversions everywhere. Fix #2: ensure the spirit/contract of `GetCheckedHandle` always work. IOW the goal of `GetCheckedHandle` is to ensure you never get a `nil` handle, since some native API will crash if they given `nil` However it's still currently possible, with multithreaded code, to get a `Zero` value from `GetCheckedHandle`. From a quick look at the C# code it seems fine. It's easier to see the problem looking at the IL. ```il .method public hidebysig instance valuetype ObjCRuntime.NativeHandle GetCheckedHandle () cil managed { // Method begins at RVA 0x6c8c // Header size: 1 // Code size: 31 (0x1f) .maxstack 8 IL_0000: ldarg.0 IL_0001: ldfld valuetype ObjCRuntime.NativeHandle ObjCRuntime.DisposableObject::handle IL_0006: ldsfld valuetype ObjCRuntime.NativeHandle ObjCRuntime.NativeHandle::Zero IL_000b: call bool ObjCRuntime.NativeHandle::op_Equality(valuetype ObjCRuntime.NativeHandle, valuetype ObjCRuntime.NativeHandle) IL_0010: brfalse.s IL_0018 IL_0012: ldarg.0 IL_0013: call void ObjCRuntime.ThrowHelper::ThrowObjectDisposedException(object) IL_0018: ldarg.0 IL_0019: ldfld valuetype ObjCRuntime.NativeHandle ObjCRuntime.DisposableObject::handle IL_001e: ret } // end of method DisposableObject::GetCheckedHandle ``` `IL_0001` loads the field and then `IL_006` compares it to `Zero`. If it's not `Zero` then `IL_0019` loads the field **again** before returning it `IL_0001e` to the caller. So if another thread sets the `handle` of this instance to `Zero` between the two `ldfld` instructions then the caller will get `Zero` and this can crash the application. Of course what will happen if you use the _old_ handle is _somewhat_ undefined and depends if the native object still exists. There's a **very** good chance it does it the managed object i running (so it has not been collected).
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Collaborator
✅ [PR Build] Tests passed on Build. ✅Tests passed on Build. API diff✅ API Diff from stable View dotnet API diffView dotnet legacy API diffView dotnet iOS-MacCatalayst API diff
API Current PR diffℹ️ API Diff (from PR only) (please review changes) View dotnet API diffView dotnet legacy API diffView dotnet iOS-MacCatalayst API diff
Generator diff✅ Generator Diff (no change) GitHub pagesResults can be found in the following github pages (it might take some time to publish): 🎉 All 108 tests passed 🎉Pipeline on Agent XAMBOT-1096.BigSur' |
rolfbjarne
approved these changes
Feb 7, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Not sure why it was added to
DisposableObjectbut it seems the compileris not happy to use the extension method for the
NativeObjectsubclasseven if it implement
INativeObject(that might be the reason why?)So remove it from
NativeObjectExtensionsfor .net - as I guess it's toolate to remove from the recent
DisposableObjectin legacy.The platform assemblies only use it for
NativeObjectsubclass so it'snot an big issue... as I assume there's not many
INativeObjectusercode that does not use (or should be using)
NativeObject.Fix 1: avoid using
IntPtrwhenNativeHandleis defined since thecompiler will add (not really needed) implicit conversions everywhere.
Fix 2: ensure the spirit/contract of
GetCheckedHandlealways work.IOW the goal of
GetCheckedHandleis to ensure you never get anilhandle, since some native API will crash if they given
nilHowever it's still currently possible, with multithreaded code, to get
a
Zerovalue fromGetCheckedHandle.From a quick look at the C# code it seems fine. It's easier to see the
problem looking at the IL.
IL_0001loads the field and thenIL_006compares it toZero.If it's not
ZerothenIL_0019loads the field again beforereturning it
IL_0001eto the caller.So if another thread sets the
handleof this instance toZerobetweenthe two
ldfldinstructions then the caller will getZeroand this cancrash the application. Of course what will happen if you use the old
handle is somewhat undefined and depends if the native object still
exists. There's a very good chance it does it the managed object i
running (so it has not been collected).