Adding Linux support to System.DirectoryServices.Protocols

[joperezr]

Fixes#23944

Creating this as a draft PR first as I expect a lot of feedback plus some things will still need to happen in order to get tests working in Linux, but I want to start getting feedback so I can address it while I finish setting up running tests in Linux. This adds support to Linux for the System.DirectoryServices.Protocols namespace, by simply creating a PAL layer that abstracts the calls between wldap32 and openldap. Given both native libraries are just implementing ldap protocol, most of their APIs are the same with very similar or same structures which makes it so that managed code didn't actually have to change that much, we just change the native calls we do underneath. There are some open questions, like for example, there are some settings that are specific to wldap32 and not supported on any other library, the question is, should they be No-Ops for compatibility with previous apps working in Windows or should they instead throw PNSE?

One thing that is missing from this PR is the support for Default Credentials. That means that if you have your code running on a Kestrel Server which is domain-joined to an AD and have a valid session token already authenticated in the machine, then you wouldn't need to pass in credentials again when creating an LDAPConnection since you would just reuse that token. That works today in Windows, and I'm working on fixing the native calls on Bind to make sure that it also works on Linux. That support will come either as part of this PR later, or it will be added as a subsequent PR.

cc: @ericstj@tarekgh@bartonjs@stephentoub@davidsh

FYI: @JunTaoLuo@Tratcher

[tarekgh]

CC @tquerec

[tarekgh]

CharSet = CharSet.Unicode [](start = 41, length = 25)

why this needed here?

[joperezr]

I'm actually not sure, this was the way this struct already existed, I just moved them to a new file.

[bartonjs]

disposeHandle here seems to be redundant with the one from the base class... which you're always giving the literal true.

If it means "false to never call ReleaseHandle", that's exactly what the base bool ownsHandle is for.

runtime/src/coreclr/src/vm/safehandle.cpp

Lines 114 to 136 in 5d5c36b

	fPerformRelease = ((oldState & (SH_State_RefCount | SH_State_Closed)) == SH_RefCountOne) && m_ownsHandle;
	if (fPerformRelease)
	{
	GCPROTECT_BEGIN(sh);
	CLR_BOOLfIsInvalid = FALSE;
	DECLARE_ARGHOLDER_ARRAY(args, 1);
	args[ARGNUM_0] = OBJECTREF_TO_ARGHOLDER(sh);
	PREPARE_SIMPLE_VIRTUAL_CALLSITE_USING_SLOT(s_IsInvalidHandleMethodSlot, sh);
	CRITICAL_CALLSITE;
	CALL_MANAGED_METHOD(fIsInvalid, CLR_BOOL, args);
	if (fIsInvalid)
	{
	fPerformRelease = false;
	}
	GCPROTECT_END();
	}

[joperezr]

So is your suggestion to basically instead of having a field for _needDispose and checking this in ReleaseHandle, I should just call base passing in disposeHandle value and not need to check in ReleaseHandle as it would not get called in the case it was set to false right? Just making sure I understood this comment..

[joperezr]

Actually we do have one problem with doing this. The one in the base I believe can't be overwritten after creation, and we have some (internal) methods in LdapConnection that can flip the _needsDispose, so I guess the reason why the original design was like this, was so that ReleaseHandle would get called always, and we would only actually dispose if we need to.

[bartonjs]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value, (maybe I'm wrong and it just finds a ctor that looks like one it knows how to call? or maybe a default is required and I'm having a very bad recollection day). I'm not sure this code actually prevents invalid handles.

A more canonical representation of this would be to declare the ldap_init call to return a SafeConnectionHandle (instead of IntPtr) and the caller should do

SafeConnectionHandlehandle=LdapPal.Initialize(null,389);

// In LdapPal.Windows.cs

