Dispose must be thread-safe ? -- dotnet clr

Lasse Vågsæther Karlsen
2/26/2004 4:30:16 AM

Continuing on my first post, I read this article today (mind the line
breaks):
http://www.theserverside.net/articles/article.aspx?l=GarbageCollection

and in this article the author says that the Dispose method must be
thread-safe.

I'm going to assume that this is correct, and as such just want to know
why this is so.

Let me explain what I thought I knew about the way Dispose is called. In
the article the author shows that he calls Dispose from the class
destructor (Finalize). This is ok (probably) in his case as he cleans up
an unmanaged java object.

The only reason I can think of why a Dispose method should be thread-safe
is that it's called from two threads simultaneously. In face, his example
clearly shows code that makes sure that the actual Dispose code is only
executed once in a thread-safe manner:

public void Dispose()
{
// thread-safe way of clearing unmanaged instance
int temp = System.Threading.Interlocked.Exchange(inst,0);

if( temp != 0 )
{
GC.SuppressFinalize( this );
cleanup( temp );
}
}

The question I'm left with is why Dispose would ever be called on more
than one thread, assuming he's not calling it on two threads himself,
which judging by his article, he's not.

The reason I can think of would be that his code is trying to call
Dispose at the same time the GC is running, finalizing his object, which
in turn calls Dispose (in his implementation).

That must be wrong however, since there is no way the GC would attempt to
finalize the object if he still has a live reference to it. And the only
way he could call Dispose would be that he has a life reference still
hanging around.

In fact, this code:

h.Dispose()

does not remove the reference. It simply calls Dispose. The reference to
the object is still left intact and the object will still be alive and
kicking until h goes out of scope or is explicitly changed away from that
reference.

There is one thing I won't touch on this question and that's that the GC
is running on a different thread. If, for some reason, the unmanaged
object I'm using cannot be operated on outside of the thread that
originally created it, then GC calling my Finalize method on a different
thread will screw that up. Let that be for this question.

An additional thing is that most Dispose implementations I've seen, and
indeed all of my own, always make sure that Dispose can safely be called
more than once (on one thread) and only disposes of the data once. The
second time it's called it won't actually do anything. Basically I'm
using Jeffrey Richters guidelines on how to implement Dispose from his
book "Applied Microsoft .NET Framework Programming".

So back to my question. Provided I make sure I'm not attempting to
dispose of an object on multiple threads, is there any reason a Dispose
method would have to be thread-safe ? If yes, why ? In what context would
a non-thread-safe Dispose method be a problem ?

--
Lasse Vågsæther Karlsen
lasse@vkarlsen.no

Lasse Vågsæther Karlsen
2/26/2004 6:07:29 AM

