Re: [Qemu-trivial] [Qemu-devel] [PULL 20/22] gdbstub: Handle errors in gdb_accept()

[Philippe Mathieu-Daudé]

On 05/24/2018 06:35 PM, Paolo Bonzini wrote:
> On 20/05/2018 08:15, Michael Tokarev wrote:
>> From: Peter Maydell <address@hidden>
>>
>> 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 <address@hidden>
>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>> Reviewed-by: Thomas Huth <address@hidden>
>> Signed-off-by: Michael Tokarev <address@hidden>
>> ---
>>  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;
>>  }