| Message ID | 1535739372-24454-5-git-send-email-Liam.Merwick@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | off-by-one and NULL pointer accesses detected by static analysis | expand |
On 08/31/2018 02:16 PM, Liam Merwick wrote: > The function block_job_get() may return NULL so before dereferencing > the 'job' pointer in img_commit() it should be checked. > > Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com> > Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com> > Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com> Reviewed-by: John Snow <jsnow@redhat.com> > --- > qemu-img.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/qemu-img.c b/qemu-img.c > index b12f4cd19b0a..51fe09bd08ed 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1029,6 +1029,9 @@ static int img_commit(int argc, char **argv) > } > > job = block_job_get("commit"); > + if (job == NULL) { > + goto unref_backing; > + } > run_block_job(job, &local_err); > if (local_err) { > goto unref_backing; >
On 31.08.18 20:16, Liam Merwick wrote: > The function block_job_get() may return NULL so before dereferencing > the 'job' pointer in img_commit() it should be checked. It may not because the job yields before executing anything (if it started successfully; but otherwise, commit_active_start() would have returned an error). Therefore, I think the better solution is to assert(job) here. (It would be a serious bug if block_job_get() returned NULL here, so it's definitely not something we can be quiet about. But this patch makes it so the user doesn't even notice.) Max > Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com> > Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com> > Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com> > --- > qemu-img.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/qemu-img.c b/qemu-img.c > index b12f4cd19b0a..51fe09bd08ed 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1029,6 +1029,9 @@ static int img_commit(int argc, char **argv) > } > > job = block_job_get("commit"); > + if (job == NULL) { > + goto unref_backing; > + } > run_block_job(job, &local_err); > if (local_err) { > goto unref_backing; >
On 12/10/18 15:51, Max Reitz wrote: > On 31.08.18 20:16, Liam Merwick wrote: >> The function block_job_get() may return NULL so before dereferencing >> the 'job' pointer in img_commit() it should be checked. > > It may not because the job yields before executing anything (if it > started successfully; but otherwise, commit_active_start() would have > returned an error). Therefore, I think the better solution is to > assert(job) here. > Switched patch to use assert() Regards, Liam > (It would be a serious bug if block_job_get() returned NULL here, so > it's definitely not something we can be quiet about. But this patch > makes it so the user doesn't even notice.) > > Max > >> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com> >> Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com> >> Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com> >> --- >> qemu-img.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/qemu-img.c b/qemu-img.c >> index b12f4cd19b0a..51fe09bd08ed 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -1029,6 +1029,9 @@ static int img_commit(int argc, char **argv) >> } >> >> job = block_job_get("commit"); >> + if (job == NULL) { >> + goto unref_backing; >> + } >> run_block_job(job, &local_err); >> if (local_err) { >> goto unref_backing; >> > >
diff --git a/qemu-img.c b/qemu-img.c index b12f4cd19b0a..51fe09bd08ed 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1029,6 +1029,9 @@ static int img_commit(int argc, char **argv) } job = block_job_get("commit"); + if (job == NULL) { + goto unref_backing; + } run_block_job(job, &local_err); if (local_err) { goto unref_backing;