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", > > > > + _("shm
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