Fix up lifetime handling of SecItemCopyMatching result parameter by Therzok · Pull Request #15468 · dotnet/macios

Therzok · 2022-07-12T09:05:06Z

Docs state it's caller retained via Create rule

…onary is gone

Therzok · 2022-07-12T11:41:23Z

 				if (status == SecStatusCode.Success){
 					if (max == 1)
-						return new NSData [] { Runtime.GetNSObject<NSData> (ptr, false)! };
+						return new NSData [] { Runtime.GetNSObject<NSData> (ptr, true)! };


Not sure what this is supposed to do.

To rephrase, it's possible that because we're not looking at this as a container of NSData, we would leak the other elements.

The native function is weird: it only returns an array if the limit is > 1:

"By default, this function returns only the first match found. To obtain more than one matching item at a time, specify the search key kSecMatchLimit with a value greater than 1. The result will be an object of type CFArrayRef containing up to that number of matching items."

This means the code is good as-is.

rolfbjarne · 2022-07-12T15:00:48Z

It fails to compile:

Security/Items.cs(244,9): error CS1525: Invalid expression term ':'
Security/Items.cs(244,9): error CS1003: Syntax error, ',' expected
Security/Items.cs(244,11): error CS1003: Syntax error, ',' expected

Therzok · 2022-07-13T11:54:09Z

It fails to compile:

Security/Items.cs(244,9): error CS1525: Invalid expression term ':'
Security/Items.cs(244,9): error CS1003: Syntax error, ',' expected
Security/Items.cs(244,11): error CS1003: Syntax error, ',' expected

My vim fu is rusty, I accidentally dw an extra time. I had built it locally a few times as well

Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>

vs-mobiletools-engineering-service2 · 2022-07-15T02:19:36Z

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

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

Pipeline on Agent
Hash: af36ecd0eb281338781d8b114d6bb741de020059 [PR build]

vs-mobiletools-engineering-service2 · 2022-07-15T02:21:11Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1044.Monterey'
Hash: af36ecd0eb281338781d8b114d6bb741de020059 [PR build]

vs-mobiletools-engineering-service2 · 2022-07-15T02:23:31Z

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
watchOS: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

.NET (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
watchOS: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

.NET (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

Legacy Xamarin (stable) vs .NET

iOS: vsdrops gist
tvOS: vsdrops gist
macOS: vsdrops gist

✅ Generator diff

Generator diff is empty

Pipeline on Agent
Hash: af36ecd0eb281338781d8b114d6bb741de020059 [PR build]

vs-mobiletools-engineering-service2 · 2022-07-15T02:46:28Z

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

Failed tests are:

xammac_tests
monotouch-test

Pipeline on Agent
Hash: af36ecd0eb281338781d8b114d6bb741de020059 [PR build]

vs-mobiletools-engineering-service2 · 2022-07-15T12:03:50Z

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: simulator tests.

🎉 All 223 tests passed 🎉

Tests counts

✅ bcl: All 69 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac_binding_project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 12 tests passed. Html Report (VSDrops) Download
✅ monotouch: All 23 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: [PR build]

rolfbjarne · 2022-07-15T13:37:21Z

 				if (status == SecStatusCode.Success){
 					if (max == 1)
-						return new NSData [] { Runtime.GetNSObject<NSData> (ptr, false)! };
+						return new NSData [] { Runtime.GetNSObject<NSData> (ptr, true)! };


The native function is weird: it only returns an array if the limit is > 1:

"By default, this function returns only the first match found. To obtain more than one matching item at a time, specify the search key kSecMatchLimit with a value greater than 1. The result will be an object of type CFArrayRef containing up to that number of matching items."

This means the code is good as-is.

Therzok added 3 commits July 12, 2022 12:04

Fix up lifetime handling of SecItemCopyMatching result parameter

35db035

Docs state it's caller retained via Create rule

Fixup

3449c72

Retain dictionary props, otherwise they will disappear when the dicti…

ab32018

…onary is gone

Therzok commented Jul 12, 2022

View reviewed changes

Fixup accidental extra dw in vim

ca32c6c

Therzok commented Jul 13, 2022

View reviewed changes

Comment thread src/Security/Items.cs Outdated

dotnet deleted a comment from vs-mobiletools-engineering-service2 Jul 13, 2022

This comment has been minimized.

Sign in to view

rolfbjarne requested changes Jul 14, 2022

View reviewed changes

Comment thread src/Security/Items.cs Outdated

Comment thread src/Security/Items.cs Outdated

Apply lifetime simplification

d3b25b6

Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>

This comment has been minimized.

Sign in to view

Code style fixup

4b8f0e9

This comment has been minimized.

Sign in to view

Merge branch 'main' into dev/therzok/secitemcopymatching-leak

af36ecd

Therzok marked this pull request as ready for review July 14, 2022 17:41

Therzok requested a review from dalexsoto as a code owner July 14, 2022 17:41

rolfbjarne approved these changes Jul 15, 2022

View reviewed changes

rolfbjarne merged commit c324882 into main Jul 15, 2022

rolfbjarne deleted the dev/therzok/secitemcopymatching-leak branch July 15, 2022 13:39

rolfbjarne added the notes-mention Deserves a mention in release notes label Jul 15, 2022

Conversation

Therzok commented Jul 12, 2022

Uh oh!

Therzok Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

Therzok Jul 13, 2022

Choose a reason for hiding this comment

Uh oh!

rolfbjarne Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

rolfbjarne commented Jul 12, 2022

Uh oh!

Uh oh!

Therzok commented Jul 13, 2022

Uh oh!

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.

Uh oh!

Uh oh!

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 Jul 15, 2022

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

Uh oh!

vs-mobiletools-engineering-service2 commented Jul 15, 2022

📚 [PR Build] Artifacts 📚

Packages generated

Uh oh!

vs-mobiletools-engineering-service2 commented Jul 15, 2022

✅ API diff for current PR / commit

✅ API diff vs stable

✅ Generator diff

Uh oh!

vs-mobiletools-engineering-service2 commented Jul 15, 2022

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

Uh oh!

vs-mobiletools-engineering-service2 commented Jul 15, 2022

🚀 [CI Build] Test results 🚀

Test results

Tests counts

Uh oh!

rolfbjarne Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants