[asterisk-bugs] [JIRA] (ASTERISK-22200) Some users of ast_cond_wait fail to properly check the predicate condition they guard
Rusty Newton (JIRA)
noreply at issues.asterisk.org
Fri Jul 26 17:11:03 CDT 2013
[ https://issues.asterisk.org/jira/browse/ASTERISK-22200?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Rusty Newton updated ASTERISK-22200:
------------------------------------
Status: Open (was: Triage)
> Some users of ast_cond_wait fail to properly check the predicate condition they guard
> -------------------------------------------------------------------------------------
>
> Key: ASTERISK-22200
> URL: https://issues.asterisk.org/jira/browse/ASTERISK-22200
> Project: Asterisk
> Issue Type: Bug
> Security Level: None
> Components: Core/General
> Reporter: Matt Jordan
>
> 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.
--
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
More information about the asterisk-bugs
mailing list