Message ID | 1473758138-19260-1-git-send-email-stefanha@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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>
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.
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; > } >
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>
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; }
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(-)