Skip to content

Commit

Permalink
[UnixRegistry] Fix leak (#25559), remove races.
Browse files Browse the repository at this point in the history 
There was a memory leak caused by not having a lookup mechanism for
the RegistryKey.  In addition, the entire code to dispose code was
never used (due to the leak), and it was wrong.

In addition, added locking to the "values" field, which currently was
racy.

No test cases, the repro is to run the above code for about 5 minutes.
  • Loading branch information
@migueldeicaza
migueldeicaza committed Mar 3, 2015
1 parent ef380e3 commit 8260601
Show file tree
Hide file tree
Showing 2 changed files with 134 additions and 114 deletions.
5 changes: 5 additions & 0 deletions mcs/class/corlib/Microsoft.Win32/RegistryKey.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,11 @@ internal RegistryKey (object data, string keyName, bool writable)
isWritable = writable;
}

static internal bool IsEquals (RegistryKey a, RegistryKey b)
{
return a.hive == b.hive && a.handle == b.handle && a.qname == b.qname && a.isRemoteRoot == b.isRemoteRoot && a.isWritable == b.isWritable;
}

#region PublicAPI

/// <summary>
Expand Down
243 changes: 129 additions & 114 deletions mcs/class/corlib/Microsoft.Win32/UnixRegistryApi.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,25 @@ public string Expand ()
}
}

class RegistryKeyComparer : IEqualityComparer {
public new bool Equals(object x, object y)
{
return RegistryKey.IsEquals ((RegistryKey) x, (RegistryKey) y);

}

public int GetHashCode(object obj)
{
var n = ((RegistryKey) obj).Name;
if (n == null)
return 0;
return n.GetHashCode ();
}
}

class KeyHandler
{
static Hashtable key_to_handler = new Hashtable ();
static Hashtable key_to_handler = new Hashtable (new RegistryKeyComparer ());
static Hashtable dir_to_handler = new Hashtable (
new CaseInsensitiveHashCodeProvider (), new CaseInsensitiveComparer ());
const string VolatileDirectoryName = "volatile-keys";
Expand Down Expand Up @@ -430,24 +446,6 @@ static string GetRootFromDir (string dir)
throw new Exception ("Could not get root for dir " + dir);
}

public static void Drop (RegistryKey rkey)
{
lock (typeof (KeyHandler)) {
KeyHandler k = (KeyHandler) key_to_handler [rkey];
if (k == null)
return;
key_to_handler.Remove (rkey);

// remove cached KeyHandler if no other keys reference it
int refCount = 0;
foreach (DictionaryEntry de in key_to_handler)
if (de.Value == k)
refCount++;
if (refCount == 0)
dir_to_handler.Remove (k.Dir);
}
}

