Re: [libvirt] [PATCH v2] conf: virDomainDefValidateInternal prohibit some characters in shmem name
On 08/01/2018 06:24 AM, skob...@redhat.com wrote:
> On Fri, 2018-07-27 at 13:19 -0400, John Ferlan wrote:
>> You shouldn't drop libvir-list when you reply...
> Sorry, I dropped it by mistake.
>>
>> On 07/27/2018 06:35 AM, skob...@redhat.com wrote:
>>> On Mon, 2018-07-23 at 17:32 -0400, John Ferlan wrote:
On 07/12/2018 09:10 AM, Simon Kobyda wrote:
> XML shmem name will not include character '/', and will not be
> equal to strings
> "." or "..", as shmem name is used in a path.
Validate that the provided XML shmem name is not directory
specific
"."
or ".." names as well as ensuring that there is no path separator
'/'
in
the name.
> https://bugzilla.redhat.com/show_bug.cgi?id=1192400
> ---
>
> Changes in V2
> - Added error reports
> - Error situation will happen only if shmem name is
> equal to
> "." or "..", however their occurence in a name
> compromised of
> more
> characters is allowed.
>
> src/conf/domain_conf.c | 22 ++
> 1 file changed, 22 insertions(+)
>
I believe this actually belongs in
virDomainDeviceDefValidateInternal
for case VIR_DOMAIN_DEVICE_SHMEM.
Also, should the docs/schemas/domaincommon.rng be modified?
Currently
it
has:
[^/]*
>>>
>>> Is it a good idea? To create such regular expression, I believe it
>>> would make it unreadable for another person, ergo useless. I don't
>>> want
>>> it to end up as IPv6. I've benn told that actual parser can be, and
>>> sometimes is stricter than scheme.
>>>
>>
>> Well this is what I was hoping others would be able to chime in on.
>> Hence why you ask on list. I don't disagree that being stricter in
>> Parse
>> is fine. Doing regexes is like reading a foreign language to me at
>> times. Some just don't make sense.
>
> Yeah, that's why I don't want to try to insert complicated regex in
> docs/schemas/domaincommon.rng.
>>
>> I think that regex is indicating - any character except '/', but I'm
>> not
>> sure. If it is, it's ironic that we have to check for '/'
>> specifically
>> since it shouldn't be passing the xml validation test - OK at least
>> if
>> one was editing via virsh.
>>
>> John
>>
> You are right about what that regex does. But from what I heard, that
> functionality is rather new to libvirt, so users have option to bypass
> that verification. So I would like to also leave it in code in case
> users try to bypass that regex verification.
>
> Simon.
>
See commit id '1c06d0fa' for the shmem validation previously added.
Ironically a higher numbered bz than referenced your commit.
And yes it's quite easy to tell virsh edit to ignore the xml validation
error. I'm fine with leaving the RNG as is. I had a different bz in my
pile related to resource names, so the topic was somewhat fresh in mind
related to looking at what various RNG "patterns" had, see:
https://www.redhat.com/archives/libvir-list/2018-July/msg02046.html
in that case the problem is that it's possible to name something " " or
" " and you don't know whether a space or tab or any combination was
used for the entirety of the name.
I think if you move the code from virDomainDefValidateInternal to a
SHMEM specific case in virDomainDeviceDefValidateInternal, then that'll
be fine. I would think that the qemuProcessStartValidateShmem call from
commit 1c06d0fa would not be possible then.
John
Consider how other names are limited in their scope. The
basictypes.rng
has a number of examples.
Naturally, the problem with changing it is that someone somewhere
will
complain, but libvirt used to accept this other format. Right now
I
would think the scope a bit too broad.
>>>
>>> But then we cannot fix most of the bugs, since there could always
>>> be
>>> some user which relies on bug behavior.
If we are to limit the name we should also document in
docs/formatdomain.html.in that the shmem name is "limited" in
name to
avoid the '/' character, ".", and "..".
BTW: My regex isn't that good, but it would seem '/' is an
invalid
character by XML standards even though the code never checked for
it.
Using virt-xml-validate would "validate" whether
someone
provides valid XML.
John
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7ab2953d83..6b34c17de4 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6107,6 +6107,8 @@ virDomainDefLifecycleActionValidate(const
> virDomainDef *def)
> static int
> virDomainDefValidateInternal(const virDomainDef *def)
> {
> +size_t i;
> +
> if (virDomainDefCheckDuplicateDiskInfo(def) < 0)
> return -1;
>
> @@ -6136,6 +6138,26
Re: [libvirt] [PATCH v2] conf: virDomainDefValidateInternal prohibit some characters in shmem name
On Fri, 2018-07-27 at 13:19 -0400, John Ferlan wrote:
> You shouldn't drop libvir-list when you reply...
Sorry, I dropped it by mistake.
>
> On 07/27/2018 06:35 AM, skob...@redhat.com wrote:
> > On Mon, 2018-07-23 at 17:32 -0400, John Ferlan wrote:
> > >
> > > On 07/12/2018 09:10 AM, Simon Kobyda wrote:
> > > > XML shmem name will not include character '/', and will not be
> > > > equal to strings
> > > > "." or "..", as shmem name is used in a path.
> > >
> > > Validate that the provided XML shmem name is not directory
> > > specific
> > > "."
> > > or ".." names as well as ensuring that there is no path separator
> > > '/'
> > > in
> > > the name.
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1192400
> > > > ---
> > > >
> > > > Changes in V2
> > > > - Added error reports
> > > > - Error situation will happen only if shmem name is
> > > > equal to
> > > > "." or "..", however their occurence in a name
> > > > compromised of
> > > > more
> > > > characters is allowed.
> > > >
> > > > src/conf/domain_conf.c | 22 ++
> > > > 1 file changed, 22 insertions(+)
> > > >
> > >
> > > I believe this actually belongs in
> > > virDomainDeviceDefValidateInternal
> > > for case VIR_DOMAIN_DEVICE_SHMEM.
> > > Also, should the docs/schemas/domaincommon.rng be modified?
> > > Currently
> > > it
> > > has:
> > >
> > >
> > >
> > >
> > >
> > > [^/]*
> > >
> > >
> >
> > Is it a good idea? To create such regular expression, I believe it
> > would make it unreadable for another person, ergo useless. I don't
> > want
> > it to end up as IPv6. I've benn told that actual parser can be, and
> > sometimes is stricter than scheme.
> >
>
> Well this is what I was hoping others would be able to chime in on.
> Hence why you ask on list. I don't disagree that being stricter in
> Parse
> is fine. Doing regexes is like reading a foreign language to me at
> times. Some just don't make sense.
Yeah, that's why I don't want to try to insert complicated regex in
docs/schemas/domaincommon.rng.
>
> I think that regex is indicating - any character except '/', but I'm
> not
> sure. If it is, it's ironic that we have to check for '/'
> specifically
> since it shouldn't be passing the xml validation test - OK at least
> if
> one was editing via virsh.
>
> John
>
You are right about what that regex does. But from what I heard, that
functionality is rather new to libvirt, so users have option to bypass
that verification. So I would like to also leave it in code in case
users try to bypass that regex verification.
Simon.
> > > Consider how other names are limited in their scope. The
> > > basictypes.rng
> > > has a number of examples.
> > >
> > > Naturally, the problem with changing it is that someone somewhere
> > > will
> > > complain, but libvirt used to accept this other format. Right now
> > > I
> > > would think the scope a bit too broad.
> >
> > But then we cannot fix most of the bugs, since there could always
> > be
> > some user which relies on bug behavior.
> > >
> > > If we are to limit the name we should also document in
> > > docs/formatdomain.html.in that the shmem name is "limited" in
> > > name to
> > > avoid the '/' character, ".", and "..".
> > >
> > > BTW: My regex isn't that good, but it would seem '/' is an
> > > invalid
> > > character by XML standards even though the code never checked for
> > > it.
> > > Using virt-xml-validate would "validate" whether
> > > someone
> > > provides valid XML.
> > >
> > >
> > > John
> > >
> > > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > > > index 7ab2953d83..6b34c17de4 100644
> > > > --- a/src/conf/domain_conf.c
> > > > +++ b/src/conf/domain_conf.c
> > > > @@ -6107,6 +6107,8 @@ virDomainDefLifecycleActionValidate(const
> > > > virDomainDef *def)
> > > > static int
> > > > virDomainDefValidateInternal(const virDomainDef *def)
> > > > {
> > > > +size_t i;
> > > > +
> > > > if (virDomainDefCheckDuplicateDiskInfo(def) < 0)
> > > > return -1;
> > > >
> > > > @@ -6136,6 +6138,26 @@ virDomainDefValidateInternal(const
> > > > virDomainDef *def)
> > > > return -1;
> > > > }
> > > >
> > > > +for (i = 0; i < def->nshmems; i++) {
> > > > +if (strchr(def->shmems[i]->name, '/')) {
> > > > +virReportError(VIR_ERR_XML_ERROR, "%s",
> > > > + _("shmem name cannot include '/'
> > > > character"));
> > > > +return -1;
> > > > +}
> > > > +
> > > > +if (STREQ(def->shmems[i]->name, ".")) {
> > > > +virReportError(VIR_ERR_XML_ERROR, "%s",
> > > > + _("shmem name cannot be equal to
> > > > '.'"));
> > > > +return -1;
> > > > +}
> > > > +
> > > > +if (STREQ(def->shmems[i]->name, "..")) {
> > > > +virReportError(VIR_ERR_XML_ERROR, "%s",
> > > > +
Re: [libvirt] [PATCH v2] conf: virDomainDefValidateInternal prohibit some characters in shmem name
On 07/12/2018 09:10 AM, Simon Kobyda wrote:
> XML shmem name will not include character '/', and will not be equal to
> strings
> "." or "..", as shmem name is used in a path.
Validate that the provided XML shmem name is not directory specific "."
or ".." names as well as ensuring that there is no path separator '/' in
the name.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1192400
> ---
>
> Changes in V2
> - Added error reports
> - Error situation will happen only if shmem name is equal to
> "." or "..", however their occurence in a name compromised of more
> characters is allowed.
>
> src/conf/domain_conf.c | 22 ++
> 1 file changed, 22 insertions(+)
>
I believe this actually belongs in virDomainDeviceDefValidateInternal
for case VIR_DOMAIN_DEVICE_SHMEM.
Also, should the docs/schemas/domaincommon.rng be modified? Currently it
has:
[^/]*
Consider how other names are limited in their scope. The basictypes.rng
has a number of examples.
Naturally, the problem with changing it is that someone somewhere will
complain, but libvirt used to accept this other format. Right now I
would think the scope a bit too broad.
If we are to limit the name we should also document in
docs/formatdomain.html.in that the shmem name is "limited" in name to
avoid the '/' character, ".", and "..".
BTW: My regex isn't that good, but it would seem '/' is an invalid
character by XML standards even though the code never checked for it.
Using virt-xml-validate would "validate" whether someone
provides valid XML.
John
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7ab2953d83..6b34c17de4 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6107,6 +6107,8 @@ virDomainDefLifecycleActionValidate(const virDomainDef
> *def)
> static int
> virDomainDefValidateInternal(const virDomainDef *def)
> {
> +size_t i;
> +
> if (virDomainDefCheckDuplicateDiskInfo(def) < 0)
> return -1;
>
> @@ -6136,6 +6138,26 @@ virDomainDefValidateInternal(const virDomainDef *def)
> return -1;
> }
>
> +for (i = 0; i < def->nshmems; i++) {
> +if (strchr(def->shmems[i]->name, '/')) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("shmem name cannot include '/' character"));
> +return -1;
> +}
> +
> +if (STREQ(def->shmems[i]->name, ".")) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("shmem name cannot be equal to '.'"));
> +return -1;
> +}
> +
> +if (STREQ(def->shmems[i]->name, "..")) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("shmem name cannot be equal to '..'"));
> +return -1;
> +}
> +}
> +
> if (virDomainDefLifecycleActionValidate(def) < 0)
> return -1;
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] conf: virDomainDefValidateInternal prohibit some characters in shmem name
Signed-off-by: Simon Kobyda
On Thu, Jul 12, 2018 at 3:10 PM Simon Kobyda wrote:
> XML shmem name will not include character '/', and will not be equal to
> strings
> "." or "..", as shmem name is used in a path.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1192400
> ---
>
> Changes in V2
> - Added error reports
> - Error situation will happen only if shmem name is equal to
> "." or "..", however their occurence in a name compromised of
> more
> characters is allowed.
>
> src/conf/domain_conf.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7ab2953d83..6b34c17de4 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6107,6 +6107,8 @@ virDomainDefLifecycleActionValidate(const
> virDomainDef *def)
> static int
> virDomainDefValidateInternal(const virDomainDef *def)
> {
> +size_t i;
> +
> if (virDomainDefCheckDuplicateDiskInfo(def) < 0)
> return -1;
>
> @@ -6136,6 +6138,26 @@ virDomainDefValidateInternal(const virDomainDef
> *def)
> return -1;
> }
>
> +for (i = 0; i < def->nshmems; i++) {
> +if (strchr(def->shmems[i]->name, '/')) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("shmem name cannot include '/' character"));
> +return -1;
> +}
> +
> +if (STREQ(def->shmems[i]->name, ".")) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("shmem name cannot be equal to '.'"));
> +return -1;
> +}
> +
> +if (STREQ(def->shmems[i]->name, "..")) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("shmem name cannot be equal to '..'"));
> +return -1;
> +}
> +}
> +
> if (virDomainDefLifecycleActionValidate(def) < 0)
> return -1;
>
> --
> 2.17.1
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] conf: virDomainDefValidateInternal prohibit some characters in shmem name
XML shmem name will not include character '/', and will not be equal to strings
"." or "..", as shmem name is used in a path.
https://bugzilla.redhat.com/show_bug.cgi?id=1192400
---
Changes in V2
- Added error reports
- Error situation will happen only if shmem name is equal to
"." or "..", however their occurence in a name compromised of more
characters is allowed.
src/conf/domain_conf.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7ab2953d83..6b34c17de4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6107,6 +6107,8 @@ virDomainDefLifecycleActionValidate(const virDomainDef
*def)
static int
virDomainDefValidateInternal(const virDomainDef *def)
{
+size_t i;
+
if (virDomainDefCheckDuplicateDiskInfo(def) < 0)
return -1;
@@ -6136,6 +6138,26 @@ virDomainDefValidateInternal(const virDomainDef *def)
return -1;
}
+for (i = 0; i < def->nshmems; i++) {
+if (strchr(def->shmems[i]->name, '/')) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("shmem name cannot include '/' character"));
+return -1;
+}
+
+if (STREQ(def->shmems[i]->name, ".")) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("shmem name cannot be equal to '.'"));
+return -1;
+}
+
+if (STREQ(def->shmems[i]->name, "..")) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("shmem name cannot be equal to '..'"));
+return -1;
+}
+}
+
if (virDomainDefLifecycleActionValidate(def) < 0)
return -1;
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
