Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disabling runtime marshaling broke DomainControllerTests.FindAll_NoSuchName_ReturnsEmpty() #65259

Closed
danmoseley opened this issue Feb 12, 2022 · 10 comments · Fixed by #65319
Closed
Labels
area-System.DirectoryServices untriaged New issue has not been triaged by the area owner

Comments

@danmoseley
Copy link
Member

     <test name="System.DirectoryServices.ActiveDirectory.Tests.DomainControllerTests.FindAll_NoSuchName_ReturnsEmpty" type="System.DirectoryServices.ActiveDirectory.Tests.DomainControllerTests" method="FindAll_NoSuchName_ReturnsEmpty" time="0.0115152" result="Fail">
        <failure exception-type="System.Runtime.InteropServices.MarshalDirectiveException">
          <message><![CDATA[System.Runtime.InteropServices.MarshalDirectiveException : Cannot marshal 'parameter #7': Cannot marshal managed types when the runtime marshalling system is disabled.]]></message>
          <stack-trace><![CDATA[   at System.Runtime.InteropServices.Marshal.GetDelegateForFunctionPointerInternal(IntPtr ptr, Type t)
   at System.Runtime.InteropServices.Marshal.GetDelegateForFunctionPointer(IntPtr ptr, Type t) in C:\git\runtime\src\libraries\System.Private.CoreLib\src\System\Runtime\InteropServices\Marshal.cs:line 1122
   at System.DirectoryServices.ActiveDirectory.Utils.GetDNFromDnsName(String dnsName) in C:\git\runtime\src\libraries\System.DirectoryServices\src\System\DirectoryServices\ActiveDirectory\Utils.cs:line 204
   at System.DirectoryServices.ActiveDirectory.DomainController.FindAllInternal(DirectoryContext context, String domainName, Boolean isDnsDomainName, String siteName) in C:\git\runtime\src\libraries\System.DirectoryServices\src\System\DirectoryServices\ActiveDirectory\DomainController.cs:line 1069
   at System.DirectoryServices.ActiveDirectory.DomainController.FindAll(DirectoryContext context) in C:\git\runtime\src\libraries\System.DirectoryServices\src\System\DirectoryServices\ActiveDirectory\DomainController.cs:line 259
   at System.DirectoryServices.ActiveDirectory.Tests.DomainControllerTests.FindAll_NoSuchName_ReturnsEmpty() in C:\git\runtime\src\libraries\System.DirectoryServices\tests\System\DirectoryServices\ActiveDirectory\DomainControllerTests.cs:line 121]]></stack-trace>
        </failure>
      </test>

NativeMethods.DsCrackNames dsCrackNames = (NativeMethods.DsCrackNames)Marshal.GetDelegateForFunctionPointer(functionPtr, typeof(NativeMethods.DsCrackNames));

It seems it cannot marshal this

This may have been an edit that was missed from #64279 perhaps because the codepath didn't exercise due to different network config. @jkoritzinsky can you advise here? Looking at the other changes you made, I'm unclear what is needed.

as an aside this code change seems like it should be made at the same time

-                NativeMethods.DsCrackNames dsCrackNames = Marshal.GetDelegateForFunctionPointer(functionPtr, typeof(NativeMethods.DsCrackNames));
+               NativeMethods.DsCrackNames dsCrackNames = Marshal.GetDelegateForFunctionPointer<NativeMethods.DsCrackNames>(functionPtr);

this is on CoreCLR build clr+libs+libs.tests -rc release -lc debug -test

@ghost
Copy link

ghost commented Feb 12, 2022

Tagging subscribers to this area: @dotnet/area-system-directoryservices, @jay98014
See info in area-owners.md if you want to be subscribed.

