[Qemu-devel] [PATCH] block: fix bs->file leak in bdrv_new_open_driver()

[Qemu-devel] [PATCH] block: fix bs->file leak in bdrv_new_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 check for this in bdrv_new_open_driver() as well.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block.c b/block.c
index 694396281b..aeacd520e0 100644
--- a/block.c
+++ b/block.c
@@ -1165,6 +1165,9 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
 
     ret = bdrv_open_driver(bs, drv, node_name, bs->options, flags, errp);
     if (ret < 0) {
+        if (bs->file != NULL) {
+            bdrv_unref_child(bs, bs->file);
+        }
         QDECREF(bs->explicit_options);
         QDECREF(bs->options);
         bdrv_unref(bs);
-- 
2.11.0

Re: [Qemu-devel] [PATCH] block: fix bs->file leak in bdrv_new_open_driver()

[Kevin Wolf]

Am 29.06.2017 um 08:03 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 check for this in bdrv_new_open_driver() as well.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 694396281b..aeacd520e0 100644
> --- a/block.c
> +++ b/block.c
> @@ -1165,6 +1165,9 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
>  
>      ret = bdrv_open_driver(bs, drv, node_name, bs->options, flags, errp);
>      if (ret < 0) {
> +        if (bs->file != NULL) {
> +            bdrv_unref_child(bs, bs->file);
> +        }
>          QDECREF(bs->explicit_options);
>          QDECREF(bs->options);
>          bdrv_unref(bs);

I think we should set bs->file = NULL here to remove the dangling
pointer. I think it is never accessed anyway because of the
bs->drv = NULL in the error path of bdrv_open_driver(), but better safe
than sorry.

But what would you think about avoiding the code duplication and just
moving the bdrv_unref_child() call from bdrv_open_inherit() down to
bdrv_open_driver(), so that bdrv_new_open_driver() is automatically
covered?

And later we can maybe move it into the individual .bdrv_open
implementations where it really belongs (whoever creates something is
responsible for cleaning it up in error cases).

Kevin

Re: [Qemu-devel] [PATCH] block: fix bs->file leak in bdrv_new_open_driver()

[Manos Pitsidianakis]

[-- Attachment #1: Type: text/plain, Size: 2389 bytes --]

On Thu, Jun 29, 2017 at 01:18:24PM +0200, Kevin Wolf wrote:
>Am 29.06.2017 um 08:03 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 check for this in bdrv_new_open_driver() as well.
>>
>> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
>> ---
>>  block.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 694396281b..aeacd520e0 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1165,6 +1165,9 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
>>
>>      ret = bdrv_open_driver(bs, drv, node_name, bs->options, flags, errp);
>>      if (ret < 0) {
>> +        if (bs->file != NULL) {
>> +            bdrv_unref_child(bs, bs->file);
>> +        }
>>          QDECREF(bs->explicit_options);
>>          QDECREF(bs->options);
>>          bdrv_unref(bs);
>
>I think we should set bs->file = NULL here to remove the dangling
>pointer. I think it is never accessed anyway because of the
>bs->drv = NULL in the error path of bdrv_open_driver(), but better safe
>than sorry.

You can't see it in the diff but after bdrv_unref(bs), 
bdrv_new_open_driver returns NULL so there won't be any access to bs 
anyway. And since bs is destroyed by bdrv_unref (its refcount is 1), 
there's not really a point in setting bs->file = NULL.

>But what would you think about avoiding the code duplication and just
>moving the bdrv_unref_child() call from bdrv_open_inherit() down to
>bdrv_open_driver(), so that bdrv_new_open_driver() is automatically
>covered?

The result would be the same, but this will cover future callers of 
bdrv_open_driver. Should I submit a v2?
>
>And later we can maybe move it into the individual .bdrv_open
>implementations where it really belongs (whoever creates something is
>responsible for cleaning it up in error cases).

freeing bs->file was recently removed from individual '.bdrv_open's 
since bdrv_open_inherit takes care of it 
(de234897b60e034ba94b307fc289e2dc692c9251). I think this is simpler 
since a driver could neglect to free their bs->file whereas this is a 
catchall solution.

>Kevin
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] block: fix bs->file leak in bdrv_new_open_driver()

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 2996 bytes --]

Am 29.06.2017 um 14:07 hat Manos Pitsidianakis geschrieben:
> On Thu, Jun 29, 2017 at 01:18:24PM +0200, Kevin Wolf wrote:
> >Am 29.06.2017 um 08:03 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 check for this in bdrv_new_open_driver() as well.
> >>
> >>Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> >>---
> >> block.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >>diff --git a/block.c b/block.c
> >>index 694396281b..aeacd520e0 100644
> >>--- a/block.c
> >>+++ b/block.c
> >>@@ -1165,6 +1165,9 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
> >>
> >>     ret = bdrv_open_driver(bs, drv, node_name, bs->options, flags, errp);
> >>     if (ret < 0) {
> >>+        if (bs->file != NULL) {
> >>+            bdrv_unref_child(bs, bs->file);
> >>+        }
> >>         QDECREF(bs->explicit_options);
> >>         QDECREF(bs->options);
> >>         bdrv_unref(bs);
> >
> >I think we should set bs->file = NULL here to remove the dangling
> >pointer. I think it is never accessed anyway because of the
> >bs->drv = NULL in the error path of bdrv_open_driver(), but better safe
> >than sorry.
> 
> You can't see it in the diff but after bdrv_unref(bs),
> bdrv_new_open_driver returns NULL so there won't be any access to bs
> anyway. And since bs is destroyed by bdrv_unref (its refcount is 1),
> there's not really a point in setting bs->file = NULL.

Yes, but bdrv_unref() doesn't have to expect inconsistent BDSes. It
doesn't access bs->file currently when bs->drv == NULL, but that's more
by luck than by design.

> >But what would you think about avoiding the code duplication and just
> >moving the bdrv_unref_child() call from bdrv_open_inherit() down to
> >bdrv_open_driver(), so that bdrv_new_open_driver() is automatically
> >covered?
> 
> The result would be the same, but this will cover future callers of
> bdrv_open_driver. Should I submit a v2?

I would prefer this, yes.

> >And later we can maybe move it into the individual .bdrv_open
> >implementations where it really belongs (whoever creates something is
> >responsible for cleaning it up in error cases).
> 
> freeing bs->file was recently removed from individual '.bdrv_open's
> since bdrv_open_inherit takes care of it
> (de234897b60e034ba94b307fc289e2dc692c9251). I think this is simpler
> since a driver could neglect to free their bs->file whereas this is
> a catchall solution.

That commit just changed an inconsistent situation where some drivers
did free it and other didn't. But we don't have to move it into the
drivers, it's just an option. As long as bs->file is special cased for
many other things, too, keeping it centrally might make sense.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH] block: fix bs->file leak in bdrv_new_open_driver()

[Manos Pitsidianakis]

[-- Attachment #1: Type: text/plain, Size: 2718 bytes --]

On Thu, Jun 29, 2017 at 03:57:49PM +0200, Kevin Wolf wrote:
>Am 29.06.2017 um 14:07 hat Manos Pitsidianakis geschrieben:
>> On Thu, Jun 29, 2017 at 01:18:24PM +0200, Kevin Wolf wrote:
>> >Am 29.06.2017 um 08:03 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 check for this in bdrv_new_open_driver() as well.
>> >>
>> >>Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
>> >>---
>> >> block.c | 3 +++
>> >> 1 file changed, 3 insertions(+)
>> >>
>> >>diff --git a/block.c b/block.c
>> >>index 694396281b..aeacd520e0 100644
>> >>--- a/block.c
>> >>+++ b/block.c
>> >>@@ -1165,6 +1165,9 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
>> >>
>> >>     ret = bdrv_open_driver(bs, drv, node_name, bs->options, flags, errp);
>> >>     if (ret < 0) {
>> >>+        if (bs->file != NULL) {
>> >>+            bdrv_unref_child(bs, bs->file);
>> >>+        }
>> >>         QDECREF(bs->explicit_options);
>> >>         QDECREF(bs->options);
>> >>         bdrv_unref(bs);
>> >
>> >I think we should set bs->file = NULL here to remove the dangling
>> >pointer. I think it is never accessed anyway because of the
>> >bs->drv = NULL in the error path of bdrv_open_driver(), but better safe
>> >than sorry.
>>
>> You can't see it in the diff but after bdrv_unref(bs),
>> bdrv_new_open_driver returns NULL so there won't be any access to bs
>> anyway. And since bs is destroyed by bdrv_unref (its refcount is 1),
>> there's not really a point in setting bs->file = NULL.
>
>Yes, but bdrv_unref() doesn't have to expect inconsistent BDSes. It
>doesn't access bs->file currently when bs->drv == NULL, but that's more
>by luck than by design.
>
>> >But what would you think about avoiding the code duplication and just
>> >moving the bdrv_unref_child() call from bdrv_open_inherit() down to
>> >bdrv_open_driver(), so that bdrv_new_open_driver() is automatically
>> >covered?
>>
>> The result would be the same, but this will cover future callers of
>> bdrv_open_driver. Should I submit a v2?
>
>I would prefer this, yes.

Perhaps it would be better to destroy bs at failure in bdrv_open_driver 
and not leave it to the caller which takes care of bdrv_close and 
unrefing bs->file anyway (Also bs->children). Setting bs->drv to NULL at 
failure in bdrv_open_driver means some things won't be executed in 
bdrv_close when the bs is destroyed eventually as well, so that fixes 
another mistake.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] block: fix bs->file leak in bdrv_new_open_driver()

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH] block: fix bs->file leak in bdrv_new_open_driver()
Message-id: 20170629060300.29869-1-el13635@mail.ntua.gr
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
fatal: Cannot update paths and switch to branch 'test' at the same time.
Did you intend to checkout 'origin/patchew/20170629060300.29869-1-el13635@mail.ntua.gr' which can not be resolved as commit?
Traceback (most recent call last):
  File "/home/fam/bin/patchew", line 440, in test_one
    git_clone_repo(clone, r["repo"], r["head"], logf)
  File "/home/fam/bin/patchew", line 53, in git_clone_repo
    cwd=clone)
  File "/usr/lib64/python3.5/subprocess.py", line 271, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'checkout', 'origin/patchew/20170629060300.29869-1-el13635@mail.ntua.gr', '-b', 'test']' returned non-zero exit status 128