public static void Drop (string dir)
{
lock (typeof (KeyHandler)) {
Expand Down Expand Up @@ -487,7 +485,11 @@ public RegistryValueKind GetValueKind (string name)
{
if (name == null)
return RegistryValueKind.Unknown;
object value = values [name];
object value;

lock (values)
value = values [name];

if (value == null)
return RegistryValueKind.Unknown;

Expand All @@ -513,7 +515,9 @@ public object GetValue (string name, RegistryValueOptions options)

if (name == null)
name = string.Empty;
object value = values [name];
object value;
lock (values)
value = values [name];
ExpandString exp = value as ExpandString;
if (exp == null)
return value;
Expand All @@ -530,24 +534,28 @@ public void SetValue (string name, object value)
if (name == null)
name = string.Empty;

// immediately convert non-native registry values to string to avoid
// returning it unmodified in calls to UnixRegistryApi.GetValue
if (value is int || value is string || value is byte[] || value is string[])
values[name] = value;
else
values[name] = value.ToString ();
lock (values){
// immediately convert non-native registry values to string to avoid
// returning it unmodified in calls to UnixRegistryApi.GetValue
if (value is int || value is string || value is byte[] || value is string[])
values[name] = value;
else
values[name] = value.ToString ();
}
SetDirty ();
}

public string [] GetValueNames ()
{
AssertNotMarkedForDeletion ();

ICollection keys = values.Keys;

string [] vals = new string [keys.Count];
keys.CopyTo (vals, 0);
return vals;
lock (values){
ICollection keys = values.Keys;

string [] vals = new string [keys.Count];
keys.CopyTo (vals, 0);
return vals;
}
}

public int GetSubKeyCount ()
Expand Down Expand Up @@ -600,52 +608,54 @@ public void SetValue (string name, object value, RegistryValueKind valueKind)
if (name == null)
name = string.Empty;

switch (valueKind){
case RegistryValueKind.String:
if (value is string){
values [name] = value;
return;
}
break;
case RegistryValueKind.ExpandString:
if (value is string){
values [name] = new ExpandString ((string)value);
return;
}
break;

case RegistryValueKind.Binary:
if (value is byte []){
values [name] = value;
return;
}
break;

case RegistryValueKind.DWord:
try {
values [name] = Convert.ToInt32 (value);
return;
} catch (OverflowException) {
lock (values){
switch (valueKind){
case RegistryValueKind.String:
if (value is string){
values [name] = value;
return;
}
break;
}

case RegistryValueKind.MultiString:
if (value is string []){
values [name] = value;
return;
}
break;

case RegistryValueKind.QWord:
try {
values [name] = Convert.ToInt64 (value);
return;
} catch (OverflowException) {
case RegistryValueKind.ExpandString:
if (value is string){
values [name] = new ExpandString ((string)value);
return;
}
break;

case RegistryValueKind.Binary:
if (value is byte []){
values [name] = value;
return;
}
break;

case RegistryValueKind.DWord:
try {
values [name] = Convert.ToInt32 (value);
return;
} catch (OverflowException) {
break;
}

case RegistryValueKind.MultiString:
if (value is string []){
values [name] = value;
return;
}
break;

case RegistryValueKind.QWord:
try {
values [name] = Convert.ToInt64 (value);
return;
} catch (OverflowException) {
break;
}

default:
throw new ArgumentException ("unknown value", "valueKind");
}

default:
throw new ArgumentException ("unknown value", "valueKind");
}
throw new ArgumentException ("Value could not be converted to specified type", "valueKind");
}
Expand Down Expand Up @@ -680,12 +690,14 @@ public bool ValueExists (string name)
if (name == null)
name = string.Empty;

return values.Contains (name);
lock (values)
return values.Contains (name);
}

public int ValueCount {
get {
return values.Keys.Count;
lock (values)
return values.Keys.Count;
}
}

Expand All @@ -699,7 +711,8 @@ public void RemoveValue (string name)
{
AssertNotMarkedForDeletion ();

values.Remove (name);
lock (values)
values.Remove (name);
SetDirty ();
}

Expand All @@ -713,45 +726,47 @@ void Save ()
if (IsMarkedForDeletion)
return;

if (!File.Exists (file) && values.Count == 0)
return;

SecurityElement se = new SecurityElement ("values");

// With SecurityElement.Text = value, and SecurityElement.AddAttribute(key, value)
// the values must be escaped prior to being assigned.
foreach (DictionaryEntry de in values){
object val = de.Value;
SecurityElement value = new SecurityElement ("value");
value.AddAttribute ("name", SecurityElement.Escape ((string) de.Key));

if (val is string){
value.AddAttribute ("type", "string");
value.Text = SecurityElement.Escape ((string) val);
} else if (val is int){
value.AddAttribute ("type", "int");
value.Text = val.ToString ();
} else if (val is long) {
value.AddAttribute ("type", "qword");
value.Text = val.ToString ();
} else if (val is byte []){
value.AddAttribute ("type", "bytearray");
value.Text = Convert.ToBase64String ((byte[]) val);
} else if (val is ExpandString){
value.AddAttribute ("type", "expand");
value.Text = SecurityElement.Escape (val.ToString ());
} else if (val is string []){
value.AddAttribute ("type", "string-array");

foreach (string ss in (string[]) val){
SecurityElement str = new SecurityElement ("string");
str.Text = SecurityElement.Escape (ss);
value.AddChild (str);
lock (values){
if (!File.Exists (file) && values.Count == 0)
return;

// With SecurityElement.Text = value, and SecurityElement.AddAttribute(key, value)
// the values must be escaped prior to being assigned.
foreach (DictionaryEntry de in values){
object val = de.Value;
SecurityElement value = new SecurityElement ("value");
value.AddAttribute ("name", SecurityElement.Escape ((string) de.Key));

if (val is string){
value.AddAttribute ("type", "string");
value.Text = SecurityElement.Escape ((string) val);
} else if (val is int){
value.AddAttribute ("type", "int");
value.Text = val.ToString ();
} else if (val is long) {
value.AddAttribute ("type", "qword");
value.Text = val.ToString ();
} else if (val is byte []){
value.AddAttribute ("type", "bytearray");
value.Text = Convert.ToBase64String ((byte[]) val);
} else if (val is ExpandString){
value.AddAttribute ("type", "expand");
value.Text = SecurityElement.Escape (val.ToString ());
} else if (val is string []){
value.AddAttribute ("type", "string-array");

foreach (string ss in (string[]) val){
SecurityElement str = new SecurityElement ("string");
str.Text = SecurityElement.Escape (ss);
value.AddChild (str);
}
}
se.AddChild (value);
}
se.AddChild (value);
}

using (FileStream fs = File.Create (file)){
StreamWriter sw = new StreamWriter (fs);

Expand Down Expand Up @@ -863,7 +878,6 @@ public void Flush (RegistryKey rkey)

public void Close (RegistryKey rkey)
{
KeyHandler.Drop (rkey);
}

public object GetValue (RegistryKey rkey, string name, object default_value, RegistryValueOptions options)
Expand Down Expand Up @@ -969,8 +983,9 @@ private RegistryKey CreateSubKey (RegistryKey rkey, string keyname, bool writabl
private RegistryKey CreateSubKey (RegistryKey rkey, string keyname, bool writable, bool is_volatile)
{
KeyHandler self = KeyHandler.Lookup (rkey, true);
if (self == null)
if (self == null){
throw RegistryKey.CreateMarkedForDeletionException ();
}
if (KeyHandler.VolatileKeyExists (self.Dir) && !is_volatile)
throw new IOException ("Cannot create a non volatile subkey under a volatile key.");

Expand Down

0 comments on commit 8260601

Please sign in to comment.