Issue Details
     <test name="System.DirectoryServices.ActiveDirectory.Tests.DomainControllerTests.FindAll_NoSuchName_ReturnsEmpty" type="System.DirectoryServices.ActiveDirectory.Tests.DomainControllerTests" method="FindAll_NoSuchName_ReturnsEmpty" time="0.0115152" result="Fail">
        <failure exception-type="System.Runtime.InteropServices.MarshalDirectiveException">
          <message><![CDATA[System.Runtime.InteropServices.MarshalDirectiveException : Cannot marshal 'parameter #7': Cannot marshal managed types when the runtime marshalling system is disabled.]]></message>
          <stack-trace><![CDATA[   at System.Runtime.InteropServices.Marshal.GetDelegateForFunctionPointerInternal(IntPtr ptr, Type t)
   at System.Runtime.InteropServices.Marshal.GetDelegateForFunctionPointer(IntPtr ptr, Type t) in C:\git\runtime\src\libraries\System.Private.CoreLib\src\System\Runtime\InteropServices\Marshal.cs:line 1122
   at System.DirectoryServices.ActiveDirectory.Utils.GetDNFromDnsName(String dnsName) in C:\git\runtime\src\libraries\System.DirectoryServices\src\System\DirectoryServices\ActiveDirectory\Utils.cs:line 204
   at System.DirectoryServices.ActiveDirectory.DomainController.FindAllInternal(DirectoryContext context, String domainName, Boolean isDnsDomainName, String siteName) in C:\git\runtime\src\libraries\System.DirectoryServices\src\System\DirectoryServices\ActiveDirectory\DomainController.cs:line 1069
   at System.DirectoryServices.ActiveDirectory.DomainController.FindAll(DirectoryContext context) in C:\git\runtime\src\libraries\System.DirectoryServices\src\System\DirectoryServices\ActiveDirectory\DomainController.cs:line 259
   at System.DirectoryServices.ActiveDirectory.Tests.DomainControllerTests.FindAll_NoSuchName_ReturnsEmpty() in C:\git\runtime\src\libraries\System.DirectoryServices\tests\System\DirectoryServices\ActiveDirectory\DomainControllerTests.cs:line 121]]></stack-trace>
        </failure>
      </test>

NativeMethods.DsCrackNames dsCrackNames = (NativeMethods.DsCrackNames)Marshal.GetDelegateForFunctionPointer(functionPtr, typeof(NativeMethods.DsCrackNames));

It seems it cannot marshal this

This may have been an edit that was missed from #64279 perhaps because the codepath didn't exercise due to different network config. @jkoritzinsky can you advise here? Looking at the other changes you made, I'm unclear what is needed.

as an aside this code change seems like it should be made at the same time

-                NativeMethods.DsCrackNames dsCrackNames = Marshal.GetDelegateForFunctionPointer(functionPtr, typeof(NativeMethods.DsCrackNames));
+               NativeMethods.DsCrackNames dsCrackNames = Marshal.GetDelegateForFunctionPointer<NativeMethods.DsCrackNames>(functionPtr);

this is on CoreCLR build clr+libs+libs.tests -rc release -lc debug -test

Author: danmoseley
Assignees: -
Labels:

area-System.DirectoryServices

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Feb 12, 2022
@danmoseley
Copy link
Member Author

Re the aside, we have ~30 uses of (type)Marshal.GetDelegateForFunctionPointer(ptr, type) that probably should use the modern version to eliminate dynamic code. Is that a safe search and replace?

@jkoritzinsky
Copy link
Member

@danmoseley if you change that last parameter to be an IntPtr* instead of out IntPtr and change the usages, then it will work with runtime marshalling disabled.

Re the aside: yes, those should be changed to use the generic version.

@danmoseley
Copy link
Member Author

@jkoritzinsky is there a way to scan, or grep, for other such issues (of the IntPtr* change) so we don't rely only on unit tests? In some libraries like this one, there are plenty of pinvokes, but the code coverage is not great, especially of error paths.

@jkoritzinsky
Copy link
Member

I have an analyzer that will catch all of these cases at dotnet/roslyn-analyzers#5822. Just waiting on review to get it merged.

@danmoseley
Copy link
Member Author

I ended up with this danmoseley@4990310

Is that right? The code looks strictly worse. Is there something I can read to help me understand why this is necessary?

There are ~20 odd pinvokes that have out Intptr in this NativeMethods file, do they all need the change? Perhaps you could take care of the various fixes here?

@danmoseley
Copy link
Member Author

I guess, you can run your analyzer/fixer locally against this library to test it that way :)

@jkoritzinsky
Copy link
Member

@danmoseley this issue explains the rationale behind the "disabled runtime marshalling" mode:

#60639

The source generator is using this mode to enable it to easily determine which types do not require any marshalling by checking if a type is unmanaged instead of building a bespoke system to check and validate if a type is blittable by the traditional blittable rules (all primitives except bool and maybe char).

I can take care of these fixes.

@jkoritzinsky
Copy link
Member

Delegate-based P/Invokes haven't yet moved a source-generated model, so they don't interact particularly cleanly with the disabled runtime marshalling mode if they use a lot of marshalling.

@danmoseley
Copy link
Member Author

Got it - essentially out IntPtr requires code generation to add pinning, whereas IntPtr* can assume that's taken care of.

Thanks for doing the cleanup @jkoritzinsky

jkoritzinsky added a commit to jkoritzinsky/runtime that referenced this issue Feb 14, 2022
…unction pointers.

All of the delegates used very simple marshalling and they were all used in one or two places immediately after a GetProcAddress call for them, so it seemed like it was worthwhile to go all the way to function pointers instead of just manually pinning the parameters and still allocating delegates.

Fixes dotnet#65259
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 14, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 14, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.DirectoryServices untriaged New issue has not been triaged by the area owner
Projects
None yet
2 participants