Jon Skeet [C# MVP] <skeet@pobox.com> wrote in
news:MPG.1aa80ba757f8073a98a26e@msnews.microsoft.com:

[quoted text, click to view]

Why do I need to make it thread-safe if I also have a finalizer ?
The only reason I can see for making the Dispose method thread-safe is if
for some reason it would be called from two different threads.

If the GC is calling my finalizer on thread B, which in turn calls
Dispose, it would mean that I have no live references to the object,
which means I cannot call Dispose on that object from anywhere else.

If on the other hand, I do have a live reference to it, so that I can
call Dispose, there should be no way for the GC to collect it, since it
should still be counted as alive.

Or.... am I still confused ? Please explain more.

--
Lasse Vågsæther Karlsen
lasse@vkarlsen.no

Lasse Vågsæther Karlsen
2/26/2004 7:12:23 AM

Jon Skeet [C# MVP] <skeet@pobox.com> wrote in
news:MPG.1aa817e5e4c26bc398a271@msnews.microsoft.com:

[quoted text, click to view]

Yes I did read it, but frankly I didn't understand it. I re-read it now and
to me this article, if it's true, means that the .NET GC is beyond
aggressive.

I did a test now and you're right, the GC will indeed collect the object if
it thinks it's no longer needed, even if the code is in the middle of a
method call in that class.

The question now remains, why isn't this documented in a bright and shiny
place for everyone to see ? Or if it is, where is that place ? I must've
missed it.

--
Lasse Vågsæther Karlsen
lasse@vkarlsen.no

Lasse Vågsæther Karlsen
2/26/2004 12:07:50 PM

"Valery Pryamikov" <Valery@nospam.harper.no> wrote in
news:#j8v7qJ$DHA.2524@tk2msftngp13.phx.gbl:

[quoted text, click to view]
<snip>

How about this example:

using System;
using System.Windows.Forms;

namespace GCTest123
{
public class TestClass : IDisposable
{
public TestClass()
{
MessageBox.Show("TestClass created");
}

~TestClass()
{
MessageBox.Show("TestClass destroyed");
Dispose();
}

public void Dispose()
{
GC.SuppressFinalize(this);
MessageBox.Show("TestClass disposed");
}

public void SomeMethod()
{
MessageBox.Show("TestClass.SomeMethod() before collect");
// point 1
GC.Collect();
// point 2
MessageBox.Show("TestClass.SomeMethod() after collect");
}
}
}

to use it:
TestClass tc = new TestClass();
tc.SomeMethod();

Note that the only way this could ever be a problem is if:
1. you work with unmanaged objects
2. at point 1, you grab a copy of the handle to whatever you're dealing
with
3. at point 2, you're using this handle to call a winapi function

note that GC.Collect in this case don't have to be explicit, and that
point 1 and 2 could very well be hidden in IL code, ie. this:

SomeApiFunc(_PrivateHandleMember);

would result in IL code that first reads the handle, then calls the
function. Would it be possible for the GC to interrupt in between these
and finalize the object before the api function is called ?

If it is, and judging by my tests and other articles on the web, it is,
would the same apply to .Dispose ?

ie. this:

tc.Dispose();
could possibly be interrupt inside by GC finalizing the object
prematurely ?

--
Lasse Vågsæther Karlsen
http://www.vkarlsen.no/

Jon Skeet [C# MVP]
2/26/2004 1:57:29 PM

[quoted text, click to view]

It's not always correct. It *is* correct if you also have a finalizer.=20
The article states that you should always have one if you implement=20
IDisposable, but that's really not true. If the only thing you do in=20
your Dispose method is call Dispose on references to other objects, you=20
don't need to worry - if they require finalization, they should have=20
their own finalizers.

You may end up delaying their disposal by not having a finalizer=20
yourself, but that's all - and by that stage you already know that=20
you've got some code which hasn't been well written, as your Dispose=20
method really should have been called explicitly. There's also a=20
performance penalty in having a finalizer when you don't need one=20
(which is why Dispose usually calls GC.SuppressFinalize(this)).
=20
[quoted text, click to view]

Yes.

And the problem is that the garbage collector is more aggressive than=20
you might think. I won't bother trying to explain it properly myself,=20
as Chris Brumme does it much better in his blog:

http://blogs.msdn.com/cbrumme/archive/2003/04/19/51365.aspx

--=20
Jon Skeet - <skeet@pobox.com>
http://www.pobox.com/~skeet

Jon Skeet [C# MVP]
2/26/2004 2:49:48 PM

[quoted text, click to view]

Did you read the article I referenced? It explains how the finalizer=20
can be called while another method is still running "in" the object.

--=20
Jon Skeet - <skeet@pobox.com>
http://www.pobox.com/~skeet

Jon Skeet [C# MVP]
2/26/2004 3:40:10 PM

[quoted text, click to view]

Well, as the article said - if you make it less aggressive, things=20
become nasty quickly, performance-wise. I don't pretend to know the ins=20
and outs, but I'm willing to trust the author on that one.
=20
[quoted text, click to view]

Yup.

[quoted text, click to view]

For the most part, it's not a problem. I certainly haven't written any=20
code which has been affected by that blog entry, although I'm glad I=20
know about it.

I suspect that unless you've got unmanaged resources involved, it's not=20
a problem - although I could be wrong...

--=20
Jon Skeet - <skeet@pobox.com>
http://www.pobox.com/~skeet

Valery Pryamikov
2/26/2004 7:59:41 PM

Hi,

Article is full of mistakes. A lot of observations and conclusions are
false. It is better to ignore the article.

There are only two ways of Dispose being called concurrently:

1.. Two threads are calling Dispose of the same object concurrently;
2.. Dispose is called on the Target of WeakReference with resurrection
tracking (possible concurrent execution of Dispose and Finalize);
Both are logical program error because Dispose must be called by ultimate
owner of object reference.

It is guaranteed that object would not be marked for GC and finalization
when object is reachable. Because stack is valid reference location means
that object can not be marked for GC while executing instance method ('this'
pointer is reachable). GC suspends application threads during its Mark (and
Collect) phase. Unreachable object could be only obtained either from other
unreachable objects (from Finalize) or by Target of WeakReference with
resurrection tracking. Calling instance methods of other objects from
Finalize is an error because order of Finalize isn't guaranteed. Using of
WeakReference contradicts to calling Dispose on its Target.

<rant>

Author didn't even check documentation about implementing Dispose pattern
when he/she wrote this article - note absence of protected Dispose(bool
disposing).

</rant>

-Valery.

See my blog at:

http://www.harper.no/valery

[quoted text, click to view]

Valery Pryamikov
2/26/2004 9:57:22 PM

So, what is your point?
Your sample doesn't provoke Finalize or Dispose on test object in
SomeMethod - object is reachable at this time. GC can suspend thread, but it
doesn't matter.
Dispose doesn't need to be thread safe, period. If object is shared across
threads, technique like ref. count could be used to identify who is
responsible for calling Dispose. In this case ref counting needs to be
thread safe, but Dispose doesn't. Excerpt that you are quoting from my mail
is correct.
See
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/cpguide/html/cpconimplementingdisposemethod.asp
for recommended Dispose pattern.

-Valery.

See my blog at:
http://www.harper.no/valery

[quoted text, click to view]

Valery Pryamikov
2/26/2004 10:14:13 PM

The only way of getting Dispose and Finalize to run concurently is (as I
written it in my first mail) is by calling Dispose on Target of
WeakReference with resurrection tracking. But this is a logical mistake
because it contradicts to the purpose of Dispose. And WeakReferences with
resurrection tracking is a very dangerous thing that you should avoid if you
don't absolutely aware of all possible implications.

Here is demo (compile and run it F5 to see result):

using System;
using System.Threading;
namespace SleepOnFinalize
{
class TestObject
{
public TestObject() {}
public void Ping()
{
Console.WriteLine("Ping-Pong");
}
~TestObject()
{
Console.WriteLine("In Finalize");
System.Threading.Thread.Sleep(new TimeSpan(40000000)); //sleep 4 seconds
Console.WriteLine("Out Finalize");
}
}
class Class1
{
static void Main(string[] args)
{
TestObject obj = new TestObject();
WeakReference wr = new WeakReference(obj, true);
obj = null;
GC.Collect();
System.Threading.Thread.Sleep(new TimeSpan(20000000)); //sleep 2 seconds;
obj = (TestObject)wr.Target;
if (obj != null)
{
obj.Ping(); //any instance method will run in the middle of finalize.
}
GC.WaitForPendingFinalizers();
}
}
}

-Valery

See my blog at:
http://www.harper.no/valery

[quoted text, click to view]

Jon Skeet [C# MVP]
2/27/2004 9:05:31 AM

[quoted text, click to view]

Well yes - but that's surely a truism. However, one of those threads
*can* be the finalizer thread. If you intended that to be read as "Two
user threads are calling Dispose of the same object concurrently" then

[quoted text, click to view]

That's the problem - the "this" reference is only considered as a root
if it's going to be used.

Here's an example which shows (when compiled and run from the command
line - my guess is that running it in a debugger will change the
behaviour) the finalizer and Dispose methods being run concurrently
(with the finalizer then running the Dispose method). Note that the
calls to Thread.Sleep and GC.Collect are merely there to provoke a
situation which could occur naturally. There's no obvious program error
here.

using System;
using System.Threading;

public class Test : IDisposable
{
~Test()
{
Console.WriteLine ("Finalizer called");
Dispose();
}

public void Dispose()
{
Console.WriteLine ("Starting Dispose");
Thread.Sleep(500);
GC.Collect();
Thread.Sleep(500);
Console.WriteLine ("Ending Dispose");
}

static void Main()
{
Test t = new Test();
t.Dispose();
}
}

See http://blogs.msdn.com/cbrumme/archive/2003/04/19/51365.aspx for a
nastier example.

--
Jon Skeet - <skeet@pobox.com>
http://www.pobox.com/~skeet

Jon Skeet [C# MVP]
2/27/2004 10:46:52 AM

[quoted text, click to view]

The other thread effectively doesn't need the reference any more - it's
(provably) not accessing any members any more. Basically, a lot of
people (me included) always assumed that any stack frame in an instance
method would include an implicit "root" (from the garbage collector's
point of view) to "this" - but it's *not* always a root, if it's not
needed.

Effectively, you could think of each method as having a local variable
called "this" which is passed to it. When that variable is no longer
being used, it doesn't stop the garbage collector from trying to
collect the object.

--
Jon Skeet - <skeet@pobox.com>
http://www.pobox.com/~skeet

Ales Pour
2/27/2004 11:30:44 AM

[quoted text, click to view]

I admit that due to my limited knowledge of GC-ing in .NET I was not able to
track the whole discussion, but I dare to wonder - how come that one of
threads can be the finalizer thread, while the object is still in use (since
other thread still references it and even calls Dispose method on it)??

Thanks!

Regards,
Ales

Valery Pryamikov
2/27/2004 1:08:31 PM

Jon, refer to my other mail with description of only situation that can
cause Dispose and Finalize to run concurrently - when Dispose is called on
Target of WeakReference with resurrection tracking.

Your sample doesn't show what you wanted to show in it, but only proves my
point. First call to dispose doesn't case reference to be collected, becase
object is reachable. Second time dispose is called directly from finalize on
finalizer thread...
Dispose doesn't need to be thread safe. period.

-Valery.

See my blog at:
http://www.harper.no/valery

Valery Pryamikov
2/27/2004 1:15:30 PM

Sorry, I run your sample with F5 on release build when I said that it is not
working.
A second after i pushed send on my mail I tried it from command line and it
shown different results.
(another reason to wait few more seconds before pussing send button).
My assertion about your sample not working is wrong. I'll look closely at it
later today, but for now - ignore my previous mail.

-Valery.

See my blog at:
http://www.harper.no/valery

[quoted text, click to view]

Jon Skeet [C# MVP]
2/27/2004 2:10:15 PM

[quoted text, click to view]

Yes.

[quoted text, click to view]

Yup. It would be easy enough to construct an example which couldn't be
inlined but still showed the same problem.

[quoted text, click to view]

Well... that assumes that the last point of access to "this" is also
the last point of access to any resource held by the class.

[quoted text, click to view]

How about this for a *somewhat* realistic scenario:

public class Foo
{
int handle; // A handle to an unmanaged resource

public void Dispose()
{
ReleaseHandle (handle);
}

static void ReleaseHandle (int handle)
{
// At this stage, the object can be finalized
}

... other stuff ...
}

Although you've *started* the call to ReleaseHandle using the "this"
reference, as soon as the value of "handle" has been evaluated, the
object can be finalized. It could be even worse, if you do:

public void Dispose()
{
int myHandle = handle;

// Do some other cleanup which doesn't use "this"

ReleaseHandle (myHandle);
}

Most of the time it won't be a problem, particularly because you'll
call SuppressFinalizer(this), but it's certainly worth being aware of.
I can't *immediately* think of a case where a properly written Dispose
implementation (which follows the guidelines) has a problem, but I'll
have a think. This also touches on the other discussion we've had
recently about memory caching etc. If the JIT knows that nothing else
in the Dispose method is going to change the value of handle, can it
read it immediately and then ignore the "logical" read later that keeps
the "this" pointer alive? I don't even want to speculate on that!

Of course, few of us (thankfully not including me) deal directly with
unmanaged resources in this fashion anyway, but knowing that the GC can
be this aggressive can be very important. (There was another thread on
the C# newsgroup where the use of a mutex was incorrect because
although it was acquired correctly, there was nothing to prevent it
from being garbage collected sooner afterwards, simply because it
wasn't used thereafter. That reminds me - I need to check that my FAQ
has that correction!)

--
Jon Skeet - <skeet@pobox.com>
http://www.pobox.com/~skeet

Valery Pryamikov
2/27/2004 2:37:17 PM

Ok, Jon, I looked closely at your sample now.

If you add any reference to 'this' pointer in Dispose's code just after
Collect (ex. add a field to your Test class and access this field right
after gc.Collect) - it will effectively prevent object from being collected.

BTW: in release build started without debugger JIT compiler inlines call to
t.Dispose. So, technically there is no 'this' pointer in the stack at this
point of time, but that is not relevant to this discussion, so we can simply
ignore it.

When running without debugging environment, JIT compiler collaborates with
GC by providing resources tables with information about last point of access
to the resource, so that GC can reclaim resources early. That also
guarantees that resource will not be collected before last point of access
to this resource will be completed(!) -> synchronization is not required.

So, let consider some realistic scenario:

Let say your class holds some unmanaged resources. These resources are
required to be disposed. Unmanaged resources/handles are hold in fields of
the managed class and must be accessed by 'this' pointer. CLR and JITter
gives you complete guarantee that your class will not be collected until
last access to 'this' pointer will be completed. This in order means that at
any point where 'this' is accessed inside of Dispose is guaranteed against
concurrent access from Finalize -> Dispose doesn't need to be thread safe.

Therefore, my point stands:

The only way of getting Dispose and Finalize to run concurrently the way
that could require mutli-threading protection is by calling Dispose on
Target of WeakReference with resurrection tracking. But this is a logical
mistake because it contradicts to the purpose of Dispose.

-Valery.

See my blog at:

http://www.harper.no/valery

Jon Skeet [C# MVP]
2/27/2004 2:44:46 PM

[quoted text, click to view]

Indeed - our posts crossed in the ether, as it were :)

--
Jon Skeet - <skeet@pobox.com>
http://www.pobox.com/~skeet

Valery Pryamikov
2/27/2004 2:59:13 PM

Just an addition to my prev. letter:

[quoted text, click to view]

Note: it is important to make sure memory read from 'this' pointer will not
be optimized away by JITter.

However following usage pattern:

IntPtr _handle = this._unmanagedHandleToDispose;

UnmanagedHelper.CloseHandle(_handle);

is guaranteed against such optimization.

If this._unmanagedHandleToDispose was the last access to 'this' pointer than
instance is eligible to collection after _handle will be initialized. GC can
mark instance for finalization and Finalize could run. However it doesn't
require multithreading protection (!), but only require reentry protection
that is guaranteed if you implemented Dispose pattern as described in
(already mentioned in one of my prev. letters to this thread):
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/cpguide/html/cpconimplementingdisposemethod.asp

-Valery

See my blog at:

http://www.harper.no/valery

[quoted text, click to view]

Valery Pryamikov
2/27/2004 3:17:29 PM

Jon,
See my prev. mail - it address some of your objections/questions .

-Valery

See my blog at:

http://www.harper.no/valery

[quoted text, click to view]

Valery Pryamikov
2/29/2004 4:39:55 PM

Hi,
I posted an article where I discuss issues related to that thread into my
blog
http://www.harper.no/valery/PermaLink,guid,6a9b1b31-166d-4ac8-a91c-0eab48234359.aspx

-Valery.

See my blog at:

http://www.harper.no/valery

cbrumme NO[at]SPAM microsoft.com
3/3/2004 2:54:54 PM

Last month I wrote an extremely detailed explanation of Finalization.

Among other things, it reveals why Dispose must be thread-safe and
idempotent. This is the case both for explicit IDisposable::Dispose and
for a Finalize method calling Dispose(disposing=false).

You can find the full article at
http://blogs.msdn.com/cbrumme/archive/2004/02/20/77460.aspx.

Valery Pryamikov
3/3/2004 7:23:20 PM

Chris,
your article is really great in-depth discussion of Finalization and I
re-read it again with my great pleasure (I'm subscribed on your blog :-),
however it doesn't reveal why Dispose must be thread safe. Your other
article http://blogs.msdn.com/cbrumme/archive/2003/04/19/51365.aspx may be
more relevant to the subject of whether or not Dispose must be thread safe,
however I'm still stand on my point that If we skip scenario of calling
Dispose on the Target of WeakReference with resurrection tracking, then we
have guarantee that part of code in Dispose starting from entry point to the
last memory read of 'this' pointer will not be executed concurrently by
finalizer thread. Therefore, simple protection from reentry will be enough.
And that is exactly what suggested on MSDN IDisposable pattern.
I'd be really glad to see proof of the opposite. You are a brilliant person,
Chris, but either I'm missing some important point (quite possible) or you
are wrong saying that IDisposable::Dispose must be thread safe.

-Valery.

See my blog at:
http://www.harper.no/valery

[quoted text, click to view]

Valery Pryamikov
3/4/2004 8:11:26 AM

Chris,

(see my prev response to your mail too).

Let say that we are only interesting in interaction between Finalizer thread
and one program thread and we don't consider calling Dispose on the Target
of WeakReference with resurrection tracking.
Now, can you show us, please, the weak point in following Dispose code?

if (!this._disposed)

{

this._disposed = true;

IntPtr _handle = this._unmanagedResourceToDispose;

UnmanagedHelper.CloseHandle(_handle);

}

My point is that GC (and Finalize) could kick off only after program thread
completes read access from this._unmanagedResourceToDispose and even if GC
starts immediately after that the rest of Dispose code in the 'if' block
protected from re-entry and therefore doesn't require thread safety.

[quoted text, click to view]

I strongly believe that you are wrong here.

-Valery.

Valery Pryamikov
3/4/2004 8:21:13 AM

Just to avoid being misinterpreted :
[quoted text, click to view]
Of course I meant that "object could be marked for Finalization only after
program thread completes ...."

GC at earlier point of that simple code can't interfere with Dispose because
object would not be marked for finalization.

-Valery.

[quoted text, click to view]

cbrumme NO[at]SPAM microsoft.com
3/4/2004 2:24:20 PM

There may be some confusion here about resurrection. It's true that
WeakReference and GCHandle give you the ability to create weak handles that
are either "short" or "long" weak handles. By short, I mean that the
handle is zeroed when the target object is first discovered to be garbage.
(As you know from my blog post on Finalization, this is the point at which
objects are moved from the RegisteredForFinalization queue to the
ReadyForFinalization queue, and the entire reachable graph from each such
finalizable object is promoted). By long, I mean that the handle is not
zeroed until after finalization has proceeded and the target of the handle
is actually collected in some subsequent GC. In the APIs, we refer to long
weak handles as 'trackResurrection' because they track the target object
even as it is resurrected by the movement from the
RegisteredForFinalization queue to the ReadyForFinalization queue and the
subsequent promotion.

Unfortunately, that use of the term resurrection is a bit different from
what I refer to in my blog post on Finalization -- although it's certainly
related to the underlying phenomenon of "bringing an object back from the
dead" by promoting it after it was determined to be garbage. The
resurrection case I'm talking about in the blog is more general. Let's say
there are two finalizable objects 'a' and 'b' of type A and B respectively.
You wrote type B and you wrote your Finalize method without any regard for
free-threading. Now I'm going to come along and create a multi-threaded
race condition in your Finalize method.

An easy way for me to do this is to create my 'a' object with a reference
to an instance of your type 'b'. If I create 'a' and 'b' at the same time,
they are extremely likely to be in the same generation. If I release both
objects after creating them, they are very likely to be collected together,
on the next GC. At that time, they will both be moved to the
ReadyForFinalization queue and will be promoted. Now the finalizer thread
starts calling Finalize methods. Let's say there is a 50% chance of 'a's
Finalize being called before 'b's Finalize method.

Inside my Finalize method, all I need to do is place 'a' into a static
field. This means that 'a' and 'b' are both reachable. Neither one is
going to be collected. Your object has just been resurrected. You had no
choice in the matter. However, your object is also on the ReadyForFinalize
queue. This means that the finalizer thread is going to call your Finalize
method as it works its way through that queue. For the example we
discussed, there's some likelihood that 'b's Finalize method will be called
as soon as I return from my 'a's Finalize method.

If I'm vicious, I can do a QueueUserWorkItem of a delegate that will call
the IDisposable::Dispose method of your 'b' instance. (Indeed, I don't
need to put your object into a static field, since the QueueUserWorkItem of
my delegate will keep 'b' alive until the ThreadPool has called Dispose on
it.

At this point, we have the finalizer thread about to call your Finalize
method. And we have another thread from the ThreadPool about to call your
Dispose method. The result is that you will receive one
Dispose(disposing=false) and one Dispose(disposing=true) call in a
free-threaded race on your object. If people can obtain references to your
object, they can subject you to this free-threaded abuse.

Do you need to worry about it? Well, if you are running in partial trust
and maintaining your own cache, the worst that can happen is that you get
confused. If someone subjects you to this sort of free-threaded abuse, you
could say that it's their bug.

But if you are running with full trust and you are managing a protected
resource, then the implementer of A might be a malicious hacker who is
trying to create a handle recycling attack.

Valery Pryamikov
3/4/2004 4:35:23 PM

Chris,

It's great to see such detailed response but somehow I feel that we are
talking about different things... None of the scenarios you given in your
response shows the weak point of the simple code that I presented in my
previous post. If we use simple reentry protection of Dispose that way:

if (!this._disposed) {
this._disposed = true;

IntPtr _handle = this._unmanagedResourceToDispose;
UnmanagedHelper.CloseHandle(_handle);
}

Dispose call could be queued to the thread pool or whatever, but the object
will be collectable only after the last read access of 'this' pointer will
be completed (object is reachable by 'this' pointer). That in order
guarantees that move to ReadyForFinalization queue would not happen until
last read access from this pointer, but at that time this._disposed will be
already set to true and any attempt to reenter that code from Finalizer
thread will be blocked by simple reentry protection if (!this._disposed).

The same concerns other scenario that you discuss in your letter - object
resurrection from Finalize. If Dispose was called from a program thread
(any), than "this._disposed" will be set to true when Finalize runs.
Resurrected or not, this code guaranteed from problems related to the
multi-thread access from finalizer thread.

-Valery.

[quoted text, click to view]

Valery Pryamikov
3/5/2004 9:56:03 AM

To make myself even more clear:

protected void Dispose(bool fDisposing) {
if (!this._disposed) { //#1
this._disposed = true; //#2
IntPtr _handle = this._unmanagedResourceToDispose; //#3
UnmanagedHelper.CloseHandle(_handle); //#4
} }

If Dispose was called from program -> the object was reachable at the start
of Dispose call. Lines #1, #2 and #3 access 'this' pointer and therefore
keep object non-Collectable. Object could become collectable somewhere at
the middle of (#3) (after complete read memory access from
this._unmanagedResourceToDispose), but at that time this._disposed will be
already set to true (#2) and any attempt to reenter that code from Finalizer
thread will be blocked by simple reentry protection if (!this._disposed)
(#1).
You don't need to write long response, just say if it possible or not for
object to be collected before it becomes unreacable...
If there is a guarantee of that object will be collected only after it
becomes unreacable (my point) - then thread-safety is not required and
reentry protection of Dispose is indeed enough and it does not matter if
call to Dispose was queued into thread pool or object was resurrected from
Finalize, but this simple reentry protection guaranteed to work in all cases
(except for calling Dispose on the Target of WeakReference with resurrection
tracking).

BTW: just in addition to you argument about malicious person queueing
dispose call - race conditions generally are not exploitable (AFAIK - there
is no known exploit of the race condition).

Best regards,
-Valery.

[quoted text, click to view]

cbrumme NO[at]SPAM microsoft.com
3/5/2004 3:49:53 PM

The code you show contains an exploitable race condition. Let's look at
the code in detail:

protected void Dispose(bool fDisposing) {
if (!this._disposed) { //#1
this._disposed = true; //#2
IntPtr _handle = this._unmanagedResourceToDispose; //#3
UnmanagedHelper.CloseHandle(_handle); //#4
}
}

There are two pathways to this code (finalization and IDisposable.Dispose).
Let's say your object is unreachable and is collected along with another
finalizable object that refers to it. I explained in my last reply how
even as the call to Finalize happens on the Finalizer thread, my malicious
finalizable object can cause another thread to call IDisposable::Dispose on
your object through the magic of resurrection.

The Finalize thread will execute statement #1 above and stop before
executing statement #2. Then either the OS will preemptively switch to the
IDisposable::Dispose thread, or we are running on a multiprocessor machine
and have truly concurrent execution. Either way, the IDisposable::Dispose
thread will now execute statement #1 above. At this point, both threads
have read that (this._disposed) is false and both threads have fallen
through to statement #2.

The result is that I can cause the handle to sometimes get closed twice.
This allows me to mount a handle recycling attack. First I need to stack
the odds in my favor. I do this by creating a ton of your objects and
calling GC.Collect / GC.WaitForPendingFinalizers. If I can get 1 out of
100 attempts to do a duplicate close, I only need to create 100 of your
objects before I have corrupted the system.

One way to exploit the duplicate close is to run on a server. If the
server is opening and closing files at some rate (e.g. each request is
opening a file) then I can use a 3rd thread of my own to open, read and
close files that I'm allowed to read in a loop. Every so often, the OS
will give my 3rd thread one of these "duplicate close" handles. And every
so often my handle will be matched up to a file that I'm not allowed to
read, which something else in the server is opening. It's a little
complicated, so let me show the steps in detail:

1) Create a finalizable object that opens a resource and receives handle 25.
2) The 1st non-malicious Finalizer thread closes 25.
3) My 3rd malicious thread opens 25 on a file it is allowed to see. I now
have a "safe" encapsulation in the form of a stream.
4) My 2nd malicious thread calls IDisposable.Dispose. This closes handle
25 because of the race in your Dispose code. This is the duplicate close,
and it has closed the handle on the stream.
5) The server application opens a file I'm not allowed to see, and it gets
handle 25.

Bingo. My malicious thread can now read from handle 25 using the stream I
was allowed to create in #3 above. But instead of reading from the file
that it was allowed to open and read, it is now reading (and writing!) to a
file that it was not allowed to see and which the server was opening for
its own use in step #5 above.

You may be tempted to say this is all highly unlikely. However, I built
essentially this exploit about 3 years ago. It worked extremely well and
in less than a minute on a normal server I was able to gain access to
"protected" files. Obviously we fixed the vulnerabilities that allowed
this sort of thing at that time.

You also said "race conditions generally are not exploitable (AFAIK - there
is no known exploit of the race condition)."

I have to disagree with this. I have seen hundreds of exploits based on
race conditions. I see these on a weekly basis. If you are saying that
nobody has built viruses and worms based on these vulnerabilities, that may
be true. I have no data on that.

Valery Pryamikov
3/5/2004 8:41:54 PM

Chris,
thank you for great response, I stand corrected.

regards,
-Valery.

P.S. you should put this your resposne on your blog :-).

[quoted text, click to view]

Valery Pryamikov
3/6/2004 9:03:24 AM

I just wanted to add my closing comment:

Race condition discussed in Chris's letter could only happen if program's
thread didn't call Dispose as the last operation on object instance. And
that was exactly my starting point in the discussion - I meant that programs
are required to call Dispose as the last operation on the object instance
from the *single thread*.

Chris's point is that if malicious program is trying to exploit handle
recycle vulnerability then it would not play by the rules and will try all
variety of calling Dispose in a wrong way, starting from calling it from
multiple threads or on the Target of WeakReference with resurrection
tracking to more elaborate attacks like the one shown in Chris's response.

Conclusions could be following:

Internal/non-shared objects used by the program don't need thread-safety of
Dispose. Reentry protection gives necessary guarantee against possible race
condition from finalizer thread for internal/non-shared objects if program
is always calling Dispose as the last operation on the object instance.

However: shared objects accessible by partially trusted code may require
thread safe code in Dispose, because partially trusted malicious programs
(for example from Internet zone) would not be playing by the rules and will
try to abuse these objects in every possible way.

I hope that Chris agrees with this my conclusions :-).

-Valery

See my blog at:

http://www.harper.no/valery

Valery Pryamikov
3/7/2004 6:56:09 PM

I had some time to think over this issue during weekend and now I want to
share my thoughts.

If class instance constructor doesn't throw exceptions (controlled by
implementation of our component) and program is calling Dispose as last
operation on this class instance, then we have guarantee that reentry
protection in Dispose will be sufficient, and thread safety is not required.

Chris pointed that in case if we think of malicious code trying to misuse
our shared component then all bets of Dispose as the last operation on class
instance will be off, and that malicious code could produce race condition
in our reentry protection. But malicious code could don't need to be as
exotic as trying to exploit race condition from Finalize thread with
resurrection of our component. It could use much easier ways to produce race
condition simply by calling Dispose from multiple threads simultaneously.
But now we come to the interesting part:

Suppose we have shared component that doesn't assert any special rights and
can be called by partially trusted code. All the time when malicious code is
calling our component - runtime walks the stack and evaluate permissions set
so that our component will be allowed to do only what is allowed to the
malicious code. But in that case - what could be the reason for malicious
code to abuse our component? Just for being cruel and unusual? If our code
was allowed to access some resource during call from malicious code and we
didn't assert that access - then malicious code could do the same thing on
its own. Even if our shared component were given some special rights by the
security policy it doesn't matter as long as we don't assert any
permission - permission set evaluation occurs only after complete stack
walk. So, making some exotic solution to abuse shared component that isn't
asserting any special right could only worth effort if this is Microsoft's
component and author of malicious code simply trying to get some publicity.

But that in order means that even shared components that can be called by
partially trusted code doesn't need thread safety in Dispose implementation
as long as this shared component doesn't assert any special permission.
Asserting permissions is a dangerous action that will surely make it
attractive target for malicious code. And making Dispose's code being thread
safe will be just a minor part of security measures required from
implementation of such component.

In other words my point is:

Dispose's code doesn't require thread safety, but require reentry protection
in all cases except for shared components that could be called by partially
trusted code and asserts some special permission.

-Valery.

See my blog at:

http://www.harper.no/valery

[quoted text, click to view]

Valery Pryamikov
3/7/2004 8:27:55 PM

Just a simple correction, to avoid being missunderstood:
[quoted text, click to view]
Should read as:
When malicious code is calling our component and security demand needed to
be evaluated - runtime walks the stack and evaluate demand...

-Valery.

See my blog at:
http://www.harper.no/valery

cbrumme NO[at]SPAM microsoft.com
3/8/2004 4:33:11 PM

I agree.

If you have private objects that cannot be called directly by other code,
then you can rely on the well-behaved nature of your own calls and the
synchronization protection offered by the Finalization mechanism. In this
case, you don't have to layer on additional thread-safety protection.

Similarly, if you aren't callable from partial trust, or if your assembly
is only granted low trust, or if you don't assert any permissions (which
includes satisfaction of link demands, use of 'unsafe' C# blocks and other
non-obvious assertion equivalents), then you don't have to worry about
malicious callers. If this is the case, the synchronization protection
offered by the Finalization mechanism is sufficient to coordinate normal
applications. You don't have to layer on additional thread-safety
protection because you are not an interesting target of attacks from
malicious clients.

In both these cases (which likely covers most user code) you don't need to
worrry about thread-safety in your Dispose code.

Valery Pryamikov
3/8/2004 8:47:08 PM

Thanks!
That was exactly my point :-).

