No subject

{quote}
When using condition variables there is always a Boolean predicate involving shared variables associated with each condition wait that is true if the thread should proceed. Spurious wakeups from the pthread_cond_timedwait() or pthread_cond_wait() functions may occur. Since the return from pthread_cond_timedwait() or pthread_cond_wait() does not imply anything about the value of this predicate, the predicate should be re-evaluated upon such return.
{quote}

There are a number of instances where the predicate that is guarded by a call to {{ast_cond_wait}} is not properly checked when {{ast_cond_wait}} returns. Spurious wakeups are possible and could cause an issue in some of these cases.

The most notable of these are listed below. Note that those that are testing thread initialization are far less serious, as (1) if a wake up occurs sooner there is still a chance that by the time code that requires that thread to be running is executed the thread will be running, (2) the condition wait variable has a very short lifetime, and (3) the time for the condition wait variable to be signaled is extremely small.

h3. Things missing a predicate check that probably should have one

*/home/mjordan/projects/11/channels/chan_iax2.c:*
{noformat}
 ....
 11932  			}
 11933  		} else {
 11934: 			ast_cond_wait(&thread->cond, &thread->lock);
 11935  		}
 11936  
 .....
{noformat}

This doesn't properly check the put_into_idle predicate. It really should.

*/home/mjordan/projects/11/main/bridging.c:*
{noformat}
  759  	} else {
  760  		ast_debug(10, "Going into a multithreaded signal wait for bridge channel %p of bridge %p\n", bridge_channel, bridge_channel->bridge);
  761: 		ast_cond_wait(&bridge_channel->cond, ao2_object_get_lockaddr(bridge_channel));
  762  		ao2_unlock(bridge_channel);
  763  	}
  ...
  779  	if (bridge_channel->state == AST_BRIDGE_CHANNEL_STATE_WAIT) {
  780  		ast_debug(1, "Going into a single threaded signal wait for bridge channel %p of bridge %p\n", bridge_channel, bridge_channel->bridge);
  781: 		ast_cond_wait(&bridge_channel->cond, ao2_object_get_lockaddr(bridge_channel));
  782  	}
  783  	ao2_unlock(bridge_channel);
{noformat}

Neither of these are properly check their predicate condition, and probably should do so.

*/home/mjordan/projects/11/res/res_config_sqlite3.c:*
{noformat}
  286  	for (;;) {
  287  		if (!db->wakeup) {
  288: 			ast_cond_wait(&db->cond, ao2_object_get_lockaddr(db));
  289  		}
  290  		db->wakeup = 0;
{noformat}

This should be fixed to properly check db->wakeup.

h3. Thread Initialization

*/home/mjordan/projects/11/apps/app_meetme.c:*

{noformat}
 6048  			ast_mutex_lock(&cond_lock);
 6049  			ast_pthread_create_detached_background(&dont_care, NULL, run_station, &args);
 6050: 			ast_cond_wait(&cond, &cond_lock);
 6051  			ast_mutex_unlock(&cond_lock);
 6052  			ast_mutex_destroy(&cond_lock);
 ....
{noformat}

This is missing a predicate. However, this is merely attempting to wait until the thread is spawned, so the likelihood of a critical error occurring because of this is slight. It could be improved to properly test a predicate that the {{run_station}} thread has started.

{noformat}
 6868  		ast_mutex_lock(&cond_lock);
 6869  		ast_pthread_create_detached_background(&dont_care, NULL, dial_trunk, &args);
 6870: 		ast_cond_wait(&cond, &cond_lock);
 6871  		ast_mutex_unlock(&cond_lock);
 6872  		ast_mutex_destroy(&cond_lock);
{noformat}

This is missing a predicate, similar to the previous SLA use of ast_cond_wait. Again, the impact of this one is less than in other situations where the expected wait time for the ast_cond_wait is significantly longer.


*/home/mjordan/projects/11/apps/app_mixmonitor.c:*
{noformat}
  706  	mixmonitor_ds_close_fs(mixmonitor->mixmonitor_ds);
  707  	if (!mixmonitor->mixmonitor_ds->destruction_ok) {
  708: 		ast_cond_wait(&mixmonitor->mixmonitor_ds->destruction_condition, &mixmonitor->mixmonitor_ds->lock);
  709  	}
  710  	ast_mutex_unlock(&mixmonitor->mixmonitor_ds->lock);
{noformat}

This is missing a check on the predicate as well. Since both the datastore and mixmonitor are being disposed of, the risk is relatively low that this would cause a problem.

*/home/mjordan/projects/11/channels/chan_iax2.c:*
{noformat}
 1501  
 1502  	/* Wait for the thread to be ready before returning it to the caller */
 1503: 	ast_cond_wait(&thread->init_cond, &thread->init_lock);
 1504  
 1505  	/* Done with init_lock */
{noformat}

Similar to the SLA code in app_meetme, this does not check the predicate condition. It probably should, but again, we are merely waiting for a thread to start, so the risk of this causing a problem is low.

{noformat}
 12455  			}
 12456  			/* Wait for the thread to be ready */
 12457: 			ast_cond_wait(&thread->init_cond, &thread->init_lock);
 12458  
 12459  			/* Done with init_lock */
{noformat}

Similar to the SLA code in app_meetme, this does not check the predicate condition. It probably should, but again, we are merely waiting for a thread to start, so the risk of this causing a problem is low.


*/home/mjordan/projects/11/main/stdtime/localtime.c:*
{noformat}
  349  		if (!(ast_pthread_create_background(&inotify_thread, NULL, inotify_daemon, NULL))) {
  350  			/* Give the thread a chance to initialize */
  351: 			ast_cond_wait(&initialization, &initialization_lock);
  352  		} else {
  353  			fprintf(stderr, "Unable to start notification thread\n");
  ...
  467  		if (!(ast_pthread_create_background(&inotify_thread, NULL, kqueue_daemon, NULL))) {
  468  			/* Give the thread a chance to initialize */
  469: 			ast_cond_wait(&initialization, &initialization_lock);
  470  		}
  471  		ast_mutex_unlock(&initialization_lock);
  ...
  612  		if (!(ast_pthread_create_background(&inotify_thread, NULL, notify_daemon, NULL))) {
  613  			/* Give the thread a chance to initialize */
  614: 			ast_cond_wait(&initialization, &initialization_lock);
  615  		}
  616  		ast_mutex_unlock(&initialization_lock);
  ...
  632  #endif
  633  		pthread_kill(inotify_thread, SIGURG);
  634: 		ast_cond_wait(&initialization, &(&zonelist)->lock);
  635  #ifdef TEST_FRAMEWORK
  636  		test = NULL;
{noformat}

Much like others, these are simply waiting on thread initialization and have a low chance of error.


--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.asterisk.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira