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

[release/5.0] Make sure we consider buffer length when marshalling back Unicode ByValTStr fields #54752

Merged
merged 1 commit into from
Jul 9, 2021

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Jun 25, 2021

Port #54695 to release/5.0

Customer Impact

The customer is trying to read a BSTR representing "012", into two strings "01" a "2" based on the buffer size. The strings point to the correct position but the parsing doesn't stop at buffer size, so the result is "012" and "2". The customer's scenario is a 64 character long string with 13 fields, this is the repro they reported the issue with.

using System;
using System.Runtime.InteropServices;
using System.Text;

public class Program
{
    public static void Main()
    {
       string str = "012";
       IntPtr pBuf = Marshal.StringToBSTR(str);
       CEStruct ces = (CEStruct)Marshal.PtrToStructure(pBuf, typeof(CEStruct));
       Console.WriteLine(ces.tipoDocumento);
       Console.WriteLine(ces.tipoContribuyente);
    }
}

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
public struct CEStruct
{
    [MarshalAs(UnmanagedType.ByValTStr, SizeConst = 2)]
    public string tipoDocumento;
    [MarshalAs(UnmanagedType.ByValTStr, SizeConst = 1)]
    public string tipoContribuyente;
}

Instead of reading only up to the constant buffer length based on the string marshalling data, the runtime would read until it found a null terminator. In the case described by the user, it would read the whole string instead of just the two characters. If the buffer itself is not null-terminated (since is supposed to be constant length), then it would read significantly more data than it was supposed to and create a string from that.

Testing

Testing added in PR.

Risk

Low. We have unit tests validating multiple edge cases with the scenario

Regression

This is a regression from 3.1

@jkoritzinsky jkoritzinsky added this to the 5.0.x milestone Jun 25, 2021
@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Jul 2, 2021
@jeffschwMSFT
Copy link
Member

@jkoritzinsky can you request a cr and take a look at the failing ci?

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. We should consider for 5.0.x

@jkoritzinsky
Copy link
Member Author

Cr is requested, just waiting for someone to do it.

CI failures are due to network blips causing DNS to intermittently fail.

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jul 6, 2021
@leecow leecow modified the milestones: 5.0.x, 5.0.9 Jul 6, 2021
@Anipik Anipik merged commit 9d5c191 into dotnet:release/5.0 Jul 9, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 8, 2021
@jkoritzinsky jkoritzinsky deleted the fixes/5.0/54662 branch September 28, 2021 00:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Interop-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants