ACK Fabio
On 3/18/2013 10:23 AM, Jan Friesse wrote: > pending_semops variable can be changed in two threads. One is actual IPC > connection and second is coropoll. It's really scholar example of race > (one thread doing i++, second doing i--). If socket is full, it can > happen that IPC will increase value and coropoll will decrease, > resulting in unpredictable value. This means, that client IPC can be > informed about more messages then really available, resulting > in reading of garbage messages in library dispatch function. > > Solution is to properly lock variable. > > Signed-off-by: Jan Friesse <[email protected]> > --- > exec/coroipcs.c | 27 +++++++++++++++------------ > 1 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/exec/coroipcs.c b/exec/coroipcs.c > index 36d18a4..a5c3248 100644 > --- a/exec/coroipcs.c > +++ b/exec/coroipcs.c > @@ -140,6 +140,7 @@ struct conn_info { > key_t semkey; > #endif > unsigned int pending_semops; > + pthread_mutex_t pending_semops_mutex; > pthread_mutex_t mutex; > struct control_buffer *control_buffer; > char *request_buffer; > @@ -1265,7 +1266,9 @@ static void msg_send (void *conn, const struct iovec > *iov, unsigned int iov_len, > buf = list_empty (&conn_info->outq_head); > res = send (conn_info->fd, &buf, 1, MSG_NOSIGNAL); > if (res != 1) { > + pthread_mutex_lock(&conn_info->pending_semops_mutex); > conn_info->pending_semops += 1; > + pthread_mutex_unlock(&conn_info->pending_semops_mutex); > if (conn_info->poll_state == POLL_STATE_IN) { > conn_info->poll_state = POLL_STATE_INOUT; > api->poll_dispatch_modify (conn_info->fd, > @@ -1648,6 +1651,7 @@ int coroipcs_handler_dispatch ( > } > > pthread_mutex_init (&conn_info->mutex, NULL); > + pthread_mutex_init (&conn_info->pending_semops_mutex, NULL); > req_setup = (mar_req_setup_t *)conn_info->setup_msg; > /* > * Is the service registered ? > @@ -1784,22 +1788,21 @@ int coroipcs_handler_dispatch ( > } > > if (revent & POLLOUT) { > - int psop = conn_info->pending_semops; > - int i; > - > - assert (psop != 0); > - for (i = 0; i < psop; i++) { > - res = send (conn_info->fd, &buf, 1, MSG_NOSIGNAL); > - if (res != 1) { > - return (0); > - } else { > - conn_info->pending_semops -= 1; > - } > + pthread_mutex_lock(&conn_info->pending_semops_mutex); > + > + assert(conn_info->pending_semops != 0); > + > + while (conn_info->pending_semops > 0 && > + ((res = send (conn_info->fd, &buf, 1, MSG_NOSIGNAL)) == > 1)) { > + > + conn_info->pending_semops -= 1; > } > - if (conn_info->poll_state == POLL_STATE_INOUT) { > + > + if (conn_info->pending_semops == 0 && conn_info->poll_state == > POLL_STATE_INOUT) { > conn_info->poll_state = POLL_STATE_IN; > api->poll_dispatch_modify (conn_info->fd, > POLLIN|POLLNVAL); > } > + pthread_mutex_unlock(&conn_info->pending_semops_mutex); > } > > return (0); > _______________________________________________ discuss mailing list [email protected] http://lists.corosync.org/mailman/listinfo/discuss
