From b4268722196a3d93183252584b86cbb719187a38 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Fri, 19 Jul 2013 18:10:02 +0000 Subject: [PATCH] Centralise calls to fcntl into functions that carefully check the error returns. [originally from svn r9940] --- unix/gtkwin.c | 2 +- unix/unix.h | 5 +++- unix/uxmisc.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++---- unix/uxnet.c | 11 +++------ unix/uxplink.c | 10 ++++---- unix/uxproxy.c | 4 ++-- unix/uxpty.c | 10 +------- 7 files changed, 76 insertions(+), 31 deletions(-) diff --git a/unix/gtkwin.c b/unix/gtkwin.c index ee91a93e..ad8ade52 100644 --- a/unix/gtkwin.c +++ b/unix/gtkwin.c @@ -3307,7 +3307,7 @@ void dup_session_menuitem(GtkMenuItem *item, gpointer gdata) } sprintf(option, "---[%d,%d]", pipefd[0], size); - fcntl(pipefd[0], F_SETFD, 0); + noncloexec(pipefd[0]); fork_and_exec_self(inst, pipefd[1], option, NULL); close(pipefd[0]); diff --git a/unix/unix.h b/unix/unix.h index 3af9eebb..a9f00bf2 100644 --- a/unix/unix.h +++ b/unix/unix.h @@ -156,7 +156,10 @@ void (*putty_signal(int sig, void (*func)(int)))(int); void block_signal(int sig, int block_it); /* uxmisc.c */ -int cloexec(int); +void cloexec(int); +void noncloexec(int); +int nonblock(int); +int no_nonblock(int); /* * Exports from unicode.c. diff --git a/unix/uxmisc.c b/unix/uxmisc.c index 300fc543..473aa94e 100644 --- a/unix/uxmisc.c +++ b/unix/uxmisc.c @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -168,14 +169,70 @@ void pgp_fingerprints(void) } /* - * Set FD_CLOEXEC on a file descriptor + * Set and clear fcntl options on a file descriptor. We don't + * realistically expect any of these operations to fail (the most + * plausible error condition is EBADF, but we always believe ourselves + * to be passing a valid fd so even that's an assertion-fail sort of + * response), so we don't make any effort to return sensible error + * codes to the caller - we just log to standard error and die + * unceremoniously. However, nonblock and no_nonblock do return the + * previous state of O_NONBLOCK. */ -int cloexec(int fd) { +void cloexec(int fd) { int fdflags; fdflags = fcntl(fd, F_GETFD); - if (fdflags == -1) return -1; - return fcntl(fd, F_SETFD, fdflags | FD_CLOEXEC); + if (fdflags < 0) { + fprintf(stderr, "%d: fcntl(F_GETFD): %s\n", fd, strerror(errno)); + exit(1); + } + if (fcntl(fd, F_SETFD, fdflags | FD_CLOEXEC) < 0) { + fprintf(stderr, "%d: fcntl(F_SETFD): %s\n", fd, strerror(errno)); + exit(1); + } +} +void noncloexec(int fd) { + int fdflags; + + fdflags = fcntl(fd, F_GETFD); + if (fdflags < 0) { + fprintf(stderr, "%d: fcntl(F_GETFD): %s\n", fd, strerror(errno)); + exit(1); + } + if (fcntl(fd, F_SETFD, fdflags & ~FD_CLOEXEC) < 0) { + fprintf(stderr, "%d: fcntl(F_SETFD): %s\n", fd, strerror(errno)); + exit(1); + } +} +int nonblock(int fd) { + int fdflags; + + fdflags = fcntl(fd, F_GETFL); + if (fdflags < 0) { + fprintf(stderr, "%d: fcntl(F_GETFL): %s\n", fd, strerror(errno)); + exit(1); + } + if (fcntl(fd, F_SETFL, fdflags | O_NONBLOCK) < 0) { + fprintf(stderr, "%d: fcntl(F_SETFL): %s\n", fd, strerror(errno)); + exit(1); + } + + return fdflags & O_NONBLOCK; +} +int no_nonblock(int fd) { + int fdflags; + + fdflags = fcntl(fd, F_GETFL); + if (fdflags < 0) { + fprintf(stderr, "%d: fcntl(F_GETFL): %s\n", fd, strerror(errno)); + exit(1); + } + if (fcntl(fd, F_SETFL, fdflags & ~O_NONBLOCK) < 0) { + fprintf(stderr, "%d: fcntl(F_SETFL): %s\n", fd, strerror(errno)); + exit(1); + } + + return fdflags & O_NONBLOCK; } FILE *f_open(const Filename *filename, char const *mode, int is_private) diff --git a/unix/uxnet.c b/unix/uxnet.c index e9515241..02894bd9 100644 --- a/unix/uxnet.c +++ b/unix/uxnet.c @@ -540,7 +540,7 @@ static int try_connect(Actual_Socket sock) const union sockaddr_union *sa; int err = 0; short localport; - int fl, salen, family; + int salen, family; /* * Remove the socket from the tree before we overwrite its @@ -695,9 +695,7 @@ static int try_connect(Actual_Socket sock) exit(1); /* XXX: GCC doesn't understand assert() on some systems. */ } - fl = fcntl(s, F_GETFL); - if (fl != -1) - fcntl(s, F_SETFL, fl | O_NONBLOCK); + nonblock(s); if ((connect(s, &(sa->sa), salen)) < 0) { if ( errno != EINPROGRESS ) { @@ -1255,7 +1253,6 @@ static int net_select_result(int fd, int event) union sockaddr_union su; socklen_t addrlen = sizeof(su); int t; /* socket of connection */ - int fl; memset(&su, 0, addrlen); t = accept(s->s, &su.sa, &addrlen); @@ -1263,9 +1260,7 @@ static int net_select_result(int fd, int event) break; } - fl = fcntl(t, F_GETFL); - if (fl != -1) - fcntl(t, F_SETFL, fl | O_NONBLOCK); + nonblock(t); if (s->localhost_only && !sockaddr_is_loopback(&su.sa)) { diff --git a/unix/uxplink.c b/unix/uxplink.c index 9e10c488..898f27c3 100644 --- a/unix/uxplink.c +++ b/unix/uxplink.c @@ -396,20 +396,18 @@ int try_output(int is_stderr) bufchain *chain = (is_stderr ? &stderr_data : &stdout_data); int fd = (is_stderr ? STDERR_FILENO : STDOUT_FILENO); void *senddata; - int sendlen, ret, fl; + int sendlen, ret; if (bufchain_size(chain) > 0) { - fl = fcntl(fd, F_GETFL); - if (fl != -1 && !(fl & O_NONBLOCK)) - fcntl(fd, F_SETFL, fl | O_NONBLOCK); + int prev_nonblock = nonblock(fd); do { bufchain_prefix(chain, &senddata, &sendlen); ret = write(fd, senddata, sendlen); if (ret > 0) bufchain_consume(chain, ret); } while (ret == sendlen && bufchain_size(chain) != 0); - if (fl != -1 && !(fl & O_NONBLOCK)) - fcntl(fd, F_SETFL, fl); + if (!prev_nonblock) + no_nonblock(fd); if (ret < 0 && errno != EAGAIN) { perror(is_stderr ? "stderr: write" : "stdout: write"); exit(1); diff --git a/unix/uxproxy.c b/unix/uxproxy.c index 9dee690e..aa1ff07a 100644 --- a/unix/uxproxy.c +++ b/unix/uxproxy.c @@ -306,8 +306,8 @@ Socket platform_new_connection(SockAddr addr, char *hostname, dup2(from_cmd_pipe[1], 1); close(to_cmd_pipe[0]); close(from_cmd_pipe[1]); - fcntl(0, F_SETFD, 0); - fcntl(1, F_SETFD, 0); + noncloexec(0); + noncloexec(1); execl("/bin/sh", "sh", "-c", cmd, (void *)NULL); _exit(255); } diff --git a/unix/uxpty.c b/unix/uxpty.c index 4a606efd..fb1bd55a 100644 --- a/unix/uxpty.c +++ b/unix/uxpty.c @@ -373,15 +373,7 @@ static void pty_open_master(Pty pty) strncpy(pty->name, ptsname(pty->master_fd), FILENAME_MAX-1); #endif - { - /* - * Set the pty master into non-blocking mode. - */ - int fl; - fl = fcntl(pty->master_fd, F_GETFL); - if (fl != -1 && !(fl & O_NONBLOCK)) - fcntl(pty->master_fd, F_SETFL, fl | O_NONBLOCK); - } + nonblock(pty->master_fd); if (!ptys_by_fd) ptys_by_fd = newtree234(pty_compare_by_fd);