[Qemu-devel] [PATCH v3 0/4] Fix some errors spotted by Coverity

[Qemu-devel] [PATCH v3 0/4] Fix some errors spotted by Coverity

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Fix some errors spotted by Coverity.

Changes since v2:
 * remove a duplicate patch about block
 * fix changelog about json-parser suggested by Luiz
 * remove a patch which have been applied about vnc.

Changes since v1:
 * rebase on Laszlo's patch set about dump
 * remove a patch for smbus
 * rework two patches by Paolo and Stefan's suggestion

Gonglei (4):
  json-parser: drop superfluous assignment for token variable
  qemu-bridge-helper: Fix fd leak in main()
  audio: Fix using freed pointer in wav_fini_out()
  vga: Fix divide-by-zero in vga_update_text

 audio/wavaudio.c      |  4 ++--
 hw/display/vga.c      |  2 +-
 qemu-bridge-helper.c  |  9 +++++++--
 qobject/json-parser.c | 15 ++-------------
 4 files changed, 12 insertions(+), 18 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v3 1/4] json-parser: drop superfluous assignment for token variable

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: ChenLiang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 qobject/json-parser.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index e46c264..4288267 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -423,7 +423,6 @@ static QObject *parse_object(JSONParserContext *ctxt, va_list *ap)
     if (!token_is_operator(token, '{')) {
         goto out;
     }
-    token = NULL;
 
     dict = qdict_new();
 
@@ -449,7 +448,6 @@ static QObject *parse_object(JSONParserContext *ctxt, va_list *ap)
                 parse_error(ctxt, token, "expected separator in dict");
                 goto out;
             }
-            token = NULL;
 
             if (parse_pair(ctxt, dict, ap) == -1) {
                 goto out;
@@ -461,10 +459,8 @@ static QObject *parse_object(JSONParserContext *ctxt, va_list *ap)
                 goto out;
             }
         }
-        token = NULL;
     } else {
-        token = parser_context_pop_token(ctxt);
-        token = NULL;
+        (void)parser_context_pop_token(ctxt);
     }
 
     return QOBJECT(dict);
@@ -487,10 +483,8 @@ static QObject *parse_array(JSONParserContext *ctxt, va_list *ap)
     }
 
     if (!token_is_operator(token, '[')) {
-        token = NULL;
         goto out;
     }
-    token = NULL;
 
     list = qlist_new();
 
@@ -523,8 +517,6 @@ static QObject *parse_array(JSONParserContext *ctxt, va_list *ap)
                 goto out;
             }
 
-            token = NULL;
-
             obj = parse_value(ctxt, ap);
             if (obj == NULL) {
                 parse_error(ctxt, token, "expecting value");
@@ -539,11 +531,8 @@ static QObject *parse_array(JSONParserContext *ctxt, va_list *ap)
                 goto out;
             }
         }
-
-        token = NULL;
     } else {
-        token = parser_context_pop_token(ctxt);
-        token = NULL;
+        (void)parser_context_pop_token(ctxt);
     }
 
     return QOBJECT(list);
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v3 1/4] json-parser: drop superfluous assignment for token variable

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 922 bytes --]

