[objcruntime] Add helper to check for disposed instances (#8679)
We're handling `ObjectDisposedException` a bit inconsistently in our
bindings.
Some, but not all, manual bindings have checks (not all of them are fully
consistent) but there's none for generated bindings, which can lead to
errors wrt nullability.
**Example**
```
[Test]
public void IncompleteNullabilityCheck ()
{
NSString s = new NSString ("bonjour");
s.Dispose ();
ManagedLayer (s);
}
void ManagedLayer (NSString s)
{
// this is similar to the generated binding code
if (s == null)
throw new ArgumentNullException ("s");
NativeCode (s.Handle);
}
void NativeCode (IntPtr p)
{
// let's assume this is native and dereference the pointer
if (p == IntPtr.Zero)
Assert.Fail ("boo");
}
```
This shows that we can, _if disposed_, provide `nil` to a native API for
which we know `nil` is not a valid argument. That can crash a process in
a situation that was possible to detect and throw a (catchable) managed
exception.
Adding dispose checks everywhere could be costly (in size) unless we
share that code with the null check (and a few other optimizations could
be applied too). Sharing the code would also ensure more consistency
across all bindings.
**Impact**
Here's the IL for an existing `CGColor` constructor that *already*
does the dispose check.
```
.method public hidebysig specialname rtspecialname
instance void .ctor (
class CoreGraphics.CGColorSpace colorspace,
valuetype System.nfloat[] components
) cil managed
{
// Method begins at RVA 0x20c834
// Code size 82 (0x52)
.maxstack 3
IL_0000: ldarg.0
IL_0001: call instance void [mscorlib]System.Object::.ctor()
IL_0006: ldarg.2
IL_0007: brtrue.s IL_0014
IL_0009: ldstr "components"
IL_000e: newobj instance void [mscorlib]System.ArgumentNullException::.ctor(string)
IL_0013: throw
IL_0014: ldarg.1
IL_0015: brtrue.s IL_0022
IL_0017: ldstr "colorspace"
IL_001c: newobj instance void [mscorlib]System.ArgumentNullException::.ctor(string)
IL_0021: throw
IL_0022: ldarg.1
IL_0023: ldfld native int CoreGraphics.CGColorSpace::handle
IL_0028: ldsfld native int [mscorlib]System.IntPtr::Zero
IL_002d: call bool [mscorlib]System.IntPtr::op_Equality(native int, native int)
IL_0032: brfalse.s IL_003f
IL_0034: ldstr "colorspace"
IL_0039: newobj instance void [mscorlib]System.ObjectDisposedException::.ctor(string)
IL_003e: throw
IL_003f: ldarg.0
IL_0040: ldarg.1
IL_0041: ldfld native int CoreGraphics.CGColorSpace::handle
IL_0046: ldarg.2
IL_0047: call native int CoreGraphics.CGColor::CGColorCreate(native int, valuetype System.nfloat[])
IL_004c: stfld native int CoreGraphics.CGColor::handle
IL_0051: ret
} // end of method CGColor::.ctor
Here's the IL code (as committed) using a helper extension method.
```
.method public hidebysig specialname rtspecialname
instance void .ctor (
class CoreGraphics.CGColorSpace colorspace,
valuetype System.nfloat[] components
) cil managed
{
// Method begins at RVA 0x210558
// Code size 46 (0x2e)
.maxstack 3
.locals init (
[0] native int handleof_colorspace
)
IL_0000: ldarg.0
IL_0001: call instance void [mscorlib]System.Object::.ctor()
IL_0006: ldarg.2
IL_0007: brtrue.s IL_0014
IL_0009: ldstr "components"
IL_000e: newobj instance void [mscorlib]System.ArgumentNullException::.ctor(string)
IL_0013: throw
IL_0014: ldarg.1
IL_0015: ldstr "colorspace"
IL_001a: call native int ObjCRuntime.NativeObjectHelper::GetNonNullHandle(class ObjCRuntime.INativeObject, string)
IL_001f: stloc.0
IL_0020: ldarg.0
IL_0021: ldloc.0
IL_0022: ldarg.2
IL_0023: call native int CoreGraphics.CGColor::CGColorCreate(native int, valuetype System.nfloat[])
IL_0028: stfld native int CoreGraphics.CGColor::handle
IL_002d: ret
} // end of method CGColor::.ctor
```
So existing checks becomes a lot smaller. Here's another example where
a dispose check is **missing** (on `pattern`). Note that the check for
`colorspace` is not updated so we can see the impact of adding missing
dispose checks in existing code (manual or generated).
```
.method public hidebysig specialname rtspecialname
instance void .ctor (
class CoreGraphics.CGColorSpace colorspace,
class CoreGraphics.CGPattern pattern,
valuetype System.nfloat[] components
) cil managed
{
// Method begins at RVA 0x210660
// Code size 126 (0x7e)
.maxstack 4
IL_0000: ldarg.0
IL_0001: call instance void [mscorlib]System.Object::.ctor()
IL_0006: ldarg.1
IL_0007: brtrue.s IL_0014
IL_0009: ldstr "colorspace"
IL_000e: newobj instance void [mscorlib]System.ArgumentNullException::.ctor(string)
IL_0013: throw
IL_0014: ldarg.1
IL_0015: ldfld native int CoreGraphics.CGColorSpace::handle
IL_001a: ldsfld native int [mscorlib]System.IntPtr::Zero
IL_001f: call bool [mscorlib]System.IntPtr::op_Equality(native int, native int)
IL_0024: brfalse.s IL_0031
IL_0026: ldstr "colorspace"
IL_002b: newobj instance void [mscorlib]System.ObjectDisposedException::.ctor(string)
IL_0030: throw
IL_0031: ldarg.2
IL_0032: brtrue.s IL_003f
IL_0034: ldstr "pattern"
IL_0039: newobj instance void [mscorlib]System.ArgumentNullException::.ctor(string)
IL_003e: throw
IL_003f: ldarg.3
IL_0040: brtrue.s IL_004d
IL_0042: ldstr "components"
IL_0047: newobj instance void [mscorlib]System.ArgumentNullException::.ctor(string)
IL_004c: throw
IL_004d: ldarg.0
IL_004e: ldarg.1
IL_004f: ldfld native int CoreGraphics.CGColorSpace::handle
IL_0054: ldarg.2
IL_0055: callvirt instance native int CoreFoundation.NativeObject::get_Handle()
IL_005a: ldarg.3
IL_005b: call native int CoreGraphics.CGColor::CGColorCreateWithPattern(native int, native int, valuetype System.nfloat[])
IL_0060: stfld native int CoreGraphics.CGColor::handle
IL_0065: ldarg.0
IL_0066: ldfld native int CoreGraphics.CGColor::handle
IL_006b: ldsfld native int [mscorlib]System.IntPtr::Zero
IL_0070: call bool [mscorlib]System.IntPtr::op_Equality(native int, native int)
IL_0075: brfalse.s IL_007d
IL_0077: newobj instance void [mscorlib]System.ArgumentException::.ctor()
IL_007c: throw
IL_007d: ret
} // end of method CGColor::.ctor
```
into
```
.method public hidebysig specialname rtspecialname
instance void .ctor (
class CoreGraphics.CGColorSpace colorspace,
class CoreGraphics.CGPattern pattern,
valuetype System.nfloat[] components
) cil managed
{
// Method begins at RVA 0x210660
// Code size 119 (0x77)
.maxstack 4
.locals init (
[0] native int handleof_pattern
)
IL_0000: ldarg.0
IL_0001: call instance void [mscorlib]System.Object::.ctor()
IL_0006: ldarg.1
IL_0007: brtrue.s IL_0014
IL_0009: ldstr "colorspace"
IL_000e: newobj instance void [mscorlib]System.ArgumentNullException::.ctor(string)
IL_0013: throw
IL_0014: ldarg.1
IL_0015: ldfld native int CoreGraphics.CGColorSpace::handle
IL_001a: ldsfld native int [mscorlib]System.IntPtr::Zero
IL_001f: call bool [mscorlib]System.IntPtr::op_Equality(native int, native int)
IL_0024: brfalse.s IL_0031
IL_0026: ldstr "colorspace"
IL_002b: newobj instance void [mscorlib]System.ObjectDisposedException::.ctor(string)
IL_0030: throw
IL_0031: ldarg.3
IL_0032: brtrue.s IL_003f
IL_0034: ldstr "components"
IL_0039: newobj instance void [mscorlib]System.ArgumentNullException::.ctor(string)
IL_003e: throw
IL_003f: ldarg.2
IL_0040: ldstr "pattern"
IL_0045: call native int ObjCRuntime.NativeObjectHelper::GetNonNullHandle(class ObjCRuntime.INativeObject, string)
IL_004a: stloc.0
IL_004b: ldarg.0
IL_004c: ldarg.1
IL_004d: ldfld native int CoreGraphics.CGColorSpace::handle
IL_0052: ldloc.0
IL_0053: ldarg.3
IL_0054: call native int CoreGraphics.CGColor::CGColorCreateWithPattern(native int, native int, valuetype System.nfloat[])
IL_0059: stfld native int CoreGraphics.CGColor::handle
IL_005e: ldarg.0
IL_005f: ldfld native int CoreGraphics.CGColor::handle
IL_0064: ldsfld native int [mscorlib]System.IntPtr::Zero
IL_0069: call bool [mscorlib]System.IntPtr::op_Equality(native int, native int)
IL_006e: brfalse.s IL_0076
IL_0070: newobj instance void [mscorlib]System.ArgumentException::.ctor()
IL_0075: throw
IL_0076: ret
} // end of method CGColor::.ctor
```
so the difference is smaller, but it's still smaller (7 bytes) than the
existing code (that did not check for a disposed instance).
There are many more optimizations that can be applied on top of this.
However the main/original goal is to have a correct and consistent
handling of disposed managed instances. IOW the size reduction is a nice
side-effect of correctness and actual optimizations can come later :)
Add `ObjectDisposedException` when required on Handle access.
Instead of providing `nil` to native API that are decorated **not**
accepting them.
Also do this using existing and new extension method helpers so the IL
size of the assemblies is (a bit) smaller than the existing code.
The latter has a small, positive impact for Xamarin.Mac (JIT'ing or
requiring IL for, non-full, AOT'ing) and for Xamarin.iOS when the
interpreter is used.
This commit is contained in:
Sebastien Pouliot
GitHub
Родитель
d51d34a0eb
Коммит
2d7edf555f
|
|
@ -85,13 +85,10 @@ namespace CoreGraphics {
|
|||
public CGColor (CGColorSpace colorspace, nfloat [] components)
|
||||
{
|
||||
if (components == null)
|
||||
throw new ArgumentNullException ("components");
|
||||
if (colorspace == null)
|
||||
throw new ArgumentNullException ("colorspace");
|
||||
if (colorspace.handle == IntPtr.Zero)
|
||||
throw new ObjectDisposedException ("colorspace");
|
||||
global::ObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (components));
|
||||
var colorspace_handle = colorspace.GetNonNullHandle (nameof (colorspace));
|
||||
|
||||
handle = CGColorCreate (colorspace.handle, components);
|
||||
handle = CGColorCreate (colorspace_handle, components);
|
||||
}
|
||||
|
||||
[DllImport(Constants.CoreGraphicsLibrary)]
|
||||
|
|
@ -148,16 +145,12 @@ namespace CoreGraphics {
|
|||
|
||||
public CGColor (CGColorSpace colorspace, CGPattern pattern, nfloat [] components)
|
||||
{
|
||||
if (colorspace == null)
|
||||
throw new ArgumentNullException ("colorspace");
|
||||
if (colorspace.handle == IntPtr.Zero)
|
||||
throw new ObjectDisposedException ("colorspace");
|
||||
if (pattern == null)
|
||||
throw new ArgumentNullException ("pattern");
|
||||
if (components == null)
|
||||
throw new ArgumentNullException ("components");
|
||||
global::ObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (components));
|
||||
var colorspace_handle = colorspace.GetNonNullHandle (nameof (colorspace));
|
||||
var pattern_handle = pattern.GetNonNullHandle (nameof (pattern));
|
||||
|
||||
handle = CGColorCreateWithPattern (colorspace.handle, pattern.Handle, components);
|
||||
handle = CGColorCreateWithPattern (colorspace_handle, pattern_handle, components);
|
||||
if (handle == IntPtr.Zero)
|
||||
throw new ArgumentException ();
|
||||
}
|
||||
|
|
|
|||
|
|
@ -92,22 +92,22 @@ namespace CoreGraphics {
|
|||
if (triples.Length > 3)
|
||||
throw new ArgumentException ("A maximum of 3 triples are supported");
|
||||
|
||||
IntPtr o = NativeObjectHelper.GetHandle (options);
|
||||
IntPtr o = options.GetHandle ();
|
||||
var first = triples [0]; // there's always one
|
||||
var second = triples.Length > 1 ? triples [1] : empty;
|
||||
var third = triples.Length > 2 ? triples [2] : empty;
|
||||
#if !MONOMAC
|
||||
if (Runtime.IsARM64CallingConvention) {
|
||||
Handle = CGColorConversionInfoCreateFromList_arm64 (o, NativeObjectHelper.GetHandle (first.Space), (uint) first.Transform, (int) first.Intent,
|
||||
Handle = CGColorConversionInfoCreateFromList_arm64 (o, first.Space.GetHandle (), (uint) first.Transform, (int) first.Intent,
|
||||
IntPtr.Zero, IntPtr.Zero, IntPtr.Zero, IntPtr.Zero,
|
||||
NativeObjectHelper.GetHandle (second.Space), (uint) second.Transform, (int) second.Intent,
|
||||
NativeObjectHelper.GetHandle (third.Space), (uint) third.Transform, (int) third.Intent,
|
||||
second.Space.GetHandle (), (uint) second.Transform, (int) second.Intent,
|
||||
third.Space.GetHandle (), (uint) third.Transform, (int) third.Intent,
|
||||
IntPtr.Zero);
|
||||
} else {
|
||||
#endif
|
||||
Handle = CGColorConversionInfoCreateFromList (o, NativeObjectHelper.GetHandle (first.Space), first.Transform, first.Intent,
|
||||
NativeObjectHelper.GetHandle (second.Space), second.Transform, second.Intent,
|
||||
NativeObjectHelper.GetHandle (third.Space), third.Transform, third.Intent,
|
||||
Handle = CGColorConversionInfoCreateFromList (o, first.Space.GetHandle (), first.Transform, first.Intent,
|
||||
second.Space.GetHandle (), second.Transform, second.Intent,
|
||||
third.Space.GetHandle (), third.Transform, third.Intent,
|
||||
IntPtr.Zero);
|
||||
#if !MONOMAC
|
||||
}
|
||||
|
|
|
|||
|
|
@ -12,7 +12,7 @@ namespace ObjCRuntime {
|
|||
}
|
||||
|
||||
#if !COREBUILD
|
||||
static class NativeObjectHelper {
|
||||
public static class NativeObjectExtensions {
|
||||
|
||||
// help to avoid the (too common pattern)
|
||||
// var p = x == null ? IntPtr.Zero : x.Handle;
|
||||
|
|
@ -20,6 +20,15 @@ namespace ObjCRuntime {
|
|||
{
|
||||
return self == null ? IntPtr.Zero : self.Handle;
|
||||
}
|
||||
|
||||
static public IntPtr GetNonNullHandle (this INativeObject self, string argumentName)
|
||||
{
|
||||
if (self == null)
|
||||
ThrowHelper.ThrowArgumentNullException (argumentName);
|
||||
if (self.Handle == IntPtr.Zero)
|
||||
ThrowHelper.ThrowObjectDisposedException (self);
|
||||
return self.Handle;
|
||||
}
|
||||
}
|
||||
#endif
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
using System;
|
||||
using System.ComponentModel;
|
||||
// the linker will remove the attributes
|
||||
using System.Diagnostics.CodeAnalysis;
|
||||
|
||||
|
|
@ -6,10 +7,17 @@ using System.Diagnostics.CodeAnalysis;
|
|||
|
||||
namespace ObjCRuntime {
|
||||
|
||||
static class ThrowHelper {
|
||||
[EditorBrowsable (EditorBrowsableState.Never)]
|
||||
public static class ThrowHelper {
|
||||
|
||||
[DoesNotReturn]
|
||||
static internal void ThrowObjectDisposedException (object o)
|
||||
public static void ThrowArgumentNullException (string argumentName)
|
||||
{
|
||||
throw new ArgumentNullException (argumentName);
|
||||
}
|
||||
|
||||
[DoesNotReturn]
|
||||
public static void ThrowObjectDisposedException (object o)
|
||||
{
|
||||
throw new ObjectDisposedException (o.GetType ().ToString ());
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1626,7 +1626,7 @@ public partial class Generator : IMemberGatherer {
|
|||
}
|
||||
else if (IsWrappedType (mi.ReturnType)) {
|
||||
returntype = "IntPtr";
|
||||
returnformat = "return {0} != null ? {0}.Handle : IntPtr.Zero;";
|
||||
returnformat = "return {0}.GetHandle ();";
|
||||
} else if (mi.ReturnType == TypeManager.System_String) {
|
||||
returntype = "IntPtr";
|
||||
returnformat = "return NSString.CreateNative ({0}, true);";
|
||||
|
|
@ -1818,11 +1818,8 @@ public partial class Generator : IMemberGatherer {
|
|||
|
||||
var safe_name = pi.Name.GetSafeParamName ();
|
||||
|
||||
if (IsWrappedType (pi.ParameterType)){
|
||||
if (null_allowed_override || AttributeManager.HasAttribute<NullAllowedAttribute> (pi))
|
||||
return String.Format ("{0} == null ? IntPtr.Zero : {0}.Handle", safe_name);
|
||||
return safe_name + ".Handle";
|
||||
}
|
||||
if (IsWrappedType (pi.ParameterType))
|
||||
return safe_name + "__handle__";
|
||||
|
||||
if (enum_mode != EnumMode.Compat && enum_mode != EnumMode.NativeBits && pi.ParameterType.IsEnum)
|
||||
return "(" + PrimitiveType (pi.ParameterType, enum_mode: enum_mode) + ")" + safe_name;
|
||||
|
|
@ -1856,10 +1853,9 @@ public partial class Generator : IMemberGatherer {
|
|||
|
||||
MarshalType mt;
|
||||
if (LookupMarshal (pi.ParameterType, out mt)){
|
||||
string access = String.Format (mt.ParameterMarshal, safe_name);
|
||||
if (null_allowed_override || AttributeManager.HasAttribute<NullAllowedAttribute> (pi))
|
||||
return String.Format ("{0} == null ? IntPtr.Zero : {1}", safe_name, access);
|
||||
return access;
|
||||
return safe_name + "__handle__";
|
||||
return String.Format (mt.ParameterMarshal, safe_name);
|
||||
}
|
||||
|
||||
if (pi.ParameterType.IsArray){
|
||||
|
|
@ -1914,6 +1910,9 @@ public partial class Generator : IMemberGatherer {
|
|||
if (AttributeManager.HasAttribute<NullAllowedAttribute> (mi)) {
|
||||
return false;
|
||||
}
|
||||
if ((propInfo != null) && AttributeManager.HasAttribute<NullAllowedAttribute> (propInfo)) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
var bindAsAtt = GetBindAsAttribute (pi) ?? GetBindAsAttribute (propInfo);
|
||||
|
|
@ -4173,35 +4172,37 @@ public partial class Generator : IMemberGatherer {
|
|||
ErrorHelper.Show (new BindingException (1118, false, mi));
|
||||
|
||||
foreach (var pi in mi.GetParameters ()) {
|
||||
var safe_name = pi.Name.GetSafeParamName ();
|
||||
bool protocolize = Protocolize (pi);
|
||||
if (!BindThirdPartyLibrary) {
|
||||
if (!mi.IsSpecialName && IsModel (pi.ParameterType) && !Protocolize (pi)) {
|
||||
if (!mi.IsSpecialName && IsModel (pi.ParameterType) && !protocolize) {
|
||||
// don't warn on obsoleted API, there's likely a new version that fix this
|
||||
// any no good reason for using the obsolete API anyway
|
||||
if (!AttributeManager.HasAttribute <ObsoleteAttribute> (mi) && !AttributeManager.HasAttribute<ObsoleteAttribute> (mi.DeclaringType))
|
||||
ErrorHelper.Warning (1106,
|
||||
mi.DeclaringType, mi.Name, pi.Name, pi.ParameterType, pi.ParameterType.Namespace, pi.ParameterType.Name);
|
||||
ErrorHelper.Warning (1106, mi.DeclaringType, mi.Name, pi.Name, pi.ParameterType, pi.ParameterType.Namespace, pi.ParameterType.Name);
|
||||
}
|
||||
}
|
||||
|
||||
if (null_allowed_override)
|
||||
continue;
|
||||
|
||||
var needs_null_check = ParameterNeedsNullCheck (pi, mi, propInfo);
|
||||
if (!needs_null_check)
|
||||
continue;
|
||||
|
||||
var safe_name = pi.Name.GetSafeParamName ();
|
||||
|
||||
if (Protocolize (pi)) {
|
||||
print ("if ({0} != null){{", safe_name);
|
||||
if (protocolize) {
|
||||
print ("if ({0} != null) {{", safe_name);
|
||||
print ("\tif (!({0} is NSObject))\n", safe_name);
|
||||
print ("\t\tthrow new ArgumentException (\"The object passed of type \" + {0}.GetType () + \" does not derive from NSObject\");", safe_name);
|
||||
print ("} else {");
|
||||
print ("\tthrow new ArgumentNullException (nameof ({0}));", safe_name);
|
||||
print ("}");
|
||||
} else {
|
||||
}
|
||||
|
||||
var cap = propInfo?.SetMethod == mi ? (ICustomAttributeProvider) propInfo : (ICustomAttributeProvider) pi;
|
||||
var bind_as = GetBindAsAttribute (cap);
|
||||
var pit = bind_as == null ? pi.ParameterType : bind_as.Type;
|
||||
if (IsWrappedType (pit) || TypeManager.INativeObject.IsAssignableFrom (pit)) {
|
||||
if (needs_null_check && !null_allowed_override) {
|
||||
print ($"var {safe_name}__handle__ = {safe_name}.GetNonNullHandle (nameof ({safe_name}));");
|
||||
} else {
|
||||
print ($"var {safe_name}__handle__ = {safe_name}.GetHandle ();");
|
||||
}
|
||||
} else if (needs_null_check) {
|
||||
print ("if ({0} == null)", safe_name);
|
||||
print ("\tthrow new ArgumentNullException (nameof ({0}));", safe_name);
|
||||
print ("\tObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof ({0}));", safe_name);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -4279,7 +4280,7 @@ public partial class Generator : IMemberGatherer {
|
|||
|
||||
Inject<PrologueSnippetAttribute> (mi);
|
||||
|
||||
GenerateArgumentChecks (mi, null_allowed_override, propInfo);
|
||||
GenerateArgumentChecks (mi, false, propInfo);
|
||||
|
||||
// Collect all strings that can be fast-marshalled
|
||||
List<string> stringParameters = CollectFastStringMarshalParameters (mi);
|
||||
|
|
@ -5376,7 +5377,7 @@ public partial class Generator : IMemberGatherer {
|
|||
if (pinfo != null)
|
||||
null_allowed = AttributeManager.HasAttribute<NullAllowedAttribute> (pinfo);
|
||||
}
|
||||
GenerateMethodBody (minfo, minfo.method, minfo.selector, null_allowed, null, BodyOption.None, null);
|
||||
GenerateMethodBody (minfo, minfo.method, minfo.selector, null_allowed, null, BodyOption.None, pinfo);
|
||||
if (minfo.is_autorelease) {
|
||||
print ("}");
|
||||
indent--;
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче