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 <jfriesse@xxxxxxxxxx>
> ---
> 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
discuss@xxxxxxxxxxxx
http://lists.corosync.org/mailman/listinfo/discuss
