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

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(a)redhat.com&gt; >>>> --- >>>> 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.

I didn't say it was exactly related, just "related somewhat". John -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list