Should RegistryXmlRepository close the regkey between reads?

[debracey]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

RegistryXmlRepository takes a RegistryKey as a parameter in its constructor (code ref), but that key is never disposed. Furthermore, if you construct this class inside a using block (see example), the class could attempt to access a closed registry key.

Expected Behavior

• Class should be disposable and dispose/close the key correctly.
• Possibly the class should reopen the key? Not sure.

Steps To Reproduce

1. Setup a standard ASP.NET core web app
2. Enable data protection for the app, persisting the data to the registry
3. Ensure that the registry key is disposed in the setup method
4. Notice that, when the protection information is accessed, the code will fail with an exception

using var registryKey = RegistryKey
      .OpenBaseKey(RegistryHive.LocalMachine, RegistryView.Registry64)
      .OpenSubKey(some_registry_path, true);

dataProtectionBuilder.ProtectKeysWithDpapi(true).PersistKeysToRegistry(registryKey);                

Exceptions (if any)

"Type":"System.ObjectDisposedException", 
"Message":"Cannot access a closed registry key."
"Object name: 'key path set in code'."

.NET Version

8.0.400

Anything else?

• Is the key even supposed to be closed/disposed?
• If not, maybe the documentation for these extension methods/classes just needs updating

[amcasey]

I think the usual pattern is that the creator of the object disposes of it - normally, it's not a class or method that accepts it as an argument. I'm certainly open to examples or guidance showing evidence of a different approach.

In the meantime, it seems like we mostly want to update the doc comment on PersistKeysToRegistry to let people know they shouldn't close the key?

I could also imagine having the registry close and re-open the regkey on demand, but I haven't thought through all the consequences of that.

[debracey]

@amcasey Agree that normally the creator would dispose of the object. In this case it becomes a bit annoying for the consumer since they'd have to maintain a reference to the (opened) registry key for (potentially) the whole life of the application.

[amcasey]

@debracey Can you elaborate? At that point, wouldn't you just declare it without a using? The finalizer should close the regkey.

[amcasey]

Here's where DataProtection creates its own instance (as a fallback). It looks like it just doesn't use a using. Are you aware of a concrete problem that holding that key open causes?

[debracey]

@amcasey Yeah - what I meant to say was that, if you want to manually close/dispose the key, you'd have to maintain a reference to that key for the whole life of the application. Since the finalizer will take care of it, there's no practical reason to do this.

Yes - the workaround I plan to use is to just remove the using statement from the example code shared previously.

I am not aware of any problem with leaving the key open. We only found this issue after we were testing some stylecop/sonarqube suggestions and found that the suggestion(s) failed our pipelines with the exception mentioned above.