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

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(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. > 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" =)