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@xxxxxxxxxx>
> ---
> 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)) {
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
