This trips Coverity, which believes the subsequent qio_channel_create_watch
can dereference a NULL pointer.  In reality, tcp_chr_connect's callers
all have s->ioc properly initialized, since they are all rooted at
tcp_chr_new_client.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 chardev/char-socket.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index d057192ced..159e69c3b1 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -550,12 +550,10 @@ static void tcp_chr_connect(void *opaque)
         s->is_listen, s->is_telnet);
 
     s->connected = 1;
-    if (s->ioc) {
-        chr->gsource = io_add_watch_poll(chr, s->ioc,
-                                           tcp_chr_read_poll,
-                                           tcp_chr_read,
-                                           chr, chr->gcontext);
-    }
+    chr->gsource = io_add_watch_poll(chr, s->ioc,
+                                       tcp_chr_read_poll,
+                                       tcp_chr_read,
+                                       chr, chr->gcontext);
 
     s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
     g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
-- 
2.16.2

On Tue, Mar 20, 2018 at 4:18 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> This trips Coverity, which believes the subsequent qio_channel_create_watch
> can dereference a NULL pointer.  In reality, tcp_chr_connect's callers
> all have s->ioc properly initialized, since they are all rooted at
> tcp_chr_new_client.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


>  chardev/char-socket.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index d057192ced..159e69c3b1 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -550,12 +550,10 @@ static void tcp_chr_connect(void *opaque)
>          s->is_listen, s->is_telnet);
>
>      s->connected = 1;
> -    if (s->ioc) {
> -        chr->gsource = io_add_watch_poll(chr, s->ioc,
> -                                           tcp_chr_read_poll,
> -                                           tcp_chr_read,
> -                                           chr, chr->gcontext);
> -    }
> +    chr->gsource = io_add_watch_poll(chr, s->ioc,
> +                                       tcp_chr_read_poll,
> +                                       tcp_chr_read,
> +                                       chr, chr->gcontext);
>
>      s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
>      g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
> --
> 2.16.2
>
>



-- 
Marc-André Lureau

On Tue, Mar 20, 2018 at 04:18:57PM +0100, Paolo Bonzini wrote:
> This trips Coverity, which believes the subsequent qio_channel_create_watch
> can dereference a NULL pointer.  In reality, tcp_chr_connect's callers
> all have s->ioc properly initialized, since they are all rooted at
> tcp_chr_new_client.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

(maybe replacing with an assertion would be nicer? No big deal.)

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

On 21/03/2018 04:25, Peter Xu wrote:
> On Tue, Mar 20, 2018 at 04:18:57PM +0100, Paolo Bonzini wrote:
>> This trips Coverity, which believes the subsequent qio_channel_create_watch
>> can dereference a NULL pointer.  In reality, tcp_chr_connect's callers
>> all have s->ioc properly initialized, since they are all rooted at
>> tcp_chr_new_client.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> (maybe replacing with an assertion would be nicer? No big deal.)

It's already asserting, it just raises SIGSEGV instead of SIGABRT. :)

Paolo

On Wed, Mar 21, 2018 at 06:50:46AM +0100, Paolo Bonzini wrote:
> On 21/03/2018 04:25, Peter Xu wrote:
> > On Tue, Mar 20, 2018 at 04:18:57PM +0100, Paolo Bonzini wrote:
> >> This trips Coverity, which believes the subsequent qio_channel_create_watch
> >> can dereference a NULL pointer.  In reality, tcp_chr_connect's callers
> >> all have s->ioc properly initialized, since they are all rooted at
> >> tcp_chr_new_client.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > (maybe replacing with an assertion would be nicer? No big deal.)
> 
> It's already asserting, it just raises SIGSEGV instead of SIGABRT. :)

Ah yes it's in the next qio_channel_create_watch(). :)

-- 
Peter Xu