Move the BSTR trailbyte tracking out of the sync block (#121362)

Co-authored-by: Jan Kotas <[email protected]>

Author: jkoritzinsky

src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs

@@ -11,6 +11,7 @@
 using System.Runtime.InteropServices.Marshalling;
 using System.Runtime.Versioning;
 using System.Text;
+using System.Threading;
 
 namespace System.StubHelpers
 {
@@ -229,6 +230,39 @@ internal static unsafe void ConvertToManaged(StringBuilder sb, IntPtr pNative)
 
     internal static class BSTRMarshaler
     {
+        private sealed class TrailByte(byte trailByte)
+        {
+            public readonly byte Value = trailByte;
+        }
+
+        // In some early version of VB when there were no arrays developers used to use BSTR as arrays
+        // The way this was done was by adding a trail byte at the end of the BSTR
+        // To support this scenario, we need to use a ConditionalWeakTable for this special case and
+        // save the trail character in here.
+        // This stores the trail character when a BSTR is used as an array.
+        private static ConditionalWeakTable<string, TrailByte>? s_trailByteTable;
+
+        private static bool TryGetTrailByte(string strManaged, out byte trailByte)
+        {
+            if (s_trailByteTable?.TryGetValue(strManaged, out TrailByte? trailByteObj) == true)
+            {
+                trailByte = trailByteObj.Value;
+                return true;
+            }
+
+            trailByte = 0;
+            return false;
+        }
+
+        private static void SetTrailByte(string strManaged, byte trailByte)
+        {
+            if (s_trailByteTable == null)
+            {
+                Interlocked.CompareExchange(ref s_trailByteTable, new ConditionalWeakTable<string, TrailByte>(), null);
+            }
+            s_trailByteTable!.Add(strManaged, new TrailByte(trailByte));
+        }
+
         internal static unsafe IntPtr ConvertToNative(string strManaged, IntPtr pNativeBuffer)
         {
             if (null == strManaged)
@@ -237,7 +271,7 @@ internal static unsafe IntPtr ConvertToNative(string strManaged, IntPtr pNativeB
             }
             else
             {
-                bool hasTrailByte = StubHelpers.TryGetStringTrailByte(strManaged, out byte trailByte);
+                bool hasTrailByte = TryGetTrailByte(strManaged, out byte trailByte);
 
                 uint lengthInBytes = (uint)strManaged.Length * 2;
 
@@ -320,8 +354,7 @@ internal static unsafe IntPtr ConvertToNative(string strManaged, IntPtr pNativeB
 
                 if ((length & 1) == 1)
                 {
-                    // odd-sized strings need to have the trailing byte saved in their sync block
-                    StubHelpers.SetStringTrailByte(ret, ((byte*)bstr)[length - 1]);
+                    SetTrailByte(ret, ((byte*)bstr)[length - 1]);
                 }
 
                 return ret;
@@ -1489,19 +1522,6 @@ internal static void CheckStringLength(uint length)
             }
         }
 
-        // Try to retrieve the extra byte - returns false if not present.
-        [MethodImpl(MethodImplOptions.InternalCall)]
-        internal static extern bool TryGetStringTrailByte(string str, out byte data);
-
-        // Set extra byte for odd-sized strings that came from interop as BSTR.
-        internal static void SetStringTrailByte(string str, byte data)
-        {
-            SetStringTrailByte(new StringHandleOnStack(ref str!), data);
-        }
-
-        [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "StubHelpers_SetStringTrailByte")]
-        private static partial void SetStringTrailByte(StringHandleOnStack str, byte data);
-
         internal static unsafe void FmtClassUpdateNativeInternal(object obj, byte* pNative, ref CleanupWorkListElement? pCleanupWorkList)
         {
             MethodTable* pMT = RuntimeHelpers.GetMethodTable(obj);

src/coreclr/vm/ecalllist.h

@@ -352,7 +352,6 @@ FCFuncEnd()
 
 FCFuncStart(gStubHelperFuncs)
     FCFuncElement("GetDelegateTarget", StubHelpers::GetDelegateTarget)
-    FCFuncElement("TryGetStringTrailByte", StubHelpers::TryGetStringTrailByte)
     FCFuncElement("SetLastError", StubHelpers::SetLastError)
     FCFuncElement("ClearLastError", StubHelpers::ClearLastError)
 #ifdef FEATURE_COMINTEROP

src/coreclr/vm/object.cpp

@@ -658,47 +658,6 @@ STRINGREF StringObject::NewString(INT32 length) {
     }
 }
 
-
-/*==================================NewString===================================
-**Action: Many years ago, VB didn't have the concept of a byte array, so enterprising
-**        users created one by allocating a BSTR with an odd length and using it to
-**        store bytes.  A generation later, we're still stuck supporting this behavior.
-**        The way that we do this is to take advantage of the difference between the
-**        array length and the string length.  The string length will always be the
-**        number of characters between the start of the string and the terminating 0.
-**        If we need an odd number of bytes, we'll take one wchar after the terminating 0.
-**        (e.g. at position StringLength+1).  The high-order byte of this wchar is
-**        reserved for flags and the low-order byte is our odd byte. This function is
-**        used to allocate a string of that shape, but we don't actually mark the
-**        trailing byte as being in use yet.
-**Returns: A newly allocated string.  Null if length is less than 0.
-**Arguments: length -- the length of the string to allocate
-**           bHasTrailByte -- whether the string also has a trailing byte.
-**Exceptions: OutOfMemoryException if AllocateString fails.
-==============================================================================*/
-STRINGREF StringObject::NewString(INT32 length, BOOL bHasTrailByte) {
-    CONTRACTL {
-        GC_TRIGGERS;
-        MODE_COOPERATIVE;
-        PRECONDITION(length>=0 && length != INT32_MAX);
-    } CONTRACTL_END;
-
-    STRINGREF pString;
-    if (length<0 || length == INT32_MAX) {
-        return NULL;
-    } else if (length == 0) {
-        return GetEmptyString();
-    } else {
-        pString = AllocateString(length);
-        _ASSERTE(pString->GetBuffer()[length]==0);
-        if (bHasTrailByte) {
-            _ASSERTE(pString->GetBuffer()[length+1]==0);
-        }
-    }
-
-    return pString;
-}
-
 //========================================================================
 // Creates a System.String object and initializes from
 // the supplied null-terminated C string.
@@ -887,74 +846,6 @@ STRINGREF* StringObject::InitEmptyStringRefPtr() {
     return EmptyStringRefPtr;
 }
 
-/*============================InternalTrailByteCheck============================
-**Action: Many years ago, VB didn't have the concept of a byte array, so enterprising
-**        users created one by allocating a BSTR with an odd length and using it to
-**        store bytes.  A generation later, we're still stuck supporting this behavior.
-**        The way that we do this is stick the trail byte in the sync block
-**        whenever we encounter such a situation. Since we expect this to be a very corner case
-**        accessing the sync block seems like a good enough solution
-**
-**Returns: True if <CODE>str</CODE> contains a VB trail byte, false otherwise.
-**Arguments: str -- The string to be examined.
-**Exceptions: None
-==============================================================================*/
-BOOL StringObject::HasTrailByte() {
-    WRAPPER_NO_CONTRACT;
-
-    SyncBlock * pSyncBlock = PassiveGetSyncBlock();
-    if(pSyncBlock != NULL)
-    {
-        return pSyncBlock->HasCOMBstrTrailByte();
-    }
-
-    return FALSE;
-}
-
-/*=================================GetTrailByte=================================
-**Action:  If <CODE>str</CODE> contains a vb trail byte, returns a copy of it.
-**Returns: True if <CODE>str</CODE> contains a trail byte.  *bTrailByte is set to
-**         the byte in question if <CODE>str</CODE> does have a trail byte, otherwise
-**         it's set to 0.
-**Arguments: str -- The string being examined.
-**           bTrailByte -- An out param to hold the value of the trail byte.
-**Exceptions: None.
-==============================================================================*/
-BOOL StringObject::GetTrailByte(BYTE *bTrailByte) {
-    CONTRACTL
-    {
-        NOTHROW;
-        GC_NOTRIGGER;
-        MODE_ANY;
-    }
-    CONTRACTL_END;
-    _ASSERTE(bTrailByte);
-    *bTrailByte=0;
-
-    BOOL retValue = HasTrailByte();
-
-    if(retValue)
-    {
-        *bTrailByte = GET_VB_TRAIL_BYTE(GetHeader()->PassiveGetSyncBlock()->GetCOMBstrTrailByte());
-    }
-
-    return retValue;
-}
-
-/*=================================SetTrailByte=================================
-**Action: Sets the trail byte in the sync block
-**Returns: True.
-**Arguments: str -- The string into which to set the trail byte.
-**           bTrailByte -- The trail byte to be added to the string.
-**Exceptions: None.
-==============================================================================*/
-BOOL StringObject::SetTrailByte(BYTE bTrailByte) {
-    WRAPPER_NO_CONTRACT;
-
-    GetHeader()->GetSyncBlock()->SetCOMBstrTrailByte(MAKE_VB_TRAIL_BYTE(bTrailByte));
-    return TRUE;
-}
-
 #ifdef USE_CHECKED_OBJECTREFS
 
 //-------------------------------------------------------------

src/coreclr/vm/object.h

@@ -832,7 +832,6 @@ class StringObject : public Object
     // characters and the null terminator you should pass in 5 and NOT 6.
     //========================================================================
     static STRINGREF NewString(int length);
-    static STRINGREF NewString(int length, BOOL bHasTrailByte);
     static STRINGREF NewString(const WCHAR *pwsz);
     static STRINGREF NewString(const WCHAR *pwsz, int length);
     static STRINGREF NewString(LPCUTF8 psz);
@@ -843,10 +842,6 @@ class StringObject : public Object
 
     static STRINGREF* InitEmptyStringRefPtr();
 
-    BOOL HasTrailByte();
-    BOOL GetTrailByte(BYTE *bTrailByte);
-    BOOL SetTrailByte(BYTE bTrailByte);
-
     /*=================RefInterpretGetStringValuesDangerousForGC======================
     **N.B.: This performs no range checking and relies on the caller to have done this.
     **Args: (IN)ref -- the String to be interpretted.