[security] Add nullability to (generated and manual) bindings #14884

tj-devel709 · 2022-05-03T18:27:03Z

This PR aims to bring nullability changes to Security.
Following the steps here:

I am adding nullable enable to all manual files that are not "API_SOURCES" in src/frameworks.sources and making the required nullability changes
Changing all throw new ArgumentNullException ("object")); to ObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (object)); for size saving optimization as well to mark that this framework contains nullability changes
Changing any == null or != null to is null and is not null

tj-devel709 · 2022-05-03T18:27:52Z

src/Security/SslConnection.cs

-			var c = (SslConnection) GCHandle.FromIntPtr (connection).Target;
-			return c.Read (data, ref System.Runtime.CompilerServices.Unsafe.AsRef<nint> (dataLength));
+			var c = GCHandle.FromIntPtr (connection).Target as SslConnection;
+			return c!.Read (data, ref System.Runtime.CompilerServices.Unsafe.AsRef<nint> (dataLength));


Does this look okay?

Why do we need the c! rather than doing an as, do an is, correct? Then you get the ! out.

@mandel-macaque if I do something like this:

if (GCHandle.FromIntPtr (connection).Target is SslConnection c) return c.Read (data, ref System.Runtime.CompilerServices.Unsafe.AsRef<nint> (dataLength));

Then I am a little confused on what I should return if GCHandle.FromIntPtr (connection).Target is not SslConnection unless I should just throw an exception?

I think and invalid cast exception would be a good option https://docs.microsoft.com/en-us/dotnet/api/system.invalidcastexception?view=net-6.0 but we should ping @rolfbjarne

This is called from native code, so any exception is probably going to have rather disastrous consequences.

On the other hand, if this code fails, something disastrous already happened, so surfacing that disaster might be the best thing to do.

My vote goes for not changing the behavior (when in doubt, keep doing the same thing is usually the safest choice), so something like this:

var c = (SslConnection) GCHandle.FromIntPtr (connection).Target!; return c.Read (data, ref System.Runtime.CompilerServices.Unsafe.AsRef<nint> (dataLength));

mandel-macaque · 2022-05-03T20:31:35Z

src/Security/SecProtocolMetadata.cs

- 		static sec_protocol_metadata_access_distinguished_names_handler_t static_DistinguishedNamesForPeer = TrampolineDistinguishedNamesForPeer;
-
- 		[MonoPInvokeCallback (typeof (sec_protocol_metadata_access_distinguished_names_handler_t))]
- 		static void TrampolineDistinguishedNamesForPeer (IntPtr block, IntPtr data)
- 		{
- 			var del = BlockLiteral.GetTarget<Action<DispatchData>> (block);
- 			if (del != null) {
- 				var dispatchData = new DispatchData (data, owns: false);
- 				del (dispatchData);
- 			}
- 		}
-
- 		[DllImport (Constants.SecurityLibrary)]


Do I agree with the re-indentation, yes. Does it make the review harder, yes. Can we do the reindentation after.

mandel-macaque · 2022-05-03T20:31:59Z

src/Security/SecProtocolMetadata.cs

- 		}
-
- 		[DllImport (Constants.SecurityLibrary)]
+		static sec_protocol_metadata_access_supported_signature_algorithms_handler_t static_SignatureAlgorithmsForPeer = TrampolineSignatureAlgorithmsForPeer;


mandel-macaque · 2022-05-03T20:32:08Z

src/Security/SecProtocolMetadata.cs


