Opened 10 years ago

Closed 9 years ago

#8306 closed Bugs (worksforme)

named mutex does not unlock as expected

Reported by: David Hebbeker <david.hebbeker@…> Owned by: Ion Gaztañaga
Milestone: To Be Determined Component: interprocess
Version: Boost 1.52.0 Severity: Optimization
Keywords: named mutex, named_mutex, semaphore, posix_named_semaphore, unlock Cc: info@…, ayman@…

Description

named_mutex internally uses a posix_named_semaphore (in file <boost/interprocess/sync/posix/named_mutex.hpp>). When a mutex is unlocked (via unlock()) multiple times consecutively a single call to lock() will not lock the mutex!

In the documentation it says:

void unlock()

Precondition: The thread must have exclusive ownership of the mutex.

[Boost]
This precondition is violated within this scenario. But the expected precondition is that no other thread must have exclusive ownership of the mutex. This way a free mutex could be unlocked safely.

The expected behaviour is that:

A mutex is a variable that can be in one of two states: unlocked or locked.

[Tanenbaum]
Thus locking an unlocked mutex would definitely lock it. To demonstrate that this expectation is not fulfilled a minimal program will be attached.

A workaround to securely unlock a named_mutex could be:

if(mutex.try_lock())
{
	mutex.unlock();
}

This way the mutex will only be unlocked if no other thread holds ownership of the mutex. Also the mutex will not be unlocked consecutively in case this snipped is called several times as it would be locked by try_lock() before. After this snipped the mutex will not be locked by the calling thread, but still might be locked by an other thread.

A real fix to this issue would probably be to use mutexes (like with pthread_mutex_unlock) instead of semaphores.

[Tanenbaum] A. S. Tanenbaum, Operating systems: design and implementation, 3rd ed. Upper Saddle River, N.J: Pearson/Prentice Hall, 2006.
[Boost] ‘Synchronization mechanisms - 1.53.0’. [Online]. Available: http://www.boost.org/doc/libs/1_53_0/doc/html/interprocess/synchronization_mechanisms.html#interprocess.synchronization_mechanisms.mutexes.mutexes_mutex_operations. [Accessed: 18-Mar-2013].

Attachments (1)

namedMutexExample.cpp (597 bytes ) - added by David Hebbeker <david.hebbeker@…> 10 years ago.
Minimal example of using named mutexes and unlocking them several times consecutively. Linked with -lpthread. The program is expected to never return.

Download all attachments as: .zip

Change History (8)

by David Hebbeker <david.hebbeker@…>, 10 years ago

Attachment: namedMutexExample.cpp added

Minimal example of using named mutexes and unlocking them several times consecutively. Linked with -lpthread. The program is expected to never return.

comment:1 by Steven Watanabe, 10 years ago

Violating the preconditions of a function has undefined behavior. Once you do this, you cannot rely on the object (or the program) being in a sane state.

in reply to:  1 comment:2 by David Hebbeker <david.hebbeker@…>, 10 years ago

Replying to steven_watanabe:

Violating the preconditions of a function has undefined behavior. Once you do this, you cannot rely on the object (or the program) being in a sane state.

Yes, I am acknowledging that this scenario would violate the preconditions defined in the documentation. My intention is that the behaviour is not as one would expect it from a mutex in general (see [Tanenbaum]).

comment:3 by Steven Watanabe, 10 years ago

I don't understand why you expect this behavior. Your quote from Tanenbaum is irrelevant because valid states only apply as long as you follow the rules. Once you break the rules, all bets are off.

$ man pthread_mutex_unlock
...
If the mutex type is PTHREAD_MUTEX_DEFAULT, ... Attempting to
unlock the mutex if it was not locked by the calling thread
results in undefined behavior.  Attempting to ulock the mutex
if it is not locked results in undefined behavior.
...

In any case, the precondition that you propose, "no other thread must have exclusive ownership of the mutex", is basically useless. If the current thread doesn't hold the mutex, how do you know that no other thread holds it?

in reply to:  3 ; comment:4 by David Hebbeker <david.hebbeker@…>, 10 years ago

Replying to steven_watanabe:

I don't understand why you expect this behavior.

I expect this behaviour as I imagine that a mutex has some kind of state machine. I hoped that there is no error / undefined state and that an attempt to unlock in the unlocked stated would result in the unlocked state. Just that simple.

Your quote from Tanenbaum is irrelevant because valid states only apply as long as you follow the rules. Once you break the rules, all bets are off.

The point is, that I wish other rules.

$ man pthread_mutex_unlock
...
If the mutex type is PTHREAD_MUTEX_DEFAULT, ... Attempting to
unlock the mutex if it was not locked by the calling thread
results in undefined behavior.  Attempting to ulock the mutex
if it is not locked results in undefined behavior.
...

