On Mon, Dec 17, 2012 at 08:58:46PM +0100, Matthias Kilian wrote: > IMHO, this is crap code. Note that INIT_LOCK and INIT_UNLOCK are > just macros basically expanding to pthread_mutex_lock and > pthread_mutex_unlock on a certain mutex (init_mtx). The comment > says that the newly created thread must do the unlock, but that > can't work (only the thread holding the lock can unlock a mutex). > I really doubt that this works as intended on any OS.
Funny enough, on linux the second lock in default mode really blocks the parent until the new thread does an unlock. Here's a fix that uses a condition variable to synchronize between the two threads: Index: Makefile =================================================================== RCS file: /cvs/ports/net/ettercap/Makefile,v retrieving revision 1.57 diff -u -p -r1.57 Makefile --- Makefile 10 Jan 2013 18:11:14 -0000 1.57 +++ Makefile 22 Jan 2013 13:18:51 -0000 @@ -1,8 +1,5 @@ # $OpenBSD: Makefile,v 1.57 2013/01/10 18:11:14 sthen Exp $ -BROKEN= broken mutex locking -# see http://marc.info/?l=openbsd-ports&m=135577437106908&w=2 - SHARED_ONLY= Yes COMMENT= multi-purpose sniffer/interceptor/logger Index: patches/patch-src_ec_threads_c =================================================================== RCS file: patches/patch-src_ec_threads_c diff -N patches/patch-src_ec_threads_c --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ patches/patch-src_ec_threads_c 22 Jan 2013 13:18:51 -0000 @@ -0,0 +1,91 @@ +$OpenBSD$ + +Use proper synchronization instead of unportable and dubious default +behaviour of mutxes on certain operating systems. + +--- src/ec_threads.c.orig Thu Jan 3 05:56:19 2013 ++++ src/ec_threads.c Tue Jan 22 14:15:56 2013 +@@ -42,8 +42,7 @@ static pthread_mutex_t threads_mutex = PTHREAD_MUTEX_I + #define THREADS_UNLOCK do{ pthread_mutex_unlock(&threads_mutex); } while(0) + + static pthread_mutex_t init_mtx = PTHREAD_MUTEX_INITIALIZER; +-#define INIT_LOCK do{ DEBUG_MSG("thread_init_lock"); pthread_mutex_lock(&init_mtx); } while(0) +-#define INIT_UNLOCK do{ DEBUG_MSG("thread_init_unlock"); pthread_mutex_unlock(&init_mtx); } while(0) ++static pthread_cond_t init_cond = PTHREAD_COND_INITIALIZER; + + /* protos... */ + +@@ -196,36 +195,38 @@ pthread_t ec_thread_new(char *name, char *desc, void * + pthread_t ec_thread_new_detached(char *name, char *desc, void *(*function)(void *), void *args, int detached) + { + pthread_t id; ++ int e; + + DEBUG_MSG("ec_thread_new -- %s detached %d", name, detached); + + /* + * lock the mutex to syncronize with the new thread. +- * the newly created thread will perform INIT_UNLOCK ++ * the newly created thread will call ec_thread_init(), + * so at the end of this function we are sure that the + * thread had be initialized + */ +- INIT_LOCK; ++ if ((e = pthread_mutex_lock(&init_mtx))) ++ FATAL_ERROR("locking init_mtx: %s", strerror(e)); + +- + if (detached == DETACHED_THREAD) { + pthread_attr_t attr; + pthread_attr_init(&attr); + pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); +- if (pthread_create(&id, &attr, function, args) != 0) +- ERROR_MSG("not enough resources to create a new thread in this process: %s", strerror(errno)); ++ if ((e = pthread_create(&id, &attr, function, args))) ++ FATAL_ERROR("not enough resources to create a new thread in this process: %s", strerror(e)); + }else { +- if (pthread_create(&id, NULL, function, args) != 0) +- ERROR_MSG("not enough resources to create a new thread in this process: %s", strerror(errno)); ++ if ((e = pthread_create(&id, NULL, function, args))) ++ FATAL_ERROR("not enough resources to create a new thread in this process: %s", strerror(e)); + } + + ec_thread_register_detached(id, name, desc, detached); + + DEBUG_MSG("ec_thread_new -- %lu created ", PTHREAD_ID(id)); + +- /* the new thread will unlock this */ +- INIT_LOCK; +- INIT_UNLOCK; ++ if ((e = pthread_cond_wait(&init_cond, &init_mtx))) ++ FATAL_ERROR("waiting on init_cond: %s", strerror(e)); ++ if ((e = pthread_mutex_unlock(&init_mtx))) ++ FATAL_ERROR("unlocking init_mtx: %s", strerror(e)); + + return id; + } +@@ -237,8 +238,12 @@ pthread_t ec_thread_new_detached(char *name, char *des + void ec_thread_init(void) + { + pthread_t id = pthread_self(); ++ int e; + + DEBUG_MSG("ec_thread_init -- %lu", PTHREAD_ID(id)); ++ ++ if ((e = pthread_mutex_lock(&init_mtx))) ++ FATAL_ERROR("locking init_mtx: %s", strerror(e)); + + /* + * allow a thread to be cancelled as soon as the +@@ -248,7 +253,10 @@ void ec_thread_init(void) + pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL); + + /* sync with the creator */ +- INIT_UNLOCK; ++ if ((e = pthread_cond_signal(&init_cond))) ++ FATAL_ERROR("raising init_cond: %s", strerror(e)); ++ if ((e = pthread_mutex_unlock(&init_mtx))) ++ FATAL_ERROR("unlocking init_mtx: %s", strerror(e)); + + DEBUG_MSG("ec_thread_init -- (%lu) ready and syncronized", PTHREAD_ID(id)); + }
