Bug 25559

Summary: Memory leak in Microsoft.Win32.Registry.toKey()
Product: [Mono] Class Libraries Reporter: vikingcoder
Component: mscorlibAssignee: Bugzilla <bugzilla>
Status: RESOLVED FIXED    
Severity: normal CC: glunardi, miguel, mono-bugs+mono, vikingcoder
Priority: ---    
Version: master   
Target Milestone: Untriaged   
Hardware: All   
OS: All   
Tags: Is this bug a regression?: ---
Last known good build:
Attachments: Demonstrate memory leak sample program

Description vikingcoder 2014-12-21 12:57:34 UTC
Created attachment 9159 [details]
Demonstrate memory leak sample program

Microsoft.Win32.Registry.cs:102

   key = nkey; // key should be Disposed() prior to assignment of new value.

Also on Line 99

   return null; // key should be disposed prior to return

Sample to show leak and proposed fix attached
Comment 1 Miguel de Icaza [MSFT] 2014-12-22 10:08:47 UTC
I am not a fan of hardcoding the root key.

It seems like the Dispose() could be hitting just about any key, even those that are not owned by this piece of code.

I'll review this when I get back from vacation.
Comment 2 vikingcoder 2014-12-22 13:53:10 UTC
Thanks Miguel.

The hardcoding concerns are understood. rootKey is a local variable and the intention is to avoid Disposing of LocalMachine etc.

Additionally, I think that the ToKey() references in this file should be wrapped in a using() {}

i.e.

GetValue should be

public static object GetValue (string keyName, string valueName, object defaultValue)
		{
			using (RegistryKey key = ToKey (keyName, false)) {
				if (key == null)
					return defaultValue;
		
				return key.GetValue (valueName, defaultValue);
			}
		}
Comment 3 Miguel de Icaza [MSFT] 2015-03-03 16:20:20 UTC
Fixed on master