[Home]

Summary:ASTERISK-22200: Some users of ast_cond_wait fail to properly check the predicate condition they guard
Reporter:Matt Jordan (mjordan)Labels:
Date Opened:2013-07-26 13:52:52Date Closed:
Priority:MajorRegression?No
Status:Open/NewComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:
Description:

From [pthread_cond_wait|http://linux.die.net/man/3/pthread_cond_wait]:

{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.
Comments: