From e1ec9ebff1a0973319a83a4b25c60f74f70cfe39 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Tue, 6 Feb 2024 09:29:17 +0100 Subject: [PATCH 1/6] api: do not leak fd on errors Signed-off-by: Giuseppe Scrivano --- api.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/api.c b/api.c index c58e0c4..c3130a5 100644 --- a/api.c +++ b/api.c @@ -17,25 +17,28 @@ int api_bindlisten(const char *api_socket) unlink(api_socket); /* avoid EADDRINUSE */ if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) { perror("api_bindlisten: socket"); - return -1; + goto cleanup_and_fail; } memset(&addr, 0, sizeof(addr)); addr.sun_family = AF_UNIX; if (strlen(api_socket) >= sizeof(addr.sun_path)) { fprintf(stderr, "the specified API socket path is too long (>= %lu)\n", sizeof(addr.sun_path)); - return -1; + goto cleanup_and_fail; } strncpy(addr.sun_path, api_socket, sizeof(addr.sun_path) - 1); if (bind(fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) { perror("api_bindlisten: bind"); - return -1; + goto cleanup_and_fail; } if (listen(fd, 0) < 0) { perror("api_bindlisten: listen"); - return -1; + goto cleanup_and_fail; } return fd; +cleanup_and_fail: + close(fd); + return -1; } struct api_hostfwd { From 63220fa1f1e901f05a031021d2af682cb81479bb Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Tue, 6 Feb 2024 09:31:53 +0100 Subject: [PATCH 2/6] api: allocate ctx with calloc it is freed with "free" in different places, so allocate using the stdlib function. Signed-off-by: Giuseppe Scrivano --- api.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api.c b/api.c index c3130a5..424be3f 100644 --- a/api.c +++ b/api.c @@ -60,7 +60,7 @@ struct api_ctx { struct api_ctx *api_ctx_alloc(struct slirp4netns_config *cfg) { - struct api_ctx *ctx = (struct api_ctx *)g_malloc0(sizeof(*ctx)); + struct api_ctx *ctx = (struct api_ctx *)calloc(1, sizeof(*ctx)); if (ctx == NULL) { return NULL; } From a6c3eb793ae9967fd3decad4779159610f894fd0 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Tue, 6 Feb 2024 09:33:30 +0100 Subject: [PATCH 3/6] api: use g_free with fwd it is allocated using g_malloc0 and passed to a list, that is later freed with g_free. Signed-off-by: Giuseppe Scrivano --- api.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/api.c b/api.c index 424be3f..09f3cac 100644 --- a/api.c +++ b/api.c @@ -119,7 +119,7 @@ static int api_handle_req_add_hostfwd(Slirp *slirp, int fd, struct api_ctx *ctx, const char *err = "{\"error\":{\"desc\":\"bad request: add_hostfwd: " "bad arguments.proto\"}}"; wrc = write(fd, err, strlen(err)); - free(fwd); + g_free(fwd); goto finish; } if (host_addr_s == NULL || host_addr_s[0] == '\0') { @@ -129,7 +129,7 @@ static int api_handle_req_add_hostfwd(Slirp *slirp, int fd, struct api_ctx *ctx, const char *err = "{\"error\":{\"desc\":\"bad request: add_hostfwd: " "bad arguments.host_addr\"}}"; wrc = write(fd, err, strlen(err)); - free(fwd); + g_free(fwd); goto finish; } fwd->host_port = (int)json_object_dotget_number(jo, "arguments.host_port"); @@ -137,7 +137,7 @@ static int api_handle_req_add_hostfwd(Slirp *slirp, int fd, struct api_ctx *ctx, const char *err = "{\"error\":{\"desc\":\"bad request: add_hostfwd: " "bad arguments.host_port\"}}"; wrc = write(fd, err, strlen(err)); - free(fwd); + g_free(fwd); goto finish; } @@ -147,7 +147,7 @@ static int api_handle_req_add_hostfwd(Slirp *slirp, int fd, struct api_ctx *ctx, const char *err = "{\"error\":{\"desc\":\"bad request: add_hostfwd: " "bad arguments.guest_addr\"}}"; wrc = write(fd, err, strlen(err)); - free(fwd); + g_free(fwd); goto finish; } fwd->guest_port = @@ -156,7 +156,7 @@ static int api_handle_req_add_hostfwd(Slirp *slirp, int fd, struct api_ctx *ctx, const char *err = "{\"error\":{\"desc\":\"bad request: add_hostfwd: " "bad arguments.guest_port\"}}"; wrc = write(fd, err, strlen(err)); - free(fwd); + g_free(fwd); goto finish; } if (slirp_add_hostfwd(slirp, fwd->is_udp, fwd->host_addr, fwd->host_port, @@ -164,7 +164,7 @@ static int api_handle_req_add_hostfwd(Slirp *slirp, int fd, struct api_ctx *ctx, const char *err = "{\"error\":{\"desc\":\"bad request: add_hostfwd: " "slirp_add_hostfwd failed\"}}"; wrc = write(fd, err, strlen(err)); - free(fwd); + g_free(fwd); goto finish; } fwd->id = ctx->hostfwds_nextid; From f5287ef1a7c1c6a05027dceb40f017dac9824793 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Tue, 6 Feb 2024 10:09:38 +0100 Subject: [PATCH 4/6] main: fix some leaks on error paths Signed-off-by: Giuseppe Scrivano --- main.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/main.c b/main.c index 1bfdd95..f664ec4 100644 --- a/main.c +++ b/main.c @@ -42,41 +42,58 @@ static int nsenter(pid_t target_pid, char *netns, char *userns, bool only_userns) { int usernsfd = -1, netnsfd = -1; + char *netns_allocated = NULL; + char *userns_allocated = NULL; + if (!only_userns && !netns) { - if (asprintf(&netns, "/proc/%d/ns/net", target_pid) < 0) { + if (asprintf(&netns_allocated, "/proc/%d/ns/net", target_pid) < 0) { perror("cannot get netns path"); - return -1; + goto fail; } + netns = netns_allocated; } if (!userns && target_pid) { - if (asprintf(&userns, "/proc/%d/ns/user", target_pid) < 0) { + if (asprintf(&userns_allocated, "/proc/%d/ns/user", target_pid) < 0) { perror("cannot get userns path"); - return -1; + goto fail; } + userns = userns_allocated; } if (!only_userns && (netnsfd = open(netns, O_RDONLY)) < 0) { perror(netns); - return netnsfd; + goto fail; } if (userns && (usernsfd = open(userns, O_RDONLY)) < 0) { perror(userns); - return usernsfd; + goto fail; } if (usernsfd != -1) { int r = setns(usernsfd, CLONE_NEWUSER); if (only_userns && r < 0) { perror("setns(CLONE_NEWUSER)"); - return -1; + goto fail; } close(usernsfd); + usernsfd = -1; } - if (netnsfd != -1 && setns(netnsfd, CLONE_NEWNET) < 0) { - perror("setns(CLONE_NEWNET)"); - return -1; + if (netnsfd != -1) { + if (setns(netnsfd, CLONE_NEWNET) < 0) { + perror("setns(CLONE_NEWNET)"); + goto fail; + } + close(netnsfd); } - close(netnsfd); return 0; +fail: + free(netns_allocated); + free(userns_allocated); + + if (usernsfd != -1) + close(usernsfd); + if (netnsfd != -1) + close(netnsfd); + return -1; } static int open_tap(const char *tapname) From fa1f9502ffd5ca80bc6816ea18de1a2f6098d7e6 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Tue, 6 Feb 2024 10:11:47 +0100 Subject: [PATCH 5/6] main: do not leak sockfd on errors Signed-off-by: Giuseppe Scrivano --- main.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/main.c b/main.c index f664ec4..28e2c23 100644 --- a/main.c +++ b/main.c @@ -165,7 +165,7 @@ static int configure_network(const char *tapname, .ifr_flags = IFF_UP | IFF_RUNNING }; if (ioctl(sockfd, SIOCSIFFLAGS, &ifr_lo) < 0) { perror("cannot set device up"); - return -1; + goto fail; } memset(&ifr, 0, sizeof(ifr)); @@ -174,20 +174,20 @@ static int configure_network(const char *tapname, if (ioctl(sockfd, SIOCSIFFLAGS, &ifr) < 0) { perror("cannot set device up"); - return -1; + goto fail; } ifr.ifr_mtu = (int)cfg->mtu; if (ioctl(sockfd, SIOCSIFMTU, &ifr) < 0) { perror("cannot set MTU"); - return -1; + goto fail; } if (cfg->vmacaddress_len > 0) { ifr.ifr_ifru.ifru_hwaddr = cfg->vmacaddress; if (ioctl(sockfd, SIOCSIFHWADDR, &ifr) < 0) { perror("cannot set MAC address"); - return -1; + goto fail; } } @@ -197,13 +197,13 @@ static int configure_network(const char *tapname, if (ioctl(sockfd, SIOCSIFADDR, &ifr) < 0) { perror("cannot set device address"); - return -1; + goto fail; } sai->sin_addr = cfg->vnetmask; if (ioctl(sockfd, SIOCSIFNETMASK, &ifr) < 0) { perror("cannot set device netmask"); - return -1; + goto fail; } memset(&route, 0, sizeof(route)); @@ -223,9 +223,12 @@ static int configure_network(const char *tapname, if (ioctl(sockfd, SIOCADDRT, &route) < 0) { perror("set route"); - return -1; + goto fail; } return 0; +fail: + close(sockfd); + return -1; } /* Child (--target-type=netns) */ From afc312613db10832235a488054f62c6dbc0de6e3 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Tue, 6 Feb 2024 10:13:55 +0100 Subject: [PATCH 6/6] main: do not leak tapfd Signed-off-by: Giuseppe Scrivano --- main.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/main.c b/main.c index 28e2c23..a089560 100644 --- a/main.c +++ b/main.c @@ -244,6 +244,7 @@ static int child(int sock, pid_t target_pid, bool do_config_network, return tapfd; } if (do_config_network && configure_network(tapname, cfg) < 0) { + close(tapfd); return -1; } if (sendfd(sock, tapfd) < 0) { @@ -252,6 +253,7 @@ static int child(int sock, pid_t target_pid, bool do_config_network, return -1; } fprintf(stderr, "sent tapfd=%d for %s\n", tapfd, tapname); + close(tapfd); close(sock); return 0; }