[Tcl-bugs] [ tcl-Bugs-992108 ] TclFinalizeLock on Unix not correct.

[SourceForge.net <no...@so...>]

Bugs item #992108, was opened at 2004-07-15 20:52
Message generated for change (Comment added) made by mistachkin
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=110894&aid=992108&group_id=10894

Category: 47. Threading
Group: None
Status: Open
Resolution: None
Priority: 5
Submitted By: Joe Mistachkin (mistachkin)
Assigned to: Zoran Vasiljevic (vasiljevic)
>Summary: TclFinalizeLock on Unix not correct.

Initial Comment:

The function currently reads:

void
TclFinalizeLock ()
{
#ifdef TCL_THREADS
    /*
     * You do not need to destroy mutexes that were
created with the
     * PTHREAD_MUTEX_INITIALIZER macro.  These mutexes
do not need
     * any destruction: masterLock, allocLock, and
initLock.
     */
    pthread_mutex_unlock(&initLock);
#endif
}

I believe that both the comment and the code are
incorrect.  Mutexes created with the pthread library
have memory associated with them internally when you
call pthread_mutex_create, therefore,
pthread_mutex_destroy must be called on them.  In my
opinion, the function should be more like this:

TclFinalizeLock ()
{
#ifdef TCL_THREADS
    /*
     * These mutexes need to be destroyed via
pthread_mutex_destroy 
     * because they were created with
pthread_mutex_create, which 
     * internally allocates memory on some platforms.
     */
    pthread_mutex_destroy(&allocLock);
    pthread_mutex_destroy(&masterLock);
    pthread_mutex_unlock(&initLock);
    pthread_mutex_destroy(&initLock);
#endif
}


----------------------------------------------------------------------

>Comment By: Joe Mistachkin (mistachkin)
Date: 2004-07-17 13:00

Message:
Logged In: YES 
user_id=113501

Actually, I don't really think it's an issue because it is
done dead-last in the finalization process and various other
things before it assume that all threads are destroyed, etc.

The Win32 code simply deletes the locks while they are being
held (not really recommended).  

What would be ideal here (and potentially quite a few other
places in the core) is a spinlock that doesn't have to be
deleted and can protect the mutexes.


----------------------------------------------------------------------

Comment By: Zoran Vasiljevic (vasiljevic)
Date: 2004-07-17 11:01

Message:
Logged In: YES 
user_id=95086

Uhuhuhu... voodoo (mutex) magic! 
 
You better check the pthread_mutex_lock implementation 
for what's gonna happen in the second thread which is 
sleeping on the initLock while you're NULL-ing it under  
its very feet :-) 

----------------------------------------------------------------------

Comment By: Joe Mistachkin (mistachkin)
Date: 2004-07-17 10:48

Message:
Logged In: YES 
user_id=113501

Maybe something like this (init lock is held):

    pthread_mutex_t tempInitLock;
    pthread_mutex_t tempMasterLock;
    pthread_mutex_t tempAllocLock;

    tempInitLock = initLock;
    initLock = NULL;

    tempMasterLock = masterLock;
    masterLock = NULL;

    tempAllocLock = allocLock;
    allocLock = NULL;

    pthread_mutex_destroy(&tempAllocLock);
    pthread_mutex_destroy(&tempMasterLock);
    pthread_mutex_unlock(&tempInitLock);
    pthread_mutex_destroy(&tempInitLock);


----------------------------------------------------------------------

Comment By: Zoran Vasiljevic (vasiljevic)
Date: 2004-07-17 03:37

Message:
Logged In: YES 
user_id=95086

OK. 
The remaining thing to be resolved here is: 
 
 pthread_mutex_unlock(&initLock); 
 pthread_mutex_destroy(&initLock); 
 
Theoretically, you have a classcal race condition here. 
If two threads attempt to call Tcl_Finalize, at the 
moment you release the  initLock mutex in the  
first thread, the other thread starts the calling  
TclFinalizeLock() and you go thru the call twice, 
destroying the already destroyed allocLock,  
masterLock and initLock mutexes. 
 
