* [Qemu-devel] [patch] qcow2: double free snapshots
@ 2014-08-09 5:33 Zhang Haoyu
2014-08-11 2:24 ` Gonglei (Arei)
0 siblings, 1 reply; 3+ messages in thread
From: Zhang Haoyu @ 2014-08-09 5:33 UTC (permalink / raw)
To: qemu-devel
In qcow2_open(), if qcow2_read_snapshots() failed, qcow2_open() -> qcow2_free_snapshots() will be called, NULL snapshots dereference happened.
because qcow2_free_snapshots has been performed before in the fail case of qcow2_read_snapshots().
shown as below callstack,
qcow2_open
|- qcow2_read_snapshots
|-- goto fail;
|-- qcow2_free_snapshots
|- goto fail;
|- qcow2_free_snapshots /* on this case, NULL snapshots dereference happened. */
Signed-off-by: Zhang Haoyu <zhanghy@sangfor.com>
---
block/qcow2-snapshot.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 0aa9def..15c0513 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -31,6 +31,10 @@ void qcow2_free_snapshots(BlockDriverState *bs)
BDRVQcowState *s = bs->opaque;
int i;
+ if (!s || !s->snapshots) {
+ return;
+ }
+
for(i = 0; i < s->nb_snapshots; i++) {
g_free(s->snapshots[i].name);
g_free(s->snapshots[i].id_str);
--
1.9.4.msysgit.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [patch] qcow2: double free snapshots
2014-08-09 5:33 [Qemu-devel] [patch] qcow2: double free snapshots Zhang Haoyu
@ 2014-08-11 2:24 ` Gonglei (Arei)
2014-08-11 6:34 ` Zhang Haoyu
0 siblings, 1 reply; 3+ messages in thread
From: Gonglei (Arei) @ 2014-08-11 2:24 UTC (permalink / raw)
To: Zhang Haoyu, qemu-devel
Hi,
> Subject: [Qemu-devel] [patch] qcow2: double free snapshots
>
> In qcow2_open(), if qcow2_read_snapshots() failed, qcow2_open() ->
> qcow2_free_snapshots() will be called, NULL snapshots dereference happened.
> because qcow2_free_snapshots has been performed before in the fail case of
> qcow2_read_snapshots().
> shown as below callstack,
> qcow2_open
> |- qcow2_read_snapshots
> |-- goto fail;
> |-- qcow2_free_snapshots
> |- goto fail;
> |- qcow2_free_snapshots /* on this case, NULL snapshots dereference
> happened. */
>
> Signed-off-by: Zhang Haoyu <zhanghy@sangfor.com>
> ---
> block/qcow2-snapshot.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 0aa9def..15c0513 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -31,6 +31,10 @@ void qcow2_free_snapshots(BlockDriverState *bs)
> BDRVQcowState *s = bs->opaque;
> int i;
>
> + if (!s || !s->snapshots) {
> + return;
> + }
> +
> for(i = 0; i < s->nb_snapshots; i++) {
> g_free(s->snapshots[i].name);
> g_free(s->snapshots[i].id_str);
> --
> 1.9.4.msysgit.0
>
Yes, the code path is right. But I don't think this check is necessary.
The first free has set "s->nb_snapshots = 0", so it is impossibility go into
g_free(s->snapshots[i].name), and g_free(s->snapshots) is OK, though it is NULL.
Best regards,
-Gonglei
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [patch] qcow2: double free snapshots
2014-08-11 2:24 ` Gonglei (Arei)
@ 2014-08-11 6:34 ` Zhang Haoyu
0 siblings, 0 replies; 3+ messages in thread
From: Zhang Haoyu @ 2014-08-11 6:34 UTC (permalink / raw)
To: Gonglei (Arei), qemu-devel
>Hi,
>
>> Subject: [Qemu-devel] [patch] qcow2: double free snapshots
>>
>> In qcow2_open(), if qcow2_read_snapshots() failed, qcow2_open() ->
>> qcow2_free_snapshots() will be called, NULL snapshots dereference happened.
>> because qcow2_free_snapshots has been performed before in the fail case of
>> qcow2_read_snapshots().
>> shown as below callstack,
>> qcow2_open
>> |- qcow2_read_snapshots
>> |-- goto fail;
>> |-- qcow2_free_snapshots
>> |- goto fail;
>> |- qcow2_free_snapshots /* on this case, NULL snapshots dereference
>> happened. */
>>
>> Signed-off-by: Zhang Haoyu <zhanghy@sangfor.com>
>> ---
>> block/qcow2-snapshot.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
>> index 0aa9def..15c0513 100644
>> --- a/block/qcow2-snapshot.c
>> +++ b/block/qcow2-snapshot.c
>> @@ -31,6 +31,10 @@ void qcow2_free_snapshots(BlockDriverState *bs)
>> BDRVQcowState *s = bs->opaque;
>> int i;
>>
>> + if (!s || !s->snapshots) {
>> + return;
>> + }
>> +
>> for(i = 0; i < s->nb_snapshots; i++) {
>> g_free(s->snapshots[i].name);
>> g_free(s->snapshots[i].id_str);
>> --
>> 1.9.4.msysgit.0
>>
>Yes, the code path is right. But I don't think this check is necessary.
>The first free has set "s->nb_snapshots = 0", so it is impossibility go into
>g_free(s->snapshots[i].name), and g_free(s->snapshots) is OK, though it is NULL.
>
Yes, you are right, ignore this patch, please.
Thanks,
Zhang Haoyu
>Best regards,
>-Gonglei
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-08-11 6:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-09 5:33 [Qemu-devel] [patch] qcow2: double free snapshots Zhang Haoyu
2014-08-11 2:24 ` Gonglei (Arei)
2014-08-11 6:34 ` Zhang Haoyu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).