| Message ID | 20170629060300.29869-1-el13635@mail.ntua.gr |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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.
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
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.
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
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.
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);
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(+)