This is very tricky to solve correctly. You  may do 
pthread_mutex_destroy(&allocLock), allocLock = NULL; 
pthread_mutex_destroy(&masterLock), masterLock = NULL; 
 
to avoid these two traps (in which case current freebsd 
implementation will return  EINVAL)  but you're left with 
the potential hazardous initLock where you can go and 
hit the destroyed but not re-initialized mutex and definitely 
get the segmentation at the line: 
  
   _SPINLOCK(&(*mutex)->lock); 
 
in pthread_destroy_mutex() 
 
In order to avoid this, you must get yet another mutex  
and the story repeats. Eventually, you must at least 
accept one leaked mutex (the initLock). 
I see no other possibility. Do you? 
 

----------------------------------------------------------------------

Comment By: Joe Mistachkin (mistachkin)
Date: 2004-07-16 15:58

Message:
Logged In: YES 
user_id=113501


I suggest that we #ifdef __FreeBSD__ for now and put a large
comment there that indicates that if you find a leak report
it and we will add that platform to the #ifdef as a platform
that needs the mutexes freed.


----------------------------------------------------------------------

Comment By: Zoran Vasiljevic (vasiljevic)
Date: 2004-07-16 15:45

Message:
Logged In: YES 
user_id=95086

Interesting.... 
Agree, for freebsd you will get the leak. 
 
So, we have now 3 cases covered. For freebsd, you can/ 
must/should destroy static mutexes, for Linux/Solaris  
you need not do that and if you do, there is no harm. 
 
The question is how are we going to conditionaly configure 
and wrap this in an elegant way?  
We must also take in the consideration that for some 
platforms (AIX, HP, Win) we have no access to sources  
to look at and vendor docs are not very helpful.  
Seems like quite a lot of work to me.  
You have any magic idea? 
 

----------------------------------------------------------------------

Comment By: Joe Mistachkin (mistachkin)
Date: 2004-07-16 15:12

Message:
Logged In: YES 
user_id=113501

The following code is from FreeBSD 4.10-STABLE.

-- in /usr/src/include/pthread.h:

#define PTHREAD_MUTEX_INITIALIZER       NULL

-- in /usr/src/lib/libc_r/uthread/uthread_mutex.c:

int
_pthread_mutex_init(pthread_mutex_t * mutex,
		   const pthread_mutexattr_t * mutex_attr)
{
	enum pthread_mutextype	type;
	int		protocol;
	int		ceiling;
	pthread_mutex_t	pmutex;
	int		ret = 0;

	if (mutex == NULL)
		ret = EINVAL;

	/* Check if default mutex attributes: */
	else if (mutex_attr == NULL || *mutex_attr == NULL) {
		/* Default to a (error checking) POSIX mutex: */
		type = PTHREAD_MUTEX_ERRORCHECK;
		protocol = PTHREAD_PRIO_NONE;
		ceiling = PTHREAD_MAX_PRIORITY;
	}

	/* Check mutex type: */
	else if (((*mutex_attr)->m_type < PTHREAD_MUTEX_ERRORCHECK) ||
	    ((*mutex_attr)->m_type >= MUTEX_TYPE_MAX))
		/* Return an invalid argument error: */
		ret = EINVAL;

	/* Check mutex protocol: */
	else if (((*mutex_attr)->m_protocol < PTHREAD_PRIO_NONE) ||
	    ((*mutex_attr)->m_protocol > PTHREAD_MUTEX_RECURSIVE))
		/* Return an invalid argument error: */
		ret = EINVAL;

	else {
		/* Use the requested mutex type and protocol: */
		type = (*mutex_attr)->m_type;
		protocol = (*mutex_attr)->m_protocol;
		ceiling = (*mutex_attr)->m_ceiling;
	}

	/* Check no errors so far: */
	if (ret == 0) {
		if ((pmutex = (pthread_mutex_t)
		    malloc(sizeof(struct pthread_mutex))) == NULL)
			ret = ENOMEM;
		else {
			/* Reset the mutex flags: */
			pmutex->m_flags = 0;

			/* Process according to mutex type: */
			switch (type) {
			/* case PTHREAD_MUTEX_DEFAULT: */
			case PTHREAD_MUTEX_ERRORCHECK:
			case PTHREAD_MUTEX_NORMAL:
				/* Nothing to do here. */
				break;

			/* Single UNIX Spec 2 recursive mutex: */
			case PTHREAD_MUTEX_RECURSIVE:
				/* Reset the mutex count: */
				pmutex->m_data.m_count = 0;
				break;

			/* Trap invalid mutex types: */
			default:
				/* Return an invalid argument error: */
				ret = EINVAL;
				break;
			}
			if (ret == 0) {
				/* Initialise the rest of the mutex: */
				TAILQ_INIT(&pmutex->m_queue);
				pmutex->m_flags |= MUTEX_FLAGS_INITED;
				pmutex->m_owner = NULL;
				pmutex->m_type = type;
				pmutex->m_protocol = protocol;
				pmutex->m_refcount = 0;
				if (protocol == PTHREAD_PRIO_PROTECT)
					pmutex->m_prio = ceiling;
				else
					pmutex->m_prio = 0;
				pmutex->m_saved_prio = 0;
				_MUTEX_INIT_LINK(pmutex);
				memset(&pmutex->lock, 0, sizeof(pmutex->lock));
				*mutex = pmutex;
			} else {
				free(pmutex);
				*mutex = NULL;
			}
		}
	}
	/* Return the completion status: */
	return(ret);
}

