[SafeHandle] Avoid handle leakage in case of ThreadAbortException

If we get a ThreadAbortException while at SafeHandle.cs:124 (before this commit), we would not set release to true, even after incrementing the internal reference count of the SafeHandle. That could lead to potential leak, as the caller (following the common following pattern) would never call DangerousRelase, thus never calling PerformRelease. The finalizer or Dispose would not lead to the call of PerformRelease either, as the DangerousAddRef / DangerousRelase calls are unbalanced.

The common pattern for DangerousAddRef / DangerousRelease is the following:

bool release = false;
try {
  safeHandle.DangerousAddRef (ref release);
  /* do something with safeHandle.DangerousGetHandle () */
} finally {
  if (release)
    safeHandle.DangerousRelease ();
}

diff --git a/mcs/class/corlib/System.Runtime.InteropServices/SafeHandle.cs b/mcs/class/corlib/System.Runtime.InteropServices/SafeHandle.cs
index 9beb48183e2af025c8e408a7509ce20348c336b8..305c087a70237b42be089727c7f74099d3cdc16c 100644 (file)
--- a/mcs/class/corlib/System.Runtime.InteropServices/SafeHandle.cs
+++ b/mcs/class/corlib/System.Runtime.InteropServices/SafeHandle.cs
@@ -80,14 +80,17 @@ namespace System.Runtime.InteropServices
                [ReliabilityContract (Consistency.WillNotCorruptState, Cer.Success)]
                public void SetHandleAsInvalid ()
                {
-                       int old_state, new_state;
+                       try {}
+                       finally {
+                               int old_state, new_state;
 
-                       do {
-                               old_state = _state;
-                               new_state = old_state | (int) State.Closed;
-                       } while (Interlocked.CompareExchange (ref _state, new_state, old_state) != old_state);
+                               do {
+                                       old_state = _state;
+                                       new_state = old_state | (int) State.Closed;
+                               } while (Interlocked.CompareExchange (ref _state, new_state, old_state) != old_state);
 
-                       GC.SuppressFinalize (this);
+                               GC.SuppressFinalize (this);
+                       }
                }
 
                /*
@@ -108,21 +111,24 @@ namespace System.Runtime.InteropServices
                [ReliabilityContract (Consistency.WillNotCorruptState, Cer.MayFail)]
                public void DangerousAddRef (ref bool success)
                {
-                       if (!_fullyInitialized)
-                               throw new InvalidOperationException ();
+                       try {}
+                       finally {
+                               if (!_fullyInitialized)
+                                       throw new InvalidOperationException ();
 
-                       int old_state, new_state;
+                               int old_state, new_state;
 
-                       do {
-                               old_state = _state;
+                               do {
+                                       old_state = _state;
 
-                               if ((old_state & (int) State.Closed) != 0)
-                                       throw new ObjectDisposedException ("handle");
+                                       if ((old_state & (int) State.Closed) != 0)
+                                               throw new ObjectDisposedException ("handle");
 
-                               new_state = old_state + RefCount_One;
-                       } while (Interlocked.CompareExchange (ref _state, new_state, old_state) != old_state);
+                                       new_state = old_state + RefCount_One;
+                               } while (Interlocked.CompareExchange (ref _state, new_state, old_state) != old_state);
 
-                       success = true;
+                               success = true;
+                       }
                }
 
                /*
@@ -159,60 +165,67 @@ namespace System.Runtime.InteropServices
 
                void DangerousReleaseInternal (bool dispose)
                {
-                       if (!_fullyInitialized)
-                               throw new InvalidOperationException ();
-
-                       int old_state, new_state;
-
-                       /* See AddRef above for the design of the synchronization here. Basically we
-                        * will try to decrement the current ref count and, if that would take us to
-                        * zero refs, set the closed state on the handle as well. */
-                       bool perform_release = false;
-
-                       do {
-                               old_state = _state;
-
-                               /* If this is a Dispose operation we have additional requirements (to
-                                * ensure that Dispose happens at most once as the comments in AddRef
-                                * detail). We must check that the dispose bit is not set in the old
-                                * state and, in the case of successful state update, leave the disposed
-                                * bit set. Silently do nothing if Dispose has already been called
-                                * (because we advertise that as a semantic of Dispose). */
-                               if (dispose && (old_state & (int) State.Disposed) != 0)
-                                       return;
-
-                               /* We should never see a ref count of zero (that would imply we have
-                                * unbalanced AddRef and Releases). (We might see a closed state before
-                                * hitting zero though -- that can happen if SetHandleAsInvalid is
-                                * used). */
-                               if ((old_state & RefCount_Mask) == 0)
-                                       throw new ObjectDisposedException ("handle");
-
-                               if ((old_state & RefCount_Mask) != RefCount_One)
-                                       perform_release = false;
-                               else if ((old_state & (int) State.Closed) != 0)
-                                       perform_release = false;
-                               else if (!_ownsHandle)
-                                       perform_release = false;
-                               else if (IsInvalid)
-                                       perform_release = false;
-                               else
-                                       perform_release = true;
-
-                               /* Attempt the update to the new state, fail and retry if the initial
-                                * state has been modified in the meantime. Decrement the ref count by
-                                * substracting SH_RefCountOne from the state then OR in the bits for
-                                * Dispose (if that's the reason for the Release) and closed (if the
-                                * initial ref count was 1). */
-                               new_state = (old_state & RefCount_Mask) - RefCount_One;
-                               if ((old_state & RefCount_Mask) == RefCount_One)
-                                       new_state |= (int) State.Closed;
-                               if (dispose)
-                                       new_state |= (int) State.Disposed;
-                       } while (Interlocked.CompareExchange (ref _state, new_state, old_state) != old_state);
-
-                       if (perform_release)
-                               ReleaseHandle ();
+                       try {}
+                       finally {
+                               if (!_fullyInitialized)
+                                       throw new InvalidOperationException ();
+
+                               int old_state, new_state;
+
+                               /* See AddRef above for the design of the synchronization here. Basically we
+                                * will try to decrement the current ref count and, if that would take us to
+                                * zero refs, set the closed state on the handle as well. */
+                               bool perform_release = false;
+
+                               do {
+                                       old_state = _state;
+
+                                       /* If this is a Dispose operation we have additional requirements (to
+                                        * ensure that Dispose happens at most once as the comments in AddRef
+                                        * detail). We must check that the dispose bit is not set in the old
+                                        * state and, in the case of successful state update, leave the disposed
+                                        * bit set. Silently do nothing if Dispose has already been called
+                                        * (because we advertise that as a semantic of Dispose). */
+                                       if (dispose && (old_state & (int) State.Disposed) != 0) {
+                                               /* we cannot use `return` in a finally block, so we have to ensure
+                                                * that we are not releasing the handle */
+                                               perform_release = false;
+                                               break;
+                                       }
+
+                                       /* We should never see a ref count of zero (that would imply we have
+                                        * unbalanced AddRef and Releases). (We might see a closed state before
+                                        * hitting zero though -- that can happen if SetHandleAsInvalid is
+                                        * used). */
+                                       if ((old_state & RefCount_Mask) == 0)
+                                               throw new ObjectDisposedException ("handle");
+
+                                       if ((old_state & RefCount_Mask) != RefCount_One)
+                                               perform_release = false;
+                                       else if ((old_state & (int) State.Closed) != 0)
+                                               perform_release = false;
+                                       else if (!_ownsHandle)
+                                               perform_release = false;
+                                       else if (IsInvalid)
+                                               perform_release = false;
+                                       else
+                                               perform_release = true;
+
+                                       /* Attempt the update to the new state, fail and retry if the initial
+                                        * state has been modified in the meantime. Decrement the ref count by
+                                        * substracting SH_RefCountOne from the state then OR in the bits for
+                                        * Dispose (if that's the reason for the Release) and closed (if the
+                                        * initial ref count was 1). */
+                                       new_state = (old_state & RefCount_Mask) - RefCount_One;
+                                       if ((old_state & RefCount_Mask) == RefCount_One)
+                                               new_state |= (int) State.Closed;
+                                       if (dispose)
+                                               new_state |= (int) State.Disposed;
+                               } while (Interlocked.CompareExchange (ref _state, new_state, old_state) != old_state);
+
+                               if (perform_release)
+                                       ReleaseHandle ();
+                       }
                }
 
                /*