- 		[BindingImpl (BindingImplOptions.Optimizable)]
- 		public void SetSignatureAlgorithmsForPeerHandler (Action<ushort> callback)
- 		{


mandel-macaque · 2022-05-03T20:34:07Z

src/Security/SslConnection.cs

-			var c = (SslConnection) GCHandle.FromIntPtr (connection).Target;
-			return c.Read (data, ref System.Runtime.CompilerServices.Unsafe.AsRef<nint> (dataLength));
+			var c = GCHandle.FromIntPtr (connection).Target as SslConnection;
+			return c!.Read (data, ref System.Runtime.CompilerServices.Unsafe.AsRef<nint> (dataLength));


Why do we need the c! rather than doing an as, do an is, correct? Then you get the ! out.

mandel-macaque · 2022-05-03T20:34:14Z

src/Security/SslConnection.cs

-			var c = (SslConnection) GCHandle.FromIntPtr (connection).Target;
-			return c.Write (data, ref System.Runtime.CompilerServices.Unsafe.AsRef<nint> (dataLength));
+			var c = GCHandle.FromIntPtr (connection).Target as SslConnection;
+			return c!.Write (data, ref System.Runtime.CompilerServices.Unsafe.AsRef<nint> (dataLength));


tj-devel709 · 2022-05-06T17:55:27Z

@rolfbjarne @mandel-macaque Ready for re-review!

vs-mobiletools-engineering-service2 · 2022-05-10T15:05:21Z

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

✅ All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Hash: be668c7319d8788f7dc0a5fea65e4f36fb5d23c6

vs-mobiletools-engineering-service2 · 2022-05-10T15:07:15Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1039.Monterey'
Hash: be668c7319d8788f7dc0a5fea65e4f36fb5d23c6

vs-mobiletools-engineering-service2 · 2022-05-10T15:07:34Z

📋 [PR Build] API Diff 📋

API Current PR diff

ℹ️ API Diff (from PR only) (please review changes)

View API diff

iOS vsdrops gist
macOS vsdrops gist
tvOS vsdrops gist
watchOS vsdrops gist

View dotnet API diff

dotnet-iOS vsdrops gist
dotnet-macOS vsdrops gist
dotnet-tvOS vsdrops gist
dotnet-MacCatalyst vsdrops gist

View dotnet legacy API diff

dotnet-legacy-iOS vsdrops gist
dotnet-legacy-macOS vsdrops gist
dotnet-legacy-tvOS vsdrops gist

View dotnet iOS-MacCatalayst API diff

dotnet-macCatiOS vsdrops gist

API diff

✅ API Diff from stable

View API diff

iOS vsdrops gist
macOS vsdrops gist
tvOS vsdrops gist
watchOS vsdrops gist

View dotnet API diff

dotnet-iOS vsdrops gist
dotnet-macOS vsdrops gist
dotnet-tvOS vsdrops gist
dotnet-MacCatalyst vsdrops gist

View dotnet legacy API diff

dotnet-legacy-iOS vsdrops gist
dotnet-legacy-macOS vsdrops gist
dotnet-legacy-tvOS vsdrops gist

View dotnet iOS-MacCatalayst API diff

dotnet-macCatiOS vsdrops gist

Generator diff

✅ Generator Diff (no change)

Pipeline on Agent XAMBOT-1100.Monterey'
Hash: be668c7319d8788f7dc0a5fea65e4f36fb5d23c6

vs-mobiletools-engineering-service2 · 2022-05-10T15:22:25Z

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

Failed tests are:

dontlink
linksdk
linkall

Pipeline on Agent
Hash: be668c7319d8788f7dc0a5fea65e4f36fb5d23c6

vs-mobiletools-engineering-service2 · 2022-05-10T22:22:35Z

❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌

Tests failed on VSTS: simulator tests iOS.

Test results

22 tests failed, 126 tests passed.

Failed tests

link sdk/Mac [dotnet]/Debug [dotnet]: Failed (Test run failed.
Tests run: 117 Passed: 107 Inconclusive: 0 Failed: 2 Ignored: 8)
link sdk/Mac [dotnet]/Release [dotnet]: Failed (Test run failed.
Tests run: 117 Passed: 106 Inconclusive: 0 Failed: 2 Ignored: 9)
link sdk/Mac Catalyst [dotnet]/Debug [dotnet]: Failed (Test run failed.
Tests run: 129 Passed: 119 Inconclusive: 0 Failed: 2 Ignored: 8)
link sdk/Mac Catalyst [dotnet]/Release [dotnet]: Failed (Test run failed.
Tests run: 129 Passed: 119 Inconclusive: 0 Failed: 1 Ignored: 9)
link sdk/Mac Modern/Debug: Failed (Test run failed.
Tests run: 9 Passed: 8 Inconclusive: 0 Failed: 1 Ignored: 0)
link sdk/iOS Unified 64-bits - simulator/Debug [dotnet]: Failed
link sdk/iOS Unified 64-bits - simulator/Release [dotnet]: Failed
link sdk/iOS Unified 64-bits - simulator/Debug: Failed
link sdk/iOS Unified 64-bits - simulator/Release: Failed
link sdk/tvOS - simulator/Debug [dotnet]: Failed
link sdk/tvOS - simulator/Release [dotnet]: Failed
link sdk/tvOS - simulator/Debug: Failed
link sdk/tvOS - simulator/Release: Failed
trimmode link/Mac [dotnet]/Debug [dotnet]: Failed (Test run failed.
Tests run: 117 Passed: 107 Inconclusive: 0 Failed: 2 Ignored: 8)
trimmode link/Mac [dotnet]/Release [dotnet]: Failed (Test run failed.
Tests run: 117 Passed: 107 Inconclusive: 0 Failed: 1 Ignored: 9)
trimmode link/Mac Catalyst [dotnet]/Debug [dotnet]: Failed (Test run failed.
Tests run: 129 Passed: 120 Inconclusive: 0 Failed: 1 Ignored: 8)
trimmode link/Mac Catalyst [dotnet]/Release [dotnet]: Failed (Test run failed.
Tests run: 129 Passed: 118 Inconclusive: 0 Failed: 2 Ignored: 9)
trimmode link/iOS Unified 64-bits - simulator/Debug [dotnet]: Failed
trimmode link/iOS Unified 64-bits - simulator/Release [dotnet]: Failed
trimmode link/tvOS - simulator/Debug [dotnet]: Failed
trimmode link/tvOS - simulator/Release [dotnet]: Failed
link all/Mac Modern/Debug: Failed (Test run failed.
Tests run: 20 Passed: 18 Inconclusive: 0 Failed: 1 Ignored: 1)

Pipeline on Agent XAMBOT-1096.Monterey
Merge be668c7 into 1be1d74

tj-devel709 · 2022-05-12T17:10:50Z

Merging since the test failures are unrelated timeouts!

TJ Lambert added 3 commits May 3, 2022 11:13

Add nullability

7ac7f25

throw better null exceptions

936854c

use is null and is not null AND fix spacing

be8fc85

tj-devel709 added the not-notes-worthy Ignore for release notes label May 3, 2022

tj-devel709 added this to the Future milestone May 3, 2022

tj-devel709 requested a review from dalexsoto as a code owner May 3, 2022 18:27

tj-devel709 commented May 3, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

mandel-macaque requested changes May 3, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

mandel-macaque approved these changes May 5, 2022

View reviewed changes

TJ Lambert and others added 4 commits May 6, 2022 12:31

Rolfs suggestions and redo spacing

18504f7

missed a few spaces

37703b4

Found one more elusive space

4f77e7a

Merge branch 'main' into Nullable-Security

f06b144

This comment has been minimized.

Sign in to view

rolfbjarne approved these changes May 9, 2022

View reviewed changes

Merge branch 'main' into Nullable-Security

0cb554c

This comment has been minimized.

Sign in to view

Merge branch 'main' into Nullable-Security

be668c7

tj-devel709 merged commit fcf1a19 into xamarin:main May 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[security] Add nullability to (generated and manual) bindings #14884

[security] Add nullability to (generated and manual) bindings #14884

tj-devel709 commented May 3, 2022

tj-devel709 May 3, 2022

mandel-macaque May 3, 2022

tj-devel709 May 4, 2022

mandel-macaque May 4, 2022

rolfbjarne May 4, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mandel-macaque May 3, 2022

mandel-macaque May 3, 2022

mandel-macaque May 3, 2022

mandel-macaque May 3, 2022

mandel-macaque May 3, 2022

This comment has been minimized.

tj-devel709 commented May 6, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vs-mobiletools-engineering-service2 commented May 10, 2022

vs-mobiletools-engineering-service2 commented May 10, 2022

vs-mobiletools-engineering-service2 commented May 10, 2022

vs-mobiletools-engineering-service2 commented May 10, 2022

vs-mobiletools-engineering-service2 commented May 10, 2022

Failed tests

tj-devel709 commented May 12, 2022

[security] Add nullability to (generated and manual) bindings #14884

[security] Add nullability to (generated and manual) bindings #14884

Conversation

tj-devel709 commented May 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

tj-devel709 commented May 6, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vs-mobiletools-engineering-service2 commented May 10, 2022

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

vs-mobiletools-engineering-service2 commented May 10, 2022

📚 [PR Build] Artifacts 📚

Packages generated

vs-mobiletools-engineering-service2 commented May 10, 2022

📋 [PR Build] API Diff 📋

API Current PR diff

API diff

Generator diff

vs-mobiletools-engineering-service2 commented May 10, 2022

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

vs-mobiletools-engineering-service2 commented May 10, 2022

❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌

Test results

Failed tests

tj-devel709 commented May 12, 2022