Skip to content

Fixed some potential null derefs in coreclr #123939

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 18 commits into
base: main
Choose a base branch
from
Open

Fixed some potential null derefs in coreclr #123939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
0072cc2
A potential null deref has been fixed.
tpa95 Feb 3, 2026
6af8682
Fixed a potential null deref.
tpa95 Feb 3, 2026
7f638f0
Merge branch 'main' into fix/coreclr-null-derefs
tpa95 Feb 3, 2026
2521671
Merge branch 'main' into fix/coreclr-null-derefs
tpa95 Feb 4, 2026
5eb983c
Merge branch 'main' into fix/coreclr-null-derefs
tpa95 Feb 4, 2026
583d586
Merge branch 'main' into fix/coreclr-null-derefs
tpa95 Feb 4, 2026
975cf57
Merge branch 'main' into fix/coreclr-null-derefs
tpa95 Feb 5, 2026
875ef59
Merge branch 'main' into fix/coreclr-null-derefs
tpa95 Feb 5, 2026
ba52dd0
Merge branch 'main' into fix/coreclr-null-derefs
tpa95 Feb 6, 2026
d77d402
Merge branch 'main' into fix/coreclr-null-derefs
tpa95 Feb 9, 2026
8c99425
Merge branch 'main' into fix/coreclr-null-derefs
tpa95 Feb 9, 2026
abbc516
Merge branch 'main' into fix/coreclr-null-derefs
tpa95 Feb 12, 2026
96c5d6f
Merge branch 'main' into fix/coreclr-null-derefs
tpa95 Feb 16, 2026
e0bfb00
Revert "Fixed a potential null deref."
tpa95 Feb 16, 2026
99174b2
Fixed a potential null deref.
tpa95 Feb 16, 2026
0049e89
Merge branch 'fix/coreclr-null-derefs' of https://github.com/tpa95/ru…
tpa95 Feb 16, 2026
5654a4e
Merge branch 'main' into fix/coreclr-null-derefs
tpa95 Feb 16, 2026
210648a
Merge branch 'main' into fix/coreclr-null-derefs
tpa95 Feb 17, 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
6 changes: 6 additions & 0 deletions src/coreclr/debug/daccess/daccess.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,6 +1121,12 @@ SplitName::CdStartField(_In_opt_ PCWSTR fullName,

if (typeHandle.IsNull())
{
if (mod == NULL)
{
status = E_INVALIDARG;
goto Fail;
}

if (typeToken == mdTypeDefNil)
{
if (!split->FindType(mod->GetMDImport()))
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/pal/src/map/virtual.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,7 +715,8 @@ VIRTUALCommitMemory(
TRACE( "Committing the memory now..\n");

nProtect = W32toUnixAccessControl(flProtect);

pRetVal = (void *) StartBoundary;

Comment on lines +718 to +719
Copy link
Member

@huoyaoyuan huoyaoyuan Feb 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This is affecting the munmap call in error branch.

Copy link
Contributor Author

@tpa95 tpa95 Feb 4, 2026

Choose a reason for hiding this comment

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

Yes. If mprotect fails, we jump to error before pRetVal is assigned, so munmap(pRetVal, MemSize) ends up being called with NULL.
Also, I can instead call munmap((void*)StartBoundary, MemSize) in the error branch.
But in the previous version of the code, before commit 435bff2, pRetVal was initialized before calling mprotect.

Copy link
Member

@noahfalk noahfalk Feb 5, 2026

Choose a reason for hiding this comment

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

fyi @janvorli

Copy link
Member

@noahfalk noahfalk Feb 5, 2026

Choose a reason for hiding this comment

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

Its not clear to me that calling munmap(NULL, ...) would cause a NULL-deref but it did seem like the intent of the code was to unmap the memory on error. Calling munmap(StartBoundary, ...) might be a clearer way to write that but I'll let @janvorli decide.

Copy link
Member

@janvorli janvorli Feb 5, 2026

Choose a reason for hiding this comment

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

I think the munmap would likely fail with EINVAL and would not dereference NULL. But maybe that if the size is large enough to reach memory mapped by some other mapping, it could cause it to be unmapped. The doc doesn't explicitly specify the behavior.
Also, the memory reserved in this case would leak.
So the fix looks good.

Copy link
Contributor Author

@tpa95 tpa95 Feb 16, 2026

Choose a reason for hiding this comment

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

Hi @janvorli . Would you prefer moving pRetVal = (void*)StartBoundary; before mprotect, or changing the error-path cleanup to call munmap((void*)StartBoundary, MemSize) instead? I can update the PR accordingly.

#ifndef TARGET_WASM
// Commit the pages
if (mprotect((void *) StartBoundary, MemSize, nProtect) != 0)
Expand All @@ -733,7 +734,6 @@ VIRTUALCommitMemory(
}
#endif

pRetVal = (void *) StartBoundary;
goto done;

#ifndef TARGET_WASM
Expand Down
Loading