[Qemu-devel] [PATCH 0/2] gdbstub: fix a couple of coverity nits

[Qemu-devel] [PATCH 0/2] gdbstub: fix a couple of coverity nits

[Peter Maydell]

Fix a couple of coverity nits in gdbstub.c regarding failing
to check error returns.

Peter Maydell (2):
  gdbstub: Use qemu_set_cloexec()
  gdbstub: Handle errors in gdb_accept()

 gdbstub.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH 1/2] gdbstub: Use qemu_set_cloexec()

[Peter Maydell]

Use the utility routine qemu_set_cloexec() rather than
manually calling fcntl(). This lets us drop the #ifndef _WIN32
guards and also means Coverity doesn't complain that we're
ignoring the fcntl error return (CID 1005665, CID 1005667).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 gdbstub.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 3c3807358c..cc7626c790 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1818,9 +1818,7 @@ static void gdb_accept(void)
             perror("accept");
             return;
         } else if (fd >= 0) {
-#ifndef _WIN32
-            fcntl(fd, F_SETFD, FD_CLOEXEC);
-#endif
+            qemu_set_cloexec(fd);
             break;
         }
     }
@@ -1847,9 +1845,7 @@ static int gdbserver_open(int port)
         perror("socket");
         return -1;
     }
-#ifndef _WIN32
-    fcntl(fd, F_SETFD, FD_CLOEXEC);
-#endif
+    qemu_set_cloexec(fd);
 
     socket_set_fast_reuse(fd);
 
-- 
2.17.0

[Qemu-devel] [PATCH 2/2] gdbstub: Handle errors in gdb_accept()

[Peter Maydell]

In gdb_accept(), we both fail to check all errors (notably
that from socket_set_nodelay(), as Coverity notes in CID 1005666),
and fail to return an error status back to our caller. Correct
both of these things, so that errors in accept() result in our
stopping with a useful error message rather than ignoring it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 gdbstub.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index cc7626c790..7c2bceb040 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1804,7 +1804,7 @@ void gdb_signalled(CPUArchState *env, int sig)
     put_packet(s, buf);
 }
 
-static void gdb_accept(void)
+static bool gdb_accept(void)
 {
     GDBState *s;
     struct sockaddr_in sockaddr;
@@ -1816,7 +1816,7 @@ static void gdb_accept(void)
         fd = accept(gdbserver_fd, (struct sockaddr *)&sockaddr, &len);
         if (fd < 0 && errno != EINTR) {
             perror("accept");
-            return;
+            return false;
         } else if (fd >= 0) {
             qemu_set_cloexec(fd);
             break;
@@ -1824,7 +1824,10 @@ static void gdb_accept(void)
     }
 
     /* set short latency */
-    socket_set_nodelay(fd);
+    if (socket_set_nodelay(fd)) {
+        perror("setsockopt");
+        return false;
+    }
 
     s = g_malloc0(sizeof(GDBState));
     s->c_cpu = first_cpu;
@@ -1833,6 +1836,7 @@ static void gdb_accept(void)
     gdb_has_xml = false;
 
     gdbserver_state = s;
+    return true;
 }
 
 static int gdbserver_open(int port)
@@ -1873,7 +1877,11 @@ int gdbserver_start(int port)
     if (gdbserver_fd < 0)
         return -1;
     /* accept connections */
-    gdb_accept();
+    if (!gdb_accept()) {
+        close(gdbserver_fd);
+        gdbserver_fd = -1;
+        return -1;
+    }
     return 0;
 }
 
-- 
2.17.0

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/2] gdbstub: Use qemu_set_cloexec()

[Philippe Mathieu-Daudé]

