From e94de6359479e04af984863407a5aea8e12be022 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Sep 14 2017 15:57:12 +0000 Subject: [PATCH 1/4] Simplify setting NONBLOCK on socket. Signed-off-by: Alexander Scheel --- diff --git a/proxy/src/client/gpm_common.c b/proxy/src/client/gpm_common.c index 6103d71..a79371c 100644 --- a/proxy/src/client/gpm_common.c +++ b/proxy/src/client/gpm_common.c @@ -80,7 +80,6 @@ static int gpm_open_socket(struct gpm_ctx *gpmctx) struct sockaddr_un addr = {0}; char name[PATH_MAX]; int ret; - unsigned flags; int fd = -1; ret = get_pipe_name(name); @@ -92,24 +91,12 @@ static int gpm_open_socket(struct gpm_ctx *gpmctx) strncpy(addr.sun_path, name, sizeof(addr.sun_path)-1); addr.sun_path[sizeof(addr.sun_path)-1] = '\0'; - fd = socket(AF_UNIX, SOCK_STREAM, 0); + fd = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0); if (fd == -1) { ret = errno; goto done; } - ret = fcntl(fd, F_GETFD, &flags); - if (ret != 0) { - ret = errno; - goto done; - } - - ret = fcntl(fd, F_SETFD, flags | O_NONBLOCK); - if (ret != 0) { - ret = errno; - goto done; - } - ret = connect(fd, (struct sockaddr *)&addr, sizeof(addr)); if (ret == -1) { ret = errno; From f6eb4493642d7226df9da379bd1483a63f7d1af8 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Sep 14 2017 19:22:28 +0000 Subject: [PATCH 2/4] Fix handling of non-EPOLLIN/EPOLLOUT events Signed-off-by: Alexander Scheel --- diff --git a/proxy/src/client/gpm_common.c b/proxy/src/client/gpm_common.c index a79371c..6d93a55 100644 --- a/proxy/src/client/gpm_common.c +++ b/proxy/src/client/gpm_common.c @@ -287,26 +287,47 @@ static int gpm_epoll_wait(struct gpm_ctx *gpmctx, uint32_t event_flags) gpm_epoll_close(gpmctx); } else if (epoll_ret == 1 && events[0].data.fd == gpmctx->timerfd) { /* Got an event which is only our timer */ - ret = read(gpmctx->timerfd, &timer_read, sizeof(uint64_t)); - if (ret == -1 && errno != EAGAIN && errno != EWOULDBLOCK) { - /* In the case when reading from the timer failed, don't hide the - * timer error behind ETIMEDOUT such that it isn't retried */ - ret = errno; + if ((events[0].events & EPOLLIN) == 0) { + /* We got an event which was not EPOLLIN; assume this is an error, + * and exit with EBADF: epoll_wait said timerfd had an event, + * but that event is not an EPOLIN event. */ + ret = EBADF; } else { - /* If ret == 0, then we definitely timed out. Else, if ret == -1 - * and errno == EAGAIN or errno == EWOULDBLOCK, we're in a weird - * edge case where epoll thinks the timer can be read, but it - * is blocking more; treat it like a TIMEOUT and retry, as - * nothing around us would handle EAGAIN from timer and retry - * it. */ - ret = ETIMEDOUT; + ret = read(gpmctx->timerfd, &timer_read, sizeof(uint64_t)); + if (ret == -1 && errno != EAGAIN && errno != EWOULDBLOCK) { + /* In the case when reading from the timer failed, don't hide the + * timer error behind ETIMEDOUT such that it isn't retried */ + ret = errno; + } else { + /* If ret == 0, then we definitely timed out. Else, if ret == -1 + * and errno == EAGAIN or errno == EWOULDBLOCK, we're in a weird + * edge case where epoll thinks the timer can be read, but it + * is blocking more; treat it like a TIMEOUT and retry, as + * nothing around us would handle EAGAIN from timer and retry + * it. */ + ret = ETIMEDOUT; + } } gpm_epoll_close(gpmctx); } else { /* If ret == 2, then we ignore the timerfd; that way if the next * operation cannot be performed immediately, we timeout and retry. - * If ret == 1 and data.fd == gpmctx->fd, return 0. */ - ret = 0; + * Always check the returned event of the socket fd. */ + int fd_index = 0; + if (epoll_ret == 2 && events[fd_index].data.fd != gpmctx->fd) { + fd_index = 1; + } + + if ((events[fd_index].events & event_flags) == 0) { + /* We cannot call EPOLLIN/EPOLLOUT at this time; assume that this + * is a fatal error; return with EBADFD to distinguish from + * EBADF in timer_fd case. */ + ret = EBADFD; + gpm_epoll_close(gpmctx); + } else { + /* We definintely got a EPOLLIN/EPOLLOUT event; return success. */ + ret = 0; + } } epoll_ret = epoll_ctl(gpmctx->epollfd, EPOLL_CTL_DEL, gpmctx->fd, NULL); From 63fe12a789136d69261375a84b5f9e2091e3768c Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Sep 14 2017 19:24:04 +0000 Subject: [PATCH 3/4] Fix error handling in gpm_send_buffer/gpm_recv_buffer. Signed-off-by: Alexander Scheel --- diff --git a/proxy/src/client/gpm_common.c b/proxy/src/client/gpm_common.c index 6d93a55..ee6306a 100644 --- a/proxy/src/client/gpm_common.c +++ b/proxy/src/client/gpm_common.c @@ -419,10 +419,7 @@ static int gpm_send_buffer(struct gpm_ctx *gpmctx, ret = 0; done: - if (ret) { - /* on errors we can only close the fd and return */ - gpm_close_socket(gpmctx); - } + /* we only need to return as gpm_retry_socket closes the socket */ return ret; } @@ -492,9 +489,10 @@ static int gpm_recv_buffer(struct gpm_ctx *gpmctx, done: if (ret) { - /* on errors we can only close the fd and return */ - gpm_close_socket(gpmctx); - gpm_epoll_close(gpmctx); + /* on errors, free the buffer to prevent calling + * xdr_destroy(&xdr_reply_ctx); */ + free(*buffer); + *buffer = NULL; } return ret; } @@ -569,10 +567,6 @@ static int gpm_send_recv_loop(struct gpm_ctx *gpmctx, char *send_buffer, /* Close and reopen socket before trying again */ ret = gpm_retry_socket(gpmctx); - /* Free buffer and set it to NULL to prevent free(xdr_reply_ctx) */ - free(*recv_buffer); - *recv_buffer = NULL; - if (ret != 0) return ret; ret = ETIMEDOUT; From 550337cc754bdb8ac4c3b4dcdfb785eb0519aff6 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Sep 14 2017 19:24:29 +0000 Subject: [PATCH 4/4] Bump copyright after modifications to gpm_common.c Signed-off-by: Alexander Scheel --- diff --git a/proxy/src/client/gpm_common.c b/proxy/src/client/gpm_common.c index ee6306a..fabc721 100644 --- a/proxy/src/client/gpm_common.c +++ b/proxy/src/client/gpm_common.c @@ -1,4 +1,4 @@ -/* Copyright (C) 2011 the GSS-PROXY contributors, see COPYING for license */ +/* Copyright (C) 2017 the GSS-PROXY contributors, see COPYING for license */ #include "gssapi_gpm.h" #include