make System.Security.Cryptography.X509Certificates compliant with interop guidelines - part 4 #61986
Conversation
…erop guidelines - part 4
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks Issue DetailsFixes partially #51564 The is the final PR! it makes the following calls compliant with Interop guidelines :
|
| @@ -51,7 +36,7 @@ IntPtr ppvContext | |||
| IntPtr pdwFormatType, | |||
| out SafeCertStoreHandle phCertStore, | |||
| IntPtr phMsg, | |||
| IntPtr ppvContext | |||
| out IntPtr ppvContext | |||
| ); | |||
There was a problem hiding this comment.
This change causes us to pass a pointer in (to receive the value) when we used to pass NULL. That potentially changes the allocation behavior of the called function and may make us need to call a free on the emitted value.. which we're not doing.
This change needs to be undone.
There was a problem hiding this comment.
Should I rollback also the added parameter modifier 'out' inside this overload src/libraries/Common/src/Interop/Windows/Crypt32/Interop.CryptQueryObject_IntPtr.cs ? The test System.Security.Cryptography.X509Certificates.Tests.TestHandle.TestHandleCtor use this overload and needs the pointer to pass ?
There was a problem hiding this comment.
If the tests are the only thing using the IntPtr variant, and they're using the last parameter differently, then the _IntPtr variant can keep the out; but the existing variant needs to go back to non-out.
If there is overlap, then I think we can change it to ref IntPtr and the existing calls can pass ref Unsafe.NullRef<IntPtr>() but we shouldn't do that unless there's a demonstrated need.
There was a problem hiding this comment.
Since overloading is allowed across by-value and out, I wouldn't combine them, just leave them as overloads. We might squish them down into one later, but overloading for now seems the easiest.
There was a problem hiding this comment.
I created an overload src/libraries/Common/src/Interop/Windows/Crypt32/Interop.CryptQueryObject_IntPtr_out.cs for test project that have the out modifier on ppvContext parameter, and rollbacked the modifier on pre-existing overloads
…rload with out modifier for tests
|
Thanks for taking care of this, @pedrobsaila. |
|
You're welcome @bartonjs 😄 , thanks to you too. I wouldn't have done it without your guidance |
ghost
locked as resolved and limited conversation to collaborators
Fixes partially #51564
The is the final PR! it makes the following calls compliant with Interop guidelines :