[libvirt] [PATCH v2] Make sure that filedescriptors are only closed once
V2: also the path where close() failed needs to be corrected; -> time for a VIR_CLOSE() ? Setting fd=-1 in these places makes sure that the fd is not closed twice later on in 'clean:'. This is of course a recipe for disaster in a multi-threaded system where the filedescriptor can belong to something else right away. At least we have a pattern... (looks like I can close the augeas bug as well...) Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- src/conf/domain_conf.c | 5 ++++- src/conf/network_conf.c | 5 ++++- src/conf/nwfilter_conf.c | 10 ++++++++-- src/conf/storage_conf.c | 5 ++++- 4 files changed, 20 insertions(+), 5 deletions(-) Index: libvirt-acl/src/conf/domain_conf.c =================================================================== --- libvirt-acl.orig/src/conf/domain_conf.c +++ libvirt-acl/src/conf/domain_conf.c @@ -6802,13 +6802,16 @@ int virDomainSaveXML(const char *configD virReportSystemError(errno, _("cannot save config file '%s'"), configFile); - goto cleanup; + goto cleanup_free; } + fd = -1; + ret = 0; cleanup: if (fd != -1) close(fd); + cleanup_free: VIR_FREE(configFile); return ret; } Index: libvirt-acl/src/conf/network_conf.c =================================================================== --- libvirt-acl.orig/src/conf/network_conf.c +++ libvirt-acl/src/conf/network_conf.c @@ -691,15 +691,18 @@ int virNetworkSaveXML(const char *config virReportSystemError(errno, _("cannot save config file '%s'"), configFile); - goto cleanup; + goto cleanup_free; } + fd = -1; + ret = 0; cleanup: if (fd != -1) close(fd); + cleanup_free: VIR_FREE(configFile); return ret; Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -2197,15 +2197,18 @@ int virNWFilterSaveXML(const char *confi virReportSystemError(errno, _("cannot save config file '%s'"), configFile); - goto cleanup; + goto cleanup_free; } + fd = -1; + ret = 0; cleanup: if (fd != -1) close(fd); + cleanup_free: VIR_FREE(configFile); return ret; @@ -2608,15 +2611,18 @@ virNWFilterPoolObjSaveDef(virNWFilterDri virReportSystemError(errno, _("cannot save config file %s"), pool->configFile); - goto cleanup; + goto cleanup_free; } + fd = -1; + ret = 0; cleanup: if (fd != -1) close(fd); + cleanup_free: VIR_FREE(xml); return ret; Index: libvirt-acl/src/conf/storage_conf.c =================================================================== --- libvirt-acl.orig/src/conf/storage_conf.c +++ libvirt-acl/src/conf/storage_conf.c @@ -1564,15 +1564,18 @@ virStoragePoolObjSaveDef(virStorageDrive virReportSystemError(errno, _("cannot save config file %s"), pool->configFile); - goto cleanup; + goto cleanup_free; } + fd = -1; + ret = 0; cleanup: if (fd != -1) close(fd); + cleanup_free: VIR_FREE(xml); return ret;
On Thu, Oct 14, 2010 at 09:31:59PM -0400, Stefan Berger wrote:
V2: also the path where close() failed needs to be corrected;
-> time for a VIR_CLOSE() ?
Setting fd=-1 in these places makes sure that the fd is not closed twice later on in 'clean:'. This is of course a recipe for disaster in a multi-threaded system where the filedescriptor can belong to something else right away.
At least we have a pattern...
(looks like I can close the augeas bug as well...)
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
--- src/conf/domain_conf.c | 5 ++++- src/conf/network_conf.c | 5 ++++- src/conf/nwfilter_conf.c | 10 ++++++++-- src/conf/storage_conf.c | 5 ++++- 4 files changed, 20 insertions(+), 5 deletions(-)
Index: libvirt-acl/src/conf/domain_conf.c =================================================================== --- libvirt-acl.orig/src/conf/domain_conf.c +++ libvirt-acl/src/conf/domain_conf.c @@ -6802,13 +6802,16 @@ int virDomainSaveXML(const char *configD virReportSystemError(errno, _("cannot save config file '%s'"), configFile); - goto cleanup; + goto cleanup_free; }
+ fd = -1; + ret = 0; cleanup: if (fd != -1) close(fd); + cleanup_free: VIR_FREE(configFile); return ret; } Index: libvirt-acl/src/conf/network_conf.c =================================================================== --- libvirt-acl.orig/src/conf/network_conf.c +++ libvirt-acl/src/conf/network_conf.c @@ -691,15 +691,18 @@ int virNetworkSaveXML(const char *config virReportSystemError(errno, _("cannot save config file '%s'"), configFile); - goto cleanup; + goto cleanup_free; }
+ fd = -1; + ret = 0;
cleanup: if (fd != -1) close(fd);
+ cleanup_free: VIR_FREE(configFile);
return ret; Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -2197,15 +2197,18 @@ int virNWFilterSaveXML(const char *confi virReportSystemError(errno, _("cannot save config file '%s'"), configFile); - goto cleanup; + goto cleanup_free; }
+ fd = -1; + ret = 0;
cleanup: if (fd != -1) close(fd);
+ cleanup_free: VIR_FREE(configFile);
return ret; @@ -2608,15 +2611,18 @@ virNWFilterPoolObjSaveDef(virNWFilterDri virReportSystemError(errno, _("cannot save config file %s"), pool->configFile); - goto cleanup; + goto cleanup_free; }
+ fd = -1; + ret = 0;
cleanup: if (fd != -1) close(fd);
+ cleanup_free: VIR_FREE(xml);
return ret; Index: libvirt-acl/src/conf/storage_conf.c =================================================================== --- libvirt-acl.orig/src/conf/storage_conf.c +++ libvirt-acl/src/conf/storage_conf.c @@ -1564,15 +1564,18 @@ virStoragePoolObjSaveDef(virStorageDrive virReportSystemError(errno, _("cannot save config file %s"), pool->configFile); - goto cleanup; + goto cleanup_free; }
+ fd = -1; + ret = 0;
cleanup: if (fd != -1) close(fd);
+ cleanup_free: VIR_FREE(xml);
return ret;
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 10/15/2010 07:16 AM, Daniel P. Berrange wrote:
On Thu, Oct 14, 2010 at 09:31:59PM -0400, Stefan Berger wrote:
V2: also the path where close() failed needs to be corrected;
-> time for a VIR_CLOSE() ?
Setting fd=-1 in these places makes sure that the fd is not closed twice later on in 'clean:'. This is of course a recipe for disaster in a multi-threaded system where the filedescriptor can belong to something else right away.
At least we have a pattern...
(looks like I can close the augeas bug as well...)
Signed-off-by: Stefan Berger<stefanb@us.ibm.com>
--- src/conf/domain_conf.c | 5 ++++- src/conf/network_conf.c | 5 ++++- src/conf/nwfilter_conf.c | 10 ++++++++-- src/conf/storage_conf.c | 5 ++++- 4 files changed, 20 insertions(+), 5 deletions(-)
Index: libvirt-acl/src/conf/domain_conf.c =================================================================== --- libvirt-acl.orig/src/conf/domain_conf.c +++ libvirt-acl/src/conf/domain_conf.c @@ -6802,13 +6802,16 @@ int virDomainSaveXML(const char *configD virReportSystemError(errno, _("cannot save config file '%s'"), configFile); - goto cleanup; + goto cleanup_free; }
+ fd = -1; + ret = 0; cleanup: if (fd != -1) close(fd); + cleanup_free: VIR_FREE(configFile); return ret; } Index: libvirt-acl/src/conf/network_conf.c =================================================================== --- libvirt-acl.orig/src/conf/network_conf.c +++ libvirt-acl/src/conf/network_conf.c @@ -691,15 +691,18 @@ int virNetworkSaveXML(const char *config virReportSystemError(errno, _("cannot save config file '%s'"), configFile); - goto cleanup; + goto cleanup_free; }
+ fd = -1; + ret = 0;
cleanup: if (fd != -1) close(fd);
+ cleanup_free: VIR_FREE(configFile);
return ret; Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -2197,15 +2197,18 @@ int virNWFilterSaveXML(const char *confi virReportSystemError(errno, _("cannot save config file '%s'"), configFile); - goto cleanup; + goto cleanup_free; }
+ fd = -1; + ret = 0;
cleanup: if (fd != -1) close(fd);
+ cleanup_free: VIR_FREE(configFile);
return ret; @@ -2608,15 +2611,18 @@ virNWFilterPoolObjSaveDef(virNWFilterDri virReportSystemError(errno, _("cannot save config file %s"), pool->configFile); - goto cleanup; + goto cleanup_free; }
+ fd = -1; + ret = 0;
cleanup: if (fd != -1) close(fd);
+ cleanup_free: VIR_FREE(xml);
return ret; Index: libvirt-acl/src/conf/storage_conf.c =================================================================== --- libvirt-acl.orig/src/conf/storage_conf.c +++ libvirt-acl/src/conf/storage_conf.c @@ -1564,15 +1564,18 @@ virStoragePoolObjSaveDef(virStorageDrive virReportSystemError(errno, _("cannot save config file %s"), pool->configFile); - goto cleanup; + goto cleanup_free; }
+ fd = -1; + ret = 0;
cleanup: if (fd != -1) close(fd);
+ cleanup_free: VIR_FREE(xml);
return ret; ACK
Well, what about a VIR_CLOSE() to prevent such mistakes in the future. Stefan
Daniel
participants (3)
-
Daniel P. Berrange -
Laine Stump -
Stefan Berger