Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Race condition in wtp #1274

Merged
merged 1 commit into from
Nov 24, 2016
Merged

Race condition in wtp #1274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions runtime/wtp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ static rsRetVal NotImplementedDummy(void) { return RS_RET_NOT_IMPLEMENTED; }
*/
BEGINobjConstruct(wtp) /* be sure to specify the object type also in END macro! */
pthread_mutex_init(&pThis->mutWtp, NULL);
pthread_cond_init(&pThis->condThrdInitDone, NULL);
pthread_cond_init(&pThis->condThrdTrm, NULL);
pthread_attr_init(&pThis->attrThrd);
/* Set thread scheduling policy to default */
Expand Down Expand Up @@ -156,6 +157,7 @@ CODESTARTobjDestruct(wtp)

/* actual destruction */
pthread_cond_destroy(&pThis->condThrdTrm);
pthread_cond_destroy(&pThis->condThrdInitDone);
pthread_mutex_destroy(&pThis->mutWtp);
pthread_attr_destroy(&pThis->attrThrd);
DESTROY_ATOMIC_HELPER_MUT(pThis->mutCurNumWrkThrd);
Expand Down Expand Up @@ -386,6 +388,12 @@ wtpWorker(void *arg) /* the arg is actually a wti object, even though we are in
# endif

pthread_cleanup_push(wtpWrkrExecCancelCleanup, pWti);

/* let the parent know we're done with initialization */
d_pthread_mutex_lock(&pThis->mutWtp);
pthread_cond_broadcast(&pThis->condThrdInitDone);
d_pthread_mutex_unlock(&pThis->mutWtp);

wtiWorker(pWti);
pthread_cleanup_pop(0);
wtpWrkrExecCleanup(pWti);
Expand Down Expand Up @@ -437,6 +445,11 @@ wtpStartWrkr(wtp_t *pThis)
wtpGetDbgHdr(pThis), iState,
ATOMIC_FETCH_32BIT(&pThis->iCurNumWrkThrd, &pThis->mutCurNumWrkThrd));

/* wait for the new thread to initialize its signal mask and
* cancelation cleanup handler before proceeding
*/
d_pthread_cond_wait(&pThis->condThrdInitDone, &pThis->mutWtp);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I realize that I am a bit late to the game here, but the condition wait call does not appear to be in a loop. Perhaps I am missing the loop? POSIX allows for spurious wake ups to occur for callers of pbench_cond_wait(), so the recommended sequence/hand shake is to:

WAITER:
lock
   loop
      check predicate (memory location value)
      if not expected state of predicate
        then call condition wait (which unlocks mutex and reacquires on wakeup)
        else exit loop
unlock

SIGNALER:
lock
    change wait predicate
    signal or broadcast
unlock

This code does not appear to do that, so it is likely wide open to race conditions.


finalize_it:
d_pthread_mutex_unlock(&pThis->mutWtp);
RETiRet;
Expand Down
1 change: 1 addition & 0 deletions runtime/wtp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ struct wtp_s {
rsRetVal (*pConsumer)(void *); /* user-supplied consumer function for dewtpd messages */
/* synchronization variables */
pthread_mutex_t mutWtp; /* mutex for the wtp's thread management */
pthread_cond_t condThrdInitDone; /* signalled when a new thread is ready for work */
pthread_cond_t condThrdTrm;/* signalled when threads terminate */
/* end sync variables */
/* user objects */
Expand Down