---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH] block: fix bs->file leak in bdrv_new_open_driver()

[Fam Zheng]

On Thu, 07/06 16:49, no-reply@patchew.org wrote:
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> fatal: Cannot update paths and switch to branch 'test' at the same time.
> Did you intend to checkout 'origin/patchew/20170629060300.29869-1-el13635@mail.ntua.gr' which can not be resolved as commit?
> Traceback (most recent call last):
>   File "/home/fam/bin/patchew", line 440, in test_one
>     git_clone_repo(clone, r["repo"], r["head"], logf)
>   File "/home/fam/bin/patchew", line 53, in git_clone_repo
>     cwd=clone)
>   File "/usr/lib64/python3.5/subprocess.py", line 271, in check_call
>     raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['git', 'checkout', 'origin/patchew/20170629060300.29869-1-el13635@mail.ntua.gr', '-b', 'test']' returned non-zero exit status 128

Ignore this please, patchew is recovering from a bad state.

Re: [Qemu-devel] [PATCH] block: fix bs->file leak in bdrv_new_open_driver()

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 2927 bytes --]

Am 29.06.2017 um 22:06 hat Manos Pitsidianakis geschrieben:
> On Thu, Jun 29, 2017 at 03:57:49PM +0200, Kevin Wolf wrote:
> >Am 29.06.2017 um 14:07 hat Manos Pitsidianakis geschrieben:
> >>On Thu, Jun 29, 2017 at 01:18:24PM +0200, Kevin Wolf wrote:
> >>>Am 29.06.2017 um 08:03 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 check for this in bdrv_new_open_driver() as well.
> >>>>
> >>>>Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> >>>>---
> >>>> block.c | 3 +++
> >>>> 1 file changed, 3 insertions(+)
> >>>>
> >>>>diff --git a/block.c b/block.c
> >>>>index 694396281b..aeacd520e0 100644
> >>>>--- a/block.c
> >>>>+++ b/block.c
> >>>>@@ -1165,6 +1165,9 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
> >>>>
> >>>>     ret = bdrv_open_driver(bs, drv, node_name, bs->options, flags, errp);
> >>>>     if (ret < 0) {
> >>>>+        if (bs->file != NULL) {
> >>>>+            bdrv_unref_child(bs, bs->file);
> >>>>+        }
> >>>>         QDECREF(bs->explicit_options);
> >>>>         QDECREF(bs->options);
> >>>>         bdrv_unref(bs);
> >>>
> >>>I think we should set bs->file = NULL here to remove the dangling
> >>>pointer. I think it is never accessed anyway because of the
> >>>bs->drv = NULL in the error path of bdrv_open_driver(), but better safe
> >>>than sorry.
> >>
> >>You can't see it in the diff but after bdrv_unref(bs),
> >>bdrv_new_open_driver returns NULL so there won't be any access to bs
> >>anyway. And since bs is destroyed by bdrv_unref (its refcount is 1),
> >>there's not really a point in setting bs->file = NULL.
> >
> >Yes, but bdrv_unref() doesn't have to expect inconsistent BDSes. It
> >doesn't access bs->file currently when bs->drv == NULL, but that's more
> >by luck than by design.
> >
> >>>But what would you think about avoiding the code duplication and just
> >>>moving the bdrv_unref_child() call from bdrv_open_inherit() down to
> >>>bdrv_open_driver(), so that bdrv_new_open_driver() is automatically
> >>>covered?
> >>
> >>The result would be the same, but this will cover future callers of
> >>bdrv_open_driver. Should I submit a v2?
> >
> >I would prefer this, yes.
> 
> Perhaps it would be better to destroy bs at failure in
> bdrv_open_driver and not leave it to the caller which takes care of
> bdrv_close and unrefing bs->file anyway (Also bs->children). Setting
> bs->drv to NULL at failure in bdrv_open_driver means some things
> won't be executed in bdrv_close when the bs is destroyed eventually
> as well, so that fixes another mistake.

Oh, didn't I reply here yet? Your suggestion sounds good to me.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH] block: fix bs->file leak in bdrv_new_open_driver()

[Manos Pitsidianakis]

[-- Attachment #1: Type: text/plain, Size: 3413 bytes --]

On Fri, Jul 07, 2017 at 11:28:15AM +0200, Kevin Wolf wrote:
>Am 29.06.2017 um 22:06 hat Manos Pitsidianakis geschrieben:
>> On Thu, Jun 29, 2017 at 03:57:49PM +0200, Kevin Wolf wrote:
>> >Am 29.06.2017 um 14:07 hat Manos Pitsidianakis geschrieben:
>> >>On Thu, Jun 29, 2017 at 01:18:24PM +0200, Kevin Wolf wrote:
>> >>>Am 29.06.2017 um 08:03 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 check for this in bdrv_new_open_driver() as well.
>> >>>>
>> >>>>Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
>> >>>>---
>> >>>> block.c | 3 +++
>> >>>> 1 file changed, 3 insertions(+)
>> >>>>
>> >>>>diff --git a/block.c b/block.c
>> >>>>index 694396281b..aeacd520e0 100644
>> >>>>--- a/block.c
>> >>>>+++ b/block.c
>> >>>>@@ -1165,6 +1165,9 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
>> >>>>
>> >>>>     ret = bdrv_open_driver(bs, drv, node_name, bs->options, flags, errp);
>> >>>>     if (ret < 0) {
>> >>>>+        if (bs->file != NULL) {
>> >>>>+            bdrv_unref_child(bs, bs->file);
>> >>>>+        }
>> >>>>         QDECREF(bs->explicit_options);
>> >>>>         QDECREF(bs->options);
>> >>>>         bdrv_unref(bs);
>> >>>
>> >>>I think we should set bs->file = NULL here to remove the dangling
>> >>>pointer. I think it is never accessed anyway because of the
>> >>>bs->drv = NULL in the error path of bdrv_open_driver(), but better safe
>> >>>than sorry.
>> >>
>> >>You can't see it in the diff but after bdrv_unref(bs),
>> >>bdrv_new_open_driver returns NULL so there won't be any access to bs
>> >>anyway. And since bs is destroyed by bdrv_unref (its refcount is 1),
>> >>there's not really a point in setting bs->file = NULL.
>> >
>> >Yes, but bdrv_unref() doesn't have to expect inconsistent BDSes. It
>> >doesn't access bs->file currently when bs->drv == NULL, but that's more
>> >by luck than by design.
>> >
>> >>>But what would you think about avoiding the code duplication and just
>> >>>moving the bdrv_unref_child() call from bdrv_open_inherit() down to
>> >>>bdrv_open_driver(), so that bdrv_new_open_driver() is automatically
>> >>>covered?
>> >>
>> >>The result would be the same, but this will cover future callers of
>> >>bdrv_open_driver. Should I submit a v2?
>> >
>> >I would prefer this, yes.
>>
>> Perhaps it would be better to destroy bs at failure in
>> bdrv_open_driver and not leave it to the caller which takes care of
>> bdrv_close and unrefing bs->file anyway (Also bs->children). Setting
>> bs->drv to NULL at failure in bdrv_open_driver means some things
>> won't be executed in bdrv_close when the bs is destroyed eventually
>> as well, so that fixes another mistake.
>
>Oh, didn't I reply here yet? Your suggestion sounds good to me.

I ended up sending a v2 some days ago, and instead just not setting 
bs->drv to NULL unless open failed which I think is cleaner.

https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg00019.html

The open's ret value is stored in a boolean, but probably would be 
better to goto a specific open_fail label. If you think the change is ok 
I will resend it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]