On 08/01/2018 06:24 AM, skobyda@xxxxxxxxxx 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@xxxxxxxxxx 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; >>>>> >>>>> -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
