Skip to content

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Aug 23, 2025

In this PR #118414, this code was changed:

FROM:
pModule = m_modules.GetBase(vmDomainAssembly.IsNull() ? VmPtrToCookie(vmModule) : mPtrToCookie(vmDomainAssembly));
TO:
pModule = m_modules.GetBase(VmPtrToCookie(vmModule));

But vmModule can contain trash value so it will not find a module in m_module and this will make it try to create a new module using this trash address in the CordbModule constructor.

I don't know if this needs to be fixed in other places, but this place for sure is causing issues. Not sure how it was not detected on other platforms, I detected it on android.

@Copilot Copilot AI review requested due to automatic review settings August 23, 2025 05:34
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 23, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where trash values in vmModule could cause incorrect module creation during debugging operations. The issue was introduced in a previous PR that simplified module lookup logic but inadvertently allowed uninitialized vmModule values to be used as valid addresses.

  • Explicitly sets vmModule to NULL to prevent using uninitialized/trash values
  • Prevents creation of spurious modules with invalid addresses in the debugger

Copy link
Member

@tommcdon tommcdon left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@teo-tsirpanis teo-tsirpanis added area-Diagnostics-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 24, 2025
Copy link
Contributor

Tagging subscribers to this area: @steveisok, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

@thaystg
Copy link
Member Author

thaystg commented Aug 25, 2025

/backport to release/10.0

Copy link
Contributor

@thaystg thaystg merged commit 2fd55e6 into dotnet:main Aug 25, 2025
95 of 99 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants