1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH v2] conf: virDomainDefValidateInternal prohibit some characters in shmem name

Simon Kobyda posted 1 patch 5 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180712131005.11000-1-skobyda@redhat.com
Test syntax-check passed
There is a newer version of this series
src/conf/domain_conf.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
[libvirt] [PATCH v2] conf: virDomainDefValidateInternal prohibit some characters in shmem name
Posted by Simon Kobyda 5 years, 9 months ago 
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
Re: [libvirt] [PATCH v2] conf: virDomainDefValidateInternal prohibit some characters in shmem name
Posted by Simon Kobyda 5 years, 9 months ago 
Signed-off-by: Simon Kobyda <skobyda@redhat.com>

On Thu, Jul 12, 2018 at 3:10 PM Simon Kobyda <skobyda@redhat.com> 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
Re: [libvirt] [PATCH v2] conf: virDomainDefValidateInternal prohibit some characters in shmem name
Posted by John Ferlan 5 years, 9 months ago 

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>

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 <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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.