int
_pthread_mutex_destroy(pthread_mutex_t * mutex)
{
	int	ret = 0;

	if (mutex == NULL || *mutex == NULL)
		ret = EINVAL;
	else {
		/* Lock the mutex structure: */
		_SPINLOCK(&(*mutex)->lock);

		/*
		 * Check to see if this mutex is in use:
		 */
		if (((*mutex)->m_owner != NULL) ||
		    (TAILQ_FIRST(&(*mutex)->m_queue) != NULL) ||
		    ((*mutex)->m_refcount != 0)) {
			ret = EBUSY;

			/* Unlock the mutex structure: */
			_SPINUNLOCK(&(*mutex)->lock);
		}
		else {
			/*
			 * Free the memory allocated for the mutex
			 * structure:
			 */
			_MUTEX_ASSERT_NOT_OWNED(*mutex);
			free(*mutex);

			/*
			 * Leave the caller's pointer NULL now that
			 * the mutex has been destroyed:
			 */
			*mutex = NULL;
		}
	}

	/* Return the completion status: */
	return (ret);
}


----------------------------------------------------------------------

Comment By: Zoran Vasiljevic (vasiljevic)
Date: 2004-07-16 15:10

Message:
Logged In: YES 
user_id=95086

Just out of the couriosity... 
I've tested Solaris and Linux, both NPTL and linuxthreads. 
No implementation does any dynamic memory allocations 
on mutexes. They are either dynmically allocated by  
the user, in which case they need to be deallocated  
by the user as well, or they are staticaly allocated 
by the compiler in which case there is nothing to 
deallocate. 
 
For those two platforms, your suggested change won't  
lead to any problem. So, at least we have two of them. 
You can look into the freebsd stuff.  
 

----------------------------------------------------------------------

Comment By: Zoran Vasiljevic (vasiljevic)
Date: 2004-07-16 14:57

Message:
Logged In: YES 
user_id=95086

Frankly speaking, the common sense tells that  
if you first destroy the thing, then you need to  
create the thing before you can use it again. 
  
So, if you destroy the static mutex (which you 
did not create at the first place, rather the 
compiler did that for you) who is going to  
create it? 
 
This is very implementation specific. For example, 
have a look on the NPTL pthread_mutex_destroy: 
 
int 
__pthread_mutex_destroy (mutex) 
     pthread_mutex_t *mutex; 
{ 
  if (mutex->__data.__nusers != 0) 
    return EBUSY; 
 
  return 0; 
} 
 
I am all with you that we should aim at cleaning 
up what's possible after us, but I'm afraid that 
in some circumstances we might clean-up too much. 
 
