8:33 a.m.
We currently ignore unknown elements in the CPU XML description, e.g. with vi=
rsh
cpu-compare and hypervisor-cpu-compare. This makes '<cpu><faeture
name=3D"...=
"/></cpu>'
(note the typo in "faeture") semantically identic to '<cpu/>'. No
error is re=
ported.
This series adds checks for unrecognized attributes and elements in the "<cpu=
" element,
catching this kind of mistake.
An alternative approach to this problem would be to create a new schema, whic=
h might
turn out awkward as cpu-compare and hypervisor-cpu-compare accept a wide rang=
e of
cpu descriptions, including full domain, capabilities, and domainCapabilities=
XML.
Tim Wiederhake (3):
tests: Allow cpuTestLoadXML to fail for the guest if we expect it the
test to fail
tests: Ensure that cpu comparison fails in the presence of unknown xml
elements
cpu: Fail CPU comparison in the presence of unknown elements.
src/conf/cpu_conf.c | 38 ++++++++++++++++++++
tests/cputest.c | 15 +++++---
tests/cputestdata/x86_64-bogus-attribute.xml | 2 ++
tests/cputestdata/x86_64-bogus-element.xml | 3 ++
4 files changed, 54 insertions(+), 4 deletions(-)
create mode 100644 tests/cputestdata/x86_64-bogus-attribute.xml
create mode 100644 tests/cputestdata/x86_64-bogus-element.xml
--=20
2.26.2
8:33 a.m.
New subject: [libvirt PATCH 1/3] tests: Allow cpuTestLoadXML to fail for the guest if we expect it the test to fail
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
tests/cputest.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/tests/cputest.c b/tests/cputest.c
index 83d63bf495..195a215b84 100644
--- a/tests/cputest.c
+++ b/tests/cputest.c
@@ -201,13 +201,18 @@ cpuTestCompare(const void *arg)
int ret = -1;
virCPUDefPtr host = NULL;
virCPUDefPtr cpu = NULL;
- virCPUCompareResult result;
+ virCPUCompareResult result = VIR_CPU_COMPARE_ERROR;
- if (!(host = cpuTestLoadXML(data->arch, data->host)) ||
- !(cpu = cpuTestLoadXML(data->arch, data->name)))
+ if (!(host = cpuTestLoadXML(data->arch, data->host)))
goto cleanup;
- result = virCPUCompare(host->arch, host, cpu, false);
+ if (!(cpu = cpuTestLoadXML(data->arch, data->name))) {
+ if (data->result != VIR_CPU_COMPARE_ERROR)
+ goto cleanup;
+ } else {
+ result = virCPUCompare(host->arch, host, cpu, false);
+ }
+
if (data->result == VIR_CPU_COMPARE_ERROR)
virResetLastError();
--
2.26.2
8:33 a.m.
New subject: [libvirt PATCH 2/3] tests: Ensure that cpu comparison fails in the presence of unknown xml elements
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
tests/cputest.c | 2 ++
tests/cputestdata/x86_64-bogus-attribute.xml | 2 ++
tests/cputestdata/x86_64-bogus-element.xml | 3 +++
3 files changed, 7 insertions(+)
create mode 100644 tests/cputestdata/x86_64-bogus-attribute.xml
create mode 100644 tests/cputestdata/x86_64-bogus-element.xml
diff --git a/tests/cputest.c b/tests/cputest.c
index 195a215b84..4bfd20dcaf 100644
--- a/tests/cputest.c
+++ b/tests/cputest.c
@@ -1114,6 +1114,8 @@ mymain(void)
/* guest to host comparison */
DO_TEST_COMPARE(VIR_ARCH_X86_64, "host", "bogus-model",
VIR_CPU_COMPARE_ERROR);
DO_TEST_COMPARE(VIR_ARCH_X86_64, "host", "bogus-feature",
VIR_CPU_COMPARE_ERROR);
+ DO_TEST_COMPARE(VIR_ARCH_X86_64, "host", "bogus-element",
VIR_CPU_COMPARE_ERROR);
+ DO_TEST_COMPARE(VIR_ARCH_X86_64, "host", "bogus-attribute",
VIR_CPU_COMPARE_ERROR);
DO_TEST_COMPARE(VIR_ARCH_X86_64, "host", "min",
VIR_CPU_COMPARE_SUPERSET);
DO_TEST_COMPARE(VIR_ARCH_X86_64, "host", "pentium3",
VIR_CPU_COMPARE_SUPERSET);
DO_TEST_COMPARE(VIR_ARCH_X86_64, "host", "exact",
VIR_CPU_COMPARE_SUPERSET);
diff --git a/tests/cputestdata/x86_64-bogus-attribute.xml
b/tests/cputestdata/x86_64-bogus-attribute.xml
new file mode 100644
index 0000000000..073ee793e6
--- /dev/null
+++ b/tests/cputestdata/x86_64-bogus-attribute.xml
@@ -0,0 +1,2 @@
+<cpu nonExistantAttribute="">
+</cpu>
diff --git a/tests/cputestdata/x86_64-bogus-element.xml
b/tests/cputestdata/x86_64-bogus-element.xml
new file mode 100644
index 0000000000..79f98bad18
--- /dev/null
+++ b/tests/cputestdata/x86_64-bogus-element.xml
@@ -0,0 +1,3 @@
+<cpu>
+ <nonExistantElement/>
+</cpu>
--
2.26.2
8:33 a.m.
New subject: [libvirt PATCH 3/3] cpu: Fail CPU comparison in the presence of unknown elements.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/conf/cpu_conf.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index dea950ce68..a2ba92ced3 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -659,6 +659,44 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
def->cache->mode = mode;
}
+ g_clear_pointer(&nodes, g_free);
+ if ((n = virXPathNodeSet("./@*", ctxt, &nodes)) < 0)
+ return -1;
+
+ for (i = 0; i < n; i++) {
+ if (virXMLNodeNameEqual(nodes[i], "check") ||
+ virXMLNodeNameEqual(nodes[i], "match") ||
+ virXMLNodeNameEqual(nodes[i], "migratable") ||
+ virXMLNodeNameEqual(nodes[i], "mode"))
+ continue;
+ virReportError(VIR_ERR_XML_ERROR,
+ _("unrecognized attribute in cpu element: %s"),
+ nodes[i]->name);
+ return -1;
+ }
+
+ g_clear_pointer(&nodes, g_free);
+ if ((n = virXPathNodeSet("./*", ctxt, &nodes)) < 0)
+ return -1;
+
+ for (i = 0; i < n; i++) {
+ if (virXMLNodeNameEqual(nodes[i], "arch") ||
+ virXMLNodeNameEqual(nodes[i], "cache") ||
+ virXMLNodeNameEqual(nodes[i], "counter") ||
+ virXMLNodeNameEqual(nodes[i], "feature") ||
+ virXMLNodeNameEqual(nodes[i], "model") ||
+ virXMLNodeNameEqual(nodes[i], "microcode") ||
+ virXMLNodeNameEqual(nodes[i], "numa") ||
+ virXMLNodeNameEqual(nodes[i], "pages") ||
+ virXMLNodeNameEqual(nodes[i], "topology") ||
+ virXMLNodeNameEqual(nodes[i], "vendor"))
+ continue;
+ virReportError(VIR_ERR_XML_ERROR,
+ _("unrecognized element in cpu element: %s"),
+ nodes[i]->name);
+ return -1;
+ }
+
*cpu = g_steal_pointer(&def);
return 0;
}
--
2.26.2
8:43 a.m.
New subject: [libvirt PATCH 3/3] cpu: Fail CPU comparison in the presence of unknown elements.
On Wed, Sep 16, 2020 at 15:33:56 +0200, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/conf/cpu_conf.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index dea950ce68..a2ba92ced3 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -659,6 +659,44 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
def->cache->mode = mode;
}
+ g_clear_pointer(&nodes, g_free);
+ if ((n = virXPathNodeSet("./@*", ctxt, &nodes)) < 0)
+ return -1;
+
+ for (i = 0; i < n; i++) {
+ if (virXMLNodeNameEqual(nodes[i], "check") ||
+ virXMLNodeNameEqual(nodes[i], "match") ||
+ virXMLNodeNameEqual(nodes[i], "migratable") ||
+ virXMLNodeNameEqual(nodes[i], "mode"))
+ continue;
+ virReportError(VIR_ERR_XML_ERROR,
+ _("unrecognized attribute in cpu element: %s"),
+ nodes[i]->name);
+ return -1;
+ }
+
+ g_clear_pointer(&nodes, g_free);
+ if ((n = virXPathNodeSet("./*", ctxt, &nodes)) < 0)
+ return -1;
+
+ for (i = 0; i < n; i++) {
+ if (virXMLNodeNameEqual(nodes[i], "arch") ||
+ virXMLNodeNameEqual(nodes[i], "cache") ||
+ virXMLNodeNameEqual(nodes[i], "counter") ||
+ virXMLNodeNameEqual(nodes[i], "feature") ||
+ virXMLNodeNameEqual(nodes[i], "model") ||
+ virXMLNodeNameEqual(nodes[i], "microcode") ||
+ virXMLNodeNameEqual(nodes[i], "numa") ||
+ virXMLNodeNameEqual(nodes[i], "pages") ||
+ virXMLNodeNameEqual(nodes[i], "topology") ||
+ virXMLNodeNameEqual(nodes[i], "vendor"))
+ continue;
+ virReportError(VIR_ERR_XML_ERROR,
+ _("unrecognized element in cpu element: %s"),
+ nodes[i]->name);
NACK this is ugly and unscalable.
Please implement it via RNG schema validation.
8:55 a.m.
New subject: [libvirt PATCH 3/3] cpu: Fail CPU comparison in the presence of unknown elements.
On a Wednesday in 2020, Peter Krempa wrote:
On Wed, Sep 16, 2020 at 15:33:56 +0200, Tim Wiederhake wrote:
NACK this is ugly and unscalable.
Please implement it via RNG schema validation.
And if you get stuck staring at a confusing validation error from xmllint like:
extra element in interlave
give jing a try, sometimes it gives more helpful errors.
Jano
8:48 a.m.
On Wed, Sep 16, 2020 at 03:33:53PM +0200, Tim Wiederhake wrote:
We currently ignore unknown elements in the CPU XML description, e.g.
with vi=
rsh
cpu-compare and hypervisor-cpu-compare. This makes '<cpu><faeture
name=3D"...=
"/></cpu>'
(note the typo in "faeture") semantically identic to '<cpu/>'. No
error is re=
ported.
This is an intentional implementation choice in libvirt. The XML documents
that we're parsing are way to large and complex for us to justify adding
code to report errors for every invalid attribute or child element name.
We provide an RNG schema, however, which declares the valid set of attrs
and thus by running RNG schema validation, a client can get a error report
of invalid info.
We enable this validation by default in "virsh edit" and a few other
similar commands.
There are certainly libvirt APIs though which accept XML and don't have
a flag defined to enable RNG schema validation. I presume the CPU APIs
are one such case. We should add support for validation to all such
APIs accepting XML documents, and wire it up in virsh.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
8:20 a.m.
On Wed, 2020-09-16 at 14:48 +0100, Daniel P. Berrangé wrote:
On Wed, Sep 16, 2020 at 03:33:53PM +0200, Tim Wiederhake wrote:
> We currently ignore unknown elements in the CPU XML description,
> e.g. with vi=
> rsh
> cpu-compare and hypervisor-cpu-compare. This makes '<cpu><faeture
> name=3D"...=
> "/></cpu>'
> (note the typo in "faeture") semantically identic to
'<cpu/>'. No
> error is re=
> ported.
This is an intentional implementation choice in libvirt. The XML
documents
that we're parsing are way to large and complex for us to justify
adding
code to report errors for every invalid attribute or child element
name.
We provide an RNG schema, however, which declares the valid set of
attrs
and thus by running RNG schema validation, a client can get a error
report
of invalid info.
We enable this validation by default in "virsh edit" and a few other
similar commands.
There are certainly libvirt APIs though which accept XML and don't
have
a flag defined to enable RNG schema validation. I presume the CPU
APIs
are one such case. We should add support for validation to all such
APIs accepting XML documents, and wire it up in virsh.
Regards,
Daniel
Hi Daniel, all,
thanks for the input!
My first attempt at the problem I described did in fact employ RNG
schema validation on the xml documents describing the CPU. The problem
though with that approach is that `virsh [hypervisor-]cpu-compare` can
consume quite different xml documents. The tests add more possible
document types, e.g. multicpu, which force the schema for a validation
performed in `virCPUDefParseXML` to basically import all other schemas
(which produced some naming conflicts that I was unable to solve) and,
secondly, perform a lot of unneccessary validation.
I created a schema that would perform only a check for "unknown"
elements and attributes in the cpu element and ignore everything else.
I attached this for your entertainment. There you can see what I meant
with "different xml documents".
Copying / moving the node into a new document just for validation seems
not like an elegant solution (if possible at all) and I am unsure about
potential performance impact.
The best solution in my opinion would be to not perform XPath
evaluation in the parser, but instead use some stream parser and error
out if the parser encounteres an element with an unknown name.
The next best solution in my opinion is the one you reviewed. I agree
with all comments made that this is far from perfect, but at least it
would trigger visible test fails when / if new elements and attributes
are added without that list being expanded.
I will further explore whether it is possible to do node-only
validation without creating an include-everything schema, but I would
be grateful for any hint or suggestion in that direction. We should
have at least some error reporting in the case I described initially.
Thanks,
Tim
3:48 a.m.
On Thu, Sep 17, 2020 at 15:20:18 +0200, Tim Wiederhake wrote:
On Wed, 2020-09-16 at 14:48 +0100, Daniel P. Berrangé wrote:
> On Wed, Sep 16, 2020 at 03:33:53PM +0200, Tim Wiederhake wrote:
> > We currently ignore unknown elements in the CPU XML description,
> > e.g. with vi=
> > rsh
> > cpu-compare and hypervisor-cpu-compare. This makes '<cpu><faeture
> > name=3D"...=
> > "/></cpu>'
> > (note the typo in "faeture") semantically identic to
'<cpu/>'. No
> > error is re=
> > ported.
>
> This is an intentional implementation choice in libvirt. The XML
> documents
> that we're parsing are way to large and complex for us to justify
> adding
> code to report errors for every invalid attribute or child element
> name.
>
> We provide an RNG schema, however, which declares the valid set of
> attrs
> and thus by running RNG schema validation, a client can get a error
> report
> of invalid info.
>
> We enable this validation by default in "virsh edit" and a few other
> similar commands.
>
> There are certainly libvirt APIs though which accept XML and don't
> have
> a flag defined to enable RNG schema validation. I presume the CPU
> APIs
> are one such case. We should add support for validation to all such
> APIs accepting XML documents, and wire it up in virsh.
>
> Regards,
> Daniel
Hi Daniel, all,
thanks for the input!
My first attempt at the problem I described did in fact employ RNG
schema validation on the xml documents describing the CPU. The problem
though with that approach is that `virsh [hypervisor-]cpu-compare` can
consume quite different xml documents. The tests add more possible
document types, e.g. multicpu, which force the schema for a validation
performed in `virCPUDefParseXML` to basically import all other schemas
(which produced some naming conflicts that I was unable to solve) and,
secondly, perform a lot of unneccessary validation.
I created a schema that would perform only a check for "unknown"
elements and attributes in the cpu element and ignore everything else.
I attached this for your entertainment. There you can see what I meant
with "different xml documents".
IMO that would not be very useful. The premise of rejecting just unknown
elements itself as described by the bug is IMO flawed and not very
useful.
Fixing this bug makes only sense when we actually validate the whole
schema.
Copying / moving the node into a new document just for validation
seems
not like an elegant solution (if possible at all) and I am unsure about
potential performance impact.
IMO the CPU comparison APIs aren't on any hot path so wasting a few
cycles here especially only when enabled by an API flag (since
validation must be enabled only as an opt-in as we were accepting broken
XMLs before, if they had valid parts in them) seems to be better than
the proposed solution with ad-hoc code validator.
The best solution in my opinion would be to not perform XPath
evaluation in the parser, but instead use some stream parser and error
out if the parser encounteres an element with an unknown name.
libxml doesn't seem to support such mode. Inventing a new parser just
for this is a waste of time.
The next best solution in my opinion is the one you reviewed. I
agree
with all comments made that this is far from perfect, but at least it
would trigger visible test fails when / if new elements and attributes
are added without that list being expanded.
I disagree. IMO the next best solution after RNG validation is just not
fixing it at all. The bug itself doesn't warant inventing ad-hoc
solutions for just this special case.
I will further explore whether it is possible to do node-only
validation without creating an include-everything schema, but I would
be grateful for any hint or suggestion in that direction. We should
have at least some error reporting in the case I described initially.
I'd say don't bother with that.
1557
days inactive
1559
days old
8 comments
4 participants
participants (4)
-
Daniel P. Berrangé -
Ján Tomko -
Peter Krempa -
Tim Wiederhake