[Qemu-devel] [PATCH] qcow2: avoid memcpy(dst, NULL, len)

[Qemu-devel] [PATCH] qcow2: avoid memcpy(dst, NULL, len)

[Stefan Hajnoczi]

Section "7.1.4 Use of library functions" in the C99 standard says:

  If an argument to a function has an invalid value (such as [...]
  a null pointer [...]) [...] the behavior is undefined.

Additionally the "searching and sorting" functions are specified as
requiring valid pointer values as described in 7.1.4.

This patch fixes the following static analyzer errors:

  block/qcow2.c:1807:41: runtime error: null pointer passed as argument 2, which is declared to never be null
  block/qcow2-cluster.c:86:26: runtime error: null pointer passed as argument 2, which is declared to never be null

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/qcow2-cluster.c | 4 +++-
 block/qcow2.c         | 5 ++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f941835..ab0dcdc 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -83,7 +83,9 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
     }
     memset(new_l1_table, 0, align_offset(new_l1_size2, 512));
 
-    memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
+    if (s->l1_table) {
+        memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
+    }
 
     /* write new table (align to cluster) */
     BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ALLOC_TABLE);
diff --git a/block/qcow2.c b/block/qcow2.c
index c079aa8..758a997 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1804,7 +1804,10 @@ static size_t header_ext_add(char *buf, uint32_t magic, const void *s,
         .magic  = cpu_to_be32(magic),
         .len    = cpu_to_be32(len),
     };
-    memcpy(buf + sizeof(QCowExtension), s, len);
+
+    if (s) {
+        memcpy(buf + sizeof(QCowExtension), s, len);
+    }
 
     return ext_len;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH] qcow2: avoid memcpy(dst, NULL, len)

[Kevin Wolf]

Am 13.09.2016 um 11:15 hat Stefan Hajnoczi geschrieben:
> Section "7.1.4 Use of library functions" in the C99 standard says:
> 
>   If an argument to a function has an invalid value (such as [...]
>   a null pointer [...]) [...] the behavior is undefined.
> 
> Additionally the "searching and sorting" functions are specified as
> requiring valid pointer values as described in 7.1.4.
> 
> This patch fixes the following static analyzer errors:

"sanitizer", not "static analyzer"

>   block/qcow2.c:1807:41: runtime error: null pointer passed as argument 2, which is declared to never be null
>   block/qcow2-cluster.c:86:26: runtime error: null pointer passed as argument 2, which is declared to never be null
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/qcow2-cluster.c | 4 +++-
>  block/qcow2.c         | 5 ++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index f941835..ab0dcdc 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -83,7 +83,9 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
>      }
>      memset(new_l1_table, 0, align_offset(new_l1_size2, 512));
>  
> -    memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
> +    if (s->l1_table) {
> +        memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
> +    }

We could add 'else assert(s->l1_size == 0)' to make the intentions
clearer.  Or actually, maybe the simpler way would be to do
'if (s->l1_size)', then it's obvious that the skipped memcpy() wouldn't
do anything anyway.

>      /* write new table (align to cluster) */
>      BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ALLOC_TABLE);
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c079aa8..758a997 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1804,7 +1804,10 @@ static size_t header_ext_add(char *buf, uint32_t magic, const void *s,
>          .magic  = cpu_to_be32(magic),
>          .len    = cpu_to_be32(len),
>      };
> -    memcpy(buf + sizeof(QCowExtension), s, len);
> +
> +    if (s) {
> +        memcpy(buf + sizeof(QCowExtension), s, len);
> +    }

Same thing here.

With or without the change:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH] qcow2: avoid memcpy(dst, NULL, len)

[Stefan Hajnoczi]

