Skip to content

Commit

Permalink
Use SafeHandle to avoid freeing native resources while in use
Browse files Browse the repository at this point in the history
Thread 1 is reading from the stream
Thread 2 closes the zstream
caos ensues
  • Loading branch information
martinpotter committed Jun 23, 2017
1 parent c09eaaa commit b8b5af9
Showing 1 changed file with 56 additions and 22 deletions.
78 changes: 56 additions & 22 deletions mcs/class/System/System.IO.Compression/DeflateStream.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
using System.IO;
using System.Runtime.InteropServices;
using System.Runtime.Remoting.Messaging;
using Microsoft.Win32.SafeHandles;

#if MONOTOUCH
using MonoTouch;
Expand Down Expand Up @@ -316,8 +317,7 @@ class DeflateStreamNative
UnmanagedReadOrWrite feeder; // This will be passed to unmanaged code and used there

Stream base_stream;
IntPtr z_stream;
GCHandle data;
SafeNativeZStreamHandle z_stream;
bool disposed;
byte [] io_buffer;

Expand All @@ -328,10 +328,9 @@ private DeflateStreamNative ()
public static DeflateStreamNative Create (Stream compressedStream, CompressionMode mode, bool gzip)
{
var dsn = new DeflateStreamNative ();
dsn.data = GCHandle.Alloc (dsn);
dsn.feeder = mode == CompressionMode.Compress ? new UnmanagedReadOrWrite (UnmanagedWrite) : new UnmanagedReadOrWrite (UnmanagedRead);
dsn.z_stream = CreateZStream (mode, gzip, dsn.feeder, GCHandle.ToIntPtr (dsn.data));
if (dsn.z_stream == IntPtr.Zero) {
dsn.z_stream = SafeNativeZStreamHandle.Create (mode, gzip, dsn, dsn.feeder);
if (dsn.z_stream == null) {
dsn.Dispose (true);
return null;
}
Expand All @@ -353,14 +352,10 @@ public void Dispose (bool disposing)

io_buffer = null;

IntPtr zz = z_stream;
z_stream = IntPtr.Zero;
if (zz != IntPtr.Zero)
CloseZStream (zz); // This will Flush() the remaining output if any
}

if (data.IsAllocated) {
data.Free ();
SafeNativeZStreamHandle zz = z_stream;
z_stream = null;
if (zz != null)
zz.Dispose();
}
}

Expand Down Expand Up @@ -476,26 +471,65 @@ static void CheckResult (int result, string where)
throw new IOException (error + " " + where);
}

class SafeNativeZStreamHandle : SafeHandleZeroOrMinusOneIsInvalid
{
public static SafeNativeZStreamHandle Create (CompressionMode mode, bool gzip, DeflateStreamNative deflateStreamNative, UnmanagedReadOrWrite feeder)
{
GCHandle deflateStreamNativeHandle = GCHandle.Alloc (deflateStreamNative);
SafeNativeZStreamHandle nativeSafeHandle = CreateZStream (mode, gzip, feeder, GCHandle.ToIntPtr (deflateStreamNativeHandle));
if (nativeSafeHandle == null || nativeSafeHandle.IsInvalid) {
if (deflateStreamNativeHandle.IsAllocated)

This comment has been minimized.

Copy link
@bgrainger

bgrainger Jun 23, 2017

Member

According to the documentation, this is only applicable to Weak handles, so seems unnecessary here (it was a Normal allocation on line 478). (The original code was in error, too.)

This comment has been minimized.

Copy link
@bgrainger

bgrainger Jun 23, 2017

Member

I guess it doesn't hurt anything to check it, but I'm pretty sure it will always return true.

This comment has been minimized.

Copy link
@martinpotter

martinpotter Jun 23, 2017

Author Member

Perhaps this was in the original code to ensure that Free was never called more than once or if the runtime fails to actually create a GCHandle? The current implementation set handle to 0, which will result in IsAllocated returning false.

deflateStreamNativeHandle.Free();

return null;
}

nativeSafeHandle.deflateStreamNativeHandle = deflateStreamNativeHandle;

return nativeSafeHandle;
}

private SafeNativeZStreamHandle () :
base (true)
{
}

protected override bool ReleaseHandle()
{
if (handle != IntPtr.Zero) {
CloseZStream (handle); // This will Flush() the remaining output if any
}

if (deflateStreamNativeHandle.IsAllocated) {

This comment has been minimized.

Copy link
@bgrainger

bgrainger Jun 23, 2017

Member

But it is good to check IsAllocated here so that Free doesn't throw.

deflateStreamNativeHandle.Free ();
}

return true;
}

[DllImport (LIBNAME, CallingConvention=CallingConvention.Cdecl)]
static extern SafeNativeZStreamHandle CreateZStream (CompressionMode compress, bool gzip, UnmanagedReadOrWrite feeder, IntPtr data);

[DllImport (LIBNAME, CallingConvention=CallingConvention.Cdecl)]
static extern int CloseZStream (IntPtr stream);

GCHandle deflateStreamNativeHandle;
}

#if MONOTOUCH || MONODROID
const string LIBNAME = "__Internal";
#else
const string LIBNAME = "MonoPosixHelper";
#endif

[DllImport (LIBNAME, CallingConvention=CallingConvention.Cdecl)]
static extern IntPtr CreateZStream (CompressionMode compress, bool gzip, UnmanagedReadOrWrite feeder, IntPtr data);

[DllImport (LIBNAME, CallingConvention=CallingConvention.Cdecl)]
static extern int CloseZStream (IntPtr stream);

[DllImport (LIBNAME, CallingConvention=CallingConvention.Cdecl)]
static extern int Flush (IntPtr stream);
static extern int Flush (SafeNativeZStreamHandle stream);

[DllImport (LIBNAME, CallingConvention=CallingConvention.Cdecl)]
static extern int ReadZStream (IntPtr stream, IntPtr buffer, int length);
static extern int ReadZStream (SafeNativeZStreamHandle stream, IntPtr buffer, int length);

[DllImport (LIBNAME, CallingConvention=CallingConvention.Cdecl)]
static extern int WriteZStream (IntPtr stream, IntPtr buffer, int length);
static extern int WriteZStream (SafeNativeZStreamHandle stream, IntPtr buffer, int length);
}
}

1 comment on commit b8b5af9

@bgrainger
Copy link
Member

Choose a reason for hiding this comment

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

LGTM.

Please sign in to comment.