Re: [Qemu-block] [PATCH v2] block: fix leaks in bdrv_open_driver()

[Manos Pitsidianakis]

On Wed, Jul 12, 2017 at 10:33:37AM +0200, Kevin Wolf wrote:

Am 11.07.2017 um 20:50 hat Manos Pitsidianakis geschrieben:

On Tue, Jul 11, 2017 at 05:16:17PM +0200, Kevin Wolf wrote:
>Am 01.07.2017 um 17:39 hat Manos Pitsidianakis geschrieben:
>>bdrv_open_driver() is called in two places, bdrv_new_open_driver() and
>>bdrv_open_common(). In the latter, failure cleanup in is in its caller,
>>bdrv_open_inherit(), which unrefs the bs->file of the failed driver open if it
>>exists.
>>
>>Let's move the bs->file cleanup to bdrv_open_driver() to take care of all
>>callers and do not set bs->drv to NULL unless the driver's open function
>>failed. When bs is destroyed by removing its last reference, bdrv_close()
>>checks bs->drv to perform the needed cleanups and also call the driver's close
>>function.
>>
>>Signed-off-by: Manos Pitsidianakis 
>>---
>>
>>v2:
>> move bdrv_unref_child(bs, bs->file) to bdrv_open_driver
>> do not set bs->drv to NULL if open succeeds
>>
>> block.c | 21 +
>> 1 file changed, 13 insertions(+), 8 deletions(-)
>>
>>diff --git a/block.c b/block.c
>>index 694396281b..df2a46990c 100644
>>--- a/block.c
>>+++ b/block.c
>>@@ -1091,6 +1091,7 @@ static int bdrv_open_driver(BlockDriverState *bs, 
BlockDriver *drv,
>> {
>> Error *local_err = NULL;
>> int ret;
>>+bool open_failed;
>>
>> bdrv_assign_node_name(bs, node_name, _err);
>> if (local_err) {
>>@@ -,7 +1112,9 @@ static int bdrv_open_driver(BlockDriverState *bs, 
BlockDriver *drv,
>> ret = 0;
>> }
>>
>>-if (ret < 0) {
>>+open_failed = ret < 0;
>>+
>>+if (open_failed) {
>> if (local_err) {
>> error_propagate(errp, local_err);
>> } else if (bs->filename[0]) {
>>@@ -1142,10 +1145,15 @@ static int bdrv_open_driver(BlockDriverState *bs, 
BlockDriver *drv,
>> return 0;
>>
>> free_and_fail:
>>-/* FIXME Close bs first if already opened*/
>>-g_free(bs->opaque);
>>-bs->opaque = NULL;
>>-bs->drv = NULL;
>>+if (open_failed) {
>>+g_free(bs->opaque);
>>+bs->opaque = NULL;
>>+bs->drv = NULL;
>>+}
>>+if (bs->file != NULL) {
>>+bdrv_unref_child(bs, bs->file);
>>+bs->file = NULL;
>>+}
>
>Is this bdrv_unref_child() safe if we leave bs->drv set? Format drivers
>expect that if an image is opened, it also has a valid bs->file.
>
>For example, if I add ret = -1 after refresh_total_sectors() (because I
>couldn't find an easier way to make it fail intentionally), I get an
>ugly heap corruption crash instead of a nice error message with this
>patch.
>
This is triggered by bdrv_open_inherit doing
QDECREF(bs->explicit_options) and leaving the dangling pointer. Not
setting bs->drv means bdrv_close was called and tried to decref it
again, causing the heap error. Setting bs->explicit_options = NULL;
right below that fixes the heap corruption for me.


Wouldn't it be better to call drv->bdrv_close() instead and then set
bs->drv/opaque = NULL like for the other error path?


That was my first approach but I thought it wouldn't look nice since 
bdrv_close is called anyway on delete. I will do it in the next version.



I can send a seperate fix for this.


No, this doesn't fail before this patch, so it's a regression and we
can't merge the patch without a fix. You need to respin this one.
Yes, I meant to first fix this and then apply this patch. It's a 
dangling pointer anyway.



I also saw that there's no reason to use a boolean, a label would do
just fine so I can change that and finalize the patch in the next
version if everything is okay with it.


Yes, that sounds better.

Kevin





signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v2] block: fix leaks in bdrv_open_driver()

[Kevin Wolf]

Am 11.07.2017 um 20:50 hat Manos Pitsidianakis geschrieben:
> On Tue, Jul 11, 2017 at 05:16:17PM +0200, Kevin Wolf wrote:
> >Am 01.07.2017 um 17:39 hat Manos Pitsidianakis geschrieben:
> >>bdrv_open_driver() is called in two places, bdrv_new_open_driver() and
> >>bdrv_open_common(). In the latter, failure cleanup in is in its caller,
> >>bdrv_open_inherit(), which unrefs the bs->file of the failed driver open if 
> >>it
> >>exists.
> >>
> >>Let's move the bs->file cleanup to bdrv_open_driver() to take care of all
> >>callers and do not set bs->drv to NULL unless the driver's open function
> >>failed. When bs is destroyed by removing its last reference, bdrv_close()
> >>checks bs->drv to perform the needed cleanups and also call the driver's 
> >>close
> >>function.
> >>
> >>Signed-off-by: Manos Pitsidianakis 
> >>---
> >>
> >>v2:
> >> move bdrv_unref_child(bs, bs->file) to bdrv_open_driver
> >> do not set bs->drv to NULL if open succeeds
> >>
> >> block.c | 21 +
> >> 1 file changed, 13 insertions(+), 8 deletions(-)
> >>
> >>diff --git a/block.c b/block.c
> >>index 694396281b..df2a46990c 100644
> >>--- a/block.c
> >>+++ b/block.c
> >>@@ -1091,6 +1091,7 @@ static int bdrv_open_driver(BlockDriverState *bs, 
> >>BlockDriver *drv,
> >> {
> >> Error *local_err = NULL;
> >> int ret;
> >>+bool open_failed;
> >>
> >> bdrv_assign_node_name(bs, node_name, _err);
> >> if (local_err) {
> >>@@ -,7 +1112,9 @@ static int bdrv_open_driver(BlockDriverState *bs, 
> >>BlockDriver *drv,
> >> ret = 0;
> >> }
> >>
> >>-if (ret < 0) {
> >>+open_failed = ret < 0;
> >>+
> >>+if (open_failed) {
> >> if (local_err) {
> >> error_propagate(errp, local_err);
> >> } else if (bs->filename[0]) {
> >>@@ -1142,10 +1145,15 @@ static int bdrv_open_driver(BlockDriverState *bs, 
> >>BlockDriver *drv,
> >> return 0;
> >>
> >> free_and_fail:
> >>-/* FIXME Close bs first if already opened*/
> >>-g_free(bs->opaque);
> >>-bs->opaque = NULL;
> >>-bs->drv = NULL;
> >>+if (open_failed) {
> >>+g_free(bs->opaque);
> >>+bs->opaque = NULL;
> >>+bs->drv = NULL;
> >>+}
> >>+if (bs->file != NULL) {
> >>+bdrv_unref_child(bs, bs->file);
> >>+bs->file = NULL;
> >>+}
> >
> >Is this bdrv_unref_child() safe if we leave bs->drv set? Format drivers
> >expect that if an image is opened, it also has a valid bs->file.
> >
> >For example, if I add ret = -1 after refresh_total_sectors() (because I
> >couldn't find an easier way to make it fail intentionally), I get an
> >ugly heap corruption crash instead of a nice error message with this
> >patch.
> >
> This is triggered by bdrv_open_inherit doing
> QDECREF(bs->explicit_options) and leaving the dangling pointer. Not
> setting bs->drv means bdrv_close was called and tried to decref it
> again, causing the heap error. Setting bs->explicit_options = NULL;
> right below that fixes the heap corruption for me.

Wouldn't it be better to call drv->bdrv_close() instead and then set
bs->drv/opaque = NULL like for the other error path?

> I can send a seperate fix for this.

No, this doesn't fail before this patch, so it's a regression and we
can't merge the patch without a fix. You need to respin this one.

> I also saw that there's no reason to use a boolean, a label would do
> just fine so I can change that and finalize the patch in the next
> version if everything is okay with it.

Yes, that sounds better.

Kevin


pgp6yc7DxYA5f.pgp
Description: PGP signature

Re: [Qemu-block] [PATCH v2] block: fix leaks in bdrv_open_driver()

[Manos Pitsidianakis]

On Tue, Jul 11, 2017 at 05:16:17PM +0200, Kevin Wolf wrote:

Am 01.07.2017 um 17:39 hat Manos Pitsidianakis geschrieben:

bdrv_open_driver() is called in two places, bdrv_new_open_driver() and
bdrv_open_common(). In the latter, failure cleanup in is in its caller,
bdrv_open_inherit(), which unrefs the bs->file of the failed driver open if it
exists.

Let's move the bs->file cleanup to bdrv_open_driver() to take care of all
callers and do not set bs->drv to NULL unless the driver's open function
failed. When bs is destroyed by removing its last reference, bdrv_close()
checks bs->drv to perform the needed cleanups and also call the driver's close
function.

Signed-off-by: Manos Pitsidianakis 
---

v2:
 move bdrv_unref_child(bs, bs->file) to bdrv_open_driver
 do not set bs->drv to NULL if open succeeds

 block.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 694396281b..df2a46990c 100644
--- a/block.c
+++ b/block.c
@@ -1091,6 +1091,7 @@ static int bdrv_open_driver(BlockDriverState *bs, 
BlockDriver *drv,
 {
 Error *local_err = NULL;
 int ret;
+bool open_failed;

 bdrv_assign_node_name(bs, node_name, _err);
 if (local_err) {
@@ -,7 +1112,9 @@ static int bdrv_open_driver(BlockDriverState *bs, 
BlockDriver *drv,
 ret = 0;
 }

-if (ret < 0) {
+open_failed = ret < 0;
+
+if (open_failed) {
 if (local_err) {
 error_propagate(errp, local_err);
 } else if (bs->filename[0]) {
@@ -1142,10 +1145,15 @@ static int bdrv_open_driver(BlockDriverState *bs, 
BlockDriver *drv,
 return 0;

 free_and_fail:
-/* FIXME Close bs first if already opened*/
-g_free(bs->opaque);
-bs->opaque = NULL;
-bs->drv = NULL;
+if (open_failed) {
+g_free(bs->opaque);
+bs->opaque = NULL;
+bs->drv = NULL;
+}
+if (bs->file != NULL) {
+bdrv_unref_child(bs, bs->file);
+bs->file = NULL;
+}


Is this bdrv_unref_child() safe if we leave bs->drv set? Format drivers
expect that if an image is opened, it also has a valid bs->file.

For example, if I add ret = -1 after refresh_total_sectors() (because I
couldn't find an easier way to make it fail intentionally), I get an
ugly heap corruption crash instead of a nice error message with this
patch.

This is triggered by bdrv_open_inherit doing 
QDECREF(bs->explicit_options) and leaving the dangling pointer. Not 
setting bs->drv means bdrv_close was called and tried to decref it 
again, causing the heap error. Setting bs->explicit_options = NULL;

right below that fixes the heap corruption for me.

I can send a seperate fix for this. I also saw that there's no reason to 
use a boolean, a label would do just fine so I can change that and 
finalize the patch in the next version if everything is okay with it.


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v2] block: fix leaks in bdrv_open_driver()

[Kevin Wolf]

Am 01.07.2017 um 17:39 hat Manos Pitsidianakis geschrieben:
> bdrv_open_driver() is called in two places, bdrv_new_open_driver() and
> bdrv_open_common(). In the latter, failure cleanup in is in its caller,
> bdrv_open_inherit(), which unrefs the bs->file of the failed driver open if it
> exists.
> 
> Let's move the bs->file cleanup to bdrv_open_driver() to take care of all
> callers and do not set bs->drv to NULL unless the driver's open function
> failed. When bs is destroyed by removing its last reference, bdrv_close()
> checks bs->drv to perform the needed cleanups and also call the driver's close
> function.
> 
> Signed-off-by: Manos Pitsidianakis 
> ---
> 
> v2:
>  move bdrv_unref_child(bs, bs->file) to bdrv_open_driver
>  do not set bs->drv to NULL if open succeeds 
> 
>  block.c | 21 +
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 694396281b..df2a46990c 100644
> --- a/block.c
> +++ b/block.c
> @@ -1091,6 +1091,7 @@ static int bdrv_open_driver(BlockDriverState *bs, 
> BlockDriver *drv,
>  {
>  Error *local_err = NULL;
>  int ret;
> +bool open_failed;
>  
>  bdrv_assign_node_name(bs, node_name, _err);
>  if (local_err) {
> @@ -,7 +1112,9 @@ static int bdrv_open_driver(BlockDriverState *bs, 
> BlockDriver *drv,
>  ret = 0;
>  }
>  
> -if (ret < 0) {
> +open_failed = ret < 0;
> +
> +if (open_failed) {
>  if (local_err) {
>  error_propagate(errp, local_err);
>  } else if (bs->filename[0]) {
> @@ -1142,10 +1145,15 @@ static int bdrv_open_driver(BlockDriverState *bs, 
> BlockDriver *drv,
>  return 0;
>  
>  free_and_fail:
> -/* FIXME Close bs first if already opened*/
> -g_free(bs->opaque);
> -bs->opaque = NULL;
> -bs->drv = NULL;
> +if (open_failed) {
> +g_free(bs->opaque);
> +bs->opaque = NULL;
> +bs->drv = NULL;
> +}
> +if (bs->file != NULL) {
> +bdrv_unref_child(bs, bs->file);
> +bs->file = NULL;
> +}

Is this bdrv_unref_child() safe if we leave bs->drv set? Format drivers
expect that if an image is opened, it also has a valid bs->file.

For example, if I add ret = -1 after refresh_total_sectors() (because I
couldn't find an easier way to make it fail intentionally), I get an
ugly heap corruption crash instead of a nice error message with this
patch.

Kevin

[Qemu-block] [PATCH v2] block: fix leaks in bdrv_open_driver()

[Manos Pitsidianakis]

bdrv_open_driver() is called in two places, bdrv_new_open_driver() and
bdrv_open_common(). In the latter, failure cleanup in is in its caller,
bdrv_open_inherit(), which unrefs the bs->file of the failed driver open if it
exists.

Let's move the bs->file cleanup to bdrv_open_driver() to take care of all
callers and do not set bs->drv to NULL unless the driver's open function
failed. When bs is destroyed by removing its last reference, bdrv_close()
checks bs->drv to perform the needed cleanups and also call the driver's close
function.

Signed-off-by: Manos Pitsidianakis 
---

v2:
 move bdrv_unref_child(bs, bs->file) to bdrv_open_driver
 do not set bs->drv to NULL if open succeeds 

 block.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 694396281b..df2a46990c 100644
--- a/block.c
+++ b/block.c
@@ -1091,6 +1091,7 @@ static int bdrv_open_driver(BlockDriverState *bs, 
BlockDriver *drv,
 {
 Error *local_err = NULL;
 int ret;
+bool open_failed;
 
 bdrv_assign_node_name(bs, node_name, _err);
 if (local_err) {
@@ -,7 +1112,9 @@ static int bdrv_open_driver(BlockDriverState *bs, 
BlockDriver *drv,
 ret = 0;
 }
 
-if (ret < 0) {
+open_failed = ret < 0;
+
+if (open_failed) {
 if (local_err) {
 error_propagate(errp, local_err);
 } else if (bs->filename[0]) {
@@ -1142,10 +1145,15 @@ static int bdrv_open_driver(BlockDriverState *bs, 
BlockDriver *drv,
 return 0;
 
 free_and_fail:
-/* FIXME Close bs first if already opened*/
-g_free(bs->opaque);
-bs->opaque = NULL;
-bs->drv = NULL;
+if (open_failed) {
+g_free(bs->opaque);
+bs->opaque = NULL;
+bs->drv = NULL;
+}
+if (bs->file != NULL) {
+bdrv_unref_child(bs, bs->file);
+bs->file = NULL;
+}
 return ret;
 }
 
@@ -2607,9 +2615,6 @@ static BlockDriverState *bdrv_open_inherit(const char 
*filename,
 
 fail:
 blk_unref(file);
-if (bs->file != NULL) {
-bdrv_unref_child(bs, bs->file);
-}
 QDECREF(snapshot_options);
 QDECREF(bs->explicit_options);
 QDECREF(bs->options);
-- 
2.11.0