* [PATCH 1/8] vchan-socket-proxy: Ensure UNIX path NUL terminated
2020-05-25 2:49 [PATCH 0/8] Coverity fixes for vchan-socket-proxy Jason Andryuk
@ 2020-05-25 2:49 ` Jason Andryuk
2020-06-13 12:40 ` Marek Marczykowski-Górecki
2020-05-25 2:49 ` [PATCH 2/8] vchan-socket-proxy: Check xs_watch return value Jason Andryuk
` (7 subsequent siblings)
8 siblings, 1 reply; 14+ messages in thread
From: Jason Andryuk @ 2020-05-25 2:49 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, marmarek, Wei Liu, Jason Andryuk
Check the socket path length to ensure sun_path is NUL terminated.
This was spotted by Citrix's Coverity.
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
tools/libvchan/vchan-socket-proxy.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/tools/libvchan/vchan-socket-proxy.c b/tools/libvchan/vchan-socket-proxy.c
index 13700c5d67..6d860af340 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -148,6 +148,12 @@ static int connect_socket(const char *path_or_fd) {
return fd;
}
+ if (strlen(path_or_fd) >= sizeof(addr.sun_path)) {
+ fprintf(stderr, "UNIX socket path \"%s\" too long (%zd >= %zd)\n",
+ path_or_fd, strlen(path_or_fd), sizeof(addr.sun_path));
+ return -1;
+ }
+
fd = socket(AF_UNIX, SOCK_STREAM, 0);
if (fd == -1)
return -1;
@@ -174,6 +180,12 @@ static int listen_socket(const char *path_or_fd) {
return fd;
}
+ if (strlen(path_or_fd) >= sizeof(addr.sun_path)) {
+ fprintf(stderr, "UNIX socket path \"%s\" too long (%zd >= %zd)\n",
+ path_or_fd, strlen(path_or_fd), sizeof(addr.sun_path));
+ return -1;
+ }
+
/* if not a number, assume a socket path */
fd = socket(AF_UNIX, SOCK_STREAM, 0);
if (fd == -1)
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 1/8] vchan-socket-proxy: Ensure UNIX path NUL terminated
2020-05-25 2:49 ` [PATCH 1/8] vchan-socket-proxy: Ensure UNIX path NUL terminated Jason Andryuk
@ 2020-06-13 12:40 ` Marek Marczykowski-Górecki
0 siblings, 0 replies; 14+ messages in thread
From: Marek Marczykowski-Górecki @ 2020-06-13 12:40 UTC (permalink / raw)
To: Jason Andryuk; +Cc: xen-devel, Ian Jackson, Wei Liu
[-- Attachment #1: Type: text/plain, Size: 1769 bytes --]
On Sun, May 24, 2020 at 10:49:48PM -0400, Jason Andryuk wrote:
> Check the socket path length to ensure sun_path is NUL terminated.
>
> This was spotted by Citrix's Coverity.
>
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> tools/libvchan/vchan-socket-proxy.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/tools/libvchan/vchan-socket-proxy.c b/tools/libvchan/vchan-socket-proxy.c
> index 13700c5d67..6d860af340 100644
> --- a/tools/libvchan/vchan-socket-proxy.c
> +++ b/tools/libvchan/vchan-socket-proxy.c
> @@ -148,6 +148,12 @@ static int connect_socket(const char *path_or_fd) {
> return fd;
> }
>
> + if (strlen(path_or_fd) >= sizeof(addr.sun_path)) {
> + fprintf(stderr, "UNIX socket path \"%s\" too long (%zd >= %zd)\n",
> + path_or_fd, strlen(path_or_fd), sizeof(addr.sun_path));
> + return -1;
> + }
> +
> fd = socket(AF_UNIX, SOCK_STREAM, 0);
> if (fd == -1)
> return -1;
> @@ -174,6 +180,12 @@ static int listen_socket(const char *path_or_fd) {
> return fd;
> }
>
> + if (strlen(path_or_fd) >= sizeof(addr.sun_path)) {
> + fprintf(stderr, "UNIX socket path \"%s\" too long (%zd >= %zd)\n",
> + path_or_fd, strlen(path_or_fd), sizeof(addr.sun_path));
> + return -1;
> + }
> +
> /* if not a number, assume a socket path */
> fd = socket(AF_UNIX, SOCK_STREAM, 0);
> if (fd == -1)
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/8] vchan-socket-proxy: Check xs_watch return value
2020-05-25 2:49 [PATCH 0/8] Coverity fixes for vchan-socket-proxy Jason Andryuk
2020-05-25 2:49 ` [PATCH 1/8] vchan-socket-proxy: Ensure UNIX path NUL terminated Jason Andryuk
@ 2020-05-25 2:49 ` Jason Andryuk
2020-05-25 2:49 ` [PATCH 3/8] vchan-socket-proxy: Unify main " Jason Andryuk
` (6 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Jason Andryuk @ 2020-05-25 2:49 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, marmarek, Wei Liu, Jason Andryuk
Check the return value of xs_watch and error out on failure.
This was found by Citrix's Coverity.
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
tools/libvchan/vchan-socket-proxy.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/libvchan/vchan-socket-proxy.c b/tools/libvchan/vchan-socket-proxy.c
index 6d860af340..bd12632311 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -225,8 +225,15 @@ static struct libxenvchan *connect_vchan(int domid, const char *path) {
goto out;
}
/* wait for vchan server to create *path* */
- xs_watch(xs, path, "path");
- xs_watch(xs, "@releaseDomain", "release");
+ if (!xs_watch(xs, path, "path")) {
+ fprintf(stderr, "xs_watch(%s) failed.\n", path);
+ goto out;
+ }
+ if (!xs_watch(xs, "@releaseDomain", "release")) {
+ fprintf(stderr, "xs_watch(@releaseDomain failed.\n");
+ goto out;
+ }
+
while ((watch_ret = xs_read_watch(xs, &watch_num))) {
/* don't care about exact which fired the watch */
free(watch_ret);
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 3/8] vchan-socket-proxy: Unify main return value
2020-05-25 2:49 [PATCH 0/8] Coverity fixes for vchan-socket-proxy Jason Andryuk
2020-05-25 2:49 ` [PATCH 1/8] vchan-socket-proxy: Ensure UNIX path NUL terminated Jason Andryuk
2020-05-25 2:49 ` [PATCH 2/8] vchan-socket-proxy: Check xs_watch return value Jason Andryuk
@ 2020-05-25 2:49 ` Jason Andryuk
2020-05-25 2:49 ` [PATCH 4/8] vchan-socket-proxy: Use a struct to store state Jason Andryuk
` (5 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Jason Andryuk @ 2020-05-25 2:49 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, marmarek, Wei Liu, Jason Andryuk
Introduce 'ret' for main's return value and remove direct returns. This
is in preparation for a unified exit path with resource cleanup.
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
tools/libvchan/vchan-socket-proxy.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/tools/libvchan/vchan-socket-proxy.c b/tools/libvchan/vchan-socket-proxy.c
index bd12632311..d85e24ee93 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -381,6 +381,7 @@ int main(int argc, char **argv)
const char *vchan_path;
const char *state_path = NULL;
int opt;
+ int ret;
while ((opt = getopt_long(argc, argv, "m:vs:", options, NULL)) != -1) {
switch (opt) {
@@ -447,6 +448,8 @@ int main(int argc, char **argv)
xs_close(xs);
}
+ ret = 0;
+
for (;;) {
if (is_server) {
/* wait for vchan connection */
@@ -461,7 +464,8 @@ int main(int argc, char **argv)
}
if (input_fd == -1) {
perror("connect socket");
- return 1;
+ ret = 1;
+ break;
}
if (data_loop(ctrl, input_fd, output_fd) != 0)
break;
@@ -474,14 +478,16 @@ int main(int argc, char **argv)
input_fd = output_fd = accept(socket_fd, NULL, NULL);
if (input_fd == -1) {
perror("accept");
- return 1;
+ ret = 1;
+ break;
}
set_nonblocking(input_fd, 1);
set_nonblocking(output_fd, 1);
ctrl = connect_vchan(domid, vchan_path);
if (!ctrl) {
perror("vchan client init");
- return 1;
+ ret = 1;
+ break;
}
if (data_loop(ctrl, input_fd, output_fd) != 0)
break;
@@ -493,5 +499,6 @@ int main(int argc, char **argv)
ctrl = NULL;
}
}
- return 0;
+
+ return ret;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 4/8] vchan-socket-proxy: Use a struct to store state
2020-05-25 2:49 [PATCH 0/8] Coverity fixes for vchan-socket-proxy Jason Andryuk
` (2 preceding siblings ...)
2020-05-25 2:49 ` [PATCH 3/8] vchan-socket-proxy: Unify main " Jason Andryuk
@ 2020-05-25 2:49 ` Jason Andryuk
2020-05-25 2:49 ` [PATCH 5/8] vchan-socket-proxy: Switch data_loop() to take state Jason Andryuk
` (4 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Jason Andryuk @ 2020-05-25 2:49 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, marmarek, Wei Liu, Jason Andryuk
Use a struct to group the vchan ctrl and FDs. This will facilite
tracking the state of open and closed FDs and ctrl in data_loop().
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
tools/libvchan/vchan-socket-proxy.c | 52 +++++++++++++++++------------
1 file changed, 30 insertions(+), 22 deletions(-)
diff --git a/tools/libvchan/vchan-socket-proxy.c b/tools/libvchan/vchan-socket-proxy.c
index d85e24ee93..39f4bb1452 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -89,6 +89,12 @@ int insiz = 0;
int outsiz = 0;
int verbose = 0;
+struct vchan_proxy_state {
+ struct libxenvchan *ctrl;
+ int output_fd;
+ int input_fd;
+};
+
static void vchan_wr(struct libxenvchan *ctrl) {
int ret;
@@ -374,8 +380,9 @@ int main(int argc, char **argv)
{
int is_server = 0;
int socket_fd = -1;
- int input_fd, output_fd;
- struct libxenvchan *ctrl = NULL;
+ struct vchan_proxy_state state = { .ctrl = NULL,
+ .input_fd = -1,
+ .output_fd = -1 };
const char *socket_path;
int domid;
const char *vchan_path;
@@ -415,15 +422,15 @@ int main(int argc, char **argv)
socket_path = argv[optind+2];
if (is_server) {
- ctrl = libxenvchan_server_init(NULL, domid, vchan_path, 0, 0);
- if (!ctrl) {
+ state.ctrl = libxenvchan_server_init(NULL, domid, vchan_path, 0, 0);
+ if (!state.ctrl) {
perror("libxenvchan_server_init");
exit(1);
}
} else {
if (strcmp(socket_path, "-") == 0) {
- input_fd = 0;
- output_fd = 1;
+ state.input_fd = 0;
+ state.output_fd = 1;
} else {
socket_fd = listen_socket(socket_path);
if (socket_fd == -1) {
@@ -453,21 +460,21 @@ int main(int argc, char **argv)
for (;;) {
if (is_server) {
/* wait for vchan connection */
- while (libxenvchan_is_open(ctrl) != 1)
- libxenvchan_wait(ctrl);
+ while (libxenvchan_is_open(state.ctrl) != 1)
+ libxenvchan_wait(state.ctrl);
/* vchan client connected, setup local FD if needed */
if (strcmp(socket_path, "-") == 0) {
- input_fd = 0;
- output_fd = 1;
+ state.input_fd = 0;
+ state.output_fd = 1;
} else {
- input_fd = output_fd = connect_socket(socket_path);
+ state.input_fd = state.output_fd = connect_socket(socket_path);
}
- if (input_fd == -1) {
+ if (state.input_fd == -1) {
perror("connect socket");
ret = 1;
break;
}
- if (data_loop(ctrl, input_fd, output_fd) != 0)
+ if (data_loop(state.ctrl, state.input_fd, state.output_fd) != 0)
break;
/* keep it running only when get UNIX socket path */
if (socket_path[0] != '/')
@@ -475,28 +482,29 @@ int main(int argc, char **argv)
} else {
/* wait for local socket connection */
if (strcmp(socket_path, "-") != 0)
- input_fd = output_fd = accept(socket_fd, NULL, NULL);
- if (input_fd == -1) {
+ state.input_fd = state.output_fd = accept(socket_fd,
+ NULL, NULL);
+ if (state.input_fd == -1) {
perror("accept");
ret = 1;
break;
}
- set_nonblocking(input_fd, 1);
- set_nonblocking(output_fd, 1);
- ctrl = connect_vchan(domid, vchan_path);
- if (!ctrl) {
+ set_nonblocking(state.input_fd, 1);
+ set_nonblocking(state.output_fd, 1);
+ state.ctrl = connect_vchan(domid, vchan_path);
+ if (!state.ctrl) {
perror("vchan client init");
ret = 1;
break;
}
- if (data_loop(ctrl, input_fd, output_fd) != 0)
+ if (data_loop(state.ctrl, state.input_fd, state.output_fd) != 0)
break;
/* don't reconnect if output was stdout */
if (strcmp(socket_path, "-") == 0)
break;
- libxenvchan_close(ctrl);
- ctrl = NULL;
+ libxenvchan_close(state.ctrl);
+ state.ctrl = NULL;
}
}
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 5/8] vchan-socket-proxy: Switch data_loop() to take state
2020-05-25 2:49 [PATCH 0/8] Coverity fixes for vchan-socket-proxy Jason Andryuk
` (3 preceding siblings ...)
2020-05-25 2:49 ` [PATCH 4/8] vchan-socket-proxy: Use a struct to store state Jason Andryuk
@ 2020-05-25 2:49 ` Jason Andryuk
2020-05-25 2:49 ` [PATCH 6/8] vchan-socket-proxy: Set closed FDs to -1 Jason Andryuk
` (3 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Jason Andryuk @ 2020-05-25 2:49 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, marmarek, Wei Liu, Jason Andryuk
Switch data_loop to take a pointer to vchan_proxy_state.
No functional change.
This removes a dead store to input_fd identified by Coverity.
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
tools/libvchan/vchan-socket-proxy.c | 65 +++++++++++++++--------------
1 file changed, 33 insertions(+), 32 deletions(-)
diff --git a/tools/libvchan/vchan-socket-proxy.c b/tools/libvchan/vchan-socket-proxy.c
index 39f4bb1452..32be410609 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -279,13 +279,13 @@ static void discard_buffers(struct libxenvchan *ctrl) {
}
}
-int data_loop(struct libxenvchan *ctrl, int input_fd, int output_fd)
+int data_loop(struct vchan_proxy_state *state)
{
int ret;
int libxenvchan_fd;
int max_fd;
- libxenvchan_fd = libxenvchan_fd_for_select(ctrl);
+ libxenvchan_fd = libxenvchan_fd_for_select(state->ctrl);
for (;;) {
fd_set rfds;
fd_set wfds;
@@ -293,15 +293,15 @@ int data_loop(struct libxenvchan *ctrl, int input_fd, int output_fd)
FD_ZERO(&wfds);
max_fd = -1;
- if (input_fd != -1 && insiz != BUFSIZE) {
- FD_SET(input_fd, &rfds);
- if (input_fd > max_fd)
- max_fd = input_fd;
+ if (state->input_fd != -1 && insiz != BUFSIZE) {
+ FD_SET(state->input_fd, &rfds);
+ if (state->input_fd > max_fd)
+ max_fd = state->input_fd;
}
- if (output_fd != -1 && outsiz) {
- FD_SET(output_fd, &wfds);
- if (output_fd > max_fd)
- max_fd = output_fd;
+ if (state->output_fd != -1 && outsiz) {
+ FD_SET(state->output_fd, &wfds);
+ if (state->output_fd > max_fd)
+ max_fd = state->output_fd;
}
FD_SET(libxenvchan_fd, &rfds);
if (libxenvchan_fd > max_fd)
@@ -312,52 +312,53 @@ int data_loop(struct libxenvchan *ctrl, int input_fd, int output_fd)
exit(1);
}
if (FD_ISSET(libxenvchan_fd, &rfds)) {
- libxenvchan_wait(ctrl);
- if (!libxenvchan_is_open(ctrl)) {
+ libxenvchan_wait(state->ctrl);
+ if (!libxenvchan_is_open(state->ctrl)) {
if (verbose)
fprintf(stderr, "vchan client disconnected\n");
while (outsiz)
- socket_wr(output_fd);
- close(output_fd);
- close(input_fd);
- discard_buffers(ctrl);
+ socket_wr(state->output_fd);
+ close(state->output_fd);
+ close(state->input_fd);
+ discard_buffers(state->ctrl);
break;
}
- vchan_wr(ctrl);
+ vchan_wr(state->ctrl);
}
- if (FD_ISSET(input_fd, &rfds)) {
- ret = read(input_fd, inbuf + insiz, BUFSIZE - insiz);
+ if (FD_ISSET(state->input_fd, &rfds)) {
+ ret = read(state->input_fd, inbuf + insiz, BUFSIZE - insiz);
if (ret < 0 && errno != EAGAIN)
exit(1);
if (verbose)
fprintf(stderr, "from-unix: %.*s\n", ret, inbuf + insiz);
if (ret == 0) {
/* EOF on socket, write everything in the buffer and close the
- * input_fd socket */
+ * state->input_fd socket */
while (insiz) {
- vchan_wr(ctrl);
- libxenvchan_wait(ctrl);
+ vchan_wr(state->ctrl);
+ libxenvchan_wait(state->ctrl);
}
- close(input_fd);
- input_fd = -1;
+ close(state->input_fd);
+ state->input_fd = -1;
/* TODO: maybe signal the vchan client somehow? */
break;
}
if (ret)
insiz += ret;
- vchan_wr(ctrl);
+ vchan_wr(state->ctrl);
}
- if (FD_ISSET(output_fd, &wfds))
- socket_wr(output_fd);
- while (libxenvchan_data_ready(ctrl) && outsiz < BUFSIZE) {
- ret = libxenvchan_read(ctrl, outbuf + outsiz, BUFSIZE - outsiz);
+ if (FD_ISSET(state->output_fd, &wfds))
+ socket_wr(state->output_fd);
+ while (libxenvchan_data_ready(state->ctrl) && outsiz < BUFSIZE) {
+ ret = libxenvchan_read(state->ctrl, outbuf + outsiz,
+ BUFSIZE - outsiz);
if (ret < 0)
exit(1);
if (verbose)
fprintf(stderr, "from-vchan: %.*s\n", ret, outbuf + outsiz);
outsiz += ret;
- socket_wr(output_fd);
+ socket_wr(state->output_fd);
}
}
return 0;
@@ -474,7 +475,7 @@ int main(int argc, char **argv)
ret = 1;
break;
}
- if (data_loop(state.ctrl, state.input_fd, state.output_fd) != 0)
+ if (data_loop(&state) != 0)
break;
/* keep it running only when get UNIX socket path */
if (socket_path[0] != '/')
@@ -497,7 +498,7 @@ int main(int argc, char **argv)
ret = 1;
break;
}
- if (data_loop(state.ctrl, state.input_fd, state.output_fd) != 0)
+ if (data_loop(&state) != 0)
break;
/* don't reconnect if output was stdout */
if (strcmp(socket_path, "-") == 0)
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 6/8] vchan-socket-proxy: Set closed FDs to -1
2020-05-25 2:49 [PATCH 0/8] Coverity fixes for vchan-socket-proxy Jason Andryuk
` (4 preceding siblings ...)
2020-05-25 2:49 ` [PATCH 5/8] vchan-socket-proxy: Switch data_loop() to take state Jason Andryuk
@ 2020-05-25 2:49 ` Jason Andryuk
2020-05-25 2:49 ` [PATCH 7/8] vchan-socket-proxy: Cleanup resources on exit Jason Andryuk
` (2 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Jason Andryuk @ 2020-05-25 2:49 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, marmarek, Wei Liu, Jason Andryuk
These FDs are closed, so set them to -1 so they are no longer valid.
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
tools/libvchan/vchan-socket-proxy.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/libvchan/vchan-socket-proxy.c b/tools/libvchan/vchan-socket-proxy.c
index 32be410609..f3f6e5ec09 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -319,7 +319,9 @@ int data_loop(struct vchan_proxy_state *state)
while (outsiz)
socket_wr(state->output_fd);
close(state->output_fd);
+ state->output_fd = -1;
close(state->input_fd);
+ state->input_fd = -1;
discard_buffers(state->ctrl);
break;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 7/8] vchan-socket-proxy: Cleanup resources on exit
2020-05-25 2:49 [PATCH 0/8] Coverity fixes for vchan-socket-proxy Jason Andryuk
` (5 preceding siblings ...)
2020-05-25 2:49 ` [PATCH 6/8] vchan-socket-proxy: Set closed FDs to -1 Jason Andryuk
@ 2020-05-25 2:49 ` Jason Andryuk
2020-05-25 2:49 ` [PATCH 8/8] vchan-socket-proxy: Handle closing shared input/output_fd Jason Andryuk
2020-05-25 22:36 ` [PATCH 0/8] Coverity fixes for vchan-socket-proxy Jason Andryuk
8 siblings, 0 replies; 14+ messages in thread
From: Jason Andryuk @ 2020-05-25 2:49 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, marmarek, Wei Liu, Jason Andryuk
Close open FDs and close th vchan connection when exiting the program.
This addresses some Coverity findings about leaking file descriptors.
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
tools/libvchan/vchan-socket-proxy.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/tools/libvchan/vchan-socket-proxy.c b/tools/libvchan/vchan-socket-proxy.c
index f3f6e5ec09..a04b46ee04 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -511,5 +511,14 @@ int main(int argc, char **argv)
}
}
+ if (state.output_fd >= 0)
+ close(state.output_fd);
+ if (state.input_fd >= 0)
+ close(state.input_fd);
+ if (state.ctrl)
+ libxenvchan_close(state.ctrl);
+ if (socket_fd >= 0)
+ close(socket_fd);
+
return ret;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 8/8] vchan-socket-proxy: Handle closing shared input/output_fd
2020-05-25 2:49 [PATCH 0/8] Coverity fixes for vchan-socket-proxy Jason Andryuk
` (6 preceding siblings ...)
2020-05-25 2:49 ` [PATCH 7/8] vchan-socket-proxy: Cleanup resources on exit Jason Andryuk
@ 2020-05-25 2:49 ` Jason Andryuk
2020-05-25 22:36 ` [PATCH 0/8] Coverity fixes for vchan-socket-proxy Jason Andryuk
8 siblings, 0 replies; 14+ messages in thread
From: Jason Andryuk @ 2020-05-25 2:49 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, marmarek, Wei Liu, Jason Andryuk
input_fd & output_fd may be the same FD. In that case, mark both as -1
when closing one. That avoids a dangling FD reference.
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
tools/libvchan/vchan-socket-proxy.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/libvchan/vchan-socket-proxy.c b/tools/libvchan/vchan-socket-proxy.c
index a04b46ee04..07ead251a2 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -342,6 +342,8 @@ int data_loop(struct vchan_proxy_state *state)
libxenvchan_wait(state->ctrl);
}
close(state->input_fd);
+ if (state->input_fd == state->output_fd)
+ state->output_fd = -1;
state->input_fd = -1;
/* TODO: maybe signal the vchan client somehow? */
break;
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 0/8] Coverity fixes for vchan-socket-proxy
2020-05-25 2:49 [PATCH 0/8] Coverity fixes for vchan-socket-proxy Jason Andryuk
` (7 preceding siblings ...)
2020-05-25 2:49 ` [PATCH 8/8] vchan-socket-proxy: Handle closing shared input/output_fd Jason Andryuk
@ 2020-05-25 22:36 ` Jason Andryuk
2020-05-28 2:59 ` Jason Andryuk
8 siblings, 1 reply; 14+ messages in thread
From: Jason Andryuk @ 2020-05-25 22:36 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Marek Marczykowski-Górecki, Wei Liu
On Sun, May 24, 2020 at 10:50 PM Jason Andryuk <jandryuk@gmail.com> wrote:
>
> This series addresses some Coverity reports. To handle closing FDs, a
> state struct is introduced to track FDs closed in both main() and
> data_loop().
I've realized the changes here are insufficient to handle the FD
leaks. That is, the accept()-ed FDs need to be closed inside the for
loop so they aren't leaked with each iteration. I'll re-work for a
v2.
-Jason
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 0/8] Coverity fixes for vchan-socket-proxy
2020-05-25 22:36 ` [PATCH 0/8] Coverity fixes for vchan-socket-proxy Jason Andryuk
@ 2020-05-28 2:59 ` Jason Andryuk
2020-06-13 12:40 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 14+ messages in thread
From: Jason Andryuk @ 2020-05-28 2:59 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Marek Marczykowski-Górecki, Wei Liu
On Mon, May 25, 2020 at 6:36 PM Jason Andryuk <jandryuk@gmail.com> wrote:
>
> On Sun, May 24, 2020 at 10:50 PM Jason Andryuk <jandryuk@gmail.com> wrote:
> >
> > This series addresses some Coverity reports. To handle closing FDs, a
> > state struct is introduced to track FDs closed in both main() and
> > data_loop().
>
> I've realized the changes here are insufficient to handle the FD
> leaks. That is, the accept()-ed FDs need to be closed inside the for
> loop so they aren't leaked with each iteration. I'll re-work for a
> v2.
So it turns out this series doesn't leak FDs in the for loop. FDs are
necessarily closed down in data_loop() when the read() returns 0. The
only returns from data_loop() are after the FDs have been closed.
data_loop() and some of the functions it calls will call exit(1) on
error, but that won't leak FDs.
Please review this series. Sorry for the confusion.
Regards,
Jason
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/8] Coverity fixes for vchan-socket-proxy
2020-05-28 2:59 ` Jason Andryuk
@ 2020-06-13 12:40 ` Marek Marczykowski-Górecki
2020-06-14 14:05 ` Jason Andryuk
0 siblings, 1 reply; 14+ messages in thread
From: Marek Marczykowski-Górecki @ 2020-06-13 12:40 UTC (permalink / raw)
To: Jason Andryuk; +Cc: xen-devel, Ian Jackson, Wei Liu
[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]
On Wed, May 27, 2020 at 10:59:55PM -0400, Jason Andryuk wrote:
> On Mon, May 25, 2020 at 6:36 PM Jason Andryuk <jandryuk@gmail.com> wrote:
> >
> > On Sun, May 24, 2020 at 10:50 PM Jason Andryuk <jandryuk@gmail.com> wrote:
> > >
> > > This series addresses some Coverity reports. To handle closing FDs, a
> > > state struct is introduced to track FDs closed in both main() and
> > > data_loop().
> >
> > I've realized the changes here are insufficient to handle the FD
> > leaks. That is, the accept()-ed FDs need to be closed inside the for
> > loop so they aren't leaked with each iteration. I'll re-work for a
> > v2.
>
> So it turns out this series doesn't leak FDs in the for loop. FDs are
> necessarily closed down in data_loop() when the read() returns 0. The
> only returns from data_loop() are after the FDs have been closed.
> data_loop() and some of the functions it calls will call exit(1) on
> error, but that won't leak FDs.
>
> Please review this series. Sorry for the confusion.
For the whole series:
Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/8] Coverity fixes for vchan-socket-proxy
2020-06-13 12:40 ` Marek Marczykowski-Górecki
@ 2020-06-14 14:05 ` Jason Andryuk
0 siblings, 0 replies; 14+ messages in thread
From: Jason Andryuk @ 2020-06-14 14:05 UTC (permalink / raw)
To: Marek Marczykowski-Górecki; +Cc: xen-devel, Ian Jackson, Wei Liu
On Sat, Jun 13, 2020 at 8:40 AM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> On Wed, May 27, 2020 at 10:59:55PM -0400, Jason Andryuk wrote:
> > On Mon, May 25, 2020 at 6:36 PM Jason Andryuk <jandryuk@gmail.com> wrote:
> > >
> > > On Sun, May 24, 2020 at 10:50 PM Jason Andryuk <jandryuk@gmail.com> wrote:
> > > >
> > > > This series addresses some Coverity reports. To handle closing FDs, a
> > > > state struct is introduced to track FDs closed in both main() and
> > > > data_loop().
> > >
> > > I've realized the changes here are insufficient to handle the FD
> > > leaks. That is, the accept()-ed FDs need to be closed inside the for
> > > loop so they aren't leaked with each iteration. I'll re-work for a
> > > v2.
> >
> > So it turns out this series doesn't leak FDs in the for loop. FDs are
> > necessarily closed down in data_loop() when the read() returns 0. The
> > only returns from data_loop() are after the FDs have been closed.
> > data_loop() and some of the functions it calls will call exit(1) on
> > error, but that won't leak FDs.
> >
> > Please review this series. Sorry for the confusion.
>
> For the whole series:
>
> Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Thanks for the review. Sorry for forgetting to CC you on this series
and the v2 posted on Jun 10th as well. For v2, patch 1 now also
changes strncpy to strcpy to avoid a gcc-10 warning reported by Olaf
Hering. Patches 2 & 3 are new to move some perror calls. v1 patches
2-8 shifted to 4-10 in v2, but are unchanged.
Thanks,
Jason
^ permalink raw reply [flat|nested] 14+ messages in thread