[libvirt] [cim][PATCH 1/2] Fix memory leak in set_other_id_info
model was allocated by asprintf but never freed after usage and assignement. Signed-off-by: Adam Majer <amajer@suse.de> --- src/Virt_ComputerSystem.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Virt_ComputerSystem.c b/src/Virt_ComputerSystem.c index da07f93..b4930ac 100644 --- a/src/Virt_ComputerSystem.c +++ b/src/Virt_ComputerSystem.c @@ -417,6 +417,8 @@ static int set_other_id_info(const CMPIBroker *broker, CMPI_string); } + free(model); + CMSetProperty(instance, "OtherIdentifyingInfo", &id_info, CMPI_stringA); -- 2.13.6
When caption is specified, cap was allocated and assigned a new pointer while the old never freed. Store the old pointer in old_cap and free it upon function exit, if required. get_input_dev_caption only returns valid cap pointer on success, so do not try to free it on failure. Signed-off-by: Adam Majer <amajer@suse.de> --- src/Virt_SettingsDefineCapabilities.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Virt_SettingsDefineCapabilities.c b/src/Virt_SettingsDefineCapabilities.c index 85cb27a..e07de59 100644 --- a/src/Virt_SettingsDefineCapabilities.c +++ b/src/Virt_SettingsDefineCapabilities.c @@ -1977,10 +1977,9 @@ static CMPIStatus set_input_props(const CMPIObjectPath *ref, { CMPIStatus s = {CMPI_RC_OK, NULL}; CMPIInstance *inst; - char *cap; + char *cap, *old_cap = NULL; if (get_input_dev_caption(type, bus, &cap) != 1) { - free(cap); cu_statusf(_BROKER, &s, CMPI_RC_ERR_NOT_FOUND, "Unable to build input caption"); @@ -1988,7 +1987,8 @@ static CMPIStatus set_input_props(const CMPIObjectPath *ref, } if (caption != NULL) { - if (asprintf(&cap, "%s %s", caption, cap) == -1) { + old_cap = cap; + if (asprintf(&cap, "%s %s", caption, old_cap) == -1) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_NOT_FOUND, "Unable to build input caption"); @@ -2012,6 +2012,7 @@ static CMPIStatus set_input_props(const CMPIObjectPath *ref, out: free(cap); + free(old_cap); return s; } -- 2.13.6
On 12/15/2017 09:05 AM, Adam Majer wrote:
> When caption is specified, cap was allocated and assigned a new pointer
> while the old never freed. Store the old pointer in old_cap and free it
> upon function exit, if required.
>
> get_input_dev_caption only returns valid cap pointer on success, so
> do not try to free it on failure.
>
> Signed-off-by: Adam Majer <amajer@suse.de>
> ---
> src/Virt_SettingsDefineCapabilities.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
> > diff --git a/src/Virt_SettingsDefineCapabilities.c
b/src/Virt_SettingsDefineCapabilities.c
> index 85cb27a..e07de59 100644
> --- a/src/Virt_SettingsDefineCapabilities.c
> +++ b/src/Virt_SettingsDefineCapabilities.c
> @@ -1977,10 +1977,9 @@ static CMPIStatus set_input_props(const CMPIObjectPath *ref,
> {
> CMPIStatus s = {CMPI_RC_OK, NULL};
> CMPIInstance *inst;
> - char *cap;
> + char *cap, *old_cap = NULL;
Instead of removing, just add "char *tmp;", then....
>
> if (get_input_dev_caption(type, bus, &cap) != 1) {
> - free(cap);
> cu_statusf(_BROKER, &s,
> CMPI_RC_ERR_NOT_FOUND,
> "Unable to build input caption");
> @@ -1988,7 +1987,8 @@ static CMPIStatus set_input_props(const CMPIObjectPath *ref,
> }
>
> if (caption != NULL) {
> - if (asprintf(&cap, "%s %s", caption, cap) == -1) {
This should be:
if (&tmp, "%s %s", caption, cap) == -1) {
> + old_cap = cap;
> + if (asprintf(&cap, "%s %s", caption, old_cap) == -1) {
> cu_statusf(_BROKER, &s,
> CMPI_RC_ERR_NOT_FOUND,
> "Unable to build input caption");
and if successful, e.g. after the closing "}"
free(cap)
cap = tmp;
> @@ -2012,6 +2012,7 @@ static CMPIStatus set_input_props(const CMPIObjectPath *ref,
>
> out:
> free(cap);
> + free(old_cap);
making this one unnecessary.
John
>
> return s;
> }
>
On 12/15/2017 09:05 AM, Adam Majer wrote:
model was allocated by asprintf but never freed after usage and assignement.
assignment
Signed-off-by: Adam Majer <amajer@suse.de> --- src/Virt_ComputerSystem.c | 2 ++ 1 file changed, 2 insertions(+)
Looks OK; however, there hasn't been a non build related source code change to libvirt-cim since June 2014. So while this could be pushed, I'm curious to understand why the "sudden interest" in CIM? The last release was July 2013! Perhaps if you'd generated a --cover-letter with your git send-email to explain that would have helped. See https://libvirt.org/hacking.html for some general patch submission guidelines when there's more than one patch in a series, although for libvirt-cim perhaps only the first section applies as virtually none of the code formatting and naming would apply... John
diff --git a/src/Virt_ComputerSystem.c b/src/Virt_ComputerSystem.c index da07f93..b4930ac 100644 --- a/src/Virt_ComputerSystem.c +++ b/src/Virt_ComputerSystem.c @@ -417,6 +417,8 @@ static int set_other_id_info(const CMPIBroker *broker, CMPI_string); }
+ free(model); + CMSetProperty(instance, "OtherIdentifyingInfo", &id_info, CMPI_stringA);
On 12/18/2017 02:31 PM, John Ferlan wrote:
Looks OK; however, there hasn't been a non build related source code change to libvirt-cim since June 2014. So while this could be pushed, I'm curious to understand why the "sudden interest" in CIM? The last release was July 2013!
I'm not sure there is any sudden interest in CIM, but few users have noticed the slow memory leak. It's better to have this fixed.
Perhaps if you'd generated a --cover-letter with your git send-email to explain that would have helped. See https://libvirt.org/hacking.html for some general patch submission guidelines when there's more than one patch in a series, although for libvirt-cim perhaps only the first section applies as virtually none of the code formatting and naming would apply...
John
OK, thank you for the tips. I've updated the patch, as you suggested, to affect less lines of code. - Adam
participants (2)
-
Adam Majer -
John Ferlan