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

[Martin Kletzander]

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 didn't say it was exactly related, just "related somewhat".
>
>John
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180111/d95224a0/attachment-0001.sig>