Skip to content

Fix potential null pCommonAssemImport handling in ImportTypeRef #126151

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.

Sign up for GitHub

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

Open
tpa95 wants to merge 3 commits into
base: main
Choose a base branch
from
+66 −57
Open

Fix potential null pCommonAssemImport handling in ImportTypeRef #126151

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80ff174
Fix potential null pCommonAssemImport handling in ImportTypeRef
tpa95 Mar 26, 2026
b0082ba
Merge branch 'main' into fix/coreclr_null_deref
tpa95 Mar 26, 2026
19f0299
Merge branch 'main' into fix/coreclr_null_deref
tpa95 Mar 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 66 additions & 57 deletions src/coreclr/md/compiler/importhelper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2681,64 +2681,70 @@ HRESULT ImportHelper::ImportTypeRef(
mdToken tkImplementation; // Implementation token for ExportedType.
if (IsNilToken(tkOuterImportRes))
{
// <REVISIT_TODO>BUG FIX:: URT 13626
// Well, before all of the clients generate AR for SPCL reference, it is not true
// that tkOuterImportRes == nil will imply that we have to find such an entry in the import manifest!!</REVISIT_TODO>

// Look for a ExportedType entry in the import Assembly. Its an error
// if we don't find a ExportedType entry.
mdExportedType tkExportedType;
hr = pCommonAssemImport->CommonFindExportedType(
cqaNesterNamespaces[cqaNesters.Size() - 1],
cqaNesterNames[cqaNesters.Size() - 1],
mdTokenNil,
&tkExportedType);
if (SUCCEEDED(hr))
{
IfFailGo(pCommonAssemImport->CommonGetExportedTypeProps(
tkExportedType,
NULL,
NULL,
&tkImplementation));
if (TypeFromToken(tkImplementation) == mdtFile)
{
// Type is from a different Assembly.
IfFailGo(CreateAssemblyRefFromAssembly(pMiniMdAssemEmit,
pMiniMdEmit,
pCommonAssemImport,
pbHashValue,
cbHashValue,
&tkOuterEmitRes));
}
else if (TypeFromToken(tkImplementation) == mdtAssemblyRef)
if (pCommonAssemImport != NULL)
Copy link
Copy Markdown
Member

@jkotas jkotas Mar 26, 2026

Choose a reason for hiding this comment

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

The other invariants in the code should guarantee that this is never null.

If this was ever null, it is better to crash or return error rather than silently misbehaving.

Copy link
Copy Markdown
Contributor Author

@tpa95 tpa95 Mar 27, 2026

Choose a reason for hiding this comment

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

@jkotas Thanks for the review. Just to make sure I understand the expected direction: would you prefer to rely on the invariant that pCommonAssemImport is never null here, or to handle a null value explicitly with an assert/error path rather than a silent check?

{
// <REVISIT_TODO>BUG FIX:: URT 13626
// Well, before all of the clients generate AR for SPCL reference, it is not true
// that tkOuterImportRes == nil will imply that we have to find such an entry in the import manifest!!</REVISIT_TODO>

// Look for a ExportedType entry in the import Assembly. Its an error
// if we don't find a ExportedType entry.
mdExportedType tkExportedType;
hr = pCommonAssemImport->CommonFindExportedType(
cqaNesterNamespaces[cqaNesters.Size() - 1],
cqaNesterNames[cqaNesters.Size() - 1],
mdTokenNil,
&tkExportedType);
if (SUCCEEDED(hr))
{
// This folds into the case where the Type is AssemblyRef. So
// let it fall through to that case.
IfFailGo(pCommonAssemImport->CommonGetExportedTypeProps(
tkExportedType,
NULL,
NULL,
&tkImplementation));
if (TypeFromToken(tkImplementation) == mdtFile)
{
// Type is from a different Assembly.
IfFailGo(CreateAssemblyRefFromAssembly(pMiniMdAssemEmit,
pMiniMdEmit,
pCommonAssemImport,
pbHashValue,
cbHashValue,
&tkOuterEmitRes));
}
else if (TypeFromToken(tkImplementation) == mdtAssemblyRef)
{
// This folds into the case where the Type is AssemblyRef. So
// let it fall through to that case.

// Remember that this AssemblyRef token is actually from the Manifest scope not
// the module scope!!!
bAssemblyRefFromAssemScope = true;
tkOuterImportRes = tkImplementation;
// Remember that this AssemblyRef token is actually from the Manifest scope not
// the module scope!!!
bAssemblyRefFromAssemScope = true;
tkOuterImportRes = tkImplementation;
}
else
_ASSERTE(!"Unexpected ExportedType implementation token.");
}
else
_ASSERTE(!"Unexpected ExportedType implementation token.");
}
else
{
// In this case, we will just move over the TypeRef with Nil ResolutionScope.
hr = NOERROR;
tkOuterEmitRes = mdTokenNil;
{
// In this case, we will just move over the TypeRef with Nil ResolutionScope.
hr = NOERROR;
tkOuterEmitRes = mdTokenNil;
}
}
}
else if (TypeFromToken(tkOuterImportRes) == mdtModule)
{
// Type is from a different Assembly.
IfFailGo(CreateAssemblyRefFromAssembly(pMiniMdAssemEmit,
pMiniMdEmit,
pCommonAssemImport,
pbHashValue,
cbHashValue,
&tkOuterEmitRes));
if (pCommonAssemImport != NULL)
{
// Type is from a different Assembly.
IfFailGo(CreateAssemblyRefFromAssembly(pMiniMdAssemEmit,
pMiniMdEmit,
pCommonAssemImport,
pbHashValue,
cbHashValue,
&tkOuterEmitRes));
}
}
// Not else if, because mdtModule case above could change
// tkOuterImportRes to an AssemblyRef.
Expand Down Expand Up @@ -2843,13 +2849,16 @@ HRESULT ImportHelper::ImportTypeRef(
}
else if (TypeFromToken(tkOuterImportRes) == mdtModuleRef)
{
// Type is from a different Assembly.
IfFailGo(CreateAssemblyRefFromAssembly(pMiniMdAssemEmit,
pMiniMdEmit,
pCommonAssemImport,
pbHashValue,
cbHashValue,
&tkOuterEmitRes));
if (pCommonAssemImport != NULL)
{
// Type is from a different Assembly.
IfFailGo(CreateAssemblyRefFromAssembly(pMiniMdAssemEmit,
pMiniMdEmit,
pCommonAssemImport,
pbHashValue,
cbHashValue,
&tkOuterEmitRes));
}
}
}

Expand Down
Loading