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, skobyda@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:
<define name="shmem"> <element name="shmem"> <attribute name="name"> <data type="string"> <param name="pattern">[^/]*</param> </data>
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 <file> <schema> 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;
On 08/01/2018 06:24 AM, skobyda@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, skobyda@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:
<define name="shmem"> <element name="shmem"> <attribute name="name"> <data type="string"> <param name="pattern">[^/]*</param> </data>
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 <file> <schema> 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;
participants (2)
-
John Ferlan -
skobyda＠redhat.com