* [Qemu-devel] [PATCH] qcow2: avoid memcpy(dst, NULL, len)
@ 2016-09-13 9:15 Stefan Hajnoczi
2016-09-13 9:30 ` Kevin Wolf
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Stefan Hajnoczi @ 2016-09-13 9:15 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, qemu-block, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] qcow2: avoid memcpy(dst, NULL, len)
2016-09-13 9:15 [Qemu-devel] [PATCH] qcow2: avoid memcpy(dst, NULL, len) Stefan Hajnoczi
@ 2016-09-13 9:30 ` Kevin Wolf
2016-09-13 9:56 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2016-09-13 13:14 ` [Qemu-devel] " Paolo Bonzini
2016-09-13 15:13 ` Eric Blake
2 siblings, 1 reply; 5+ messages in thread
From: Kevin Wolf @ 2016-09-13 9:30 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, qemu-block
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>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH] qcow2: avoid memcpy(dst, NULL, len)
2016-09-13 9:30 ` Kevin Wolf
@ 2016-09-13 9:56 ` Stefan Hajnoczi
0 siblings, 0 replies; 5+ messages in thread
From: Stefan Hajnoczi @ 2016-09-13 9:56 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Stefan Hajnoczi, qemu-devel, qemu block
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] qcow2: avoid memcpy(dst, NULL, len)
2016-09-13 9:15 [Qemu-devel] [PATCH] qcow2: avoid memcpy(dst, NULL, len) Stefan Hajnoczi
2016-09-13 9:30 ` Kevin Wolf
@ 2016-09-13 13:14 ` Paolo Bonzini
2016-09-13 15:13 ` Eric Blake
2 siblings, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2016-09-13 13:14 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel; +Cc: kwolf, qemu-block
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;
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] qcow2: avoid memcpy(dst, NULL, len)
2016-09-13 9:15 [Qemu-devel] [PATCH] qcow2: avoid memcpy(dst, NULL, len) Stefan Hajnoczi
2016-09-13 9:30 ` Kevin Wolf
2016-09-13 13:14 ` [Qemu-devel] " Paolo Bonzini
@ 2016-09-13 15:13 ` Eric Blake
2 siblings, 0 replies; 5+ messages in thread
From: Eric Blake @ 2016-09-13 15:13 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel; +Cc: kwolf, qemu-block
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-09-13 15:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-13 9:15 [Qemu-devel] [PATCH] qcow2: avoid memcpy(dst, NULL, len) Stefan Hajnoczi
2016-09-13 9:30 ` Kevin Wolf
2016-09-13 9:56 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2016-09-13 13:14 ` [Qemu-devel] " Paolo Bonzini
2016-09-13 15:13 ` Eric Blake
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).