On 06/10/2014 03:20 AM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Signed-off-by: ChenLiang <chenliang88@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  qobject/json-parser.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> @@ -461,10 +459,8 @@ static QObject *parse_object(JSONParserContext *ctxt, va_list *ap)
>                  goto out;
>              }
>          }
> -        token = NULL;
>      } else {
> -        token = parser_context_pop_token(ctxt);
> -        token = NULL;
> +        (void)parser_context_pop_token(ctxt);

Does the compiler warn if you drop the cast-to-void? I tend to think of
it as fluff that usually doesn't add anything.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 1/4] json-parser: drop superfluous assignment for token variable

[Luiz Capitulino]

On Tue, 10 Jun 2014 17:20:24 +0800
<arei.gonglei@huawei.com> wrote:

> From: Gonglei <arei.gonglei@huawei.com>
> 
> Signed-off-by: ChenLiang <chenliang88@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>

Applied to the qmp branch, thanks.

> ---
>  qobject/json-parser.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index e46c264..4288267 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -423,7 +423,6 @@ static QObject *parse_object(JSONParserContext *ctxt, va_list *ap)
>      if (!token_is_operator(token, '{')) {
>          goto out;
>      }
> -    token = NULL;
>  
>      dict = qdict_new();
>  
> @@ -449,7 +448,6 @@ static QObject *parse_object(JSONParserContext *ctxt, va_list *ap)
>                  parse_error(ctxt, token, "expected separator in dict");
>                  goto out;
>              }
> -            token = NULL;
>  
>              if (parse_pair(ctxt, dict, ap) == -1) {
>                  goto out;
> @@ -461,10 +459,8 @@ static QObject *parse_object(JSONParserContext *ctxt, va_list *ap)
>                  goto out;
>              }
>          }
> -        token = NULL;
>      } else {
> -        token = parser_context_pop_token(ctxt);
> -        token = NULL;
> +        (void)parser_context_pop_token(ctxt);
>      }
>  
>      return QOBJECT(dict);
> @@ -487,10 +483,8 @@ static QObject *parse_array(JSONParserContext *ctxt, va_list *ap)
>      }
>  
>      if (!token_is_operator(token, '[')) {
> -        token = NULL;
>          goto out;
>      }
> -    token = NULL;
>  
>      list = qlist_new();
>  
> @@ -523,8 +517,6 @@ static QObject *parse_array(JSONParserContext *ctxt, va_list *ap)
>                  goto out;
>              }
>  
> -            token = NULL;
> -
>              obj = parse_value(ctxt, ap);
>              if (obj == NULL) {
>                  parse_error(ctxt, token, "expecting value");
> @@ -539,11 +531,8 @@ static QObject *parse_array(JSONParserContext *ctxt, va_list *ap)
>                  goto out;
>              }
>          }
> -
> -        token = NULL;
>      } else {
> -        token = parser_context_pop_token(ctxt);
> -        token = NULL;
> +        (void)parser_context_pop_token(ctxt);
>      }
>  
>      return QOBJECT(list);

[Qemu-devel] [PATCH v3 2/4] qemu-bridge-helper: Fix fd leak in main()

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

initialize fd and ctlfd, and close them at the end

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 qemu-bridge-helper.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 6a0974e..36eb3bc 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -229,7 +229,7 @@ int main(int argc, char **argv)
     unsigned long ifargs[4];
 #endif
     int ifindex;
-    int fd, ctlfd, unixfd = -1;
+    int fd = -1, ctlfd = -1, unixfd = -1;
     int use_vnet = 0;
     int mtu;
     const char *bridge = NULL;
@@ -436,7 +436,12 @@ int main(int argc, char **argv)
     /* profit! */
 
 cleanup:
-
+    if (fd >= 0) {
+        close(fd);
+    }
+    if (ctlfd >= 0) {
+        close(ctlfd);
+    }
     while ((acl_rule = QSIMPLEQ_FIRST(&acl_list)) != NULL) {
         QSIMPLEQ_REMOVE_HEAD(&acl_list, entry);
         g_free(acl_rule);
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v3 2/4] qemu-bridge-helper: Fix fd leak in main()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

On Tue, Jun 10, 2014 at 05:20:25PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> initialize fd and ctlfd, and close them at the end
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  qemu-bridge-helper.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Thanks, applied to my net tree:
https://github.com/stefanha/qemu/commits/net

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH v3 3/4] audio: Fix using freed pointer in wav_fini_out()

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Spotted by Coverity:

(8) Event freed_arg:  "fclose(FILE *)" frees "wav->f".
(9) Event cond_true:  Condition "fclose(wav->f)", taking true branch
Also see events:  [pass_freed_arg]

212         if (fclose (wav->f))  {
(10) Event pass_freed_arg:  Passing freed pointer "wav->f" as an argument
to function "AUD_log(char const *, char const *, ...)".
Also see events:  [freed_arg]

213             dolog ("wav_fini_out: fclose %p failed\nReason: %s\n",
214                    wav->f, strerror (errno));

Removed wav->f's pointer in error log, actually it's uselessly.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 audio/wavaudio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/audio/wavaudio.c b/audio/wavaudio.c
index 6846a1a..9bbe8e9 100644
--- a/audio/wavaudio.c
+++ b/audio/wavaudio.c
@@ -210,8 +210,8 @@ static void wav_fini_out (HWVoiceOut *hw)
 
  doclose:
     if (fclose (wav->f))  {
-        dolog ("wav_fini_out: fclose %p failed\nReason: %s\n",
-               wav->f, strerror (errno));
+        dolog ("wav_fini_out: fclose 'wav->f' failed\nReason: %s\n",
+              strerror (errno));
     }
     wav->f = NULL;
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v3 3/4] audio: Fix using freed pointer in wav_fini_out()

[Peter Maydell]

