Fix field offset computation for large version bubble (#25029)
authorJan Vorlicek <janvorli@microsoft.com>
Mon, 10 Jun 2019 14:33:42 +0000 (16:33 +0200)
committerGitHub <noreply@github.com>
Mon, 10 Jun 2019 14:33:42 +0000 (16:33 +0200)
There was a discrepancy in field offset calculations at crossgen time
and at runtime in some rare cases due to the alignment of a derived
class offset.
The issue happened due to MethodTableBuilder::NeedsAlignedBaseOffset not
taking into account the fact that the module of the parent and child
class can both be in large version bubble.

We also had a bug in the PEDecoder::GetNativeManifestMetadata. When it
was called for regular crossgened image without large version bubble, it
left the pDir uninitialized due to the fact that there was no
READYTORUN_SECTION_MANIFEST_METADATA. And then it tried to dereference
that.

src/utilcode/pedecoder.cpp
src/vm/ceeload.cpp
src/vm/ceeload.h
src/vm/methodtablebuilder.cpp

index a3e3b73..47f1574 100644 (file)
@@ -2839,7 +2839,7 @@ PTR_CVOID PEDecoder::GetNativeManifestMetadata(COUNT_T *pSize) const
     }
     CONTRACT_END;
     
-    IMAGE_DATA_DIRECTORY *pDir;
+    IMAGE_DATA_DIRECTORY *pDir = NULL;
 #ifdef FEATURE_PREJIT
     if (!HasReadyToRunHeader())
     {
@@ -2858,8 +2858,22 @@ PTR_CVOID PEDecoder::GetNativeManifestMetadata(COUNT_T *pSize) const
 
             READYTORUN_SECTION * pSection = pSections + i;
             if (pSection->Type == READYTORUN_SECTION_MANIFEST_METADATA)
+            {
                 // Set pDir to the address of the manifest metadata section
                 pDir = &pSection->Section;
+                break;
+            }
+        }
+
+        // ReadyToRun file without large version bubble support doesn't have the READYTORUN_SECTION_MANIFEST_METADATA
+        if (pDir == NULL)
+        {
+            if (pSize != NULL)
+            {
+                *pSize = 0;
+            }
+
+            RETURN NULL;
         }
     }
 
index 4235376..49d5fb5 100644 (file)
@@ -3406,6 +3406,67 @@ BOOL Module::IsInCurrentVersionBubble()
 #endif // FEATURE_NATIVE_IMAGE_GENERATION
 }
 
+#if defined(FEATURE_READYTORUN) && !defined(FEATURE_READYTORUN_COMPILER)
+//---------------------------------------------------------------------------------------
+// Check if the target module is in the same version bubble as this one
+// The current implementation uses the presence of an AssemblyRef for the target module's assembly in
+// the native manifest metadata.
+//
+// Arguments:
+//      * target - target module to check
+//
+// Return Value:
+//      TRUE if the target module is in the same version bubble as this one
+//
+BOOL Module::IsInSameVersionBubble(Module *target)
+{
+    CONTRACT(BOOL)
+    {
+        NOTHROW;
+        GC_NOTRIGGER;
+        MODE_ANY;
+    }
+    CONTRACT_END;
+
+    if (this == target)
+    {
+        RETURN TRUE;
+    }
+
+    if (!HasNativeOrReadyToRunImage())
+    {
+        RETURN FALSE;
+    }
+
+    // Check if the current module's image has native manifest metadata, otherwise the current->GetNativeAssemblyImport() asserts.
+    COUNT_T cMeta=0;
+    const void* pMeta = GetFile()->GetOpenedILimage()->GetNativeManifestMetadata(&cMeta);
+    if (pMeta == NULL)
+    {
+        RETURN FALSE;
+    }
+
+    IMDInternalImport* pMdImport = GetNativeAssemblyImport();
+
+    LPCUTF8 targetName = target->GetAssembly()->GetSimpleName();
+
+    HENUMInternal assemblyEnum;
+    HRESULT hr = pMdImport->EnumAllInit(mdtAssemblyRef, &assemblyEnum);
+    mdAssemblyRef assemblyRef;
+    while (pMdImport->EnumNext(&assemblyEnum, &assemblyRef))
+    {
+        LPCSTR assemblyName;
+        hr = pMdImport->GetAssemblyRefProps(assemblyRef, NULL, NULL, &assemblyName, NULL, NULL, NULL, NULL);
+        if (strcmp(assemblyName, targetName) == 0)
+        {
+            RETURN TRUE;
+        }
+    }
+
+    RETURN FALSE;
+}
+#endif // FEATURE_READYTORUN && !FEATURE_READYTORUN_COMPILER
+
 //---------------------------------------------------------------------------------------
 //
 // WinMD-aware helper to grab a readable public metadata interface. Any place that thinks
index 5cffacd..b602ac6 100644 (file)
@@ -1952,6 +1952,11 @@ protected:
 
     BOOL IsInCurrentVersionBubble();
 
+#if defined(FEATURE_READYTORUN) && !defined(FEATURE_READYTORUN_COMPILER)
+    BOOL IsInSameVersionBubble(Module *target);
+#endif // FEATURE_READYTORUN && !FEATURE_READYTORUN_COMPILER
+
+
     LPCWSTR GetPathForErrorMessages();
 
 
index 31c4b0a..2a59be4 100644 (file)
@@ -11302,6 +11302,12 @@ BOOL MethodTableBuilder::NeedsAlignedBaseOffset()
     if (IsValueClass())
         return FALSE;
 
+    MethodTable * pParentMT = GetParentMethodTable();
+
+    // Trivial parents
+    if (pParentMT == NULL || pParentMT == g_pObjectClass)
+        return FALSE;
+
     // Always use the ReadyToRun field layout algorithm if the source IL image was ReadyToRun, independent on
     // whether ReadyToRun is actually enabled for the module. It is required to allow mixing and matching 
     // ReadyToRun images with NGen.
@@ -11312,17 +11318,31 @@ BOOL MethodTableBuilder::NeedsAlignedBaseOffset()
             return FALSE;
     }
 
-    MethodTable * pParentMT = GetParentMethodTable();
-
-    // Trivial parents
-    if (pParentMT == NULL || pParentMT == g_pObjectClass)
-        return FALSE;
-
     if (pParentMT->GetModule() == GetModule())
     {
         if (!pParentMT->GetClass()->HasLayoutDependsOnOtherModules())
             return FALSE;
     }
+    else
+    {
+#ifdef FEATURE_READYTORUN_COMPILER
+        if (IsReadyToRunCompilation())
+        {
+            if (pParentMT->GetModule()->IsInCurrentVersionBubble())
+            {
+                return FALSE;
+            }
+        }
+#else // FEATURE_READYTORUN_COMPILER
+        if (GetModule()->GetFile()->IsILImageReadyToRun())
+        {
+            if (GetModule()->IsInSameVersionBubble(pParentMT->GetModule()))
+            {
+                return FALSE;
+            }
+        }
+#endif // FEATURE_READYTORUN_COMPILER
+    }
 
     return TRUE;
 }