Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix some FDs leaks #334

Merged
merged 6 commits into from
Feb 6, 2024
Merged

fix some FDs leaks #334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e1ec9eb
api: do not leak fd on errors
giuseppe Feb 6, 2024
63220fa
api: allocate ctx with calloc
giuseppe Feb 6, 2024
a6c3eb7
api: use g_free with fwd
giuseppe Feb 6, 2024
f5287ef
main: fix some leaks on error paths
giuseppe Feb 6, 2024
fa1f950
main: do not leak sockfd on errors
giuseppe Feb 6, 2024
afc3126
main: do not leak tapfd
giuseppe Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -57,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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

g_malloc0 using malloc internally is just an implementation detail. Since we free ctx with free, we should either change g_malloc() -> malloc(), or free() with g_free().

if (ctx == NULL) {
return NULL;
}
Expand Down Expand Up @@ -116,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') {
Expand All @@ -126,15 +129,15 @@ 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");
if (fwd->host_port == 0) {
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;
}

Expand All @@ -144,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 =
Expand All @@ -153,15 +156,15 @@ 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,
fwd->guest_addr, fwd->guest_port) < 0) {
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;
Expand Down
58 changes: 40 additions & 18 deletions main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -148,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));
Expand All @@ -157,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;
}
}

Expand All @@ -180,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));
Expand All @@ -206,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) */
Expand All @@ -224,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) {
Expand All @@ -232,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;
}
Expand Down
Loading