[libvirt] [PATCH 08/18] vshCommandOpt: Allow caller avoiding assert()

[Michal Privoznik]

On 01/11/2018 02:31 PM, Martin Kletzander wrote:
> On Thu, Jan 11, 2018 at 08:09:25AM -0500, John Ferlan wrote:
>>
>>
>> On 01/11/2018 07:55 AM, Martin Kletzander wrote:
>>> On Thu, Jan 11, 2018 at 06:02:58AM -0500, John Ferlan wrote:
>>>>
>>>>
>>>> On 01/11/2018 05:50 AM, Martin Kletzander wrote:
>>>>> On Tue, Jan 02, 2018 at 06:12:01PM +0100, Michal Privoznik wrote:
>>>>>> In the future, completer callbacks will receive partially parsed
>>>>>> command (and thus possibly incomplete). However, we still want
>>>>>> them to use command options fetching APIs we already have (e.g.
>>>>>> vshCommandOpt*()) and at the same time don't report any errors
>>>>>> (nor call any asserts).
>>>>>>
>>>>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>>>>> ---
>>>>>> tools/vsh.c | 7 ++++---
>>>>>> tools/vsh.h | 3 ++-
>>>>>> 2 files changed, 6 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/vsh.c b/tools/vsh.c
>>>>>> index ebc8d9cb1..d27acb95b 100644
>>>>>> --- a/tools/vsh.c
>>>>>> +++ b/tools/vsh.c
>>>>>> @@ -815,8 +815,8 @@ vshCommandFree(vshCmd *cmd)
>>>>>>  * to the option if found, 0 with *OPT set to NULL if the name is
>>>>>>  * valid and the option is not required, -1 with *OPT set to NULL if
>>>>>>  * the option is required but not present, and assert if NAME is not
>>>>>> - * valid (which indicates a programming error).  No error
>>>>>> messages are
>>>>>> - * issued if a value is returned.
>>>>>> + * valid (which indicates a programming error) unless
>>>>>> cmd->skipChecks
>>>>>> + * is set. No error messages are issued if a value is returned.
>>>>>>  */
>>>>>> static int
>>>>>> vshCommandOpt(const vshCmd *cmd, const char *name, vshCmdOpt **opt,
>>>>>> @@ -829,7 +829,8 @@ vshCommandOpt(const vshCmd *cmd, const char
>>>>>> *name,
>>>>>> vshCmdOpt **opt,
>>>>>>     /* See if option is valid and/or required.  */
>>>>>>     *opt = NULL;
>>>>>>     while (valid) {
>>>>>> -        assert(valid->name);
>>>>>> +        if (!cmd->skipChecks)
>>>>>> +            assert(valid->name);
>>>>>
>>>>> This can segfault when cmd->skipChecks == False && valid->name ==
>>>>> NULL,
>>>>> which is what the assert() guarded before.
>>>>>
>>>>> So either STREQ_NULLABLE or another if.
>>>>>
>>>>
>>>> Hmmm... Also see:
>>>>
>>>> https://www.redhat.com/archives/libvir-list/2017-December/msg00605.html
>>>>
>>>> it's related somewhat...
>>>>
>>>
>>> I don't see how, this is all wrapped in `while (valid)`
>>
>> The other patch is "after" the loop.
>>
>> Look at the entire context... although we know it's a software
>> engineering error to not have some sort of match, some compiler believes
>> we can exit the "while (valid)" loop with "valid == NULL", follwed by
>> the next assert which dereferences @valid without asserting if valid is
>> non-NULL.
>>
> 
> Oh, I guess that patch from Marc should be pushed (I still didn't get to
> soooo many patches on the list) and this should then handle addition to
> that change as well.  I got confused by your vague "somewhat related" =)

I don't think it should be pushed. The whole loop is foobared. I'm
looking into it. The problem is, we initialize an array of opts like this:

static const vshCmdOptDef opts_iothreadpin[] = {
    VIRSH_COMMON_OPT_DOMAIN_FULL(0),
    {.name = "iothread",
     .type = VSH_OT_INT,
     .flags = VSH_OFLAG_REQ,
     .help = N_("IOThread ID number")
    },
    {.name = "cpulist",
     .type = VSH_OT_DATA,
     .flags = VSH_OFLAG_REQ,
     .help = N_("host cpu number(s) to set")
    },
    VIRSH_COMMON_OPT_DOMAIN_CONFIG,
    VIRSH_COMMON_OPT_DOMAIN_LIVE,
    VIRSH_COMMON_OPT_DOMAIN_CURRENT,
    {.name = NULL}
};


So the array is NOT NULL terminated. Therefore, if we iterate over it
like this:

while (valid) {
    ...
    valid++;
}

we will definitely read outside of the array. But as I've said, I'm
looking into this and I think I have a patch ready. But before that,
please don't push that patch of Marc's as it's not fixing the real
issue.

Michal