Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Explicitly handle char marshalling in OleVariant::GetNativeMethodTableForVarType. #26218

Merged

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Aug 16, 2019

The default handling for char marshalling in OleVariant::GetNativeMethodTableForVarType() doesn't correctly handle char[]s marshaled as ByValArray. The MethodTable* calculates for char elements doesn't correctly account for CharSet.

In master, this code-path is only executed as part of SystemV classification of by-val arrays, so it requires a contrived struct to encounter, such as the one below:

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
struct S
{
    [MarshalAs(UnmanagedType.ByValArray, SizeConst=6)]
    public char[] arr;

	public float f;
}

Currently, this classifies the first eight bytes as INTEGER and the second eight bytes as SSE. However, both eight bytes should be classified as INTEGER since the char array extends into the second eight byte.

I discovered this bug as part of my work on unifying the struct marshaling with the parameter and return value marshaling where I am using the OleVariant::GetNativeMethodTableForVarType() in more locations.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a test for this case.

@jkoritzinsky jkoritzinsky merged commit 72ceb36 into dotnet:master Aug 16, 2019
@jkoritzinsky jkoritzinsky deleted the char-fixed-arrays-marshalling-fix branch August 16, 2019 22:33
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…et/coreclr#26218)

* Explicitly handle char marshalling in OleVariant::GetNativeMethodTableForVarType.

* Added test case for ByVal Unicode Char Array.

* Fix contracts and remove unneeded GC transition.


Commit migrated from dotnet/coreclr@72ceb36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants