| Message ID | 1318958255-14008-1-git-send-email-pavel.borzenkov@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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.
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.
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);
Spotted by Clang Analyzer Signed-off-by: Pavel Borzenkov <pavel.borzenkov@gmail.com> --- block/qed.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)