[libvirt] [PATCH] Revert "nwfilter: Move save of config until after successful assign"
This reverts commit b3e71a8830b2683ee88fa10cb048eabb99a446c0. As it turns out this ends up very badly as the @def could be Free'd even though it's owned by @obj as a result of the AssignDef. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index b5aaa6b..b36eda1 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -501,14 +501,14 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, goto error; } - if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) - goto error; - /* We generated a UUID, make it permanent by saving the config to disk */ if (!def->uuid_specified && virNWFilterSaveConfig(configDir, def) < 0) goto error; + if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) + goto error; + VIR_FREE(configFile); return obj; -- 2.9.4
On Sat, Jul 15, 2017 at 02:40:06PM -0400, John Ferlan wrote:
This reverts commit b3e71a8830b2683ee88fa10cb048eabb99a446c0.
As it turns out this ends up very badly as the @def could be Free'd even though it's owned by @obj as a result of the AssignDef.
I don't see a reason to revert it. What do you mean that the @def can be freed? The virNWFilterObjListAssignDef() doesn't free the @def that is passed to it, it only assign it to nwfilter object and returns it immediately. Pavel
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index b5aaa6b..b36eda1 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -501,14 +501,14 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, goto error; }
- if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) - goto error; - /* We generated a UUID, make it permanent by saving the config to disk */ if (!def->uuid_specified && virNWFilterSaveConfig(configDir, def) < 0) goto error;
+ if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) + goto error; + VIR_FREE(configFile); return obj;
-- 2.9.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/17/2017 04:15 AM, Pavel Hrdina wrote:
On Sat, Jul 15, 2017 at 02:40:06PM -0400, John Ferlan wrote:
This reverts commit b3e71a8830b2683ee88fa10cb048eabb99a446c0.
As it turns out this ends up very badly as the @def could be Free'd even though it's owned by @obj as a result of the AssignDef.
I don't see a reason to revert it. What do you mean that the @def can be freed? The virNWFilterObjListAssignDef() doesn't free the @def that is passed to it, it only assign it to nwfilter object and returns it immediately.
Pavel
After ListAssignDef the @def is owned by @obj If the SaveConfig fails, we jump to error: which will free @def. Now we have an @obj in the nwfilters->objs list which has obj->def entry using that address. At some point in the future this will be undefined, which calls virNWFilterObjFree from virNWFilterObjListRemove at which time we'll free something that's already free'd. In between that future free, something else could use obj->def thinking it hasn't been free'd. I realized this as part of the review to the nwfilter patch series John
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index b5aaa6b..b36eda1 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -501,14 +501,14 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, goto error; }
- if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) - goto error; - /* We generated a UUID, make it permanent by saving the config to disk */ if (!def->uuid_specified && virNWFilterSaveConfig(configDir, def) < 0) goto error;
+ if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) + goto error; + VIR_FREE(configFile); return obj;
-- 2.9.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jul 17, 2017 at 07:39:28AM -0400, John Ferlan wrote:
On 07/17/2017 04:15 AM, Pavel Hrdina wrote:
On Sat, Jul 15, 2017 at 02:40:06PM -0400, John Ferlan wrote:
This reverts commit b3e71a8830b2683ee88fa10cb048eabb99a446c0.
As it turns out this ends up very badly as the @def could be Free'd even though it's owned by @obj as a result of the AssignDef.
I don't see a reason to revert it. What do you mean that the @def can be freed? The virNWFilterObjListAssignDef() doesn't free the @def that is passed to it, it only assign it to nwfilter object and returns it immediately.
Pavel
After ListAssignDef the @def is owned by @obj
If the SaveConfig fails, we jump to error: which will free @def. Now we have an @obj in the nwfilters->objs list which has obj->def entry using that address.
Oh, right, it might happen while reloading libvirtd daemon. Sigh, I hate the nwfilter code, it's way to complex and ugly and has a lot of workaround, like this UUID generation and saving it back to the config file while loading the config file. Anyway, we should fix it even though reloading of libvirtd probably don't work. Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On 07/17/2017 08:44 AM, Pavel Hrdina wrote:
On Mon, Jul 17, 2017 at 07:39:28AM -0400, John Ferlan wrote:
On 07/17/2017 04:15 AM, Pavel Hrdina wrote:
On Sat, Jul 15, 2017 at 02:40:06PM -0400, John Ferlan wrote:
This reverts commit b3e71a8830b2683ee88fa10cb048eabb99a446c0.
As it turns out this ends up very badly as the @def could be Free'd even though it's owned by @obj as a result of the AssignDef.
I don't see a reason to revert it. What do you mean that the @def can be freed? The virNWFilterObjListAssignDef() doesn't free the @def that is passed to it, it only assign it to nwfilter object and returns it immediately.
Pavel
After ListAssignDef the @def is owned by @obj
If the SaveConfig fails, we jump to error: which will free @def. Now we have an @obj in the nwfilters->objs list which has obj->def entry using that address.
Oh, right, it might happen while reloading libvirtd daemon.
Fortunately to a degree it's an error path on saving a file and only if needing to generate the UUID - so the impact should be low. It's the improbable, but possible case. If they were available, the patch should be backported to 3.3-maint, 3.4-maint, and 3.5-maint trees. I've never created one of those before though...
Sigh, I hate the nwfilter code, it's way to complex and ugly and has a lot of workaround, like this UUID generation and saving it back to the config file while loading the config file.
Yeah - it's really ugly and has quite a few "gotchas" that should be handled better. The recursive locks are especially nightmarish. Once my other series is complete, having hash table lookups instead of list traversal and comparison should make a few things easier. Doubt anyone wants to profess too much knowledge of the nwfilters in order to take on the effort to clean things up. Tks - John
Anyway, we should fix it even though reloading of libvirtd probably don't work.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (2)
-
John Ferlan -
Pavel Hrdina