[PATCH 0/8] Coverity fixes for vchan-socket-proxy

[PATCH 0/8] Coverity fixes for vchan-socket-proxy

[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().

Jason Andryuk (8):
  vchan-socket-proxy: Ensure UNIX path NUL terminated
  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 | 164 ++++++++++++++++++----------
 1 file changed, 106 insertions(+), 58 deletions(-)

-- 
2.25.1

[PATCH 1/8] vchan-socket-proxy: Ensure UNIX path NUL terminated

[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

[PATCH 2/8] vchan-socket-proxy: Check xs_watch return value

[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

[PATCH 3/8] vchan-socket-proxy: Unify main return value

[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

[PATCH 4/8] vchan-socket-proxy: Use a struct to store state

[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

[PATCH 5/8] vchan-socket-proxy: Switch data_loop() to take state

[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

[PATCH 6/8] vchan-socket-proxy: Set closed FDs to -1

[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

[PATCH 7/8] vchan-socket-proxy: Cleanup resources on exit

[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

[PATCH 8/8] vchan-socket-proxy: Handle closing shared input/output_fd

[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

Re: [PATCH 0/8] Coverity fixes for vchan-socket-proxy

[Jason Andryuk]

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

Re: [PATCH 0/8] Coverity fixes for vchan-socket-proxy

[Jason Andryuk]

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

Re: [PATCH 1/8] vchan-socket-proxy: Ensure UNIX path NUL terminated

[Marek Marczykowski-Górecki]

[-- 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 --]

Re: [PATCH 0/8] Coverity fixes for vchan-socket-proxy

[Marek Marczykowski-Górecki]

[-- 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 --]

Re: [PATCH 0/8] Coverity fixes for vchan-socket-proxy

[Jason Andryuk]

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