I just wanted to add that for calling shared component containing 'unsafe'
code blocks malicious code would require SkipVerification permission. This
permission can't be Asserted because it is checked during assembly load time
only. And if malicious code was able to call assembly containing 'unsafe'
code blocks = this malicious code could do anything with computer where it
was allowed to skip verification.
So, it is only code asserting some permissions or satisfying
link/inheritance demands that requires thread safety of Dispose
implementation.

-Valery.

See my blog at:
http://www.harper.no/valery

[quoted text, click to view]

cbrumme NO[at]SPAM microsoft.com
3/9/2004 6:17:06 PM

[quoted text, click to view]

Not quite.

If some code with APTCA contains unsafe code blocks, this is roughly
equivalent to that same code asserting or satisfying link demands or
defining PInvokes with SuppressUnmanagedCodeSecurityAttribute or declaring
a union type via ExplicitLayout.

In all these cases, the code is using its own high trust to perform a
privileged operation. In the case of unsafe code blocks, this privileged
operation is the ability to violate the type system or the memory system.
And in all of the cases I listed, any partially trusted callers may be
allowed access to the privileged operation. I say "may be allowed" because
it depends on the existence of APTCA, the accessiblity / visibility of the
methods in question, the arguments that the caller is allowed to pass,
whether there is a call path that extends from a public API to these
"asserting" methods, etc.

