Patchwork qed: don't pass NULL to memcpy

login
register
mail settings
Submitter Pavel Borzenkov
Date Oct. 18, 2011, 5:17 p.m.
Message ID <1318958255-14008-1-git-send-email-pavel.borzenkov@gmail.com>
Download mbox | patch
Permalink /patch/120467/
State New
Headers show

Comments

Pavel Borzenkov - Oct. 18, 2011, 5:17 p.m.
Spotted by Clang Analyzer

Signed-off-by: Pavel Borzenkov <pavel.borzenkov@gmail.com>
---
 block/qed.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Stefan Hajnoczi - Oct. 20, 2011, 5:23 p.m.
On Tue, Oct 18, 2011 at 09:17:35PM +0400, Pavel Borzenkov wrote:
> Spotted by Clang Analyzer
> 
> Signed-off-by: Pavel Borzenkov <pavel.borzenkov@gmail.com>
> ---
>  block/qed.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)

Thanks, applied to the trivial patches tree:
http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches

Stefan
Paolo Bonzini - Oct. 21, 2011, 6:35 a.m.
On 10/20/2011 07:23 PM, Stefan Hajnoczi wrote:
> On Tue, Oct 18, 2011 at 09:17:35PM +0400, Pavel Borzenkov wrote:
>> Spotted by Clang Analyzer
>>
>> Signed-off-by: Pavel Borzenkov<pavel.borzenkov@gmail.com>
>> ---
>>   block/qed.c |    6 ++++--
>>   1 files changed, 4 insertions(+), 2 deletions(-)
>
> Thanks, applied to the trivial patches tree:
> http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches

I think there are other places in the tree where we assume that 
"memcpy(dest, NULL, 0);" works.

Paolo
Markus Armbruster - Oct. 21, 2011, 8:31 a.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 10/20/2011 07:23 PM, Stefan Hajnoczi wrote:
>> On Tue, Oct 18, 2011 at 09:17:35PM +0400, Pavel Borzenkov wrote:
>>> Spotted by Clang Analyzer
>>>
>>> Signed-off-by: Pavel Borzenkov<pavel.borzenkov@gmail.com>
>>> ---
>>>   block/qed.c |    6 ++++--
>>>   1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> Thanks, applied to the trivial patches tree:
>> http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches
>
> I think there are other places in the tree where we assume that
> "memcpy(dest, NULL, 0);" works.

Looks like a fair assumption to me.
Pavel Borzenkov - Oct. 21, 2011, 9:06 a.m.
On 10/21/2011 12:31 PM, Markus Armbruster wrote:
> Paolo Bonzini<pbonzini@redhat.com>  writes:
>
>> On 10/20/2011 07:23 PM, Stefan Hajnoczi wrote:
>>> On Tue, Oct 18, 2011 at 09:17:35PM +0400, Pavel Borzenkov wrote:
>>>> Spotted by Clang Analyzer
>>>>
>>>> Signed-off-by: Pavel Borzenkov<pavel.borzenkov@gmail.com>
>>>> ---
>>>>    block/qed.c |    6 ++++--
>>>>    1 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> Thanks, applied to the trivial patches tree:
>>> http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches
>>
>> I think there are other places in the tree where we assume that
>> "memcpy(dest, NULL, 0);" works.
>
> Looks like a fair assumption to me.

Such calls work, but this is UB. Calling string functions with NULL
pointers and zero length is a violation of the C99 standard:

7.21.1 String function conventions
Where an argument declared as size_t n specifies the length of the
array for a function, n can have the value zero on a call to that
function. Unless explicitly stated otherwise in the description of a
particular function in this subclause, pointer arguments on such a
call shall still have valid values, as described in 7.1.4.
Pavel Borzenkov - Oct. 21, 2011, 9:08 a.m.
On Fri, Oct 21, 2011 at 12:31 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 10/20/2011 07:23 PM, Stefan Hajnoczi wrote:
>>> On Tue, Oct 18, 2011 at 09:17:35PM +0400, Pavel Borzenkov wrote:
>>>> Spotted by Clang Analyzer
>>>>
>>>> Signed-off-by: Pavel Borzenkov<pavel.borzenkov@gmail.com>
>>>> ---
>>>>   block/qed.c |    6 ++++--
>>>>   1 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> Thanks, applied to the trivial patches tree:
>>> http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches
>>
>> I think there are other places in the tree where we assume that
>> "memcpy(dest, NULL, 0);" works.
>
> Looks like a fair assumption to me.
>

Such calls work, but this is UB. Calling string functions with NULL
pointers and zero length is a violation of the C99 standard:

7.21.1 String function conventions
Where an argument declared as size_t n specifies the length of the
array for a function, n can have the value zero on a call to that
function. Unless explicitly stated otherwise in the description of a
particular function in this subclause, pointer arguments on such a
call shall still have valid values, as described in 7.1.4.

Patch

diff --git a/block/qed.c b/block/qed.c
index c3e45af..e6720db 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1424,8 +1424,10 @@  static int bdrv_qed_change_backing_file(BlockDriverState *bs,
     memcpy(buffer, &le_header, sizeof(le_header));
     buffer_len = sizeof(le_header);
 
-    memcpy(buffer + buffer_len, backing_file, backing_file_len);
-    buffer_len += backing_file_len;
+    if (backing_file) {
+        memcpy(buffer + buffer_len, backing_file, backing_file_len);
+        buffer_len += backing_file_len;
+    }
 
     /* Write new header */
     ret = bdrv_pwrite_sync(bs->file, 0, buffer, buffer_len);