[libvirt PATCH v2 0/5] Fix some memory leaks
Issues were found by llvm's asan and ubsan sanitizers. V1: https://listman.redhat.com/archives/libvir-list/2021-April/msg00640.html Changes since V1: * Edited patch #1 to use two g_autofree variables instead of VIR_FREE'ing a reused variable * Removed "virQEMUCapsSetHostModel: Fix memory leak", I will address this issue seperately * Added fixes for two more memory leaks and one issue found by ubsan (passing NULL to qsort) Tim Wiederhake (5): xenParseHypervisorFeatures: Fix memory leak virDomainFeaturesDefParse: Fix memory leak cmdCheckpointList: Fix memory leak cmdSnapshotList: Fix memory leak virshCheckpointListCollect: Do not pass NULL to qsort src/conf/domain_conf.c | 2 +- src/libxl/xen_common.c | 28 +++++++++++++++------------- tools/virsh-checkpoint.c | 5 +++-- tools/virsh-snapshot.c | 2 +- 4 files changed, 20 insertions(+), 17 deletions(-) -- 2.26.3
Fixes:b523e22521afe733165869c9e1ae18e88536acd6 Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/libxl/xen_common.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 12a44280cb..6fa69fbdf0 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -543,14 +543,15 @@ xenParseCPU(virConf *conf, static int xenParseHypervisorFeatures(virConf *conf, virDomainDef *def) { - g_autofree char *strval = NULL; + g_autofree char *tscmode = NULL; + g_autofree char *passthrough = NULL; virDomainTimerDef *timer; int val = 0; - if (xenConfigGetString(conf, "tsc_mode", &strval, NULL) < 0) + if (xenConfigGetString(conf, "tsc_mode", &tscmode, NULL) < 0) return -1; - if (strval) { + if (tscmode) { VIR_EXPAND_N(def->clock.timers, def->clock.ntimers, 1); timer = g_new0(virDomainTimerDef, 1); @@ -559,37 +560,38 @@ xenParseHypervisorFeatures(virConf *conf, virDomainDef *def) timer->tickpolicy = -1; timer->mode = VIR_DOMAIN_TIMER_MODE_AUTO; timer->track = -1; - if (STREQ_NULLABLE(strval, "always_emulate")) + if (STREQ_NULLABLE(tscmode, "always_emulate")) timer->mode = VIR_DOMAIN_TIMER_MODE_EMULATE; - else if (STREQ_NULLABLE(strval, "native")) + else if (STREQ_NULLABLE(tscmode, "native")) timer->mode = VIR_DOMAIN_TIMER_MODE_NATIVE; - else if (STREQ_NULLABLE(strval, "native_paravirt")) + else if (STREQ_NULLABLE(tscmode, "native_paravirt")) timer->mode = VIR_DOMAIN_TIMER_MODE_PARAVIRT; def->clock.timers[def->clock.ntimers - 1] = timer; + VIR_FREE(tscmode); } - if (xenConfigGetString(conf, "passthrough", &strval, NULL) < 0) + if (xenConfigGetString(conf, "passthrough", &passthrough, NULL) < 0) return -1; - if (strval) { - if (STREQ(strval, "disabled")) { + if (passthrough) { + if (STREQ(passthrough, "disabled")) { def->features[VIR_DOMAIN_FEATURE_XEN] = VIR_TRISTATE_SWITCH_OFF; def->xen_features[VIR_DOMAIN_XEN_PASSTHROUGH] = VIR_TRISTATE_SWITCH_OFF; - } else if (STREQ(strval, "enabled")) { + } else if (STREQ(passthrough, "enabled")) { def->features[VIR_DOMAIN_FEATURE_XEN] = VIR_TRISTATE_SWITCH_ON; def->xen_features[VIR_DOMAIN_XEN_PASSTHROUGH] = VIR_TRISTATE_SWITCH_ON; - } else if (STREQ(strval, "sync_pt")) { + } else if (STREQ(passthrough, "sync_pt")) { def->features[VIR_DOMAIN_FEATURE_XEN] = VIR_TRISTATE_SWITCH_ON; def->xen_features[VIR_DOMAIN_XEN_PASSTHROUGH] = VIR_TRISTATE_SWITCH_ON; def->xen_passthrough_mode = VIR_DOMAIN_XEN_PASSTHROUGH_MODE_SYNC_PT; - } else if (STREQ(strval, "share_pt")) { + } else if (STREQ(passthrough, "share_pt")) { def->features[VIR_DOMAIN_FEATURE_XEN] = VIR_TRISTATE_SWITCH_ON; def->xen_features[VIR_DOMAIN_XEN_PASSTHROUGH] = VIR_TRISTATE_SWITCH_ON; def->xen_passthrough_mode = VIR_DOMAIN_XEN_PASSTHROUGH_MODE_SHARE_PT; } else { virReportError(VIR_ERR_CONF_SYNTAX, - _("Invalid passthrough mode %s"), strval); + _("Invalid passthrough mode %s"), passthrough); } } -- 2.26.3
On 4/19/21 7:54 AM, Tim Wiederhake wrote:
Fixes:b523e22521afe733165869c9e1ae18e88536acd6 Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/libxl/xen_common.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 12a44280cb..6fa69fbdf0 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -543,14 +543,15 @@ xenParseCPU(virConf *conf, static int xenParseHypervisorFeatures(virConf *conf, virDomainDef *def) { - g_autofree char *strval = NULL; + g_autofree char *tscmode = NULL; + g_autofree char *passthrough = NULL; virDomainTimerDef *timer; int val = 0;
- if (xenConfigGetString(conf, "tsc_mode", &strval, NULL) < 0) + if (xenConfigGetString(conf, "tsc_mode", &tscmode, NULL) < 0) return -1;
- if (strval) { + if (tscmode) { VIR_EXPAND_N(def->clock.timers, def->clock.ntimers, 1);
timer = g_new0(virDomainTimerDef, 1); @@ -559,37 +560,38 @@ xenParseHypervisorFeatures(virConf *conf, virDomainDef *def) timer->tickpolicy = -1; timer->mode = VIR_DOMAIN_TIMER_MODE_AUTO; timer->track = -1; - if (STREQ_NULLABLE(strval, "always_emulate")) + if (STREQ_NULLABLE(tscmode, "always_emulate")) timer->mode = VIR_DOMAIN_TIMER_MODE_EMULATE; - else if (STREQ_NULLABLE(strval, "native")) + else if (STREQ_NULLABLE(tscmode, "native")) timer->mode = VIR_DOMAIN_TIMER_MODE_NATIVE; - else if (STREQ_NULLABLE(strval, "native_paravirt")) + else if (STREQ_NULLABLE(tscmode, "native_paravirt")) timer->mode = VIR_DOMAIN_TIMER_MODE_PARAVIRT;
def->clock.timers[def->clock.ntimers - 1] = timer; + VIR_FREE(tscmode);
I think this ^^^ was left over from V1 With that removed Reviewed-by: Laine Stump <laine@redhat.com>
}
- if (xenConfigGetString(conf, "passthrough", &strval, NULL) < 0) + if (xenConfigGetString(conf, "passthrough", &passthrough, NULL) < 0) return -1;
- if (strval) { - if (STREQ(strval, "disabled")) { + if (passthrough) { + if (STREQ(passthrough, "disabled")) { def->features[VIR_DOMAIN_FEATURE_XEN] = VIR_TRISTATE_SWITCH_OFF; def->xen_features[VIR_DOMAIN_XEN_PASSTHROUGH] = VIR_TRISTATE_SWITCH_OFF; - } else if (STREQ(strval, "enabled")) { + } else if (STREQ(passthrough, "enabled")) { def->features[VIR_DOMAIN_FEATURE_XEN] = VIR_TRISTATE_SWITCH_ON; def->xen_features[VIR_DOMAIN_XEN_PASSTHROUGH] = VIR_TRISTATE_SWITCH_ON; - } else if (STREQ(strval, "sync_pt")) { + } else if (STREQ(passthrough, "sync_pt")) { def->features[VIR_DOMAIN_FEATURE_XEN] = VIR_TRISTATE_SWITCH_ON; def->xen_features[VIR_DOMAIN_XEN_PASSTHROUGH] = VIR_TRISTATE_SWITCH_ON; def->xen_passthrough_mode = VIR_DOMAIN_XEN_PASSTHROUGH_MODE_SYNC_PT; - } else if (STREQ(strval, "share_pt")) { + } else if (STREQ(passthrough, "share_pt")) { def->features[VIR_DOMAIN_FEATURE_XEN] = VIR_TRISTATE_SWITCH_ON; def->xen_features[VIR_DOMAIN_XEN_PASSTHROUGH] = VIR_TRISTATE_SWITCH_ON; def->xen_passthrough_mode = VIR_DOMAIN_XEN_PASSTHROUGH_MODE_SHARE_PT; } else { virReportError(VIR_ERR_CONF_SYNTAX, - _("Invalid passthrough mode %s"), strval); + _("Invalid passthrough mode %s"), passthrough); } }
Fixes: 94013ee04e3945307a71f5c4897d78729e7eaff4 Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 17bbeddec6..356398294e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19007,12 +19007,12 @@ virDomainFeaturesDefParse(virDomainDef *def, int feature; int value; g_autofree char *ptval = NULL; - g_autofree char *tmp = NULL; if ((n = virXPathNodeSet("./features/xen/*", ctxt, &nodes)) < 0) return -1; for (i = 0; i < n; i++) { + g_autofree char *tmp = NULL; feature = virDomainXenTypeFromString((const char *)nodes[i]->name); if (feature < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, -- 2.26.3
On 4/19/21 7:54 AM, Tim Wiederhake wrote:
Fixes: 94013ee04e3945307a71f5c4897d78729e7eaff4
While not necessary, a short explanation would have led to less time spent loooking into the original patch.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 17bbeddec6..356398294e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19007,12 +19007,12 @@ virDomainFeaturesDefParse(virDomainDef *def, int feature; int value; g_autofree char *ptval = NULL; - g_autofree char *tmp = NULL;
if ((n = virXPathNodeSet("./features/xen/*", ctxt, &nodes)) < 0) return -1;
for (i = 0; i < n; i++) { + g_autofree char *tmp = NULL; feature = virDomainXenTypeFromString((const char *)nodes[i]->name); if (feature < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
Fixes: 3caa28dc50df7ec215713075d669b20bef6473a2 Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tools/virsh-checkpoint.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-checkpoint.c b/tools/virsh-checkpoint.c index 20a02b0b02..afe849dd16 100644 --- a/tools/virsh-checkpoint.c +++ b/tools/virsh-checkpoint.c @@ -721,7 +721,6 @@ cmdCheckpointList(vshControl *ctl, virDomainCheckpointPtr checkpoint = NULL; long long creation_longlong; g_autoptr(GDateTime) then = NULL; - g_autofree gchar *thenstr = NULL; bool tree = vshCommandOptBool(cmd, "tree"); bool name = vshCommandOptBool(cmd, "name"); bool from = vshCommandOptBool(cmd, "from"); @@ -804,6 +803,7 @@ cmdCheckpointList(vshControl *ctl, } for (i = 0; i < checkpointlist->nchks; i++) { + g_autofree gchar *thenstr = NULL; const char *chk_name; /* free up memory from previous iterations of the loop */ -- 2.26.3
On 4/19/21 7:54 AM, Tim Wiederhake wrote:
Fixes: 3caa28dc50df7ec215713075d669b20bef6473a2
As with the last patch, a short description would have gained me the time I spent looking into the original patch (on the other hand, not having the explanation forces me to go back to the source to verify the problem and the fix, so...)
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
Fixes: 3caa28dc50df7ec215713075d669b20bef6473a2 Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tools/virsh-snapshot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 2bec722c61..e64117785c 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -1497,7 +1497,6 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) char *state = NULL; long long creation_longlong; g_autoptr(GDateTime) then = NULL; - g_autofree gchar *thenstr = NULL; bool tree = vshCommandOptBool(cmd, "tree"); bool name = vshCommandOptBool(cmd, "name"); bool from = vshCommandOptBool(cmd, "from"); @@ -1592,6 +1591,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) } for (i = 0; i < snaplist->nsnaps; i++) { + g_autofree gchar *thenstr = NULL; const char *snap_name; /* free up memory from previous iterations of the loop */ -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tools/virsh-checkpoint.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/virsh-checkpoint.c b/tools/virsh-checkpoint.c index afe849dd16..e88f9ffb47 100644 --- a/tools/virsh-checkpoint.c +++ b/tools/virsh-checkpoint.c @@ -628,7 +628,8 @@ virshCheckpointListCollect(vshControl *ctl, } } - if (!(orig_flags & VIR_DOMAIN_CHECKPOINT_LIST_TOPOLOGICAL)) + if (!(orig_flags & VIR_DOMAIN_CHECKPOINT_LIST_TOPOLOGICAL) && + checkpointlist->chks) qsort(checkpointlist->chks, checkpointlist->nchks, sizeof(*checkpointlist->chks), virshChkSorter); -- 2.26.3
participants (2)
-
Laine Stump -
Tim Wiederhake