Boost C++ Libraries: Ticket #4415: Posible "Segmentation Fault" in shared_ptr destructor

Peter Dimov — Fri, 09 Jul 2010 08:43:16 GMT

If two threads enter release, the initial value of use_count_ must be at least 2, and only the thread that last decrements it will proceed to dispose(). I don't see how this can lead to the other thread suffering a segmentation fault while accessing use_count_, as its access must already have been performed. Can you please attach a reasonably complete example that demonstrates the problem?

attachment set

Fri, 09 Jul 2010 09:20:33 GMT

attachment → SharedPtrCrushTest.cpp

Fri, 09 Jul 2010 09:31:39 GMT

Here's a trivial crush test with singly linked list.

I agree the logic looks correct. But it crushes.

In my test, the same logic actually works, and runs fine on the same test.

		if(counter->decrement() == 0){
			delete counter;
			delete pointee_;
		}

With the following two implementations.

	unsigned int decrement(){
		return i.fetch_sub( 1, std::memory_order_relaxed );
	}
	unsigned int decrement(){
		assert(pthread_mutex_lock(&mutex) == 0);
		i++;
		assert(pthread_mutex_unlock(&mutex) == 0);
		return i;
	}

I'm not familier with assembly, but is it really atomic?

    __asm__ __volatile__
    (
        "lock\n\t"
        "xadd %1, %0":
        "=m"( *pw ), "=r"( r ): // outputs (%0, %1)
        "m"( *pw ), "1"( dv ): // inputs (%2, %3 == %1)
        "memory", "cc" // clobbers
    );

Peter Dimov — Fri, 09 Jul 2010 10:05:53 GMT

Yes, lock xadd is atomic. What version of g++ are you using, and what optimization level? There are no threads in your example, so it's probably a code generation problem.

Fri, 09 Jul 2010 11:00:54 GMT

I used two versions, the same results. 4.1.2 and 4.4.0.

The build script I used is the following.

gcc -g -Iinc -IBoost -c -o bin/SharedPtrCrushTest.o src/SharedPtrCrushTest.cpp
gcc -g -lstdc++ -o bin/SharedPtrCrushTest bin/SharedPtrCrushTest.o

Peter Dimov — Fri, 09 Jul 2010 11:24:56 GMT

Does it make any difference if you use g++ instead of gcc?

Peter Dimov — Fri, 09 Jul 2010 11:36:23 GMT

FWIW, your program works for me with g++ 4.3.4 under Windows/Cygwin. I have verified that the assembly contains the portions from sp_counted_base_gcc_x86.hpp. What OS are you using?

Fri, 09 Jul 2010 12:23:09 GMT

Thanks for checking.

No, g++ didn't make any difference.

My OS is CentOS 5.5(final). Kernel is 2.6.18. CPU is Core 2 Duo P7350 @2GHz(64bit).

Could 64bit make any difference?

Peter Dimov — Fri, 09 Jul 2010 13:24:59 GMT

I can reproduce the crash under CentOS 5.4. It is not a 64 bit issue, as g++ -m32 crashes as well. I don't know yet what's causing it or whether the problem is in our code or a Red Hat g++ issue.

anonymous — Fri, 09 Jul 2010 14:05:29 GMT

I get it crushed also on Max OS X(10.6.4), gcc 4.2.1.

But on Windows with Cygwin, it doesn't.

Peter Dimov — Fri, 09 Jul 2010 14:11:36 GMT

It is a stack overflow due to the 1000000 recursive destructor calls.

anonymous — Fri, 09 Jul 2010 16:07:28 GMT

Oops!

Is there any reason that this happens only with shared_ptr?

I mean, two other implementations with c++0x atomic operations and mutex don't fail.

Anyway, I check stack myself. Thanks.

Sat, 10 Jul 2010 02:47:42 GMT

This was completely my fault. I found some leaks that was considered working, which didn't call destructor recursively.

Thanks.

Sat, 10 Jul 2010 04:27:27 GMT

For those who have the same problem. One little change solves this issue.

	~IntSLList(){
		while(head != tail){
			// replace
			head = head->next;
		}
	}

shared_ptr carefully manages counter, so this does not trigger recursive destruction.

This works for millions or whatever until exausted.

status changed; resolution set

Peter Dimov — Mon, 12 Jul 2010 09:37:08 GMT

status new → closed
resolution → invalid