On 05/14/2018 02:30 PM, Peter Maydell wrote:
> Use the utility routine qemu_set_cloexec() rather than
> manually calling fcntl(). This lets us drop the #ifndef _WIN32
> guards and also means Coverity doesn't complain that we're
> ignoring the fcntl error return (CID 1005665, CID 1005667).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  gdbstub.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 3c3807358c..cc7626c790 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1818,9 +1818,7 @@ static void gdb_accept(void)
>              perror("accept");
>              return;
>          } else if (fd >= 0) {
> -#ifndef _WIN32
> -            fcntl(fd, F_SETFD, FD_CLOEXEC);
> -#endif
> +            qemu_set_cloexec(fd);
>              break;
>          }
>      }
> @@ -1847,9 +1845,7 @@ static int gdbserver_open(int port)
>          perror("socket");
>          return -1;
>      }
> -#ifndef _WIN32
> -    fcntl(fd, F_SETFD, FD_CLOEXEC);
> -#endif
> +    qemu_set_cloexec(fd);
>  
>      socket_set_fast_reuse(fd);
>  
> 

Re: [Qemu-devel] [PATCH 2/2] gdbstub: Handle errors in gdb_accept()

[Philippe Mathieu-Daudé]

On 05/14/2018 02:30 PM, Peter Maydell wrote:
> In gdb_accept(), we both fail to check all errors (notably
> that from socket_set_nodelay(), as Coverity notes in CID 1005666),
> and fail to return an error status back to our caller. Correct
> both of these things, so that errors in accept() result in our
> stopping with a useful error message rather than ignoring it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  gdbstub.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index cc7626c790..7c2bceb040 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1804,7 +1804,7 @@ void gdb_signalled(CPUArchState *env, int sig)
>      put_packet(s, buf);
>  }
>  
> -static void gdb_accept(void)
> +static bool gdb_accept(void)
>  {
>      GDBState *s;
>      struct sockaddr_in sockaddr;
> @@ -1816,7 +1816,7 @@ static void gdb_accept(void)
>          fd = accept(gdbserver_fd, (struct sockaddr *)&sockaddr, &len);
>          if (fd < 0 && errno != EINTR) {
>              perror("accept");
> -            return;
> +            return false;
>          } else if (fd >= 0) {
>              qemu_set_cloexec(fd);
>              break;
> @@ -1824,7 +1824,10 @@ static void gdb_accept(void)
>      }
>  
>      /* set short latency */
> -    socket_set_nodelay(fd);
> +    if (socket_set_nodelay(fd)) {
> +        perror("setsockopt");
> +        return false;
> +    }
>  
>      s = g_malloc0(sizeof(GDBState));
>      s->c_cpu = first_cpu;
> @@ -1833,6 +1836,7 @@ static void gdb_accept(void)
>      gdb_has_xml = false;
>  
>      gdbserver_state = s;
> +    return true;
>  }
>  
>  static int gdbserver_open(int port)
> @@ -1873,7 +1877,11 @@ int gdbserver_start(int port)
>      if (gdbserver_fd < 0)
>          return -1;
>      /* accept connections */
> -    gdb_accept();
> +    if (!gdb_accept()) {
> +        close(gdbserver_fd);
> +        gdbserver_fd = -1;
> +        return -1;
> +    }
>      return 0;
>  }
>  
> 

Re: [Qemu-devel] [PATCH 1/2] gdbstub: Use qemu_set_cloexec()

[Thomas Huth]

On 14.05.2018 19:30, Peter Maydell wrote:
> Use the utility routine qemu_set_cloexec() rather than
> manually calling fcntl(). This lets us drop the #ifndef _WIN32
> guards and also means Coverity doesn't complain that we're
> ignoring the fcntl error return (CID 1005665, CID 1005667).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  gdbstub.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 2/2] gdbstub: Handle errors in gdb_accept()

[Thomas Huth]

On 14.05.2018 19:30, Peter Maydell wrote:
> In gdb_accept(), we both fail to check all errors (notably
> that from socket_set_nodelay(), as Coverity notes in CID 1005666),
> and fail to return an error status back to our caller. Correct
> both of these things, so that errors in accept() result in our
> stopping with a useful error message rather than ignoring it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  gdbstub.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>