On 10 June 2014 10:20,  <arei.gonglei@huawei.com> wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> Spotted by Coverity:
>
> (8) Event freed_arg:  "fclose(FILE *)" frees "wav->f".
> (9) Event cond_true:  Condition "fclose(wav->f)", taking true branch
> Also see events:  [pass_freed_arg]
>
> 212         if (fclose (wav->f))  {
> (10) Event pass_freed_arg:  Passing freed pointer "wav->f" as an argument
> to function "AUD_log(char const *, char const *, ...)".
> Also see events:  [freed_arg]
>
> 213             dolog ("wav_fini_out: fclose %p failed\nReason: %s\n",
> 214                    wav->f, strerror (errno));

This is actually a coverity false positive -- we're
not using the freed pointer, just printing its value.
However, there's not much value in printing the pointer
value to the error log, especially since we don't
print the pointer value on fopen() or any other logging,
so you can't match up the fclose pointer with anything else.

> Removed wav->f's pointer in error log, actually it's uselessly.
>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  audio/wavaudio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/audio/wavaudio.c b/audio/wavaudio.c
> index 6846a1a..9bbe8e9 100644
> --- a/audio/wavaudio.c
> +++ b/audio/wavaudio.c
> @@ -210,8 +210,8 @@ static void wav_fini_out (HWVoiceOut *hw)
>
>   doclose:
>      if (fclose (wav->f))  {
> -        dolog ("wav_fini_out: fclose %p failed\nReason: %s\n",
> -               wav->f, strerror (errno));
> +        dolog ("wav_fini_out: fclose 'wav->f' failed\nReason: %s\n",
> +              strerror (errno));

I would just drop the 'wav->f' here.

>      }
>      wav->f = NULL;
>
> --

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 3/4] audio: Fix using freed pointer in wav_fini_out()

[Gerd Hoffmann]

  Hi,

> >   doclose:
> >      if (fclose (wav->f))  {
> > -        dolog ("wav_fini_out: fclose %p failed\nReason: %s\n",
> > -               wav->f, strerror (errno));
> > +        dolog ("wav_fini_out: fclose 'wav->f' failed\nReason: %s\n",
> > +              strerror (errno));
> 
> I would just drop the 'wav->f' here.

Or drop the whole message?  It's a highly unlikely error condition after
all, and we can't do much about it anyway.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v3 3/4] audio: Fix using freed pointer in wav_fini_out()

[Gonglei (Arei)]

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Thursday, June 12, 2014 6:32 PM
> To: Peter Maydell
> Cc: Gonglei (Arei); QEMU Developers; Huangweidong (C); Luonengjun; Luiz
> Capitulino; Vassili Karpov; Stefan Hajnoczi; Paolo Bonzini
> Subject: Re: [Qemu-devel] [PATCH v3 3/4] audio: Fix using freed pointer in
> wav_fini_out()
> 
>   Hi,
> 
> > >   doclose:
> > >      if (fclose (wav->f))  {
> > > -        dolog ("wav_fini_out: fclose %p failed\nReason: %s\n",
> > > -               wav->f, strerror (errno));
> > > +        dolog ("wav_fini_out: fclose 'wav->f' failed\nReason: %s\n",
> > > +              strerror (errno));
> >
> > I would just drop the 'wav->f' here.
> 
> Or drop the whole message?  It's a highly unlikely error condition after
> all, and we can't do much about it anyway.
> 
Agreed.  Gerd, would you like a new version?


Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v3 3/4] audio: Fix using freed pointer in wav_fini_out()

[Gerd Hoffmann]

> -        dolog ("wav_fini_out: fclose %p failed\nReason: %s\n",
> > > > -               wav->f, strerror (errno));
> > > > +        dolog ("wav_fini_out: fclose 'wav->f' failed\nReason: %s\n",
> > > > +              strerror (errno));
> > >
> > > I would just drop the 'wav->f' here.
> > 
> > Or drop the whole message?  It's a highly unlikely error condition after
> > all, and we can't do much about it anyway.
> > 
> Agreed.  Gerd, would you like a new version?

yes, please.

thanks,
  Gerd

[Qemu-devel] [PATCH v3 4/4] vga: Fix divide-by-zero in vga_update_text

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Spotted by Coverity:

(20) Event cond_true:  Condition "cursor_visible", taking true branch
(21) Event cond_true:  Condition "cursor_offset < size", taking true branch
(22) Event cond_true:  Condition "cursor_offset >= 0", taking true branch

2097    if (cursor_visible && cursor_offset < size && cursor_offset >= 0)
(23) Event divide_by_zero:  In expression "cursor_offset / width",
division by expression "width" which may be zero has undefined behavior.

2098                    dpy_text_cursor(s->con,
2099                                    TEXTMODE_X(cursor_offset),
2100                                    TEXTMODE_Y(cursor_offset));

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/display/vga.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 8cd6afe..3c1c6eb 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2094,7 +2094,7 @@ static void vga_update_text(void *opaque, console_ch_t *chardata)
             s->cr[VGA_CRTC_CURSOR_START] != s->cursor_start ||
             s->cr[VGA_CRTC_CURSOR_END] != s->cursor_end || full_update) {
             cursor_visible = !(s->cr[VGA_CRTC_CURSOR_START] & 0x20);
-            if (cursor_visible && cursor_offset < size && cursor_offset >= 0)
+            if (cursor_visible && cursor_offset < size && cursor_offset > 0)
                 dpy_text_cursor(s->con,
                                 TEXTMODE_X(cursor_offset),
                                 TEXTMODE_Y(cursor_offset));
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v3 4/4] vga: Fix divide-by-zero in vga_update_text

