From 13f17d7bd93ba6d676383ea89c3d405c7842a148 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Thu, 24 Oct 2019 12:19:45 +0200 Subject: [PATCH 1/3] Reduces code duplication This patch makes use of existing sss_fd_nonblocking() function where applicable to reduce code duplication. --- src/monitor/monitor_netlink.c | 9 ++--- src/sss_client/ssh/sss_ssh_knownhostsproxy.c | 36 ++++---------------- 2 files changed, 9 insertions(+), 36 deletions(-) diff --git a/src/monitor/monitor_netlink.c b/src/monitor/monitor_netlink.c index a54ae5ad6d..3703a148dc 100644 --- a/src/monitor/monitor_netlink.c +++ b/src/monitor/monitor_netlink.c @@ -791,7 +791,6 @@ int setup_netlink(TALLOC_CTX *mem_ctx, struct tevent_context *ev, struct netlink_ctx *nlctx; int ret; int nlfd; - unsigned flags; int groups[] = { RTNLGRP_LINK, RTNLGRP_IPV4_ROUTE, RTNLGRP_IPV6_ROUTE, RTNLGRP_IPV4_IFADDR, RTNLGRP_IPV6_IFADDR, 0 }; @@ -847,12 +846,8 @@ int setup_netlink(TALLOC_CTX *mem_ctx, struct tevent_context *ev, nlw_disable_seq_check(nlctx->nlp); nlfd = nl_socket_get_fd(nlctx->nlp); - flags = fcntl(nlfd, F_GETFL, 0); - - errno = 0; - ret = fcntl(nlfd, F_SETFL, flags | O_NONBLOCK); - if (ret < 0) { - ret = errno; + ret = sss_fd_nonblocking(nlfd); + if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Cannot set the netlink fd to nonblocking\n"); goto fail; diff --git a/src/sss_client/ssh/sss_ssh_knownhostsproxy.c b/src/sss_client/ssh/sss_ssh_knownhostsproxy.c index c714506323..445ab9fc6f 100644 --- a/src/sss_client/ssh/sss_ssh_knownhostsproxy.c +++ b/src/sss_client/ssh/sss_ssh_knownhostsproxy.c @@ -42,25 +42,14 @@ static int connect_socket(int family, struct sockaddr *addr, size_t addr_len, int *sd) { - int flags; int sock = -1; int ret; /* set O_NONBLOCK on standard input */ - flags = fcntl(0, F_GETFL); - if (flags == -1) { - ret = errno; - DEBUG(SSSDBG_OP_FAILURE, "fcntl() failed (%d): %s\n", - ret, strerror(ret)); - goto done; - } - - ret = fcntl(0, F_SETFL, flags | O_NONBLOCK); - if (ret == -1) { - ret = errno; - DEBUG(SSSDBG_OP_FAILURE, "fcntl() failed (%d): %s\n", - ret, strerror(ret)); - goto done; + ret = sss_fd_nonblocking(0); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "Failed to make fd=0 nonblocking\n"); + return ret; } /* create socket */ @@ -90,7 +79,6 @@ connect_socket(int family, struct sockaddr *addr, size_t addr_len, int *sd) static int proxy_data(int sock) { - int flags; struct pollfd fds[2]; char buffer[BUFFER_SIZE]; int i; @@ -98,19 +86,9 @@ static int proxy_data(int sock) int ret; /* set O_NONBLOCK on the socket */ - flags = fcntl(sock, F_GETFL); - if (flags == -1) { - ret = errno; - DEBUG(SSSDBG_OP_FAILURE, "fcntl() failed (%d): %s\n", - ret, strerror(ret)); - goto done; - } - - ret = fcntl(sock, F_SETFL, flags | O_NONBLOCK); - if (ret == -1) { - ret = errno; - DEBUG(SSSDBG_OP_FAILURE, "fcntl() failed (%d): %s\n", - ret, strerror(ret)); + ret = sss_fd_nonblocking(sock); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "Failed to make socket nonblocking\n"); goto done; } From 9af728b2302ba863b0f1c6df3402f43526ca542c Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Thu, 24 Oct 2019 12:44:31 +0200 Subject: [PATCH 2/3] sss_ssh_knownhostsproxy: relocated O_NONBLOCK setting Relocated sss_fd_nonblocking(0) to proxy_data() from connect_socket() as logically it makes more sense and avoids redundant operations in case connect_socket() is called several times in a loop. --- src/sss_client/ssh/sss_ssh_knownhostsproxy.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/sss_client/ssh/sss_ssh_knownhostsproxy.c b/src/sss_client/ssh/sss_ssh_knownhostsproxy.c index 445ab9fc6f..102f5622c2 100644 --- a/src/sss_client/ssh/sss_ssh_knownhostsproxy.c +++ b/src/sss_client/ssh/sss_ssh_knownhostsproxy.c @@ -45,13 +45,6 @@ connect_socket(int family, struct sockaddr *addr, size_t addr_len, int *sd) int sock = -1; int ret; - /* set O_NONBLOCK on standard input */ - ret = sss_fd_nonblocking(0); - if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, "Failed to make fd=0 nonblocking\n"); - return ret; - } - /* create socket */ sock = socket(family, SOCK_STREAM, IPPROTO_TCP); if (sock == -1) { @@ -85,6 +78,13 @@ static int proxy_data(int sock) ssize_t res; int ret; + /* set O_NONBLOCK on standard input */ + ret = sss_fd_nonblocking(0); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "Failed to make fd=0 nonblocking\n"); + goto done; + } + /* set O_NONBLOCK on the socket */ ret = sss_fd_nonblocking(sock); if (ret != EOK) { From 09fde270e885db37d559be86d8fc2776677afeaa Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Tue, 5 Nov 2019 15:35:35 +0100 Subject: [PATCH 3/3] sss_ssh_knownhostsproxy: fixed Coverity issue Actually I think this Coverity error was "false positive": ``` Error: RESOURCE_LEAK (CWE-772): sssd-2.2.3/src/sss_client/ssh/sss_ssh_knownhostsproxy.c:67: open_fn: Returning handle opened by "socket". sssd-2.2.3/src/sss_client/ssh/sss_ssh_knownhostsproxy.c:67: var_assign: Assigning: "sock" = handle returned from "socket(family, SOCK_STREAM, IPPROTO_TCP)". sssd-2.2.3/src/sss_client/ssh/sss_ssh_knownhostsproxy.c:76: noescape: Resource "sock" is not freed or pointed-to in "connect". sssd-2.2.3/src/sss_client/ssh/sss_ssh_knownhostsproxy.c:88: leaked_handle: Handle variable "sock" going out of scope leaks the handle. 86| done: 87| if (ret != 0 && sock >= 0) close(sock); 88|-> return ret; 89| } 90| ``` Nonetheless it is easier to adjust the code to avoid a complaint. --- src/sss_client/ssh/sss_ssh_knownhostsproxy.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/sss_client/ssh/sss_ssh_knownhostsproxy.c b/src/sss_client/ssh/sss_ssh_knownhostsproxy.c index 102f5622c2..051f51c382 100644 --- a/src/sss_client/ssh/sss_ssh_knownhostsproxy.c +++ b/src/sss_client/ssh/sss_ssh_knownhostsproxy.c @@ -63,10 +63,14 @@ connect_socket(int family, struct sockaddr *addr, size_t addr_len, int *sd) goto done; } - *sd = sock; - done: - if (ret != 0 && sock >= 0) close(sock); + if (ret != 0) { + if (sock >= 0) { + close(sock); + } + } else { + *sd = sock; + } return ret; }