What about PTHREAD_MUTEX_ERRORCHECK? In that case an EPERM error would be returned, which is better than undefined behaviour.

In any case, the precondition that you propose, "no other thread must have exclusive ownership of the mutex", is basically useless. If the current thread doesn't hold the mutex, how do you know that no other thread holds it?

I am not sure if I got your question: I could use try_lock() to test if an other thread holds it.

Also my workaround from above is not complete! Maybe this can be considered, which is still not perfect:

if(mutex.try_lock())
{	// The mutex was free and is now locked by this thread
	mutex.unlock();
}
else {	// The mutex is locked, try to unlock it
	mutex.unlock(); // Hopefully it is locked by this thread
	// in case an other thread holds the mutex > bad things happen
}

The whole point is that I would like to be able to safely call unlock on a mutex. And the behaviour should not be undefined in case it already was unlocked.

I am not saying, that the current implementation is defect'' But that it could maybe be improved by removing the restriction applied in the precondition?

By the way: Thanks for the fast responses!

in reply to:  4 ; comment:5 by Steven Watanabe, 10 years ago

Replying to David Hebbeker <david.hebbeker@…>:

I expect this behaviour as I imagine that a mutex has some kind of state machine. I hoped that there is no error / undefined state and that an attempt to unlock in the unlocked stated would result in the unlocked state. Just that simple.

I'm afraid that isn't how mutexes work.

What about PTHREAD_MUTEX_ERRORCHECK? In that case an EPERM error would be returned, which is better than undefined behaviour.

But note that it's still considered an error. It's just detected. Also, in the standard, C++11 30.4.1.2: "21 The expression m.unlock() shall be well-formed and have the following semantics. 22 Requires: The calling thread shall own the mutex."

In any case, the precondition that you propose, "no other thread must have exclusive ownership of the mutex", is basically useless. If the current thread doesn't hold the mutex, how do you know that no other thread holds it?

I am not sure if I got your question: I could use try_lock() to test if an other thread holds it.

If you already hold the mutex, then for a non-recursive mutex, you can't call try_lock and for a recursive mutex, calling unlock after try_lock will leave you exactly where you started.

Suppose that your semantics were implemented. Then consider the following code:

Thread A:
1 - if(foo()) m.lock();
2 - m.unlock();

Thread B:
3 - m.lock();
4 - // do something
5 - m.unlock();

If B is at (4) and foo() returns false then, thread A has undefined behavior. In order to make A correct, we'd have to make m.unlock() be a no-op even if another thread holds m. It seems rather counterintuitive that m.unlock() should be legal and leave the mutex locked.

Also my workaround from above is not complete! Maybe this can be considered, which is still not perfect:

if(mutex.try_lock())
{	// The mutex was free and is now locked by this thread
	mutex.unlock();
}
else {	// The mutex is locked, try to unlock it
	mutex.unlock(); // Hopefully it is locked by this thread
	// in case an other thread holds the mutex > bad things happen
}

This code has exactly the same problems as plain mutex.unlock().

The whole point is that I would like to be able to safely call unlock on a mutex. And the behaviour should not be undefined in case it already was unlocked.

I am not saying, that the current implementation is defect'' But that it could maybe be improved by removing the restriction applied in the precondition?

It's not going to happen. The bottom line is that you have to know whether you hold a mutex in order to do anything useful with it. I don't think that the library should go out of its way to support usage which is very dangerous and which is not supported by any other mutex implementation that I'm aware of.

Also, I don't even want to think about the implications of your proposed behavior for recursive mutexes.

in reply to:  5 comment:6 by David Hebbeker <david.hebbeker@…>, 10 years ago

Replying to steven_watanabe:

Replying to David Hebbeker <david.hebbeker@…>:

What about PTHREAD_MUTEX_ERRORCHECK? In that case an EPERM error would be returned, which is better than undefined behaviour.

But note that it's still considered an error. It's just detected.

[...]

It's not going to happen. The bottom line is that you have to know whether you hold a mutex in order to do anything useful with it. I don't think that the library should go out of its way to support usage which is very dangerous and which is not supported by any other mutex implementation that I'm aware of.

As I understand the behaviour I described is implemented by pthread mutexes of type PTHREAD_MUTEX_ERRORCHECK. It improves the behaviour as it may not result in an undefined state in contrast to the default mutex, but it returns an error value. The downside would be the performance overhead.

comment:7 by Ion Gaztañaga, 9 years ago

Resolution: worksforme
Status: newclosed

As Steven correctly points out, violating the precondition must not be supported. std::mutex behaves also this way. ERRORCHECK mutexes are not available in al platforms and they have a perfomance impact.

Note: See TracTickets for help on using tickets.