From 66928488c40c0d5cd341c3952bee00509949b72b Mon Sep 17 00:00:00 2001 From: Christian Thaeter Date: Thu, 17 Dec 2009 01:27:09 +0100 Subject: [PATCH] Fix: race conditions in RESOURCE_ENTER/STATE/LEAVE (API Change!) * RESOURCE_ENTER/STATE/LEAVE become Statement heads now and protect the user supplied statement with the resource_mutex. This makes resource transitions atomic. * moved all locking into the macros * refined example code --- Makefile.am | 2 +- doc/resourceexample.txt | 32 +++-- src/nobug.h | 228 +++++++++++++++++++++++----------- src/nobug_resources.c | 19 +-- tests/test_nobug_resources_threaded.c | 8 +- 5 files changed, 179 insertions(+), 110 deletions(-) diff --git a/Makefile.am b/Makefile.am index 4328a16..e9d9638 100644 --- a/Makefile.am +++ b/Makefile.am @@ -22,7 +22,7 @@ pc_DATA = nobug.pc nobugmt.pc ACLOCAL_AMFLAGS = -I m4 -AM_CFLAGS = -std=gnu99 -Wall -Wextra -Werror +AM_CFLAGS = -D_GNU_SOURCE -std=gnu99 -Wall -Wextra -Werror CLEANFILES = EXTRA_DIST = diff --git a/doc/resourceexample.txt b/doc/resourceexample.txt index e5aba5e..42a7f3c 100644 --- a/doc/resourceexample.txt +++ b/doc/resourceexample.txt @@ -13,18 +13,26 @@ void example() RESOURCE_ANNOUNCE(test, "mutex", "my_mutex", example, resource); // the following would be done in a different thread in a real program - RESOURCE_HANDLE(enter); // define a handle - - RESOURCE_ENTER(flag, resource, &enter, NOBUG_RESOURCE_WAITING, enter); - // announce that we want to use the resource - // &enter also suffices as unique pointer - pthread_mutex_lock (&my_mutex); // this might block - RESOURCE_STATE(test, NOBUG_RESOURCE_EXCLUSIVE, enter); // we got it, change the state - - // program does something useful here - - pthread_mutex_lock (&my_mutex); // and instantly unlock - RESOURCE_LEAVE(test, enter); // we don't need it anymore + // define a handle + RESOURCE_HANDLE(enter); + + // announce that we want to use the resource + // &enter also suffices as unique pointer, which is all we need as identifer here + RESOURCE_WAIT(flag, resource, &enter, enter) + { + // lock() might block + pthread_mutex_lock (&my_mutex); + // assume no errors, got it, change the state + RESOURCE_STATE(test, NOBUG_RESOURCE_EXCLUSIVE, enter); + } + + //////////////////////////////////////// + // program does something useful here // + //////////////////////////////////////// + + // we don't need it anymore + RESOURCE_LEAVE(test, enter) // << no semicolon + pthread_mutex_unlock (&my_mutex); // back in the main thread RESOURCE_FORGET(test, resource); // remove the resource from the public registry diff --git a/src/nobug.h b/src/nobug.h index cbb0a9e..546eb92 100644 --- a/src/nobug.h +++ b/src/nobug.h @@ -1067,6 +1067,7 @@ nobug_env_init_flag (&NOBUG_FLAG(name), NOBUG_LOG_TARGET, default) #define NOBUG_RESOURCE_ANNOUNCE_RAW(flag, type, name, ptr, handle) \ NOBUG_IF_ALPHA( do { \ + NOBUG_RESOURCE_LOCK; \ NOBUG_REQUIRE(!handle, "Announced resource handle not initialized"); \ NOBUG_IF(NOBUG_RESOURCE_LOGGING, \ NOBUG_LOG_(flag, NOBUG_RESOURCE_LOG_LEVEL, \ @@ -1076,6 +1077,7 @@ nobug_env_init_flag (&NOBUG_FLAG(name), NOBUG_LOG_TARGET, default) NOBUG_BASENAME(__FILE__ ":" NOBUG_STRINGIZE(__LINE__)))->hdr, \ "RESOURCE_ASSERT_ANNOUNCE", NOBUG_LOCATION_INFO, \ "%s: %s@%p %s", type, name, ptr, nobug_resource_error); \ + NOBUG_RESOURCE_UNLOCK; \ } while (0)) @@ -1099,6 +1101,7 @@ nobug_env_init_flag (&NOBUG_FLAG(name), NOBUG_LOG_TARGET, default) #define NOBUG_RESOURCE_FORGET_RAW(flag, handle) \ NOBUG_IF_ALPHA( do { \ + NOBUG_RESOURCE_LOCK; \ NOBUG_IF(NOBUG_RESOURCE_LOGGING, \ NOBUG_LOG_(flag, NOBUG_RESOURCE_LOG_LEVEL, NOBUG_LOCATION_INFO, \ "RESOURCE_FORGET", "%s: %s@%p", \ @@ -1112,12 +1115,13 @@ nobug_env_init_flag (&NOBUG_FLAG(name), NOBUG_LOG_TARGET, default) handle?((struct nobug_resource_record*)handle)->object_id:NULL, \ nobug_resource_error); \ handle = NULL; \ - } while (0)) + NOBUG_RESOURCE_UNLOCK; \ + } while (0)) /* //resourcemacros PARA RESOURCE_ENTER; RESOURCE_ENTER; claim a resource -//resourcemacros RESOURCE_ENTER(flag, announced, user, state, handle) +//resourcemacros RESOURCE_ENTER(flag, announced, user, state, handle){...} //resourcemacros //resourcemacros Acquire a resource. //resourcemacros @@ -1134,31 +1138,80 @@ nobug_env_init_flag (&NOBUG_FLAG(name), NOBUG_LOG_TARGET, default) //resourcemacros a `NOBUG_RESOURCE_HANDLE` which will be initialized to the //resourcemacros entering node //resourcemacros +//resourcemacros 'RESOURCE_ENTER()' acts like the head of a C loop statement, it ties to the following +//resourcemacros (block-) statement. Entering and the user defined following statement are atomic. +//resourcemacros This statement must not be left by break, return or any other kind of jump. NoBug does +//resourcemacros not assert this (for for Performance reasons). +//resourcemacros +//resourcemacros .How to use it +//resourcemacros [source,c] +//resourcemacros ---- +//resourcemacros // in the simplest form just terminate it with a semicolon, +//resourcemacros // no user statement is executed +//resourcemacros NOBUG_RESOURCE_ENTER(flag, resource, user, state, handle); +//resourcemacros +//resourcemacros // followed by a single statement +//resourcemacros NOBUG_RESOURCE_ENTER(flag, resource, user, state, handle) +//resourcemacros lock_my_resource(); +//resourcemacros +//resourcemacros // or a block statement +//resourcemacros NOBUG_RESOURCE_ENTER(flag, resource, user, state, handle) +//resourcemacros { +//resourcemacros lock_my_resource(); +//resourcemacros } +//resourcemacros ---- +//resourcemacros */ -#define NOBUG_RESOURCE_ENTER(flag, resource, user, state, handle) \ - NOBUG_IF_ALPHA( do { \ - NOBUG_REQUIRE(resource, "Announced resource handle not initialized"); \ - NOBUG_REQUIRE(!handle, "Resource handle already entered"); \ - NOBUG_IF(NOBUG_RESOURCE_LOGGING, \ - NOBUG_LOG_(&NOBUG_FLAG(flag), NOBUG_RESOURCE_LOG_LEVEL, \ - NOBUG_LOCATION_INFO, "RESOURCE_ENTER", \ - "%s: %s@%p: %s: %s", \ - resource?((struct nobug_resource_record*)resource)->type:"", \ - resource?resource->name:"", \ - resource?((struct nobug_resource_record*)resource)->object_id:NULL, \ - user, \ - nobug_resource_states[state]);) \ - NOBUG_RESOURCE_ASSERT(handle = &nobug_resource_enter ((struct nobug_resource_record*)resource, \ - user, state, \ - NOBUG_BASENAME(__FILE__ ":" NOBUG_STRINGIZE(__LINE__)))->hdr, \ - "RESOURCE_ASSERT_ENTER", NOBUG_LOCATION_INFO, \ - "%s: %s@%p: %s: %s: %s", \ - resource?((struct nobug_resource_record*)resource)->type:"", \ - resource?resource->name:"", \ - resource?((struct nobug_resource_record*)resource)->object_id:NULL, \ - user, nobug_resource_states[state], \ - nobug_resource_error); \ - } while (0)) +#define NOBUG_RESOURCE_ENTER(flag, resource, user, state, handle) \ + for ( \ + int nobug_locked_ = ({ \ + NOBUG_REQUIRE(resource, "Announced resource handle not initialized"); \ + NOBUG_REQUIRE(!handle, "Resource handle already entered"); \ + NOBUG_RESOURCE_LOCK; \ + NOBUG_IF(NOBUG_RESOURCE_LOGGING, \ + NOBUG_LOG_(&NOBUG_FLAG(flag), NOBUG_RESOURCE_LOG_LEVEL, \ + NOBUG_LOCATION_INFO, "RESOURCE_ENTER", \ + "%s: %s@%p: %s: %s", \ + resource?((struct nobug_resource_record*)resource)->type:"", \ + resource?resource->name:"", \ + resource?((struct nobug_resource_record*)resource)->object_id:NULL, \ + user, \ + nobug_resource_states[state]);) \ + NOBUG_RESOURCE_ASSERT(handle = \ + &nobug_resource_enter ((struct nobug_resource_record*)resource, \ + user, state, \ + NOBUG_BASENAME(__FILE__ ":" NOBUG_STRINGIZE(__LINE__)))->hdr, \ + "RESOURCE_ASSERT_ENTER", NOBUG_LOCATION_INFO, \ + "%s: %s@%p: %s: %s: %s", \ + resource?((struct nobug_resource_record*)resource)->type:"", \ + resource?resource->name:"", \ + resource?((struct nobug_resource_record*)resource)->object_id:NULL, \ + user, nobug_resource_states[state], \ + nobug_resource_error); \ + 1; \ + }); \ + nobug_locked_; \ + NOBUG_RESOURCE_UNLOCK, nobug_locked_ = 0) + + +//resourcemacros PARA RESOURCE_WAIT; RESOURCE_WAIT; wait that a resource becomes available +//resourcemacros RESOURCE_WAIT(flag, resource, user, handle) +//resourcemacros +//resourcemacros This is just an alias for RESOURCE_ENTER(flag, resource, user, NOBUG_RESOURCE_WAITING, handle) +//resourcemacros +//resourcemacros .How to use it +//resourcemacros [source,c] +//resourcemacros ---- +//resourcemacros RESOURCE_WAIT(flag, resource, user, handle) +//resourcemacros { +//resourcemacros if (lock_my_resource() == ERROR) +//resourcemacros NOBUG_RESOURCE_LEAVE(flag, handle); +//resourcemacros else +//resourcemacros RESOURCE_STATE(flag, NOBUG_RESOURCE_EXCLUSIVE, handle); +//resourcemacros } +//resourcemacros ---- +#define NOBUG_RESOURCE_WAIT(flag, resource, user, handle) \ + NOBUG_RESOURCE_ENTER(flag, resource, user, NOBUG_RESOURCE_WAITING, handle) /* @@ -1175,35 +1228,47 @@ nobug_env_init_flag (&NOBUG_FLAG(name), NOBUG_LOG_TARGET, default) //resourcemacros `entered`:: //resourcemacros the handle set by `RESOURCE_ENTER` //resourcemacros +//resourcemacros 'RESOURCE_STATE()' acts like the head of a C loop statement, it ties to the following +//resourcemacros (block-) statement. Entering and the user defined following statement are atomic. +//resourcemacros This statement must not be left by break, return or any other kind of jump. NoBug does +//resourcemacros not assert this (for for Performance reasons). +//resourcemacros */ #define NOBUG_RESOURCE_STATE(flag, state, entered) \ NOBUG_RESOURCE_STATE_RAW(&NOBUG_FLAG(flag), state, entered) -#define NOBUG_RESOURCE_STATE_RAW(flag, nstate, entered) \ - NOBUG_IF_ALPHA( do { \ - NOBUG_IF(NOBUG_RESOURCE_LOGGING, \ - NOBUG_LOG_(flag, NOBUG_RESOURCE_LOG_LEVEL, NOBUG_LOCATION_INFO, \ - "RESOURCE_STATE", "%s: %s@%p: %s: %s->%s", \ - entered?((struct nobug_resource_user*)entered)->current->resource->type:"", \ - entered?((struct nobug_resource_user*)entered)->current->resource->hdr.name:"", \ - entered?((struct nobug_resource_user*)entered)->current->resource->object_id:"", \ - entered?entered->name:"", \ - nobug_resource_states[entered?((struct nobug_resource_user*)entered)->state \ - :NOBUG_RESOURCE_INVALID], \ - nobug_resource_states[nstate]); \ - ) \ - NOBUG_RESOURCE_ASSERT(nobug_resource_state ((struct nobug_resource_user*)entered, nstate), \ - "RESOURCE_ASSERT_STATE", NOBUG_LOCATION_INFO, \ - "%s: %s@%p: %s: %s->%s: %s", \ - entered?((struct nobug_resource_user*)entered)->current->resource->type:"", \ - entered?((struct nobug_resource_user*)entered)->current->resource->hdr.name:"", \ - entered?((struct nobug_resource_user*)entered)->current->resource->object_id:"", \ - entered?entered->name:"", \ - nobug_resource_states[entered?((struct nobug_resource_user*)entered)->state \ - :NOBUG_RESOURCE_INVALID], \ - nobug_resource_states[nstate], \ - nobug_resource_error); \ - } while (0)) + +#define NOBUG_RESOURCE_STATE_RAW(flag, nstate, entered) \ + NOBUG_IF_ALPHA( \ + for ( \ + int nobug_locked_ = ({ \ + NOBUG_RESOURCE_LOCK; \ + NOBUG_IF(NOBUG_RESOURCE_LOGGING, \ + NOBUG_LOG_(flag, NOBUG_RESOURCE_LOG_LEVEL, NOBUG_LOCATION_INFO, \ + "RESOURCE_STATE", "%s: %s@%p: %s: %s->%s", \ + entered?((struct nobug_resource_user*)entered)->current->resource->type:"", \ + entered?((struct nobug_resource_user*)entered)->current->resource->hdr.name:"", \ + entered?((struct nobug_resource_user*)entered)->current->resource->object_id:"", \ + entered?entered->name:"", \ + nobug_resource_states[entered?((struct nobug_resource_user*)entered)->state \ + :NOBUG_RESOURCE_INVALID], \ + nobug_resource_states[nstate]); \ + ) \ + NOBUG_RESOURCE_ASSERT(nobug_resource_state ((struct nobug_resource_user*)entered, nstate), \ + "RESOURCE_ASSERT_STATE", NOBUG_LOCATION_INFO, \ + "%s: %s@%p: %s: %s->%s: %s", \ + entered?((struct nobug_resource_user*)entered)->current->resource->type:"", \ + entered?((struct nobug_resource_user*)entered)->current->resource->hdr.name:"", \ + entered?((struct nobug_resource_user*)entered)->current->resource->object_id:"", \ + entered?entered->name:"", \ + nobug_resource_states[entered?((struct nobug_resource_user*)entered)->state \ + :NOBUG_RESOURCE_INVALID], \ + nobug_resource_states[nstate], \ + nobug_resource_error); \ + 1; \ + }); \ + nobug_locked_; \ + NOBUG_RESOURCE_UNLOCK, nobug_locked_ = 0)) /* @@ -1217,34 +1282,44 @@ nobug_env_init_flag (&NOBUG_FLAG(name), NOBUG_LOG_TARGET, default) //resourcemacros `handle`:: //resourcemacros the handle you got while entering the resource //resourcemacros +//resourcemacros 'RESOURCE_LEAVE()' acts like the head of a C loop statement, it ties to the following +//resourcemacros (block-) statement. Entering and the user defined following statement are atomic. +//resourcemacros This statement must not be left by break, return or any other kind of jump. NoBug does +//resourcemacros not assert this (for for Performance reasons). +//resourcemacros */ #define NOBUG_RESOURCE_LEAVE(flag, handle) \ NOBUG_RESOURCE_LEAVE_RAW(&NOBUG_FLAG(flag), handle) -#define NOBUG_RESOURCE_LEAVE_RAW(flag, handle) \ - NOBUG_IF_ALPHA( do { \ - NOBUG_IF(NOBUG_RESOURCE_LOGGING, \ - NOBUG_LOG_(flag, NOBUG_RESOURCE_LOG_LEVEL, NOBUG_LOCATION_INFO, \ - "RESOURCE_LEAVE", "%s: %s@%p: %s: %s", \ - handle?((struct nobug_resource_user*)handle)->current->resource->type:"", \ - handle?((struct nobug_resource_user*)handle)->current->resource->hdr.name:"", \ - handle?((struct nobug_resource_user*)handle)->current->resource->object_id:"", \ - handle?handle->name:"", \ - nobug_resource_states[handle?((struct nobug_resource_user*)handle)->state \ - :NOBUG_RESOURCE_INVALID]); \ - ) \ - NOBUG_RESOURCE_ASSERT(nobug_resource_leave ((struct nobug_resource_user*)handle), \ - "RESOURCE_ASSERT_LEAVE", NOBUG_LOCATION_INFO, "%s: %s@%p: %s: %s: %s", \ - handle?((struct nobug_resource_user*)handle)->current->resource->type:"", \ - handle?((struct nobug_resource_user*)handle)->current->resource->hdr.name:"", \ - handle?((struct nobug_resource_user*)handle)->current->resource->object_id:"", \ - handle?handle->name:"", \ - nobug_resource_states[handle?((struct nobug_resource_user*)handle)->state \ - :NOBUG_RESOURCE_INVALID], \ - nobug_resource_error); \ - handle = NULL; \ - } while (0)) - +#define NOBUG_RESOURCE_LEAVE_RAW(flag, handle) \ + NOBUG_IF_ALPHA( \ + for ( \ + int nobug_locked_ = (NOBUG_RESOURCE_LOCK, 1); \ + nobug_locked_; \ + ({ \ + NOBUG_IF(NOBUG_RESOURCE_LOGGING, \ + NOBUG_LOG_(flag, NOBUG_RESOURCE_LOG_LEVEL, NOBUG_LOCATION_INFO, \ + "RESOURCE_LEAVE", "%s: %s@%p: %s: %s", \ + handle?((struct nobug_resource_user*)handle)->current->resource->type:"", \ + handle?((struct nobug_resource_user*)handle)->current->resource->hdr.name:"", \ + handle?((struct nobug_resource_user*)handle)->current->resource->object_id:"", \ + handle?handle->name:"", \ + nobug_resource_states[handle?((struct nobug_resource_user*)handle)->state \ + :NOBUG_RESOURCE_INVALID]); \ + ) \ + NOBUG_RESOURCE_ASSERT(nobug_resource_leave ((struct nobug_resource_user*)handle), \ + "RESOURCE_ASSERT_LEAVE", NOBUG_LOCATION_INFO, "%s: %s@%p: %s: %s: %s", \ + handle?((struct nobug_resource_user*)handle)->current->resource->type:"", \ + handle?((struct nobug_resource_user*)handle)->current->resource->hdr.name:"", \ + handle?((struct nobug_resource_user*)handle)->current->resource->object_id:"", \ + handle?handle->name:"", \ + nobug_resource_states[handle?((struct nobug_resource_user*)handle)->state \ + :NOBUG_RESOURCE_INVALID], \ + nobug_resource_error); \ + handle = NULL; \ + nobug_locked_ = 0; \ + NOBUG_RESOURCE_UNLOCK; \ + }))) /* assertion which dumps all resources */ @@ -1733,6 +1808,9 @@ VALGRIND_PRINTF_BACKTRACE("BACKTRACE: %s@%d %s", \ #ifndef RESOURCE_ENTER #define RESOURCE_ENTER NOBUG_RESOURCE_ENTER #endif +#ifndef RESOURCE_WAIT +#define RESOURCE_WAIT NOBUG_RESOURCE_WAIT +#endif #ifndef RESOURCE_STATE #define RESOURCE_STATE NOBUG_RESOURCE_STATE #endif diff --git a/src/nobug_resources.c b/src/nobug_resources.c index 9003595..fe0132e 100644 --- a/src/nobug_resources.c +++ b/src/nobug_resources.c @@ -54,12 +54,7 @@ #endif #if NOBUG_USE_PTHREAD -pthread_mutex_t nobug_resource_mutex = PTHREAD_MUTEX_INITIALIZER; -#define NOBUG_RESOURCE_LOCK pthread_mutex_lock (&nobug_resource_mutex) -#define NOBUG_RESOURCE_UNLOCK pthread_mutex_unlock (&nobug_resource_mutex) -#else -#define NOBUG_RESOURCE_LOCK -#define NOBUG_RESOURCE_UNLOCK +pthread_mutex_t nobug_resource_mutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP; #endif #define nobug_resourcestates \ @@ -214,7 +209,6 @@ compare_resource_records (const_LList av, const_LList bv, void* unused) struct nobug_resource_record* nobug_resource_announce (const char* type, const char* name, const void* object_id, const char* extra) { - NOBUG_RESOURCE_LOCK; struct nobug_resource_record* node = mpool_alloc (&nobug_resource_record_pool); if (!node) @@ -242,7 +236,6 @@ nobug_resource_announce (const char* type, const char* name, const void* object_ llist_insert_head (&nobug_resource_registry, llist_init (&node->hdr.node)); - NOBUG_RESOURCE_UNLOCK; return node; } @@ -250,7 +243,6 @@ nobug_resource_announce (const char* type, const char* name, const void* object_ int nobug_resource_forget (struct nobug_resource_record* self) { - NOBUG_RESOURCE_LOCK; if (!llist_find (&nobug_resource_registry, &self->hdr.node, compare_resource_records, NULL)) { nobug_resource_error = "not registered"; @@ -266,7 +258,6 @@ nobug_resource_forget (struct nobug_resource_record* self) nobug_resource_record_dtor (self); mpool_free (&nobug_resource_record_pool, self); - NOBUG_RESOURCE_UNLOCK; return 1; } @@ -322,8 +313,6 @@ nobug_resource_enter (struct nobug_resource_record* resource, enum nobug_resource_state state, const char* extra) { - NOBUG_RESOURCE_LOCK; - if (!resource) { nobug_resource_error = "no resource"; @@ -551,7 +540,6 @@ nobug_resource_enter (struct nobug_resource_record* resource, llist_insert_tail (&tls->res_stack, llist_init (&new_user->res_stack)); #endif - NOBUG_RESOURCE_UNLOCK; return new_user; } @@ -570,8 +558,6 @@ nobug_resource_node_parent_cmpfn (const_LList a, const_LList b, void* extra) int nobug_resource_leave (struct nobug_resource_user* user) { - NOBUG_RESOURCE_LOCK; - if (!user) { nobug_resource_error = "no handle"; @@ -691,7 +677,6 @@ nobug_resource_leave (struct nobug_resource_user* user) mpool_free (&nobug_resource_user_pool, user); } - NOBUG_RESOURCE_UNLOCK; return 1; } @@ -700,7 +685,6 @@ int nobug_resource_state (struct nobug_resource_user* user, enum nobug_resource_state state) { - NOBUG_RESOURCE_LOCK; if (!user) { nobug_resource_error = "no user handle"; @@ -758,7 +742,6 @@ nobug_resource_state (struct nobug_resource_user* user, if (nobug_resource_error) return 0; - NOBUG_RESOURCE_UNLOCK; return 1; } diff --git a/tests/test_nobug_resources_threaded.c b/tests/test_nobug_resources_threaded.c index 4e3056f..08e022a 100644 --- a/tests/test_nobug_resources_threaded.c +++ b/tests/test_nobug_resources_threaded.c @@ -14,14 +14,14 @@ void* threadfn(void* nop) if (threadenter != NOBUG_RESOURCE_WAITING) { ECHO ("enter via wait"); - RESOURCE_ENTER(NOBUG_ON, t, "thread", NOBUG_RESOURCE_WAITING, e); - NOBUG_RESOURCE_STATE(NOBUG_ON, threadenter, e); + RESOURCE_ENTER(NOBUG_ON, t, "thread", NOBUG_RESOURCE_WAITING, e) + NOBUG_RESOURCE_STATE(NOBUG_ON, threadenter, e); RESOURCE_LEAVE(NOBUG_ON, e); } ECHO ("enter direct"); - RESOURCE_ENTER(NOBUG_ON, t, "thread", threadenter, e); - RESOURCE_LEAVE(NOBUG_ON, e); + RESOURCE_ENTER(NOBUG_ON, t, "thread", threadenter, e) + RESOURCE_LEAVE(NOBUG_ON, e); ECHO ("thread finished"); return nop; -- 2.11.4.GIT