If fully trusted code contains unsafe code blocks that are exposed to
partially trusted callers (perhaps through layers of other APIs, so the
partially trusted caller doesn't bind directly to the unsafe code blocks),
then it is the responsibility of the fully trusted code to avoid creating
security holes.

Valery Pryamikov
3/10/2004 8:31:47 AM

Chris,
Did you mean that code doesn't require SkipVerification privilege to load
APTCA assembly with unsafe code blocks??? (Declaring union type makes code
unverifiable too, and therefore requires SkipVerification). The only
documented effect of AllowPartiallyTrustedCallersAttribute that I'm aware of
is preventing placing FullTrust LinkDemand on public members - therefore
making strong named assembly callable by untrusted callers. But APTCA
doesn't (or at least should not) affect SkipVerification permission.
I'd be really surprised to find that it works the way you are saying. It's
definitely against everything I ever read in ECMA specs, and other docs
concerning .Net security! If this is true (which I simply refuse to
believe) - than it's definitely a scary thing! Unverifiable codes is what
its name says - unverifiable and if malicious code running with low trust
can easily get access to unverifiable code - that is really scary!
I agree that SuppressUnmanagedCodeSecurityAttribute is analogue of asserting
UnmanagedCode permssion - this is documented effect of
SuppressUnmanagedCodeSecurityAttribute and its purpose.

-Valery.

P.S. I have to send my computer to repair now - got some ugly problems with
it, but I'll check your assertion that SkipVerification previlege is not
required for loading unverifiable assemblies if they are marked with APTCA.
I'd be really surprised to find if this is true.

[quoted text, click to view]

cbrumme NO[at]SPAM microsoft.com
3/10/2004 7:29:23 PM

We may be talking past each other. Let me be more precise here:

Assembly 1 is partially trusted and contains no unsafe blocks (obviously).
It is fully verifiable.
Assembly 2 is fully trusted and contains unsafe blocks. It has
SkipVerificationPermission.
Assembly 2 is marked with APTCA, indicating that it can be called from
partial trust.

I just had a quick look, and System.dll looks like a popular example for
Assembly 2. It seems to contain some unverifiable methods and it is marked
with APTCA.

What I'm saying is that Assembly1 may be able to call unverifiable methods
in System.dll, perhaps indirectly. This is fully supported. Keep in mind
that just because a method is unverifiable, it doesn't mean that it is
insecure. It just means that the author is responsible for the security,
rather than the system.

If the author of Assembly 2 is concerned that these unverifiable methods
should not be callable somehow from partial trust, then either Assembly 2
should not have APTCA, or the pathways to the unverifiable methods should
contain appropriate permission demands.

Valery Pryamikov
3/12/2004 2:08:46 PM

When I wrote my last mail to that thread my ability to think rationally was
problably affected by the computer troubles that I had that day:-),
therefore I put several unnecassary exclamations there (sorry for that). Of
course SkipVerification permission should be checked on the assembly that
requires to skip verificaton, not on the calling assembly - othervise
untrusted code would never be able to call System.dll and others
(SkipVerification can't be asserted). I'm perfectly aware that unsafe code
could be perfectly safe - its just that system can't verify its safety (btw
that's why I wrote "...unverifiable code is what its name says -
unverifiable...").
However - only exploitable unverifiable code must be protected from access
by untrusted callers, and that effectively excludes all system dlls that are
signed by Microsoft (otherwise it would be security vulnurability). So,
system.dll and family is off the hook.
Now, even if we say that shared components code that assert permissions,
satisfy link/inheritance demands or use unsafe code (excluding all code
signed by Microsoft), may require thread safety of Dispose implementation
(not must, but only may!) - then it still concerns only a tiny fraction of
..Net code developed outside of Microsoft. Adding unnecessary thread safety
means adding nontrivial performance impact to the code that doesn't require
it. In February 2002 I wrote sample that shown that adding 2 LOCKs
(InterlockedIncrement/InterlockedDecrement) to the code that already used
more than 6 other interlocked operations (which includes 4
InterlockedCompareExchange in
cycle+InterlockedIncrement/InterlockedDecrement) on Intel Pentium 3/4
processors could result in doubling time required to run the code (instead
of expected 20~25%). Single LOCK could weight more than several hundreds of
the processor instructions and LOCK time isn't always linear. (If there will
be interest, I can prepare that code again and post it to the group after I
get my computer back from repair :-).
Additionally, thread safety isn't the easiest part of programming and naive
code written for providing thread safety could just introduce extra errors
instead of protecting from race condition...
Therefore the point that I'm trying to make here is that for the major part
of .Net code developed outside of Microsoft (and rather significant part of
the .Net code developed inside Microsoft too) Dispose doesn't require thread
safety but only require reentry protection.

In other words:
Provided reentry protecttion of Dispose and guarantee that Dispose is called
as the last operation on the object instance, thread-safety of Dispose is
not required for:
- non-shared components;
- shared components that is not marked with AllowPartiallyTrustedCallers
attribute;
- shared components that is marked with APTCA but that doesn't assert
permissions, doesn't satisfy link/inheritance demands and has no
unverifiable code;

Shared components that are marked with APTCA AND either assert permissions,
satisfy link/inheritance demands or have unverifiable code may require
thread safety of Dispose implementation in some cases. Analyze required for
making conclusion of whether or not such shared component requires
thread-safety of Dispose isn't trivial, therefore thread safety of Dispose
could be recommended here.

-Valery.

See my blog at:
http://www.harper.no/valery

[quoted text, click to view]

dotnet clr : Dispose must be thread-safe ?