[Qemu-devel] [PATCH RESEND 0/3] easy-to-fix clang warnings

[Qemu-devel] [PATCH RESEND 0/3] easy-to-fix clang warnings

[Philippe Mathieu-Daudé]

This patchset fixes three easy-to-fix clang warnings.

Resent adding Marc-André Lureau's Reviewed-by and CC'ing qemu-trivial as
suggested by Markus Armbruster.

Philippe Mathieu-Daudé (3):
  usb-ccid: make ccid_write_data_block() cope with null buffers
  device_tree: fix compiler warnings (clang 5)
  qga: fix compiler warnings (clang 5)

 device_tree.c                 | 1 +
 hw/usb/dev-smartcard-reader.c | 8 +++++++-
 qga/commands-posix.c          | 8 +++++---
 3 files changed, 13 insertions(+), 4 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers

[Philippe Mathieu-Daudé]

static code analyzer complain:

hw/usb/dev-smartcard-reader.c:816:5: warning: Null pointer passed as an argument to a 'nonnull' parameter
    memcpy(p->abData, data, len);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/usb/dev-smartcard-reader.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 757b8b3f5a..c38a4e5886 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -799,8 +799,14 @@ static void ccid_write_parameters(USBCCIDState *s, CCID_Header *recv)
 static void ccid_write_data_block(USBCCIDState *s, uint8_t slot, uint8_t seq,
                                   const uint8_t *data, uint32_t len)
 {
-    CCID_DataBlock *p = ccid_reserve_recv_buf(s, sizeof(*p) + len);
+    CCID_DataBlock *p;
 
+    if (len == 0) {
+        return;
+    }
+    g_assert(data != NULL);
+
+    p = ccid_reserve_recv_buf(s, sizeof(*p) + len);
     if (p == NULL) {
         return;
     }
-- 
2.11.0

Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers

[Markus Armbruster]

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

> static code analyzer complain:
>
> hw/usb/dev-smartcard-reader.c:816:5: warning: Null pointer passed as an argument to a 'nonnull' parameter
>     memcpy(p->abData, data, len);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/usb/dev-smartcard-reader.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> index 757b8b3f5a..c38a4e5886 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -799,8 +799,14 @@ static void ccid_write_parameters(USBCCIDState *s, CCID_Header *recv)
>  static void ccid_write_data_block(USBCCIDState *s, uint8_t slot, uint8_t seq,
>                                    const uint8_t *data, uint32_t len)
>  {
> -    CCID_DataBlock *p = ccid_reserve_recv_buf(s, sizeof(*p) + len);
> +    CCID_DataBlock *p;
>  
> +    if (len == 0) {
> +        return;

Correct only if messages without data always have the same meaning as no
message.  Gerd?

> +    }
> +    g_assert(data != NULL);

The assertion feels like noise to me.

> +
> +    p = ccid_reserve_recv_buf(s, sizeof(*p) + len);
>      if (p == NULL) {
>          return;
>      }

Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers

[Gerd Hoffmann]

  Hi,

> > +    if (len == 0) {
> > +        return;
> 
> Correct only if messages without data always have the same meaning as no
> message.  Gerd?

Not a ccid expert, but looking through the code it seems writing a
(reply) data block with status and without payload (data = NULL and len
= 0) is perfectly fine and can happen in case no (virtual) smartcard is
inserted into the card reader.  Which this patch breaks.  So,

NACK.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers

[Marc-André Lureau]

Hi

On Thu, Mar 23, 2017 at 11:44 AM Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
>
> > > +    if (len == 0) {
> > > +        return;
> >
> > Correct only if messages without data always have the same meaning as no
> > message.  Gerd?
>
> Not a ccid expert, but looking through the code it seems writing a
> (reply) data block with status and without payload (data = NULL and len
> = 0) is perfectly fine and can happen in case no (virtual) smartcard is
> inserted into the card reader.  Which this patch breaks.  So,
>
> NACK.
>

 oops, there are hard-coded calls with NULL/0. I suppose to fix clang
warning, it would need to check if data != null for memcpy.
-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers

[Gerd Hoffmann]

  Hi,

>  oops, there are hard-coded calls with NULL/0. I suppose to fix clang
> warning, it would need to check if data != null for memcpy.

I'd check for len > 0, and in that if branch we can also assert on data
== NULL and thereby check that len and data are consistent.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>>  oops, there are hard-coded calls with NULL/0. I suppose to fix clang
>> warning, it would need to check if data != null for memcpy.
>
> I'd check for len > 0, and in that if branch we can also assert on data
> == NULL and thereby check that len and data are consistent.

If len is non-zero but data is null, memcpy() will crash just fine by
itself, so why bother asserting.  If len is zero, there's nothing to
assert.

I'd simply protect memcpy() with if (len) and call it a day.

Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers

[Gerd Hoffmann]

On Do, 2017-03-23 at 13:41 +0100, Markus Armbruster wrote:
> Gerd Hoffmann <kraxel@redhat.com> writes:
> 
> >   Hi,
> >
> >>  oops, there are hard-coded calls with NULL/0. I suppose to fix clang
> >> warning, it would need to check if data != null for memcpy.
> >
> > I'd check for len > 0, and in that if branch we can also assert on data
> > == NULL and thereby check that len and data are consistent.
> 
> If len is non-zero but data is null, memcpy() will crash just fine by
> itself, so why bother asserting.

To make clang happy?  But maybe clang is clever enough to figure data
can't be null at that point in case we call memcpy with len != 0
only ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On Do, 2017-03-23 at 13:41 +0100, Markus Armbruster wrote:
>> Gerd Hoffmann <kraxel@redhat.com> writes:
>> 
>> >   Hi,
>> >
>> >>  oops, there are hard-coded calls with NULL/0. I suppose to fix clang
>> >> warning, it would need to check if data != null for memcpy.
>> >
>> > I'd check for len > 0, and in that if branch we can also assert on data
>> > == NULL and thereby check that len and data are consistent.
>> 
>> If len is non-zero but data is null, memcpy() will crash just fine by
>> itself, so why bother asserting.
>
> To make clang happy?  But maybe clang is clever enough to figure data
> can't be null at that point in case we call memcpy with len != 0
> only ...

If Clang needs another hint to become happy, then an assertion is a fine
way to provide it.

Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers

[Philippe Mathieu-Daudé]

Hi Markus, Gerd.

On 03/23/2017 11:08 AM, Markus Armbruster wrote:
> Gerd Hoffmann <kraxel@redhat.com> writes:
>
>> On Do, 2017-03-23 at 13:41 +0100, Markus Armbruster wrote:
>>> Gerd Hoffmann <kraxel@redhat.com> writes:
>>>
>>>>   Hi,
>>>>
>>>>>  oops, there are hard-coded calls with NULL/0. I suppose to fix clang
>>>>> warning, it would need to check if data != null for memcpy.
>>>>
>>>> I'd check for len > 0, and in that if branch we can also assert on data
>>>> == NULL and thereby check that len and data are consistent.
>>>
>>> If len is non-zero but data is null, memcpy() will crash just fine by
>>> itself, so why bother asserting.
>>
>> To make clang happy?  But maybe clang is clever enough to figure data
>> can't be null at that point in case we call memcpy with len != 0
>> only ...
>
> If Clang needs another hint to become happy, then an assertion is a fine
> way to provide it.

Apparently Clang isn't clever enough using an assertion.

I'll resend checking len.

[Qemu-devel] [PATCH RESEND 2/3] device_tree: fix compiler warnings (clang 5)

[Philippe Mathieu-Daudé]

static code analyzer complain:

device_tree.c:155:18: warning: Null pointer passed as an argument to a 'nonnull' parameter
    while ((de = readdir(d)) != NULL) {
                 ^~~~~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 device_tree.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/device_tree.c b/device_tree.c
index 6e06320830..a24ddff02b 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -148,6 +148,7 @@ static void read_fstree(void *fdt, const char *dirname)
     d = opendir(dirname);
     if (!d) {
         error_setg(&error_fatal, "%s cannot open %s", __func__, dirname);
+        return;
     }
 
     while ((de = readdir(d)) != NULL) {
-- 
2.11.0

Re: [Qemu-devel] [PATCH RESEND 2/3] device_tree: fix compiler warnings (clang 5)

[Marc-André Lureau]

On Thu, Mar 23, 2017 at 12:49 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> static code analyzer complain:
>
> device_tree.c:155:18: warning: Null pointer passed as an argument to a
> 'nonnull' parameter
>     while ((de = readdir(d)) != NULL) {
>                  ^~~~~~~~~~
>
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>


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



> ---
>  device_tree.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index 6e06320830..a24ddff02b 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -148,6 +148,7 @@ static void read_fstree(void *fdt, const char *dirname)
>      d = opendir(dirname);
>      if (!d) {
>          error_setg(&error_fatal, "%s cannot open %s", __func__, dirname);
> +        return;
>      }
>
>      while ((de = readdir(d)) != NULL) {
> --
> 2.11.0
>
>
> --
Marc-André Lureau

[Qemu-devel] [PATCH RESEND 3/3] qga: fix compiler warnings (clang 5)

[Philippe Mathieu-Daudé]

static code analyzer complain:

qga/commands-posix.c:2127:9: warning: Null pointer passed as an argument to a 'nonnull' parameter
        closedir(dp);
        ^~~~~~~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/commands-posix.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 73d93eb5ce..8028141534 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2122,9 +2122,11 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
          * we think this VM does not support online/offline memory block,
          * any other solution?
          */
-        if (!dp && errno == ENOENT) {
-            result->response =
-                GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
+        if (!dp) {
+            if (errno == ENOENT) {
+                result->response =
+                    GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
+            }
             goto out1;
         }
         closedir(dp);
-- 
2.11.0

Re: [Qemu-devel] [PATCH RESEND 3/3] qga: fix compiler warnings (clang 5)

[Michael Roth]

Quoting Philippe Mathieu-Daudé (2017-03-22 15:48:44)
> static code analyzer complain:
> 
> qga/commands-posix.c:2127:9: warning: Null pointer passed as an argument to a 'nonnull' parameter
>         closedir(dp);
>         ^~~~~~~~~~~~
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/commands-posix.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 73d93eb5ce..8028141534 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2122,9 +2122,11 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
>           * we think this VM does not support online/offline memory block,
>           * any other solution?
>           */
> -        if (!dp && errno == ENOENT) {
> -            result->response =
> -                GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
> +        if (!dp) {
> +            if (errno == ENOENT) {
> +                result->response =
> +                    GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
> +            }
>              goto out1;
>          }

I think this should be:

    if (!dp) {
        if (errno == ENOENT) {
            result->response =
                GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
        } else {
            result->response =
                GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_FAILED;
        }
        goto out1;
    }

otherwise we might set result->error_code while still indicating
success for the operation. That wasn't handled properly before your
patch either, it's just more apparent now.

>          closedir(dp);
> -- 
> 2.11.0
> 

Re: [Qemu-devel] [PATCH RESEND 3/3] qga: fix compiler warnings (clang 5)

[Philippe Mathieu-Daudé]

Hi Michael,

On 03/22/2017 06:22 PM, Michael Roth wrote:
> Quoting Philippe Mathieu-Daudé (2017-03-22 15:48:44)
>> static code analyzer complain:
>>
>> qga/commands-posix.c:2127:9: warning: Null pointer passed as an argument to a 'nonnull' parameter
>>         closedir(dp);
>>         ^~~~~~~~~~~~
>>
>> Reported-by: Clang Static Analyzer
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  qga/commands-posix.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 73d93eb5ce..8028141534 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -2122,9 +2122,11 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
>>           * we think this VM does not support online/offline memory block,
>>           * any other solution?
>>           */
>> -        if (!dp && errno == ENOENT) {
>> -            result->response =
>> -                GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
>> +        if (!dp) {
>> +            if (errno == ENOENT) {
>> +                result->response =
>> +                    GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
>> +            }
>>              goto out1;
>>          }
>
> I think this should be:
>
>     if (!dp) {
>         if (errno == ENOENT) {
>             result->response =
>                 GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
>         } else {
>             result->response =
>                 GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_FAILED;
>         }
>         goto out1;
>     }
>
> otherwise we might set result->error_code while still indicating
> success for the operation. That wasn't handled properly before your
> patch either, it's just more apparent now.

Indeed, I'll resend it in 2 patches It seems easier to me to understand.

>
>>          closedir(dp);
>> --
>> 2.11.0
>>
>
>