Ayende @ Rahien

Refunds available at head office

Find the bug: Why do I get a Null Reference Exception?

Originally posted at 4/12/2011

Can you spot the NRE hiding in the code?

if (parentObject!= null)
{
    lock (parentObject)
    {
        if (parentObject != null)
        {
            properties = properties.Clone();
            parentObject[parentKey] = this;
            parentObject = null;
        }
    }
}

Comments

jmwong
04/20/2011 09:10 AM by
jmwong

other thread assigned null to parentObject while this thread is executing right after the second if clause but before the first statement

Scooletz
04/20/2011 09:11 AM by
Scooletz

Nullifying the parent object prevents from entering this code again. Is this your NRE?:>

Fredrik Mörk
04/20/2011 09:11 AM by
Fredrik Mörk

If parentObjectCopy is null you should get an ArgumentNullException, so my vote goes to properties being null.

hangy
04/20/2011 09:13 AM by
hangy

.NET may fail when trying to lock (parentObjectCopy) and that is null. You probably wanted to lock on parentObject instead.

Iain
04/20/2011 09:14 AM by
Iain

either:

properties is null

or

parentKey is null

Alex Reitbort
04/20/2011 09:18 AM by
Alex Reitbort

Why would you do this:

properties = properties.Clone();

What purpose does it serve?

Ayende Rahien
04/20/2011 09:21 AM by
Ayende Rahien

Fredrik,

You are correct, I published part of the solution to this, opps. Fixed now

Iulian Margarintescu
04/20/2011 09:23 AM by
Iulian Margarintescu

It depends on what kind of variables parentObject & parentObjectCopy are. If parentObjectCopy is not shared between threads ( private static ) then the lock is kind of useless since other threads will not block on it and can set partenObject to null while other threads try to call the indexer.

mjkirkham
04/20/2011 09:29 AM by
mjkirkham

Looks like you have a race condition between checking the parentObject for null and then attempting to lock it.

Patrick Huizinga
04/20/2011 09:35 AM by
Patrick Huizinga

As mjkirkham says, there's a race condition.

When a context switch happens right after the first if statement, but before the lock, another thread could execute "parentObject = null;". When the 'first' thread then resumes control, it will try to lock on a parentObject which is now null.

hangy
04/20/2011 09:35 AM by
hangy

Okay, I was wrong. The (now removed) parentObjectCopy being null would have caused an ArgumentNullException, as Frederik said.

Iain: parentKey being null should cause an ArgumentNullException, too.

In that case, setting properties (let's assume it's a instance variable) to properties.Clone() does seem suspicious ...

Adam
04/20/2011 10:33 AM by
Adam

You are setting the lock object to null.

When the lock context ends, Monitor.Exit(parentObject) will be called and will throw.

But I thought it threw ArgumentNullException as well

Linkgoron
04/20/2011 12:06 PM by
Linkgoron

I've run the code and Monitor.Exit does not throw an exception. I'm guessing that lock creates a temp reference object.

I wish we had a bit more context to this (i/e scope of variables etc.)

Adam
04/20/2011 02:05 PM by
Adam

Its no NRE, but other ArgumentNullException would be if a second entrant attempted to lock ParentObject after it was set to null.

2nd Entrant -> parentObject != null, continue

1st entrant -> parentobject = null

2nd Entrant -> lock(parentObject), throws

Setting the lock to null seems like a bad idea.

Oleksii
04/20/2011 03:09 PM by
Oleksii

The LOCK statement is converted into two methods:

==============

Monitor.Enter(object o)

//code

Monitor.Exit(object o)

==============

In the Exit method the locking object has been set to null, and as described in the documentation of the method an ArgumentNullException would be thrown.

Duarte Nunes
04/20/2011 03:17 PM by
Duarte Nunes

Actually, on .NET 4.0, lock (obj) translates into try { Monitor.Enter(local = obj, ref lockTaken); ... } finally { if (lockTaken) Monitor.Exit(local); }. This means that the problem is not the Exit method; the Enter method is the one throwing NRE.

Bek
04/20/2011 03:17 PM by
Bek

Monitor.Enter will throw an ArgumentNullException...Not sure about NullReferenceException.

Duarte Nunes
04/20/2011 03:23 PM by
Duarte Nunes

Oh yeah, you're right. Don't know about the NullReferenceException then.

Bek
04/20/2011 03:24 PM by
Bek

even in 4.0 Enter throws an ArgumentNullException. if obj is null.

Duarte Nunes
04/20/2011 03:26 PM by
Duarte Nunes

Well, since all accesses to "parentObject" are safe, and Monitor.Enter throws ArgumentNullException, then I guess "properties" must be null.

Bek
04/20/2011 03:27 PM by
Bek

One more thing: generally the framework doesn't allow NRE to bubble up. it's usually something more meaningful. It has to be in his code.

Alex Espinoza
04/20/2011 03:29 PM by
Alex Espinoza

The compiler is optimizing the output with reordering:

Compiler Reordering:

 parentObject = null;

 parentObject[parentKey] = this;

This can be prevented if a barrier is used and/or making the object volatile to avoid reodering.

Duarte Nunes
04/20/2011 03:37 PM by
Duarte Nunes

That's not it. There's a dataflow dependence so the compiler would never make that optimization. Also, on the CLR, all stores have release semantics, meaning there's an implicit barrier that does not allow stores to be reordered with other stores.

Duarte Nunes
04/20/2011 03:40 PM by
Duarte Nunes

@Bek Yep. My initial mix up stemmed from a bit too much java programming, where there's no equivalent to ArgumentNullException: when validating parameters, you throw a NullPointerException.

Basarat
04/21/2011 03:55 AM by
Basarat

The object used for a lock is being set to null inside the lock.

Abhi
04/21/2011 04:34 AM by
Abhi

I think "properties" is not initialized, so when calling properties.clone() , you get a NRE ..?

Fabian Schmied
04/21/2011 02:01 PM by
Fabian Schmied

If some other piece of code sets parentObject to a different (non-null) value, then the parentObject you lock on might not be the same parentObject you use a few lines down. This might cause a NullReferenceException (and other strange behavior).

This is far-fetched, since the snippet doesn't show anyone setting parentObject, but still... Locking an object that can be replaced (by null or whatever) doesn't seem like a good idea to me.

Peter Gfader
04/23/2011 07:10 AM by
Peter Gfader

properties is null

or

parentKey is null

This line is wierd

properties = properties.Clone();

Mark Knell
05/03/2011 07:55 PM by
Mark Knell

lock() is not safe against reentrant code on the same thread. So maybe the implementation of properties.Clone() recurses to the block of code in question (and does not infinitely recurse, since we didn't get a stack overflow; suppose it has some sort of recursion limit). It will re-enter the lock, Clone() once more, null out the parentObject (I'm assuming it's not a local variable on the stack, since you're locking on it), then unwind to find parentObject null. Boom.

User24
05/19/2011 09:20 AM by
User24

How to install rhino mocks? the usability of this page is very bad. where can i get binaries. i dont want to install git or something like that. this is to freaky. it should be easier to use at all.

Ayende Rahien
05/19/2011 09:25 AM by
Ayende Rahien

User24, And this has what to do with this post?

User24
05/19/2011 10:45 AM by
User24

If i would have spend less time in looking how to get rhino mocks installed, i would have more time for things like this (finding NULL References).

Kind Regards && sorry for wrong Post ;)

Comments have been closed on this topic.