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@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);
if (STREQ(name, valid->name))
break;
valid++;
diff --git a/tools/vsh.h b/tools/vsh.h
index 8f7df9ff8..112b1b57d 100644
--- a/tools/vsh.h
+++ b/tools/vsh.h
@@ -188,7 +188,8 @@ struct _vshCmdDef {
struct _vshCmd {
const vshCmdDef *def; /* command definition */
vshCmdOpt *opts; /* list of command arguments */
- vshCmd *next; /* next command */
+ vshCmd *next; /* next command */
+ bool skipChecks; /* skip validity checks when retrieving opts */
};
/*
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
> if (STREQ(name, valid->name))
> break;
> valid++;
>diff --git a/tools/vsh.h b/tools/vsh.h
>index 8f7df9ff8..112b1b57d 100644
>--- a/tools/vsh.h
>+++ b/tools/vsh.h
>@@ -188,7 +188,8 @@ struct _vshCmdDef {
> struct _vshCmd {
> const vshCmdDef *def; /* command definition */
> vshCmdOpt *opts; /* list of command arguments */
>- vshCmd *next; /* next command */
>+ vshCmd *next; /* next command */
>+ bool skipChecks; /* skip validity checks when retrieving opts */
> };
>
> /*
>--
>2.13.6
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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...
John
>> if (STREQ(name, valid->name))
>> break;
>> valid++;
>> diff --git a/tools/vsh.h b/tools/vsh.h
>> index 8f7df9ff8..112b1b57d 100644
>> --- a/tools/vsh.h
>> +++ b/tools/vsh.h
>> @@ -188,7 +188,8 @@ struct _vshCmdDef {
>> struct _vshCmd {
>> const vshCmdDef *def; /* command definition */
>> vshCmdOpt *opts; /* list of command arguments */
>> - vshCmd *next; /* next command */
>> + vshCmd *next; /* next command */
>> + bool skipChecks; /* skip validity checks when
>> retrieving opts */
>> };
>>
>> /*
>> --
>> 2.13.6
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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)`
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
I didn't say it was exactly related, just "related somewhat".
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.