On Tue, Sep 13, 2016 at 10:30 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 13.09.2016 um 11:15 hat Stefan Hajnoczi geschrieben:
>> Section "7.1.4 Use of library functions" in the C99 standard says:
>>
>>   If an argument to a function has an invalid value (such as [...]
>>   a null pointer [...]) [...] the behavior is undefined.
>>
>> Additionally the "searching and sorting" functions are specified as
>> requiring valid pointer values as described in 7.1.4.
>>
>> This patch fixes the following static analyzer errors:
>
> "sanitizer", not "static analyzer"
>
>>   block/qcow2.c:1807:41: runtime error: null pointer passed as argument 2, which is declared to never be null
>>   block/qcow2-cluster.c:86:26: runtime error: null pointer passed as argument 2, which is declared to never be null
>>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  block/qcow2-cluster.c | 4 +++-
>>  block/qcow2.c         | 5 ++++-
>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index f941835..ab0dcdc 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -83,7 +83,9 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
>>      }
>>      memset(new_l1_table, 0, align_offset(new_l1_size2, 512));
>>
>> -    memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
>> +    if (s->l1_table) {
>> +        memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
>> +    }
>
> We could add 'else assert(s->l1_size == 0)' to make the intentions
> clearer.  Or actually, maybe the simpler way would be to do
> 'if (s->l1_size)', then it's obvious that the skipped memcpy() wouldn't
> do anything anyway.
>
>>      /* write new table (align to cluster) */
>>      BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ALLOC_TABLE);
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index c079aa8..758a997 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1804,7 +1804,10 @@ static size_t header_ext_add(char *buf, uint32_t magic, const void *s,
>>          .magic  = cpu_to_be32(magic),
>>          .len    = cpu_to_be32(len),
>>      };
>> -    memcpy(buf + sizeof(QCowExtension), s, len);
>> +
>> +    if (s) {
>> +        memcpy(buf + sizeof(QCowExtension), s, len);
>> +    }
>
> Same thing here.
>
> With or without the change:
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Thanks, I've made your suggested changes.

Re: [Qemu-devel] [PATCH] qcow2: avoid memcpy(dst, NULL, len)

[Paolo Bonzini]

On 13/09/2016 11:15, Stefan Hajnoczi wrote:
> Section "7.1.4 Use of library functions" in the C99 standard says:
> 
>   If an argument to a function has an invalid value (such as [...]
>   a null pointer [...]) [...] the behavior is undefined.
> 
> Additionally the "searching and sorting" functions are specified as
> requiring valid pointer values as described in 7.1.4.
> 
> This patch fixes the following static analyzer errors:
> 
>   block/qcow2.c:1807:41: runtime error: null pointer passed as argument 2, which is declared to never be null
>   block/qcow2-cluster.c:86:26: runtime error: null pointer passed as argument 2, which is declared to never be null
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/qcow2-cluster.c | 4 +++-
>  block/qcow2.c         | 5 ++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index f941835..ab0dcdc 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -83,7 +83,9 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
>      }
>      memset(new_l1_table, 0, align_offset(new_l1_size2, 512));
>  
> -    memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
> +    if (s->l1_table) {
> +        memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
> +    }
>  
>      /* write new table (align to cluster) */
>      BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ALLOC_TABLE);
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c079aa8..758a997 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1804,7 +1804,10 @@ static size_t header_ext_add(char *buf, uint32_t magic, const void *s,
>          .magic  = cpu_to_be32(magic),
>          .len    = cpu_to_be32(len),
>      };
> -    memcpy(buf + sizeof(QCowExtension), s, len);
> +
> +    if (s) {

Should they be "if (len)" and "if (s->l1_size)"?

I think a SIGSEGV is the right thing to do if s->l1_table is NULL but
s->l1_size is non-zero, or if s is NULL but len is non-zero.

Paolo

> +        memcpy(buf + sizeof(QCowExtension), s, len);
> +    }
>  
>      return ext_len;
>  }
> 

Re: [Qemu-devel] [PATCH] qcow2: avoid memcpy(dst, NULL, len)

[Eric Blake]

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

On 09/13/2016 04:15 AM, Stefan Hajnoczi wrote:
> Section "7.1.4 Use of library functions" in the C99 standard says:
> 
>   If an argument to a function has an invalid value (such as [...]
>   a null pointer [...]) [...] the behavior is undefined.
> 
> Additionally the "searching and sorting" functions are specified as
> requiring valid pointer values as described in 7.1.4.
> 
> This patch fixes the following static analyzer errors:
> 
>   block/qcow2.c:1807:41: runtime error: null pointer passed as argument 2, which is declared to never be null
>   block/qcow2-cluster.c:86:26: runtime error: null pointer passed as argument 2, which is declared to never be null
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/qcow2-cluster.c | 4 +++-
>  block/qcow2.c         | 5 ++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)

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

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