* [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy
@ 2020-06-11 3:29 Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 01/10] vchan-socket-proxy: Ensure UNIX path NUL terminated Jason Andryuk
` (10 more replies)
0 siblings, 11 replies; 15+ messages in thread
From: Jason Andryuk @ 2020-06-11 3:29 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Wei Liu, Jason Andryuk
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().
v2 changes "Ensure UNIX path NUL terminated" to avoid a warning with
gcc-10. Also, "Move perror() into listen_socket" and "Move perror()
into connect_socket" are new.
Jason Andryuk (10):
vchan-socket-proxy: Ensure UNIX path NUL terminated
vchan-socket-proxy: Move perror() into listen_socket
vchan-socket-proxy: Move perror() into connect_socket
vchan-socket-proxy: Check xs_watch return value
vchan-socket-proxy: Unify main return value
vchan-socket-proxy: Use a struct to store state
vchan-socket-proxy: Switch data_loop() to take state
vchan-socket-proxy: Set closed FDs to -1
vchan-socket-proxy: Cleanup resources on exit
vchan-socket-proxy: Handle closing shared input/output_fd
tools/libvchan/vchan-socket-proxy.c | 183 ++++++++++++++++++----------
1 file changed, 119 insertions(+), 64 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 01/10] vchan-socket-proxy: Ensure UNIX path NUL terminated
2020-06-11 3:29 [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Jason Andryuk
@ 2020-06-11 3:29 ` Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 02/10] vchan-socket-proxy: Move perror() into listen_socket Jason Andryuk
` (9 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Jason Andryuk @ 2020-06-11 3:29 UTC (permalink / raw)
To: xen-devel; +Cc: Olaf Hering, Ian Jackson, Wei Liu, Jason Andryuk
Check the socket path length to ensure sun_path is NUL terminated.
This was spotted by Citrix's Coverity.
Also use strcpy to avoid a warning "'__builtin_strncpy' specified bound
108 equals destination size [-Werror=stringop-truncation]" flagged by
gcc 10.
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
CC: Olaf Hering <olaf@aepfle.de>
With Ubuntu's gcc-10, which is a pre-release "gcc-10 (Ubuntu
10-20200411-0ubuntu1) 10.0.1 20200411 (experimental)", I couldn't
actualy generate the strncpy warning.
tools/libvchan/vchan-socket-proxy.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/tools/libvchan/vchan-socket-proxy.c b/tools/libvchan/vchan-socket-proxy.c
index 13700c5d67..6ae1d84143 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -148,12 +148,18 @@ 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;
addr.sun_family = AF_UNIX;
- strncpy(addr.sun_path, path_or_fd, sizeof(addr.sun_path));
+ strcpy(addr.sun_path, path_or_fd);
if (connect(fd, (const struct sockaddr *)&addr, sizeof(addr)) == -1) {
close(fd);
return -1;
@@ -174,13 +180,19 @@ 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)
return -1;
addr.sun_family = AF_UNIX;
- strncpy(addr.sun_path, path_or_fd, sizeof(addr.sun_path));
+ strcpy(addr.sun_path, path_or_fd);
if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) == -1) {
close(fd);
return -1;
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 02/10] vchan-socket-proxy: Move perror() into listen_socket
2020-06-11 3:29 [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 01/10] vchan-socket-proxy: Ensure UNIX path NUL terminated Jason Andryuk
@ 2020-06-11 3:29 ` Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 03/10] vchan-socket-proxy: Move perror() into connect_socket Jason Andryuk
` (8 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Jason Andryuk @ 2020-06-11 3:29 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Wei Liu, Jason Andryuk
The use of perror on the return from listen_socket can produce
misleading results like:
UNIX socket path "/tmp/aa....aa" too long (156 >= 108)
listen socket: Success
errno is reset by subsequent system & library calls, so it may be
inaccurate by the time listen_socket returns. Call perror immediately
after failing system calls to print the proper message.
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
tools/libvchan/vchan-socket-proxy.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/libvchan/vchan-socket-proxy.c b/tools/libvchan/vchan-socket-proxy.c
index 6ae1d84143..4edc3a44f5 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -188,16 +188,20 @@ static int listen_socket(const char *path_or_fd) {
/* if not a number, assume a socket path */
fd = socket(AF_UNIX, SOCK_STREAM, 0);
- if (fd == -1)
+ if (fd == -1) {
+ perror("socket");
return -1;
+ }
addr.sun_family = AF_UNIX;
strcpy(addr.sun_path, path_or_fd);
if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) == -1) {
+ perror("bind");
close(fd);
return -1;
}
if (listen(fd, 5) != 0) {
+ perror("listen");
close(fd);
return -1;
}
@@ -419,7 +423,7 @@ int main(int argc, char **argv)
} else {
socket_fd = listen_socket(socket_path);
if (socket_fd == -1) {
- perror("listen socket");
+ fprintf(stderr, "listen socket failed\n");
return 1;
}
}
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 03/10] vchan-socket-proxy: Move perror() into connect_socket
2020-06-11 3:29 [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 01/10] vchan-socket-proxy: Ensure UNIX path NUL terminated Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 02/10] vchan-socket-proxy: Move perror() into listen_socket Jason Andryuk
@ 2020-06-11 3:29 ` Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 04/10] vchan-socket-proxy: Check xs_watch return value Jason Andryuk
` (7 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Jason Andryuk @ 2020-06-11 3:29 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Wei Liu, Jason Andryuk
errno is reset by subsequent system & library calls, so it may be
inaccurate by the time connect_socket returns. Call perror immediately
after failing system calls to print the proper message.
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
tools/libvchan/vchan-socket-proxy.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/libvchan/vchan-socket-proxy.c b/tools/libvchan/vchan-socket-proxy.c
index 4edc3a44f5..c6a83e4712 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -155,12 +155,15 @@ static int connect_socket(const char *path_or_fd) {
}
fd = socket(AF_UNIX, SOCK_STREAM, 0);
- if (fd == -1)
+ if (fd == -1) {
+ perror("socket");
return -1;
+ }
addr.sun_family = AF_UNIX;
strcpy(addr.sun_path, path_or_fd);
if (connect(fd, (const struct sockaddr *)&addr, sizeof(addr)) == -1) {
+ perror("connect");
close(fd);
return -1;
}
@@ -457,7 +460,7 @@ int main(int argc, char **argv)
input_fd = output_fd = connect_socket(socket_path);
}
if (input_fd == -1) {
- perror("connect socket");
+ fprintf(stderr, "connect_socket failed\n");
return 1;
}
if (data_loop(ctrl, input_fd, output_fd) != 0)
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 04/10] vchan-socket-proxy: Check xs_watch return value
2020-06-11 3:29 [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Jason Andryuk
` (2 preceding siblings ...)
2020-06-11 3:29 ` [PATCH v2 03/10] vchan-socket-proxy: Move perror() into connect_socket Jason Andryuk
@ 2020-06-11 3:29 ` Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 05/10] vchan-socket-proxy: Unify main " Jason Andryuk
` (6 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Jason Andryuk @ 2020-06-11 3:29 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, 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 c6a83e4712..196f6016b9 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -232,8 +232,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] 15+ messages in thread
* [PATCH v2 05/10] vchan-socket-proxy: Unify main return value
2020-06-11 3:29 [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Jason Andryuk
` (3 preceding siblings ...)
2020-06-11 3:29 ` [PATCH v2 04/10] vchan-socket-proxy: Check xs_watch return value Jason Andryuk
@ 2020-06-11 3:29 ` Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 06/10] vchan-socket-proxy: Use a struct to store state Jason Andryuk
` (5 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Jason Andryuk @ 2020-06-11 3:29 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, 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 196f6016b9..36a2fe2cb8 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -388,6 +388,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) {
@@ -454,6 +455,8 @@ int main(int argc, char **argv)
xs_close(xs);
}
+ ret = 0;
+
for (;;) {
if (is_server) {
/* wait for vchan connection */
@@ -468,7 +471,8 @@ int main(int argc, char **argv)
}
if (input_fd == -1) {
fprintf(stderr, "connect_socket failed\n");
- return 1;
+ ret = 1;
+ break;
}
if (data_loop(ctrl, input_fd, output_fd) != 0)
break;
@@ -481,14 +485,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;
@@ -500,5 +506,6 @@ int main(int argc, char **argv)
ctrl = NULL;
}
}
- return 0;
+
+ return ret;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 06/10] vchan-socket-proxy: Use a struct to store state
2020-06-11 3:29 [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Jason Andryuk
` (4 preceding siblings ...)
2020-06-11 3:29 ` [PATCH v2 05/10] vchan-socket-proxy: Unify main " Jason Andryuk
@ 2020-06-11 3:29 ` Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 07/10] vchan-socket-proxy: Switch data_loop() to take state Jason Andryuk
` (4 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Jason Andryuk @ 2020-06-11 3:29 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, 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 36a2fe2cb8..a932c94c97 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;
@@ -381,8 +387,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;
@@ -422,15 +429,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) {
@@ -460,21 +467,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) {
fprintf(stderr, "connect_socket failed\n");
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] != '/')
@@ -482,28 +489,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] 15+ messages in thread
* [PATCH v2 07/10] vchan-socket-proxy: Switch data_loop() to take state
2020-06-11 3:29 [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Jason Andryuk
` (5 preceding siblings ...)
2020-06-11 3:29 ` [PATCH v2 06/10] vchan-socket-proxy: Use a struct to store state Jason Andryuk
@ 2020-06-11 3:29 ` Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 08/10] vchan-socket-proxy: Set closed FDs to -1 Jason Andryuk
` (3 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Jason Andryuk @ 2020-06-11 3:29 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, 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 a932c94c97..29a12260ed 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -286,13 +286,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;
@@ -300,15 +300,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)
@@ -319,52 +319,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;
@@ -481,7 +482,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] != '/')
@@ -504,7 +505,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] 15+ messages in thread
* [PATCH v2 08/10] vchan-socket-proxy: Set closed FDs to -1
2020-06-11 3:29 [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Jason Andryuk
` (6 preceding siblings ...)
2020-06-11 3:29 ` [PATCH v2 07/10] vchan-socket-proxy: Switch data_loop() to take state Jason Andryuk
@ 2020-06-11 3:29 ` Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 09/10] vchan-socket-proxy: Cleanup resources on exit Jason Andryuk
` (2 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Jason Andryuk @ 2020-06-11 3:29 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, 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 29a12260ed..cd7629bc4e 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -326,7 +326,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] 15+ messages in thread
* [PATCH v2 09/10] vchan-socket-proxy: Cleanup resources on exit
2020-06-11 3:29 [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Jason Andryuk
` (7 preceding siblings ...)
2020-06-11 3:29 ` [PATCH v2 08/10] vchan-socket-proxy: Set closed FDs to -1 Jason Andryuk
@ 2020-06-11 3:29 ` Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 10/10] vchan-socket-proxy: Handle closing shared input/output_fd Jason Andryuk
2020-06-26 10:18 ` [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Wei Liu
10 siblings, 0 replies; 15+ messages in thread
From: Jason Andryuk @ 2020-06-11 3:29 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, 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 cd7629bc4e..3552783ec2 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -518,5 +518,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] 15+ messages in thread
* [PATCH v2 10/10] vchan-socket-proxy: Handle closing shared input/output_fd
2020-06-11 3:29 [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Jason Andryuk
` (8 preceding siblings ...)
2020-06-11 3:29 ` [PATCH v2 09/10] vchan-socket-proxy: Cleanup resources on exit Jason Andryuk
@ 2020-06-11 3:29 ` Jason Andryuk
2020-06-26 10:18 ` [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Wei Liu
10 siblings, 0 replies; 15+ messages in thread
From: Jason Andryuk @ 2020-06-11 3:29 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, 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 3552783ec2..e1d959c6d1 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -349,6 +349,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] 15+ messages in thread
* Re: [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy
2020-06-11 3:29 [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Jason Andryuk
` (9 preceding siblings ...)
2020-06-11 3:29 ` [PATCH v2 10/10] vchan-socket-proxy: Handle closing shared input/output_fd Jason Andryuk
@ 2020-06-26 10:18 ` Wei Liu
2020-06-26 10:41 ` Paul Durrant
2020-06-26 11:12 ` Marek Marczykowski-Górecki
10 siblings, 2 replies; 15+ messages in thread
From: Wei Liu @ 2020-06-26 10:18 UTC (permalink / raw)
To: Jason Andryuk; +Cc: xen-devel, Ian Jackson, marmarek, Wei Liu, Paul Durrant
On Wed, Jun 10, 2020 at 11:29:26PM -0400, Jason Andryuk 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().
>
> v2 changes "Ensure UNIX path NUL terminated" to avoid a warning with
> gcc-10. Also, "Move perror() into listen_socket" and "Move perror()
> into connect_socket" are new.
>
> Jason Andryuk (10):
> vchan-socket-proxy: Ensure UNIX path NUL terminated
> vchan-socket-proxy: Move perror() into listen_socket
> vchan-socket-proxy: Move perror() into connect_socket
> vchan-socket-proxy: Check xs_watch return value
> vchan-socket-proxy: Unify main return value
> vchan-socket-proxy: Use a struct to store state
> vchan-socket-proxy: Switch data_loop() to take state
> vchan-socket-proxy: Set closed FDs to -1
> vchan-socket-proxy: Cleanup resources on exit
> vchan-socket-proxy: Handle closing shared input/output_fd
Acked-by: Wei Liu <wl@xen.org>
Cc Paul. V1 of this series was posted back in May. I consider this
series bug fixes, so they should be applied for 4.14. The risk is low
because vchan-socket-proxy is a small utility used by a small number of
users.
Marek, you gave Review tags in v1. Do they still apply here?
>
> tools/libvchan/vchan-socket-proxy.c | 183 ++++++++++++++++++----------
> 1 file changed, 119 insertions(+), 64 deletions(-)
>
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy
2020-06-26 10:18 ` [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Wei Liu
@ 2020-06-26 10:41 ` Paul Durrant
2020-06-26 11:12 ` Marek Marczykowski-Górecki
1 sibling, 0 replies; 15+ messages in thread
From: Paul Durrant @ 2020-06-26 10:41 UTC (permalink / raw)
To: 'Wei Liu', 'Jason Andryuk'
Cc: xen-devel, 'Ian Jackson', marmarek
> -----Original Message-----
> From: Wei Liu <wl@xen.org>
> Sent: 26 June 2020 11:18
> To: Jason Andryuk <jandryuk@gmail.com>
> Cc: xen-devel@lists.xenproject.org; Ian Jackson <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org>;
> Paul Durrant <paul@xen.org>; marmarek@invisiblethingslab.com
> Subject: Re: [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy
>
> On Wed, Jun 10, 2020 at 11:29:26PM -0400, Jason Andryuk 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().
> >
> > v2 changes "Ensure UNIX path NUL terminated" to avoid a warning with
> > gcc-10. Also, "Move perror() into listen_socket" and "Move perror()
> > into connect_socket" are new.
> >
> > Jason Andryuk (10):
> > vchan-socket-proxy: Ensure UNIX path NUL terminated
> > vchan-socket-proxy: Move perror() into listen_socket
> > vchan-socket-proxy: Move perror() into connect_socket
> > vchan-socket-proxy: Check xs_watch return value
> > vchan-socket-proxy: Unify main return value
> > vchan-socket-proxy: Use a struct to store state
> > vchan-socket-proxy: Switch data_loop() to take state
> > vchan-socket-proxy: Set closed FDs to -1
> > vchan-socket-proxy: Cleanup resources on exit
> > vchan-socket-proxy: Handle closing shared input/output_fd
>
> Acked-by: Wei Liu <wl@xen.org>
>
> Cc Paul. V1 of this series was posted back in May. I consider this
> series bug fixes, so they should be applied for 4.14. The risk is low
> because vchan-socket-proxy is a small utility used by a small number of
> users.
>
Agreed. Series...
Release-acked-by: Paul Durrant <paul@xen.org>
> Marek, you gave Review tags in v1. Do they still apply here?
>
> >
> > tools/libvchan/vchan-socket-proxy.c | 183 ++++++++++++++++++----------
> > 1 file changed, 119 insertions(+), 64 deletions(-)
> >
> > --
> > 2.25.1
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy
2020-06-26 10:18 ` [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Wei Liu
2020-06-26 10:41 ` Paul Durrant
@ 2020-06-26 11:12 ` Marek Marczykowski-Górecki
2020-06-26 11:31 ` Wei Liu
1 sibling, 1 reply; 15+ messages in thread
From: Marek Marczykowski-Górecki @ 2020-06-26 11:12 UTC (permalink / raw)
To: Wei Liu; +Cc: xen-devel, Paul Durrant, Ian Jackson, Jason Andryuk
[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]
On Fri, Jun 26, 2020 at 10:18:07AM +0000, Wei Liu wrote:
> On Wed, Jun 10, 2020 at 11:29:26PM -0400, Jason Andryuk 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().
> >
> > v2 changes "Ensure UNIX path NUL terminated" to avoid a warning with
> > gcc-10. Also, "Move perror() into listen_socket" and "Move perror()
> > into connect_socket" are new.
> >
> > Jason Andryuk (10):
> > vchan-socket-proxy: Ensure UNIX path NUL terminated
> > vchan-socket-proxy: Move perror() into listen_socket
> > vchan-socket-proxy: Move perror() into connect_socket
> > vchan-socket-proxy: Check xs_watch return value
> > vchan-socket-proxy: Unify main return value
> > vchan-socket-proxy: Use a struct to store state
> > vchan-socket-proxy: Switch data_loop() to take state
> > vchan-socket-proxy: Set closed FDs to -1
> > vchan-socket-proxy: Cleanup resources on exit
> > vchan-socket-proxy: Handle closing shared input/output_fd
>
> Acked-by: Wei Liu <wl@xen.org>
>
> Cc Paul. V1 of this series was posted back in May. I consider this
> series bug fixes, so they should be applied for 4.14. The risk is low
> because vchan-socket-proxy is a small utility used by a small number of
> users.
>
> Marek, you gave Review tags in v1. Do they still apply here?
Yes. And also for the new patches 2-3. I thought I've posted it here,
but must have missed clicking "send"...
> >
> > tools/libvchan/vchan-socket-proxy.c | 183 ++++++++++++++++++----------
> > 1 file changed, 119 insertions(+), 64 deletions(-)
> >
> > --
> > 2.25.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] 15+ messages in thread
* Re: [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy
2020-06-26 11:12 ` Marek Marczykowski-Górecki
@ 2020-06-26 11:31 ` Wei Liu
0 siblings, 0 replies; 15+ messages in thread
From: Wei Liu @ 2020-06-26 11:31 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: xen-devel, Paul Durrant, Ian Jackson, Wei Liu, Jason Andryuk
On Fri, Jun 26, 2020 at 01:12:01PM +0200, Marek Marczykowski-Górecki wrote:
> On Fri, Jun 26, 2020 at 10:18:07AM +0000, Wei Liu wrote:
> > On Wed, Jun 10, 2020 at 11:29:26PM -0400, Jason Andryuk 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().
> > >
> > > v2 changes "Ensure UNIX path NUL terminated" to avoid a warning with
> > > gcc-10. Also, "Move perror() into listen_socket" and "Move perror()
> > > into connect_socket" are new.
> > >
> > > Jason Andryuk (10):
> > > vchan-socket-proxy: Ensure UNIX path NUL terminated
> > > vchan-socket-proxy: Move perror() into listen_socket
> > > vchan-socket-proxy: Move perror() into connect_socket
> > > vchan-socket-proxy: Check xs_watch return value
> > > vchan-socket-proxy: Unify main return value
> > > vchan-socket-proxy: Use a struct to store state
> > > vchan-socket-proxy: Switch data_loop() to take state
> > > vchan-socket-proxy: Set closed FDs to -1
> > > vchan-socket-proxy: Cleanup resources on exit
> > > vchan-socket-proxy: Handle closing shared input/output_fd
> >
> > Acked-by: Wei Liu <wl@xen.org>
> >
> > Cc Paul. V1 of this series was posted back in May. I consider this
> > series bug fixes, so they should be applied for 4.14. The risk is low
> > because vchan-socket-proxy is a small utility used by a small number of
> > users.
> >
> > Marek, you gave Review tags in v1. Do they still apply here?
>
> Yes. And also for the new patches 2-3. I thought I've posted it here,
> but must have missed clicking "send"...
Thanks for confirming.
Wei.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-06-26 11:31 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-11 3:29 [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 01/10] vchan-socket-proxy: Ensure UNIX path NUL terminated Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 02/10] vchan-socket-proxy: Move perror() into listen_socket Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 03/10] vchan-socket-proxy: Move perror() into connect_socket Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 04/10] vchan-socket-proxy: Check xs_watch return value Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 05/10] vchan-socket-proxy: Unify main " Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 06/10] vchan-socket-proxy: Use a struct to store state Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 07/10] vchan-socket-proxy: Switch data_loop() to take state Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 08/10] vchan-socket-proxy: Set closed FDs to -1 Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 09/10] vchan-socket-proxy: Cleanup resources on exit Jason Andryuk
2020-06-11 3:29 ` [PATCH v2 10/10] vchan-socket-proxy: Handle closing shared input/output_fd Jason Andryuk
2020-06-26 10:18 ` [PATCH v2 00/10] Coverity fixes for vchan-socket-proxy Wei Liu
2020-06-26 10:41 ` Paul Durrant
2020-06-26 11:12 ` Marek Marczykowski-Górecki
2020-06-26 11:31 ` Wei Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).