[Gerd Hoffmann]

  Hi,

> 2097    if (cursor_visible && cursor_offset < size && cursor_offset >= 0)
> (23) Event divide_by_zero:  In expression "cursor_offset / width",
> division by expression "width" which may be zero has undefined behavior.

> -            if (cursor_visible && cursor_offset < size && cursor_offset >= 0)
> +            if (cursor_visible && cursor_offset < size && cursor_offset > 0)
>                  dpy_text_cursor(s->con,
>                                  TEXTMODE_X(cursor_offset),
>                                  TEXTMODE_Y(cursor_offset));

That doesn't fix the reported issue.  It's "width" which Coverity thinks
might be zero, not cursor_offset.  And cursor_offset being zero is
perfectly fine, happens when the cursor is in the upper left corner.

I have no idea why Coverity thinks width can be zero there.  Line 2047:

        width = (s->cr[VGA_CRTC_H_DISP] + 1);

(where cr is uint8_t).  Hmm, maybe for the wraparound case (i.e.
s->cr[VGA_CRTC_H_DISP] == 0xff)?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v3 4/4] vga: Fix divide-by-zero in vga_update_text

[Paolo Bonzini]

Il 12/06/2014 12:43, Gerd Hoffmann ha scritto:
> That doesn't fix the reported issue.  It's "width" which Coverity thinks
> might be zero, not cursor_offset.  And cursor_offset being zero is
> perfectly fine, happens when the cursor is in the upper left corner.
>
> I have no idea why Coverity thinks width can be zero there.  Line 2047:
>
>         width = (s->cr[VGA_CRTC_H_DISP] + 1);
>
> (where cr is uint8_t).  Hmm, maybe for the wraparound case (i.e.
> s->cr[VGA_CRTC_H_DISP] == 0xff)?

Not even that, the result is 0x100, math is done on the "int" data type.

In fact I don't even see this defect on scan.coverity.com.

Paolo

Re: [Qemu-devel] [PATCH v3 4/4] vga: Fix divide-by-zero in vga_update_text

[Gonglei (Arei)]

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Thursday, June 12, 2014 6:44 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; lcapitulino@redhat.com;
> av1474@comtv.ru; stefanha@redhat.com; Luonengjun; Huangweidong (C)
> Subject: Re: [PATCH v3 4/4] vga: Fix divide-by-zero in vga_update_text
> 
>   Hi,
> 
> > 2097    if (cursor_visible && cursor_offset < size && cursor_offset >= 0)
> > (23) Event divide_by_zero:  In expression "cursor_offset / width",
> > division by expression "width" which may be zero has undefined behavior.
> 
> > -            if (cursor_visible && cursor_offset < size && cursor_offset >=
> 0)
> > +            if (cursor_visible && cursor_offset < size && cursor_offset >
> 0)
> >                  dpy_text_cursor(s->con,
> >                                  TEXTMODE_X(cursor_offset),
> >                                  TEXTMODE_Y(cursor_offset));
> 
> That doesn't fix the reported issue.  It's "width" which Coverity thinks
> might be zero, not cursor_offset.  And cursor_offset being zero is
> perfectly fine, happens when the cursor is in the upper left corner.
> 
Yep, I'm sorry for this fault.

> I have no idea why Coverity thinks width can be zero there.  Line 2047:
> 
>         width = (s->cr[VGA_CRTC_H_DISP] + 1);
> 
> (where cr is uint8_t).  Hmm, maybe for the wraparound case (i.e.
> s->cr[VGA_CRTC_H_DISP] == 0xff)?
> 
> cheers,
>   Gerd
> 

Best regards,
-Gonglei