-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix COM source generation for arrays with null pointers and SizeParamIndex in both stateful and stateless marshalling #118136
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
Conversation
Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
| // we need to check if the native pointer is null before using the size parameter to avoid | ||
| // allocating arrays for null pointers. This applies to LPArray scenarios. | ||
| if (CodeContext.Direction == MarshalDirection.UnmanagedToManaged && | ||
| innerMarshaller.NativeType.FullTypeName.EndsWith("*")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| innerMarshaller.NativeType.FullTypeName.EndsWith("*")) | |
| innerMarshaller.NativeType.FullTypeName.EndsWith("*")) |
@jkoritzinsky or @jtschuster This seems like a suspect check. Is there a better way to check for array scenarios?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be checking if NativeType is a PointerTypeInfo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please try to apply the suggestion by @jtschuster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right. I've replaced the fragile type name check with a more semantic approach that checks if we have CountInfo that uses a parameter (like SizeParamIndex). The new check countInfo is SizeAndParamIndexInfo { ParamAtIndex: not null } specifically targets scenarios where we have size parameter information, which is exactly when null pointer issues can occur. This is much more precise and maintainable than checking if the type name ends with "*". (2e2e628)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied @jtschuster's suggestion to use innerMarshaller.NativeType is PointerTypeInfo instead of the parameter-based check. This is much more semantic and type-safe than the previous approaches. (c2942df)
… checking Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
|
@copilot You've updated the Stateful marshaller. You need to also update the Stateless marshaller. See |
Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
Applied the same null pointer fix to the stateless marshalling strategy. Updated three methods in StatelessLinearCollectionSpaceAllocator: GenerateUnmarshalStatements, GenerateGuaranteedUnmarshalStatements, and GenerateCleanupCallerAllocatedResourcesStatements to use the same semantic |
| // If the marshalling direction is unmanaged-to-managed and we have a native pointer type, | ||
| // we need to check if the native pointer is null before using the size parameter to avoid | ||
| // allocating arrays for null pointers. | ||
| if (CodeContext.Direction == MarshalDirection.UnmanagedToManaged && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to check the direction here as we're already under a "not ManagedToUnmanaged" check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right. Removed the redundant direction check since we're already under a "not ManagedToUnmanaged" condition at line 322. (265a03f)
| // If the marshalling direction is unmanaged-to-managed and we have a native pointer type, | ||
| // we need to check if the native pointer is null before using the size parameter to avoid | ||
| // allocating arrays for null pointers. | ||
| if (CodeContext.Direction == MarshalDirection.UnmanagedToManaged && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the direction checks in your changes to check for "not ManagedToUnmanaged" and add some tests for arrays passed by ref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the direction check and added tests for arrays passed by ref. The GenerateGuaranteedUnmarshalStatements method only runs for unmarshal scenarios so the direction check was redundant. Added TestMethodWithRef to the test interface and implementation to ensure ref array scenarios are covered. (265a03f)
|
@copilot please address my review comments |
…nd add ref array tests Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Addressed both review comments: removed redundant direction checks in stateless marshalling strategy and added tests for arrays passed by ref. The changes ensure consistency with the "not ManagedToUnmanaged" pattern and provide better test coverage. (265a03f) |
|
Superseded by #118190. |
|
Valiant effort @copilot! Thanks. |
The COM interface source generator was generating incorrect marshalling code for array parameters with
SizeParamIndexwhen the native array pointer could be null, affecting both stateful and stateless marshalling strategies.Problem
When generating unmanaged-to-managed stubs for COM interfaces with array parameters using
[MarshalAs(UnmanagedType.LPArray, SizeParamIndex = 0)], the generator would create code like:This caused issues when the native array pointer was null but the buffer size was non-zero, as it would attempt to allocate an array for a null pointer using the provided size.
Solution
Modified both
StatefulLinearCollectionMarshallingandStatelessLinearCollectionSpaceAllocatorto generate null-aware size calculation:The fix:
NativeType is PointerTypeInfocheck instead of fragile type name checkingTesting
Added
ArrayBufferMarshallingTests.cswith a test case that reproduces the scenario described in the issue, ensuring the generated code compiles and handles null pointer cases correctly.Fixes #118135.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.