internalstaticSafeConnectionHandleInitialize(stringhost,intport){SafeConnectionHandlehandle=Interop.wldap32.ldap_init(host,port);if(handle.IsInvalid){handle.Dispose();// this error handling code}returnhandle;}

[stephentoub]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value

Nope :)

e.g. try this:

usingSystem;usingSystem.Runtime.InteropServices;classProgram{staticvoidMain()=>GetStdHandle(-10);[DllImport("kernel32.dll",SetLastError=true)]privatestaticexternMySafeHandleGetStdHandle(intnStdHandle);}classMySafeHandle:SafeHandle{publicMySafeHandle(stringhello,stringworld):base(IntPtr.Zero,ownsHandle:false){}publicoverrideboolIsInvalid=>true;protectedoverrideboolReleaseHandle()=>true;}

You'll get an exception like this:

Unhandled Exception: System.MissingMethodException: .ctor
at Program.GetStdHandle(Int32 nStdHandle)
at Program.Main()

[bartonjs]

Since this is used by the P/Invoke declarations in Interop, I'd put this file in that same directory (similar to the Windows one). e.g. https://github.com/dotnet/runtime/blob/master/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OCSP.cs#L103-L134

[bartonjs]

(Following the OCSP example, you could just nestle the appropriate SafeHandle types in the bottom of the Interop.Ber.cs and Interop.Ldap.cs files, so that if some other library needs them it's just one include)

[bartonjs]

For long-term test-building maintenance, consider a helper. E.g.

yieldreturnnewobject[]{0,0,null,FormatLengths("30{0}020100020100A0{1}020100020100",18,6)};
...private static byte[]FormatLengths(stringhexFormat,paramsint[]lengths){string[]hexLengths=newstring[lengths.Length];for(inti=0;i<lengths.Length;i++){intcurLen=lengths[i];if(PlatformDetection.IsWindows){hexLengths[i]=$"84{curLen:X8}";}elseif(curLen<0x80){hexLengths[i]=curLen.ToString("X2");}elseif(curLen<=byte.MaxValue){hexLengths[i]=$"81{curLen:X2}";}elseif(curLen<=ushort.MaxValue){hexLengths[i]=$"82{curLen:X4}";}elseif(curLen<=0xFFFFFF){hexLengths[i]=$"83{curLen:X6}";}else{hexLengths[i]=$"84{curLen:X8}";}}stringhex=string.Format(hexFormat,hexLengths);// This presupposes something like the crypto tests have:returnhex.HexToByteArray();}

[joperezr]

I agree this would be valuable, but if that's ok with you I would rather do that in a subsequent PR so to not add more complexity to this PR for now. Once I have tests running on Mono and OSX and passing, then I think it would be a great time to address this.

[bartonjs]

Deferring the test helper is totally fine.

[joperezr]

Thanks a lot to all for the reviews! 😄

[danmoseley]

Nice! 🔥

[JunTaoLuo]

🎆

[joperezr]

Fixes#23944

Creating this as a draft PR first as I expect a lot of feedback plus some things will still need to happen in order to get tests working in Linux, but I want to start getting feedback so I can address it while I finish setting up running tests in Linux. This adds support to Linux for the System.DirectoryServices.Protocols namespace, by simply creating a PAL layer that abstracts the calls between wldap32 and openldap. Given both native libraries are just implementing ldap protocol, most of their APIs are the same with very similar or same structures which makes it so that managed code didn't actually have to change that much, we just change the native calls we do underneath. There are some open questions, like for example, there are some settings that are specific to wldap32 and not supported on any other library, the question is, should they be No-Ops for compatibility with previous apps working in Windows or should they instead throw PNSE?

One thing that is missing from this PR is the support for Default Credentials. That means that if you have your code running on a Kestrel Server which is domain-joined to an AD and have a valid session token already authenticated in the machine, then you wouldn't need to pass in credentials again when creating an LDAPConnection since you would just reuse that token. That works today in Windows, and I'm working on fixing the native calls on Bind to make sure that it also works on Linux. That support will come either as part of this PR later, or it will be added as a subsequent PR.

cc: @ericstj@tarekgh@bartonjs@stephentoub@davidsh

FYI: @JunTaoLuo@Tratcher

[tarekgh]

CC @tquerec

[tarekgh]

CharSet = CharSet.Unicode [](start = 41, length = 25)

why this needed here?

[joperezr]

I'm actually not sure, this was the way this struct already existed, I just moved them to a new file.

[bartonjs]

disposeHandle here seems to be redundant with the one from the base class... which you're always giving the literal true.

If it means "false to never call ReleaseHandle", that's exactly what the base bool ownsHandle is for.

runtime/src/coreclr/src/vm/safehandle.cpp

Lines 114 to 136 in 5d5c36b

	fPerformRelease = ((oldState & (SH_State_RefCount | SH_State_Closed)) == SH_RefCountOne) && m_ownsHandle;
	if (fPerformRelease)
	{
	GCPROTECT_BEGIN(sh);
	CLR_BOOLfIsInvalid = FALSE;
	DECLARE_ARGHOLDER_ARRAY(args, 1);
	args[ARGNUM_0] = OBJECTREF_TO_ARGHOLDER(sh);
	PREPARE_SIMPLE_VIRTUAL_CALLSITE_USING_SLOT(s_IsInvalidHandleMethodSlot, sh);
	CRITICAL_CALLSITE;
	CALL_MANAGED_METHOD(fIsInvalid, CLR_BOOL, args);
	if (fIsInvalid)
	{
	fPerformRelease = false;
	}
	GCPROTECT_END();
	}

[joperezr]

So is your suggestion to basically instead of having a field for _needDispose and checking this in ReleaseHandle, I should just call base passing in disposeHandle value and not need to check in ReleaseHandle as it would not get called in the case it was set to false right? Just making sure I understood this comment..

[joperezr]

Actually we do have one problem with doing this. The one in the base I believe can't be overwritten after creation, and we have some (internal) methods in LdapConnection that can flip the _needsDispose, so I guess the reason why the original design was like this, was so that ReleaseHandle would get called always, and we would only actually dispose if we need to.

[bartonjs]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value, (maybe I'm wrong and it just finds a ctor that looks like one it knows how to call? or maybe a default is required and I'm having a very bad recollection day). I'm not sure this code actually prevents invalid handles.

A more canonical representation of this would be to declare the ldap_init call to return a SafeConnectionHandle (instead of IntPtr) and the caller should do

SafeConnectionHandlehandle=LdapPal.Initialize(null,389);

// In LdapPal.Windows.cs

internalstaticSafeConnectionHandleInitialize(stringhost,intport){SafeConnectionHandlehandle=Interop.wldap32.ldap_init(host,port);if(handle.IsInvalid){handle.Dispose();// this error handling code}returnhandle;}

[stephentoub]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value

Nope :)

e.g. try this:

usingSystem;usingSystem.Runtime.InteropServices;classProgram{staticvoidMain()=>GetStdHandle(-10);[DllImport("kernel32.dll",SetLastError=true)]privatestaticexternMySafeHandleGetStdHandle(intnStdHandle);}classMySafeHandle:SafeHandle{publicMySafeHandle(stringhello,stringworld):base(IntPtr.Zero,ownsHandle:false){}publicoverrideboolIsInvalid=>true;protectedoverrideboolReleaseHandle()=>true;}

You'll get an exception like this:

Unhandled Exception: System.MissingMethodException: .ctor
at Program.GetStdHandle(Int32 nStdHandle)
at Program.Main()

[bartonjs]

Since this is used by the P/Invoke declarations in Interop, I'd put this file in that same directory (similar to the Windows one). e.g. https://github.com/dotnet/runtime/blob/master/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OCSP.cs#L103-L134

[bartonjs]

(Following the OCSP example, you could just nestle the appropriate SafeHandle types in the bottom of the Interop.Ber.cs and Interop.Ldap.cs files, so that if some other library needs them it's just one include)

[bartonjs]

For long-term test-building maintenance, consider a helper. E.g.

yieldreturnnewobject[]{0,0,null,FormatLengths("30{0}020100020100A0{1}020100020100",18,6)};
...private static byte[]FormatLengths(stringhexFormat,paramsint[]lengths){string[]hexLengths=newstring[lengths.Length];for(inti=0;i<lengths.Length;i++){intcurLen=lengths[i];if(PlatformDetection.IsWindows){hexLengths[i]=$"84{curLen:X8}";}elseif(curLen<0x80){hexLengths[i]=curLen.ToString("X2");}elseif(curLen<=byte.MaxValue){hexLengths[i]=$"81{curLen:X2}";}elseif(curLen<=ushort.MaxValue){hexLengths[i]=$"82{curLen:X4}";}elseif(curLen<=0xFFFFFF){hexLengths[i]=$"83{curLen:X6}";}else{hexLengths[i]=$"84{curLen:X8}";}}stringhex=string.Format(hexFormat,hexLengths);// This presupposes something like the crypto tests have:returnhex.HexToByteArray();}

[joperezr]

I agree this would be valuable, but if that's ok with you I would rather do that in a subsequent PR so to not add more complexity to this PR for now. Once I have tests running on Mono and OSX and passing, then I think it would be a great time to address this.

[bartonjs]

Deferring the test helper is totally fine.

[joperezr]

Thanks a lot to all for the reviews! 😄

[danmoseley]

Nice! 🔥

[JunTaoLuo]

🎆

[joperezr]

Fixes#23944

Creating this as a draft PR first as I expect a lot of feedback plus some things will still need to happen in order to get tests working in Linux, but I want to start getting feedback so I can address it while I finish setting up running tests in Linux. This adds support to Linux for the System.DirectoryServices.Protocols namespace, by simply creating a PAL layer that abstracts the calls between wldap32 and openldap. Given both native libraries are just implementing ldap protocol, most of their APIs are the same with very similar or same structures which makes it so that managed code didn't actually have to change that much, we just change the native calls we do underneath. There are some open questions, like for example, there are some settings that are specific to wldap32 and not supported on any other library, the question is, should they be No-Ops for compatibility with previous apps working in Windows or should they instead throw PNSE?

One thing that is missing from this PR is the support for Default Credentials. That means that if you have your code running on a Kestrel Server which is domain-joined to an AD and have a valid session token already authenticated in the machine, then you wouldn't need to pass in credentials again when creating an LDAPConnection since you would just reuse that token. That works today in Windows, and I'm working on fixing the native calls on Bind to make sure that it also works on Linux. That support will come either as part of this PR later, or it will be added as a subsequent PR.

cc: @ericstj@tarekgh@bartonjs@stephentoub@davidsh

FYI: @JunTaoLuo@Tratcher

[tarekgh]

CC @tquerec

[tarekgh]

CharSet = CharSet.Unicode [](start = 41, length = 25)

why this needed here?

[joperezr]

I'm actually not sure, this was the way this struct already existed, I just moved them to a new file.

[bartonjs]

disposeHandle here seems to be redundant with the one from the base class... which you're always giving the literal true.

If it means "false to never call ReleaseHandle", that's exactly what the base bool ownsHandle is for.

runtime/src/coreclr/src/vm/safehandle.cpp

Lines 114 to 136 in 5d5c36b

	fPerformRelease = ((oldState & (SH_State_RefCount | SH_State_Closed)) == SH_RefCountOne) && m_ownsHandle;
	if (fPerformRelease)
	{
	GCPROTECT_BEGIN(sh);
	CLR_BOOLfIsInvalid = FALSE;
	DECLARE_ARGHOLDER_ARRAY(args, 1);
	args[ARGNUM_0] = OBJECTREF_TO_ARGHOLDER(sh);
	PREPARE_SIMPLE_VIRTUAL_CALLSITE_USING_SLOT(s_IsInvalidHandleMethodSlot, sh);
	CRITICAL_CALLSITE;
	CALL_MANAGED_METHOD(fIsInvalid, CLR_BOOL, args);
	if (fIsInvalid)
	{
	fPerformRelease = false;
	}
	GCPROTECT_END();
	}

[joperezr]

So is your suggestion to basically instead of having a field for _needDispose and checking this in ReleaseHandle, I should just call base passing in disposeHandle value and not need to check in ReleaseHandle as it would not get called in the case it was set to false right? Just making sure I understood this comment..

[joperezr]

Actually we do have one problem with doing this. The one in the base I believe can't be overwritten after creation, and we have some (internal) methods in LdapConnection that can flip the _needsDispose, so I guess the reason why the original design was like this, was so that ReleaseHandle would get called always, and we would only actually dispose if we need to.

[bartonjs]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value, (maybe I'm wrong and it just finds a ctor that looks like one it knows how to call? or maybe a default is required and I'm having a very bad recollection day). I'm not sure this code actually prevents invalid handles.

A more canonical representation of this would be to declare the ldap_init call to return a SafeConnectionHandle (instead of IntPtr) and the caller should do

SafeConnectionHandlehandle=LdapPal.Initialize(null,389);

// In LdapPal.Windows.cs

internalstaticSafeConnectionHandleInitialize(stringhost,intport){SafeConnectionHandlehandle=Interop.wldap32.ldap_init(host,port);if(handle.IsInvalid){handle.Dispose();// this error handling code}returnhandle;}

[stephentoub]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value

Nope :)

e.g. try this:

usingSystem;usingSystem.Runtime.InteropServices;classProgram{staticvoidMain()=>GetStdHandle(-10);[DllImport("kernel32.dll",SetLastError=true)]privatestaticexternMySafeHandleGetStdHandle(intnStdHandle);}classMySafeHandle:SafeHandle{publicMySafeHandle(stringhello,stringworld):base(IntPtr.Zero,ownsHandle:false){}publicoverrideboolIsInvalid=>true;protectedoverrideboolReleaseHandle()=>true;}

You'll get an exception like this:

Unhandled Exception: System.MissingMethodException: .ctor
at Program.GetStdHandle(Int32 nStdHandle)
at Program.Main()

[bartonjs]

Since this is used by the P/Invoke declarations in Interop, I'd put this file in that same directory (similar to the Windows one). e.g. https://github.com/dotnet/runtime/blob/master/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OCSP.cs#L103-L134

[bartonjs]

(Following the OCSP example, you could just nestle the appropriate SafeHandle types in the bottom of the Interop.Ber.cs and Interop.Ldap.cs files, so that if some other library needs them it's just one include)

[bartonjs]

For long-term test-building maintenance, consider a helper. E.g.

yieldreturnnewobject[]{0,0,null,FormatLengths("30{0}020100020100A0{1}020100020100",18,6)};
...private static byte[]FormatLengths(stringhexFormat,paramsint[]lengths){string[]hexLengths=newstring[lengths.Length];for(inti=0;i<lengths.Length;i++){intcurLen=lengths[i];if(PlatformDetection.IsWindows){hexLengths[i]=$"84{curLen:X8}";}elseif(curLen<0x80){hexLengths[i]=curLen.ToString("X2");}elseif(curLen<=byte.MaxValue){hexLengths[i]=$"81{curLen:X2}";}elseif(curLen<=ushort.MaxValue){hexLengths[i]=$"82{curLen:X4}";}elseif(curLen<=0xFFFFFF){hexLengths[i]=$"83{curLen:X6}";}else{hexLengths[i]=$"84{curLen:X8}";}}stringhex=string.Format(hexFormat,hexLengths);// This presupposes something like the crypto tests have:returnhex.HexToByteArray();}

[joperezr]

I agree this would be valuable, but if that's ok with you I would rather do that in a subsequent PR so to not add more complexity to this PR for now. Once I have tests running on Mono and OSX and passing, then I think it would be a great time to address this.

[bartonjs]

Deferring the test helper is totally fine.

[joperezr]

Thanks a lot to all for the reviews! 😄

[danmoseley]

Nice! 🔥

[JunTaoLuo]

🎆

[joperezr]

Fixes#23944

Creating this as a draft PR first as I expect a lot of feedback plus some things will still need to happen in order to get tests working in Linux, but I want to start getting feedback so I can address it while I finish setting up running tests in Linux. This adds support to Linux for the System.DirectoryServices.Protocols namespace, by simply creating a PAL layer that abstracts the calls between wldap32 and openldap. Given both native libraries are just implementing ldap protocol, most of their APIs are the same with very similar or same structures which makes it so that managed code didn't actually have to change that much, we just change the native calls we do underneath. There are some open questions, like for example, there are some settings that are specific to wldap32 and not supported on any other library, the question is, should they be No-Ops for compatibility with previous apps working in Windows or should they instead throw PNSE?

One thing that is missing from this PR is the support for Default Credentials. That means that if you have your code running on a Kestrel Server which is domain-joined to an AD and have a valid session token already authenticated in the machine, then you wouldn't need to pass in credentials again when creating an LDAPConnection since you would just reuse that token. That works today in Windows, and I'm working on fixing the native calls on Bind to make sure that it also works on Linux. That support will come either as part of this PR later, or it will be added as a subsequent PR.

cc: @ericstj@tarekgh@bartonjs@stephentoub@davidsh

FYI: @JunTaoLuo@Tratcher

[tarekgh]

CC @tquerec

[tarekgh]

CharSet = CharSet.Unicode [](start = 41, length = 25)

why this needed here?

[joperezr]

I'm actually not sure, this was the way this struct already existed, I just moved them to a new file.

[bartonjs]

disposeHandle here seems to be redundant with the one from the base class... which you're always giving the literal true.

If it means "false to never call ReleaseHandle", that's exactly what the base bool ownsHandle is for.

runtime/src/coreclr/src/vm/safehandle.cpp

Lines 114 to 136 in 5d5c36b

	fPerformRelease = ((oldState & (SH_State_RefCount | SH_State_Closed)) == SH_RefCountOne) && m_ownsHandle;
	if (fPerformRelease)
	{
	GCPROTECT_BEGIN(sh);
	CLR_BOOLfIsInvalid = FALSE;
	DECLARE_ARGHOLDER_ARRAY(args, 1);
	args[ARGNUM_0] = OBJECTREF_TO_ARGHOLDER(sh);
	PREPARE_SIMPLE_VIRTUAL_CALLSITE_USING_SLOT(s_IsInvalidHandleMethodSlot, sh);
	CRITICAL_CALLSITE;
	CALL_MANAGED_METHOD(fIsInvalid, CLR_BOOL, args);
	if (fIsInvalid)
	{
	fPerformRelease = false;
	}
	GCPROTECT_END();
	}

[joperezr]

So is your suggestion to basically instead of having a field for _needDispose and checking this in ReleaseHandle, I should just call base passing in disposeHandle value and not need to check in ReleaseHandle as it would not get called in the case it was set to false right? Just making sure I understood this comment..

[joperezr]

Actually we do have one problem with doing this. The one in the base I believe can't be overwritten after creation, and we have some (internal) methods in LdapConnection that can flip the _needsDispose, so I guess the reason why the original design was like this, was so that ReleaseHandle would get called always, and we would only actually dispose if we need to.

[bartonjs]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value, (maybe I'm wrong and it just finds a ctor that looks like one it knows how to call? or maybe a default is required and I'm having a very bad recollection day). I'm not sure this code actually prevents invalid handles.

A more canonical representation of this would be to declare the ldap_init call to return a SafeConnectionHandle (instead of IntPtr) and the caller should do

SafeConnectionHandlehandle=LdapPal.Initialize(null,389);

// In LdapPal.Windows.cs

internalstaticSafeConnectionHandleInitialize(stringhost,intport){SafeConnectionHandlehandle=Interop.wldap32.ldap_init(host,port);if(handle.IsInvalid){handle.Dispose();// this error handling code}returnhandle;}

[stephentoub]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value

Nope :)

e.g. try this:

usingSystem;usingSystem.Runtime.InteropServices;classProgram{staticvoidMain()=>GetStdHandle(-10);[DllImport("kernel32.dll",SetLastError=true)]privatestaticexternMySafeHandleGetStdHandle(intnStdHandle);}classMySafeHandle:SafeHandle{publicMySafeHandle(stringhello,stringworld):base(IntPtr.Zero,ownsHandle:false){}publicoverrideboolIsInvalid=>true;protectedoverrideboolReleaseHandle()=>true;}

You'll get an exception like this:

Unhandled Exception: System.MissingMethodException: .ctor
at Program.GetStdHandle(Int32 nStdHandle)
at Program.Main()

[bartonjs]

Since this is used by the P/Invoke declarations in Interop, I'd put this file in that same directory (similar to the Windows one). e.g. https://github.com/dotnet/runtime/blob/master/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OCSP.cs#L103-L134

[bartonjs]

(Following the OCSP example, you could just nestle the appropriate SafeHandle types in the bottom of the Interop.Ber.cs and Interop.Ldap.cs files, so that if some other library needs them it's just one include)

[bartonjs]

For long-term test-building maintenance, consider a helper. E.g.

yieldreturnnewobject[]{0,0,null,FormatLengths("30{0}020100020100A0{1}020100020100",18,6)};
...private static byte[]FormatLengths(stringhexFormat,paramsint[]lengths){string[]hexLengths=newstring[lengths.Length];for(inti=0;i<lengths.Length;i++){intcurLen=lengths[i];if(PlatformDetection.IsWindows){hexLengths[i]=$"84{curLen:X8}";}elseif(curLen<0x80){hexLengths[i]=curLen.ToString("X2");}elseif(curLen<=byte.MaxValue){hexLengths[i]=$"81{curLen:X2}";}elseif(curLen<=ushort.MaxValue){hexLengths[i]=$"82{curLen:X4}";}elseif(curLen<=0xFFFFFF){hexLengths[i]=$"83{curLen:X6}";}else{hexLengths[i]=$"84{curLen:X8}";}}stringhex=string.Format(hexFormat,hexLengths);// This presupposes something like the crypto tests have:returnhex.HexToByteArray();}

[joperezr]

I agree this would be valuable, but if that's ok with you I would rather do that in a subsequent PR so to not add more complexity to this PR for now. Once I have tests running on Mono and OSX and passing, then I think it would be a great time to address this.

[bartonjs]

Deferring the test helper is totally fine.

[joperezr]

Thanks a lot to all for the reviews! 😄

[danmoseley]

Nice! 🔥

[JunTaoLuo]

🎆

[joperezr]

Fixes#23944

Creating this as a draft PR first as I expect a lot of feedback plus some things will still need to happen in order to get tests working in Linux, but I want to start getting feedback so I can address it while I finish setting up running tests in Linux. This adds support to Linux for the System.DirectoryServices.Protocols namespace, by simply creating a PAL layer that abstracts the calls between wldap32 and openldap. Given both native libraries are just implementing ldap protocol, most of their APIs are the same with very similar or same structures which makes it so that managed code didn't actually have to change that much, we just change the native calls we do underneath. There are some open questions, like for example, there are some settings that are specific to wldap32 and not supported on any other library, the question is, should they be No-Ops for compatibility with previous apps working in Windows or should they instead throw PNSE?

One thing that is missing from this PR is the support for Default Credentials. That means that if you have your code running on a Kestrel Server which is domain-joined to an AD and have a valid session token already authenticated in the machine, then you wouldn't need to pass in credentials again when creating an LDAPConnection since you would just reuse that token. That works today in Windows, and I'm working on fixing the native calls on Bind to make sure that it also works on Linux. That support will come either as part of this PR later, or it will be added as a subsequent PR.

cc: @ericstj@tarekgh@bartonjs@stephentoub@davidsh

FYI: @JunTaoLuo@Tratcher

[tarekgh]

CC @tquerec

[tarekgh]

CharSet = CharSet.Unicode [](start = 41, length = 25)

why this needed here?

[joperezr]

I'm actually not sure, this was the way this struct already existed, I just moved them to a new file.

[bartonjs]

disposeHandle here seems to be redundant with the one from the base class... which you're always giving the literal true.

If it means "false to never call ReleaseHandle", that's exactly what the base bool ownsHandle is for.

runtime/src/coreclr/src/vm/safehandle.cpp

Lines 114 to 136 in 5d5c36b

	fPerformRelease = ((oldState & (SH_State_RefCount | SH_State_Closed)) == SH_RefCountOne) && m_ownsHandle;
	if (fPerformRelease)
	{
	GCPROTECT_BEGIN(sh);
	CLR_BOOLfIsInvalid = FALSE;
	DECLARE_ARGHOLDER_ARRAY(args, 1);
	args[ARGNUM_0] = OBJECTREF_TO_ARGHOLDER(sh);
	PREPARE_SIMPLE_VIRTUAL_CALLSITE_USING_SLOT(s_IsInvalidHandleMethodSlot, sh);
	CRITICAL_CALLSITE;
	CALL_MANAGED_METHOD(fIsInvalid, CLR_BOOL, args);
	if (fIsInvalid)
	{
	fPerformRelease = false;
	}
	GCPROTECT_END();
	}

[joperezr]

So is your suggestion to basically instead of having a field for _needDispose and checking this in ReleaseHandle, I should just call base passing in disposeHandle value and not need to check in ReleaseHandle as it would not get called in the case it was set to false right? Just making sure I understood this comment..

[joperezr]

Actually we do have one problem with doing this. The one in the base I believe can't be overwritten after creation, and we have some (internal) methods in LdapConnection that can flip the _needsDispose, so I guess the reason why the original design was like this, was so that ReleaseHandle would get called always, and we would only actually dispose if we need to.

[bartonjs]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value, (maybe I'm wrong and it just finds a ctor that looks like one it knows how to call? or maybe a default is required and I'm having a very bad recollection day). I'm not sure this code actually prevents invalid handles.

A more canonical representation of this would be to declare the ldap_init call to return a SafeConnectionHandle (instead of IntPtr) and the caller should do

SafeConnectionHandlehandle=LdapPal.Initialize(null,389);

// In LdapPal.Windows.cs

internalstaticSafeConnectionHandleInitialize(stringhost,intport){SafeConnectionHandlehandle=Interop.wldap32.ldap_init(host,port);if(handle.IsInvalid){handle.Dispose();// this error handling code}returnhandle;}

[stephentoub]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value

Nope :)

e.g. try this:

usingSystem;usingSystem.Runtime.InteropServices;classProgram{staticvoidMain()=>GetStdHandle(-10);[DllImport("kernel32.dll",SetLastError=true)]privatestaticexternMySafeHandleGetStdHandle(intnStdHandle);}classMySafeHandle:SafeHandle{publicMySafeHandle(stringhello,stringworld):base(IntPtr.Zero,ownsHandle:false){}publicoverrideboolIsInvalid=>true;protectedoverrideboolReleaseHandle()=>true;}

You'll get an exception like this:

Unhandled Exception: System.MissingMethodException: .ctor
at Program.GetStdHandle(Int32 nStdHandle)
at Program.Main()

[bartonjs]

Since this is used by the P/Invoke declarations in Interop, I'd put this file in that same directory (similar to the Windows one). e.g. https://github.com/dotnet/runtime/blob/master/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OCSP.cs#L103-L134

[bartonjs]

(Following the OCSP example, you could just nestle the appropriate SafeHandle types in the bottom of the Interop.Ber.cs and Interop.Ldap.cs files, so that if some other library needs them it's just one include)

[bartonjs]

For long-term test-building maintenance, consider a helper. E.g.

yieldreturnnewobject[]{0,0,null,FormatLengths("30{0}020100020100A0{1}020100020100",18,6)};
...private static byte[]FormatLengths(stringhexFormat,paramsint[]lengths){string[]hexLengths=newstring[lengths.Length];for(inti=0;i<lengths.Length;i++){intcurLen=lengths[i];if(PlatformDetection.IsWindows){hexLengths[i]=$"84{curLen:X8}";}elseif(curLen<0x80){hexLengths[i]=curLen.ToString("X2");}elseif(curLen<=byte.MaxValue){hexLengths[i]=$"81{curLen:X2}";}elseif(curLen<=ushort.MaxValue){hexLengths[i]=$"82{curLen:X4}";}elseif(curLen<=0xFFFFFF){hexLengths[i]=$"83{curLen:X6}";}else{hexLengths[i]=$"84{curLen:X8}";}}stringhex=string.Format(hexFormat,hexLengths);// This presupposes something like the crypto tests have:returnhex.HexToByteArray();}

[joperezr]

I agree this would be valuable, but if that's ok with you I would rather do that in a subsequent PR so to not add more complexity to this PR for now. Once I have tests running on Mono and OSX and passing, then I think it would be a great time to address this.

[bartonjs]

Deferring the test helper is totally fine.

[joperezr]

Thanks a lot to all for the reviews! 😄

[danmoseley]

Nice! 🔥

[JunTaoLuo]

🎆

[joperezr]

Fixes#23944

Creating this as a draft PR first as I expect a lot of feedback plus some things will still need to happen in order to get tests working in Linux, but I want to start getting feedback so I can address it while I finish setting up running tests in Linux. This adds support to Linux for the System.DirectoryServices.Protocols namespace, by simply creating a PAL layer that abstracts the calls between wldap32 and openldap. Given both native libraries are just implementing ldap protocol, most of their APIs are the same with very similar or same structures which makes it so that managed code didn't actually have to change that much, we just change the native calls we do underneath. There are some open questions, like for example, there are some settings that are specific to wldap32 and not supported on any other library, the question is, should they be No-Ops for compatibility with previous apps working in Windows or should they instead throw PNSE?

One thing that is missing from this PR is the support for Default Credentials. That means that if you have your code running on a Kestrel Server which is domain-joined to an AD and have a valid session token already authenticated in the machine, then you wouldn't need to pass in credentials again when creating an LDAPConnection since you would just reuse that token. That works today in Windows, and I'm working on fixing the native calls on Bind to make sure that it also works on Linux. That support will come either as part of this PR later, or it will be added as a subsequent PR.

cc: @ericstj@tarekgh@bartonjs@stephentoub@davidsh

FYI: @JunTaoLuo@Tratcher

[tarekgh]

CC @tquerec

[tarekgh]

CharSet = CharSet.Unicode [](start = 41, length = 25)

why this needed here?

[joperezr]

I'm actually not sure, this was the way this struct already existed, I just moved them to a new file.

[bartonjs]

disposeHandle here seems to be redundant with the one from the base class... which you're always giving the literal true.

If it means "false to never call ReleaseHandle", that's exactly what the base bool ownsHandle is for.

runtime/src/coreclr/src/vm/safehandle.cpp

Lines 114 to 136 in 5d5c36b

	fPerformRelease = ((oldState & (SH_State_RefCount | SH_State_Closed)) == SH_RefCountOne) && m_ownsHandle;
	if (fPerformRelease)
	{
	GCPROTECT_BEGIN(sh);
	CLR_BOOLfIsInvalid = FALSE;
	DECLARE_ARGHOLDER_ARRAY(args, 1);
	args[ARGNUM_0] = OBJECTREF_TO_ARGHOLDER(sh);
	PREPARE_SIMPLE_VIRTUAL_CALLSITE_USING_SLOT(s_IsInvalidHandleMethodSlot, sh);
	CRITICAL_CALLSITE;
	CALL_MANAGED_METHOD(fIsInvalid, CLR_BOOL, args);
	if (fIsInvalid)
	{
	fPerformRelease = false;
	}
	GCPROTECT_END();
	}

[joperezr]

So is your suggestion to basically instead of having a field for _needDispose and checking this in ReleaseHandle, I should just call base passing in disposeHandle value and not need to check in ReleaseHandle as it would not get called in the case it was set to false right? Just making sure I understood this comment..

[joperezr]

Actually we do have one problem with doing this. The one in the base I believe can't be overwritten after creation, and we have some (internal) methods in LdapConnection that can flip the _needsDispose, so I guess the reason why the original design was like this, was so that ReleaseHandle would get called always, and we would only actually dispose if we need to.

[bartonjs]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value, (maybe I'm wrong and it just finds a ctor that looks like one it knows how to call? or maybe a default is required and I'm having a very bad recollection day). I'm not sure this code actually prevents invalid handles.

A more canonical representation of this would be to declare the ldap_init call to return a SafeConnectionHandle (instead of IntPtr) and the caller should do

SafeConnectionHandlehandle=LdapPal.Initialize(null,389);

// In LdapPal.Windows.cs

internalstaticSafeConnectionHandleInitialize(stringhost,intport){SafeConnectionHandlehandle=Interop.wldap32.ldap_init(host,port);if(handle.IsInvalid){handle.Dispose();// this error handling code}returnhandle;}

[stephentoub]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value

Nope :)

e.g. try this:

usingSystem;usingSystem.Runtime.InteropServices;classProgram{staticvoidMain()=>GetStdHandle(-10);[DllImport("kernel32.dll",SetLastError=true)]privatestaticexternMySafeHandleGetStdHandle(intnStdHandle);}classMySafeHandle:SafeHandle{publicMySafeHandle(stringhello,stringworld):base(IntPtr.Zero,ownsHandle:false){}publicoverrideboolIsInvalid=>true;protectedoverrideboolReleaseHandle()=>true;}

You'll get an exception like this:

Unhandled Exception: System.MissingMethodException: .ctor
at Program.GetStdHandle(Int32 nStdHandle)
at Program.Main()

[bartonjs]

Since this is used by the P/Invoke declarations in Interop, I'd put this file in that same directory (similar to the Windows one). e.g. https://github.com/dotnet/runtime/blob/master/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OCSP.cs#L103-L134

[bartonjs]

(Following the OCSP example, you could just nestle the appropriate SafeHandle types in the bottom of the Interop.Ber.cs and Interop.Ldap.cs files, so that if some other library needs them it's just one include)

[bartonjs]

For long-term test-building maintenance, consider a helper. E.g.

yieldreturnnewobject[]{0,0,null,FormatLengths("30{0}020100020100A0{1}020100020100",18,6)};
...private static byte[]FormatLengths(stringhexFormat,paramsint[]lengths){string[]hexLengths=newstring[lengths.Length];for(inti=0;i<lengths.Length;i++){intcurLen=lengths[i];if(PlatformDetection.IsWindows){hexLengths[i]=$"84{curLen:X8}";}elseif(curLen<0x80){hexLengths[i]=curLen.ToString("X2");}elseif(curLen<=byte.MaxValue){hexLengths[i]=$"81{curLen:X2}";}elseif(curLen<=ushort.MaxValue){hexLengths[i]=$"82{curLen:X4}";}elseif(curLen<=0xFFFFFF){hexLengths[i]=$"83{curLen:X6}";}else{hexLengths[i]=$"84{curLen:X8}";}}stringhex=string.Format(hexFormat,hexLengths);// This presupposes something like the crypto tests have:returnhex.HexToByteArray();}

[joperezr]

I agree this would be valuable, but if that's ok with you I would rather do that in a subsequent PR so to not add more complexity to this PR for now. Once I have tests running on Mono and OSX and passing, then I think it would be a great time to address this.

[bartonjs]

Deferring the test helper is totally fine.

[joperezr]

Thanks a lot to all for the reviews! 😄

[danmoseley]

Nice! 🔥

[JunTaoLuo]

🎆

[joperezr]

Fixes#23944

Creating this as a draft PR first as I expect a lot of feedback plus some things will still need to happen in order to get tests working in Linux, but I want to start getting feedback so I can address it while I finish setting up running tests in Linux. This adds support to Linux for the System.DirectoryServices.Protocols namespace, by simply creating a PAL layer that abstracts the calls between wldap32 and openldap. Given both native libraries are just implementing ldap protocol, most of their APIs are the same with very similar or same structures which makes it so that managed code didn't actually have to change that much, we just change the native calls we do underneath. There are some open questions, like for example, there are some settings that are specific to wldap32 and not supported on any other library, the question is, should they be No-Ops for compatibility with previous apps working in Windows or should they instead throw PNSE?

One thing that is missing from this PR is the support for Default Credentials. That means that if you have your code running on a Kestrel Server which is domain-joined to an AD and have a valid session token already authenticated in the machine, then you wouldn't need to pass in credentials again when creating an LDAPConnection since you would just reuse that token. That works today in Windows, and I'm working on fixing the native calls on Bind to make sure that it also works on Linux. That support will come either as part of this PR later, or it will be added as a subsequent PR.

cc: @ericstj@tarekgh@bartonjs@stephentoub@davidsh

FYI: @JunTaoLuo@Tratcher

[tarekgh]

CC @tquerec

[tarekgh]

CharSet = CharSet.Unicode [](start = 41, length = 25)

why this needed here?

[joperezr]

I'm actually not sure, this was the way this struct already existed, I just moved them to a new file.

[bartonjs]

disposeHandle here seems to be redundant with the one from the base class... which you're always giving the literal true.

If it means "false to never call ReleaseHandle", that's exactly what the base bool ownsHandle is for.

runtime/src/coreclr/src/vm/safehandle.cpp

Lines 114 to 136 in 5d5c36b

	fPerformRelease = ((oldState & (SH_State_RefCount | SH_State_Closed)) == SH_RefCountOne) && m_ownsHandle;
	if (fPerformRelease)
	{
	GCPROTECT_BEGIN(sh);
	CLR_BOOLfIsInvalid = FALSE;
	DECLARE_ARGHOLDER_ARRAY(args, 1);
	args[ARGNUM_0] = OBJECTREF_TO_ARGHOLDER(sh);
	PREPARE_SIMPLE_VIRTUAL_CALLSITE_USING_SLOT(s_IsInvalidHandleMethodSlot, sh);
	CRITICAL_CALLSITE;
	CALL_MANAGED_METHOD(fIsInvalid, CLR_BOOL, args);
	if (fIsInvalid)
	{
	fPerformRelease = false;
	}
	GCPROTECT_END();
	}

[joperezr]

So is your suggestion to basically instead of having a field for _needDispose and checking this in ReleaseHandle, I should just call base passing in disposeHandle value and not need to check in ReleaseHandle as it would not get called in the case it was set to false right? Just making sure I understood this comment..

[joperezr]

Actually we do have one problem with doing this. The one in the base I believe can't be overwritten after creation, and we have some (internal) methods in LdapConnection that can flip the _needsDispose, so I guess the reason why the original design was like this, was so that ReleaseHandle would get called always, and we would only actually dispose if we need to.

[bartonjs]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value, (maybe I'm wrong and it just finds a ctor that looks like one it knows how to call? or maybe a default is required and I'm having a very bad recollection day). I'm not sure this code actually prevents invalid handles.

A more canonical representation of this would be to declare the ldap_init call to return a SafeConnectionHandle (instead of IntPtr) and the caller should do

SafeConnectionHandlehandle=LdapPal.Initialize(null,389);

// In LdapPal.Windows.cs

internalstaticSafeConnectionHandleInitialize(stringhost,intport){SafeConnectionHandlehandle=Interop.wldap32.ldap_init(host,port);if(handle.IsInvalid){handle.Dispose();// this error handling code}returnhandle;}

[stephentoub]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value

Nope :)

e.g. try this:

usingSystem;usingSystem.Runtime.InteropServices;classProgram{staticvoidMain()=>GetStdHandle(-10);[DllImport("kernel32.dll",SetLastError=true)]privatestaticexternMySafeHandleGetStdHandle(intnStdHandle);}classMySafeHandle:SafeHandle{publicMySafeHandle(stringhello,stringworld):base(IntPtr.Zero,ownsHandle:false){}publicoverrideboolIsInvalid=>true;protectedoverrideboolReleaseHandle()=>true;}

You'll get an exception like this:

Unhandled Exception: System.MissingMethodException: .ctor
at Program.GetStdHandle(Int32 nStdHandle)
at Program.Main()

[bartonjs]

Since this is used by the P/Invoke declarations in Interop, I'd put this file in that same directory (similar to the Windows one). e.g. https://github.com/dotnet/runtime/blob/master/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OCSP.cs#L103-L134

[bartonjs]

(Following the OCSP example, you could just nestle the appropriate SafeHandle types in the bottom of the Interop.Ber.cs and Interop.Ldap.cs files, so that if some other library needs them it's just one include)

[bartonjs]

For long-term test-building maintenance, consider a helper. E.g.

yieldreturnnewobject[]{0,0,null,FormatLengths("30{0}020100020100A0{1}020100020100",18,6)};
...private static byte[]FormatLengths(stringhexFormat,paramsint[]lengths){string[]hexLengths=newstring[lengths.Length];for(inti=0;i<lengths.Length;i++){intcurLen=lengths[i];if(PlatformDetection.IsWindows){hexLengths[i]=$"84{curLen:X8}";}elseif(curLen<0x80){hexLengths[i]=curLen.ToString("X2");}elseif(curLen<=byte.MaxValue){hexLengths[i]=$"81{curLen:X2}";}elseif(curLen<=ushort.MaxValue){hexLengths[i]=$"82{curLen:X4}";}elseif(curLen<=0xFFFFFF){hexLengths[i]=$"83{curLen:X6}";}else{hexLengths[i]=$"84{curLen:X8}";}}stringhex=string.Format(hexFormat,hexLengths);// This presupposes something like the crypto tests have:returnhex.HexToByteArray();}

[joperezr]

I agree this would be valuable, but if that's ok with you I would rather do that in a subsequent PR so to not add more complexity to this PR for now. Once I have tests running on Mono and OSX and passing, then I think it would be a great time to address this.

[bartonjs]

Deferring the test helper is totally fine.

[joperezr]

Thanks a lot to all for the reviews! 😄

[danmoseley]

Nice! 🔥

[JunTaoLuo]

🎆

[joperezr]

Fixes#23944

Creating this as a draft PR first as I expect a lot of feedback plus some things will still need to happen in order to get tests working in Linux, but I want to start getting feedback so I can address it while I finish setting up running tests in Linux. This adds support to Linux for the System.DirectoryServices.Protocols namespace, by simply creating a PAL layer that abstracts the calls between wldap32 and openldap. Given both native libraries are just implementing ldap protocol, most of their APIs are the same with very similar or same structures which makes it so that managed code didn't actually have to change that much, we just change the native calls we do underneath. There are some open questions, like for example, there are some settings that are specific to wldap32 and not supported on any other library, the question is, should they be No-Ops for compatibility with previous apps working in Windows or should they instead throw PNSE?

One thing that is missing from this PR is the support for Default Credentials. That means that if you have your code running on a Kestrel Server which is domain-joined to an AD and have a valid session token already authenticated in the machine, then you wouldn't need to pass in credentials again when creating an LDAPConnection since you would just reuse that token. That works today in Windows, and I'm working on fixing the native calls on Bind to make sure that it also works on Linux. That support will come either as part of this PR later, or it will be added as a subsequent PR.

cc: @ericstj@tarekgh@bartonjs@stephentoub@davidsh

FYI: @JunTaoLuo@Tratcher

[tarekgh]

CC @tquerec

[tarekgh]

CharSet = CharSet.Unicode [](start = 41, length = 25)

why this needed here?

[joperezr]

I'm actually not sure, this was the way this struct already existed, I just moved them to a new file.

[bartonjs]

disposeHandle here seems to be redundant with the one from the base class... which you're always giving the literal true.

If it means "false to never call ReleaseHandle", that's exactly what the base bool ownsHandle is for.

runtime/src/coreclr/src/vm/safehandle.cpp

Lines 114 to 136 in 5d5c36b

	fPerformRelease = ((oldState & (SH_State_RefCount | SH_State_Closed)) == SH_RefCountOne) && m_ownsHandle;
	if (fPerformRelease)
	{
	GCPROTECT_BEGIN(sh);
	CLR_BOOLfIsInvalid = FALSE;
	DECLARE_ARGHOLDER_ARRAY(args, 1);
	args[ARGNUM_0] = OBJECTREF_TO_ARGHOLDER(sh);
	PREPARE_SIMPLE_VIRTUAL_CALLSITE_USING_SLOT(s_IsInvalidHandleMethodSlot, sh);
	CRITICAL_CALLSITE;
	CALL_MANAGED_METHOD(fIsInvalid, CLR_BOOL, args);
	if (fIsInvalid)
	{
	fPerformRelease = false;
	}
	GCPROTECT_END();
	}

[joperezr]

So is your suggestion to basically instead of having a field for _needDispose and checking this in ReleaseHandle, I should just call base passing in disposeHandle value and not need to check in ReleaseHandle as it would not get called in the case it was set to false right? Just making sure I understood this comment..

[joperezr]

Actually we do have one problem with doing this. The one in the base I believe can't be overwritten after creation, and we have some (internal) methods in LdapConnection that can flip the _needsDispose, so I guess the reason why the original design was like this, was so that ReleaseHandle would get called always, and we would only actually dispose if we need to.

[bartonjs]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value, (maybe I'm wrong and it just finds a ctor that looks like one it knows how to call? or maybe a default is required and I'm having a very bad recollection day). I'm not sure this code actually prevents invalid handles.

A more canonical representation of this would be to declare the ldap_init call to return a SafeConnectionHandle (instead of IntPtr) and the caller should do

SafeConnectionHandlehandle=LdapPal.Initialize(null,389);

// In LdapPal.Windows.cs

internalstaticSafeConnectionHandleInitialize(stringhost,intport){SafeConnectionHandlehandle=Interop.wldap32.ldap_init(host,port);if(handle.IsInvalid){handle.Dispose();// this error handling code}returnhandle;}

[stephentoub]

I feel like the marshaller bypasses ctors when it creates SafeHandles as a return value

Nope :)

e.g. try this:

usingSystem;usingSystem.Runtime.InteropServices;classProgram{staticvoidMain()=>GetStdHandle(-10);[DllImport("kernel32.dll",SetLastError=true)]privatestaticexternMySafeHandleGetStdHandle(intnStdHandle);}classMySafeHandle:SafeHandle{publicMySafeHandle(stringhello,stringworld):base(IntPtr.Zero,ownsHandle:false){}publicoverrideboolIsInvalid=>true;protectedoverrideboolReleaseHandle()=>true;}

You'll get an exception like this:

Unhandled Exception: System.MissingMethodException: .ctor
at Program.GetStdHandle(Int32 nStdHandle)
at Program.Main()

[bartonjs]

Since this is used by the P/Invoke declarations in Interop, I'd put this file in that same directory (similar to the Windows one). e.g. https://github.com/dotnet/runtime/blob/master/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OCSP.cs#L103-L134

[bartonjs]

(Following the OCSP example, you could just nestle the appropriate SafeHandle types in the bottom of the Interop.Ber.cs and Interop.Ldap.cs files, so that if some other library needs them it's just one include)

[bartonjs]

For long-term test-building maintenance, consider a helper. E.g.

yieldreturnnewobject[]{0,0,null,FormatLengths("30{0}020100020100A0{1}020100020100",18,6)};
...private static byte[]FormatLengths(stringhexFormat,paramsint[]lengths){string[]hexLengths=newstring[lengths.Length];for(inti=0;i<lengths.Length;i++){intcurLen=lengths[i];if(PlatformDetection.IsWindows){hexLengths[i]=$"84{curLen:X8}";}elseif(curLen<0x80){hexLengths[i]=curLen.ToString("X2");}elseif(curLen<=byte.MaxValue){hexLengths[i]=$"81{curLen:X2}";}elseif(curLen<=ushort.MaxValue){hexLengths[i]=$"82{curLen:X4}";}elseif(curLen<=0xFFFFFF){hexLengths[i]=$"83{curLen:X6}";}else{hexLengths[i]=$"84{curLen:X8}";}}stringhex=string.Format(hexFormat,hexLengths);// This presupposes something like the crypto tests have:returnhex.HexToByteArray();}

[joperezr]

I agree this would be valuable, but if that's ok with you I would rather do that in a subsequent PR so to not add more complexity to this PR for now. Once I have tests running on Mono and OSX and passing, then I think it would be a great time to address this.

[bartonjs]

Deferring the test helper is totally fine.

[joperezr]

Thanks a lot to all for the reviews! 😄

[danmoseley]

Nice! 🔥

[JunTaoLuo]

🎆