[v3] off-by-one and NULL pointer accesses detected by static analysis

[Qemu-devel] [PATCH v3 4/8] qemu-img: potential Null pointer deref in img_commit()

Posted by Liam Merwick 7 years, 5 months ago

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>
---
 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;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 4/8] qemu-img: potential Null pointer deref in img_commit()

Posted by John Snow 7 years, 4 months ago


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;
>

Re: [Qemu-devel] [PATCH v3 4/8] qemu-img: potential Null pointer deref in img_commit()

Posted by Max Reitz 7 years, 3 months ago

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;
>

Re: [Qemu-devel] [PATCH v3 4/8] qemu-img: potential Null pointer deref in img_commit()

Posted by Liam Merwick 7 years, 3 months ago


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;
>>
> 
>