To be on the safe side, I'd rather risk a leak 
then a (eventual) memory corruption. 
Ideally, one would grab sources of all thread 
library implementations, look what is really 
done under the hood and make a wrapper with 
ifdef's for all supported variants.  
This is hard job to do. And implementor docs 
are more than confusing in that respect, as 
Andreas have found out. 
 

----------------------------------------------------------------------

Comment By: Joe Mistachkin (mistachkin)
Date: 2004-07-16 14:17

Message:
Logged In: YES 
user_id=113501

If you unload the Tcl library and reload it, the memory will
be leaked by pthread_mutex_init for all these mutexes
(verified under valgrind on FreeBSD).

And quite frankly, since Tcl is supposed to be fully
supported in application embedding scenerios, I do not agree
with the assertion that memory and resources (both internal
and external) should not be reclaimed whenever possible.  

Is there any platform where calling pthread_mutex_destroy
would cause any harm in this case?

On Windows, this would be analogous to not calling
DeleteCriticalSection on the same three locks (which are
also statically allocated).


----------------------------------------------------------------------

Comment By: Zoran Vasiljevic (vasiljevic)
Date: 2004-07-16 14:00

Message:
Logged In: YES 
user_id=95086

I understand your (symetric) reasoning very well. 
But... 
Let us forget memory leaks at this place and think  
little bit differently... 
 
Say, you were to destroy static mutexes on Tcl_Finalize(), 
that is, on unload of Tcl library, and then load the same  
Tcl library *again* in the same process space. What is 
going to happen when you try to access one of those 
mutexes (which is absolutely certain, per-se) again? 
Hm? 
 
Therefore, I recommend you not to touch them, really. 
Just let them live their static life. 
Besides, there is absolutely no issue with memory leaks 
here. Those are just part of the process memory and  
will be reclaimed when the process exits. 
 
HTH 

----------------------------------------------------------------------

Comment By: Andreas Kupries (andreas_kupries)
Date: 2004-07-16 11:18

Message:
Logged In: YES 
user_id=75003

The macro PTHREAD_MUTEX_INITIALIZER does not seem to use
'pthread_mutex_create' (Linux). ... Googling. I found this

http://nf.apac.edu.au/facilities/sc/compaq_mirror/HTML/ARH9RBTE/DOCU0022.HTM
Docs for "True64 Unix". In the description of
'pthread_mutex_init' I find:

<quote>
Use the PTHREAD_MUTEX_INITIALIZER  macro to statically
initialize a mutex without calling this routine. Statically
initialized mutexes need not be destroyed using
pthread_mutex_destroy()
</quote>

The opengroup has
http://www.opengroup.org/onlinepubs/007908799/xsh/pthread_mutex_init.html
which says

<quote>
In cases where default mutex attributes are appropriate, the
macro PTHREAD_MUTEX_INITIALIZER can be used to initialise
mutexes that are statically allocated. The effect is
equivalent to dynamic initialisation by a call to
pthread_mutex_init() with parameter attr specified as NULL,
except that no error checks are performed.
</quote>

For Java
http://java.icmc.sc.usp.br/library/books/ibm_pthreads/users-61.htm

<quote>
Mutex initialization using the PTHREAD_MUTEX_INITIALIZER
does not immediately initialize the mutex. Instead, on first
use, the pthread_mutex_lock() or pthread_mutex_trylock()
functions branch into a slow path and cause the
initialization of the mutex. Because a mutex isn't just a
simple memory object, and requires that some resources be
allocated by the system, an attempt to call
pthread_mutex_destroy() or pthread_mutex_unlock() on a mutex
that has was statically initialized using
PTHREAD_MUTEX_INITIALER and was not yet locked will result
in an EINVAL error.
</quote>


So for at least some platforms this is publically documented
behaviour, and for others the interaction is not really
talked about, more implicit. And for some you have to call
the destroy(), it seems.

Confusing. 

Reassigning to Zoran for his opinion. David Gravereaux's
opinion is important as welll IMHO.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=110894&aid=992108&group_id=10894