[libvirt] [PATCH] vshReadlineOptionsPrune: Fix possible leak
The function should prune list of --options so that options already specified are not offered to user for completion again. However, if the list of offered options contains a string that doesn't start with double dash the function returns leaking partially constructed list. There's not much benefit from trying to roll back. Just free everything up - our only caller would do that anyway. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tools/vsh.c b/tools/vsh.c index 4426c08d6..7db0a16f1 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2798,8 +2798,17 @@ vshReadlineOptionsPrune(char ***list, vshCmdOpt *opt = last->opts; /* Should never happen (TM) */ - if (!list_opt) + if (!list_opt) { + /* But in case it does, we're in a tough situation + * because @list[0..i-1] is possibly sparse. That + * means if caller were to call virStringListFree + * over it some memory is definitely going to be + * leaked. The best we can do is to free from list[i] + * as our only caller is just fine with it. */ + virStringListFree(list[i]); + virStringListFree(newList); return -1; + } while (opt) { if (STREQ(opt->def->name, list_opt)) { -- 2.13.6
On 01/12/2018 11:08 AM, Michal Privoznik wrote:
The function should prune list of --options so that options already specified are not offered to user for completion again. However, if the list of offered options contains a string that doesn't start with double dash the function returns leaking partially constructed list. There's not much benefit from trying to roll back. Just free everything up - our only caller would do that anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/tools/vsh.c b/tools/vsh.c index 4426c08d6..7db0a16f1 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2798,8 +2798,17 @@ vshReadlineOptionsPrune(char ***list, vshCmdOpt *opt = last->opts;
/* Should never happen (TM) */ - if (!list_opt) + if (!list_opt) { + /* But in case it does, we're in a tough situation + * because @list[0..i-1] is possibly sparse. That + * means if caller were to call virStringListFree + * over it some memory is definitely going to be + * leaked. The best we can do is to free from list[i] + * as our only caller is just fine with it. */ + virStringListFree(list[i]);
Sorry for opening Pandora's box of pain and suffering. There's something about this that is just strange... Since list is a VIR_ALLOC_N list with N entries filled in.... Passing virStringListFree(list[i]) would be the current entry and I think would be fine in the "while (tmp && *tmp) loop; however, when the code gets to VIR_FREE(strings), wouldn't that be the "wrong place" (e.g. list[i] instead of list)? Later we would then VIR_FREE(list) because of the -1 return, which would be correct. So all that said, should this be something like: while (list_len > i) VIR_FREE((*list)[--list_len]); (could be gated with an i > 0 too). then later when the caller runs virStringListFree it does the VIR_FREE(strings) avoiding of course the while(tmp && *tmp) loop. John
+ virStringListFree(newList); return -1; + }
while (opt) { if (STREQ(opt->def->name, list_opt)) {
On 01/12/2018 10:07 PM, John Ferlan wrote:
On 01/12/2018 11:08 AM, Michal Privoznik wrote:
The function should prune list of --options so that options already specified are not offered to user for completion again. However, if the list of offered options contains a string that doesn't start with double dash the function returns leaking partially constructed list. There's not much benefit from trying to roll back. Just free everything up - our only caller would do that anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/tools/vsh.c b/tools/vsh.c index 4426c08d6..7db0a16f1 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2798,8 +2798,17 @@ vshReadlineOptionsPrune(char ***list, vshCmdOpt *opt = last->opts;
/* Should never happen (TM) */ - if (!list_opt) + if (!list_opt) { + /* But in case it does, we're in a tough situation + * because @list[0..i-1] is possibly sparse. That + * means if caller were to call virStringListFree + * over it some memory is definitely going to be + * leaked. The best we can do is to free from list[i] + * as our only caller is just fine with it. */ + virStringListFree(list[i]);
Sorry for opening Pandora's box of pain and suffering.
There's something about this that is just strange... Since list is a VIR_ALLOC_N list with N entries filled in....
Passing virStringListFree(list[i]) would be the current entry and I think would be fine in the "while (tmp && *tmp) loop; however, when the code gets to VIR_FREE(strings), wouldn't that be the "wrong place" (e.g. list[i] instead of list)? Later we would then VIR_FREE(list) because of the -1 return, which would be correct.
So all that said, should this be something like:
while (list_len > i) VIR_FREE((*list)[--list_len]);
(could be gated with an i > 0 too).
then later when the caller runs virStringListFree it does the VIR_FREE(strings) avoiding of course the while(tmp && *tmp) loop.
Okay, I think something fishy is going on here. I mean, if I modify the condition to: if (!list_opt || i > 10) { I can not only see the leak but even virsh is crashing for me. Anyway, why don't we take a different approach - when constructing the list of --options simply don't put already specified --options there instead of pruning the list later. I'm gonna send a patch for that in a moment. Michal
participants (2)
-
John Ferlan -
Michal Privoznik