[PULL,20/22] gdbstub: Handle errors in gdb_accept()

Message ID 2f652224f76c115f6c991766b7acac1e22580954.1526796813.git.mjt@msgid.tls.msk.ru
State New
Headers show
Series
  • Untitled series #11519
Related show

Commit Message

Michael Tokarev May 20, 2018, 6:15 a.m.
From: Peter Maydell <peter.maydell@linaro.org>


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>

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

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

---
 gdbstub.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

-- 
2.11.0

Comments

Paolo Bonzini May 24, 2018, 9:35 p.m. | #1
On 20/05/2018 08:15, Michael Tokarev wrote:
> From: Peter Maydell <peter.maydell@linaro.org>

> 

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

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

> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

> ---

>  gdbstub.c | 16 ++++++++++++----

>  1 file changed, 12 insertions(+), 4 deletions(-)

> 

> diff --git a/gdbstub.c b/gdbstub.c

> index b99980d2e2..e4ece2f5bc 100644

> --- a/gdbstub.c

> +++ b/gdbstub.c

> @@ -1814,7 +1814,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;

> @@ -1826,7 +1826,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;

> @@ -1834,7 +1834,10 @@ static void gdb_accept(void)

>      }

>  

>      /* set short latency */

> -    socket_set_nodelay(fd);

> +    if (socket_set_nodelay(fd)) {

> +        perror("setsockopt");

> +        return false;


Coverity notes that this leaks fd.

Paolo

> +    }

>  

>      s = g_malloc0(sizeof(GDBState));

>      s->c_cpu = first_cpu;

> @@ -1843,6 +1846,7 @@ static void gdb_accept(void)

>      gdb_has_xml = false;

>  

>      gdbserver_state = s;

> +    return true;

>  }

>  

>  static int gdbserver_open(int port)

> @@ -1883,7 +1887,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;

>  }

>  

>
Philippe Mathieu-Daudé May 24, 2018, 10:23 p.m. | #2
On 05/24/2018 06:35 PM, Paolo Bonzini wrote:
> On 20/05/2018 08:15, Michael Tokarev wrote:

>> From: Peter Maydell <peter.maydell@linaro.org>

>>

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

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

>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

>> ---

>>  gdbstub.c | 16 ++++++++++++----

>>  1 file changed, 12 insertions(+), 4 deletions(-)

>>

>> diff --git a/gdbstub.c b/gdbstub.c

>> index b99980d2e2..e4ece2f5bc 100644

>> --- a/gdbstub.c

>> +++ b/gdbstub.c

>> @@ -1814,7 +1814,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;

>> @@ -1826,7 +1826,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;

>> @@ -1834,7 +1834,10 @@ static void gdb_accept(void)

>>      }

>>  

>>      /* set short latency */

>> -    socket_set_nodelay(fd);

>> +    if (socket_set_nodelay(fd)) {

>> +        perror("setsockopt");

>> +        return false;

> 

> Coverity notes that this leaks fd.


Oops didn't noticed. It this is so trivial than Coverity could directly
send the fix to the list, like modern compilers.
>> +    }

>>  

>>      s = g_malloc0(sizeof(GDBState));

>>      s->c_cpu = first_cpu;

>> @@ -1843,6 +1846,7 @@ static void gdb_accept(void)

>>      gdb_has_xml = false;

>>  

>>      gdbserver_state = s;

>> +    return true;

>>  }

>>  

>>  static int gdbserver_open(int port)

>> @@ -1883,7 +1887,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;

>>  }

Patch

diff --git a/gdbstub.c b/gdbstub.c
index b99980d2e2..e4ece2f5bc 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1814,7 +1814,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;
@@ -1826,7 +1826,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;
@@ -1834,7 +1834,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;
@@ -1843,6 +1846,7 @@  static void gdb_accept(void)
     gdb_has_xml = false;
 
     gdbserver_state = s;
+    return true;
 }
 
 static int gdbserver_open(int port)
@@ -1883,7 +1887,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;
 }