Add null check in LCGMethodResolver::GetManagedResolver

[leculver]

SOS's dumplog hits a crash when calling GetManagedResolver, because it can see a null m_managedResolver handle in diagnostic code that we wouldn't normally run into in the runtime. This avoids the crash while writing out the stresslog.

I took a look at the callsites of the function, some of them do not check that the return value isn't null...but this change is safe simply because we would have been crashing on return ObjectFromHandle(m_managedResolver); anyway if this were a problem. I don't think we need an assert at those callsites, but happy to add them if requested.

@jkotas fixes the issue you filed here: dotnet/diagnostics#1837

[Copilot]

Pull request overview

Prevents SOS dumplog from crashing when it encounters an LCGMethodResolver instance with a null m_managedResolver handle during diagnostic/stresslog processing, by making GetManagedResolver() safely return NULL in that case.

Changes:

• Add a null-handle guard in LCGMethodResolver::GetManagedResolver() before calling ObjectFromHandle(m_managedResolver).

[jkotas]

fixes the issue you filed here

Is this fixing the issue or just making it less likely for a crash like that to happen? I think it is the later.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[leculver]

My understanding is:

1. These objects are allocated on the LoaderHeap, so we generally won't free this chunk of memory.
2. Freeing one of these adds it to a free list and zeros it. In that case, we'll see a null value here.
3. It's possible it could be reused, at which point it will either be null (as it's being initialized) or some different method, which is misleading but not crashing. (Reused the object I believe, I don't think this goes into a general memory buffer pool so we'd read a different field/different type. We'd still be operating on the same m_managedResolver field but maybe on a different iteration.)
4. DacValidateMD is not foolproof, but in addition to the above, the md pointer we pass in needs to round trip from md -> mt -> md slot -> md, and all of that has to be valid memory (iirc).

So, we either fail DacValidateMD, or we make it to where this change is made and either get a nullptr or the wrong methoddesc from something eating that slot.

As best I can tell, that's preventing the crash, though it wouldn't prevent us from reporting the wrong lcg if the slot is reused. (Or if there's a window in here we might crash it's being greatly reduced to nearly never.)

Assuming I got all of the above right, I can't think of anything that would be a more complete fix without a much bigger lift here. (And that's a big assumption. :)) This all relies on a lot of implementation detail that could change at any time, but that's true of a lot of what SOS/Dac does...

I'm happy to take this in a different direction if you have a different idea of what a fix might look like?

Thanks for the feedback on this and other issues! I'm trying to ramp back up on doing actual runtime work after a long break, and I'm definitely a bit rusty.

[dotnet-policy-service]

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

[jkotas]

These objects are allocated on the LoaderHeap, so we generally won't free this chunk of memory.

Nit: We would free this memory if the dynamic method was attached to collectible module.

DacValidateMD is not foolproof

Right. Also, DAC APIs are wrapped in SEH handlers on Windows that allows us to swallow crashes like this one. SEH is not a thing on non-Windows, we depend on a combination of luck and workarounds like the one added in this PR to avoid crashes.

Hopefully, we will be able to get rid of these problems by switching to cDAC in not-too-distant future.