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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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)) {
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list