Skip to content

Conversation

@stephentoub
Copy link
Member

Fixes #71773

@ghost ghost assigned stephentoub Jul 8, 2022
@ghost ghost added the area-Microsoft.Win32 label Jul 8, 2022
@ghost
Copy link

ghost commented Jul 8, 2022

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

Issue Details

Fixes #71773

Author: stephentoub
Assignees: stephentoub
Labels:

area-Microsoft.Win32

Milestone: -

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

Just curious, do we make any attempt in tests of some sort to look for unintended finalizations? I guess without some kind of diagnostics into the GC there is no reliable way and we just rely on code inspection, and the unlikely situation of it being exposed by what stress tests we do run.

@danmoseley danmoseley merged commit 1605800 into dotnet:main Jul 11, 2022
@danmoseley
Copy link
Member

Thinking, perhaps it would look something like: an environment variable to disable the finalizer thread (does that exist?); then manually running a set of unit tests (presumably we have unit tests for invalid reg keys e.g.), forcing GC, and inspecting a dump to see objects pending finalization, perhaps focusing on certain types. Or simply events/tracing of finalizer activity .

@stephentoub stephentoub deleted the registrysafehandledispose branch July 11, 2022 11:56
@stephentoub
Copy link
Member Author

stephentoub commented Jul 11, 2022

Just curious, do we make any attempt in tests of some sort to look for unintended finalizations?

We've done things in the past like this:

#if DEBUG
private static readonly bool s_captureTrace =
Environment.GetEnvironmentVariable("DEBUG_SAFEX509HANDLE_FINALIZATION") != null;
private readonly StackTrace? _stacktrace =
s_captureTrace ? new StackTrace(fNeedFileInfo: true) : null;
~SafeX509Handle()
{
if (s_captureTrace)
{
Console.WriteLine($"0x{handle.ToInt64():x} {_stacktrace?.ToString() ?? "no stacktrace..."}");
}
}
#endif

I'll play around with what that would look like in the base type. That of course would only handle safe handles, only with a debug/checked runtime, and only if enabled.

@danmoseley
Copy link
Member

Curious how many hits we'd get if we ran our unit tests now with that pasted into SafeRegistryHandle, e.g.. I'm guessing leaky tests would hide such bugs. Would it be worth creating an issue in case a community member is interested in such an experiment? If it was not too painful they could then try with SafeFileHandle E.g.

@stephentoub
Copy link
Member Author

I'm already working on it. It's a lot.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 11, 2022
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.

Passing a path to a key that doesn't exist to OpenSubKey() causes a SafeRegistryHandle to be finalized

2 participants