The current code doesn't properly handle errors when parsing a video
device's resolution.
This patch changes the parse function signature to return an error
when we're missing an 'x' or 'y' parameter or when the 'x' or 'y'
parameters are not positive integers. No error is returned when no
'resolution' element is found.
Previously we were returning a NULL structure for the case where 'x' or
'y' were missing, but were returning an under-specified structure for
the other error cases.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++--------------
1 file changed, 30 insertions(+), 14 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 269a6ec2c3..b3f32d0b63 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15338,45 +15338,57 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
return g_steal_pointer(&def);
}
-static virDomainVideoResolutionDefPtr
-virDomainVideoResolutionDefParseXML(xmlNodePtr node)
+static int
+virDomainVideoResolutionDefParseXML(xmlNodePtr node,
+ virDomainVideoResolutionDefPtr *res)
{
xmlNodePtr cur;
g_autofree virDomainVideoResolutionDefPtr def = NULL;
g_autofree char *x = NULL;
g_autofree char *y = NULL;
+ *res = NULL;
cur = node->children;
while (cur != NULL) {
- if (cur->type == XML_ELEMENT_NODE) {
- if (!x && !y &&
- virXMLNodeNameEqual(cur, "resolution")) {
- x = virXMLPropString(cur, "x");
- y = virXMLPropString(cur, "y");
- }
+ if ((cur->type == XML_ELEMENT_NODE) &&
+ virXMLNodeNameEqual(cur, "resolution")) {
+ x = virXMLPropString(cur, "x");
+ y = virXMLPropString(cur, "y");
+ break;
}
cur = cur->next;
}
+ /* resolution not specified */
+ if (cur == NULL)
+ return 0;
+
+ /* resolution specified, but x or y is missing. report error */
if (!x || !y)
- return NULL;
+ return -1;
def = g_new0(virDomainVideoResolutionDef, 1);
if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("cannot parse video x-resolution '%s'"), x);
- goto cleanup;
+ return -1;
}
if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("cannot parse video y-resolution '%s'"), y);
- goto cleanup;
+ return -1;
}
- cleanup:
- return g_steal_pointer(&def);
+ if (def->x == 0 || def->y == 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("resolution values must be greater than 0"));
+ return -1;
+ }
+
+ *res = g_steal_pointer(&def);
+ return 0;
}
static virDomainVideoDriverDefPtr
@@ -15458,7 +15470,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
}
def->accel = virDomainVideoAccelDefParseXML(cur);
- def->res = virDomainVideoResolutionDefParseXML(cur);
+ if (virDomainVideoResolutionDefParseXML(cur, &def->res) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("Cannot parse video resolution"));
+ goto error;
+ }
}
if (virXMLNodeNameEqual(cur, "driver")) {
if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0)
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
I pushed the first three patches. Comments below
On 10/23/19 1:46 PM, Jonathon Jongsma wrote:
> The current code doesn't properly handle errors when parsing a video
> device's resolution.
>
> This patch changes the parse function signature to return an error
> when we're missing an 'x' or 'y' parameter or when the 'x' or 'y'
> parameters are not positive integers. No error is returned when no
> 'resolution' element is found.
>
> Previously we were returning a NULL structure for the case where 'x' or
> 'y' were missing, but were returning an under-specified structure for
> the other error cases.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
> src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++--------------
> 1 file changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 269a6ec2c3..b3f32d0b63 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15338,45 +15338,57 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
> return g_steal_pointer(&def);
> }
>
> -static virDomainVideoResolutionDefPtr
> -virDomainVideoResolutionDefParseXML(xmlNodePtr node)
> +static int
> +virDomainVideoResolutionDefParseXML(xmlNodePtr node,
> + virDomainVideoResolutionDefPtr *res)
> {
> xmlNodePtr cur;
> g_autofree virDomainVideoResolutionDefPtr def = NULL;
> g_autofree char *x = NULL;
> g_autofree char *y = NULL;
>
> + *res = NULL;
> cur = node->children;
> while (cur != NULL) {
> - if (cur->type == XML_ELEMENT_NODE) {
> - if (!x && !y &&
> - virXMLNodeNameEqual(cur, "resolution")) {
> - x = virXMLPropString(cur, "x");
> - y = virXMLPropString(cur, "y");
> - }
> + if ((cur->type == XML_ELEMENT_NODE) &&
> + virXMLNodeNameEqual(cur, "resolution")) {
> + x = virXMLPropString(cur, "x");
> + y = virXMLPropString(cur, "y");
> + break;
> }
> cur = cur->next;
> }
>
This loop can be removed if you move the virXMLNodeNameEqual to the
parent function, like how it is handled for parent call of
virDomainVirtioOptionsParseXML
> + /* resolution not specified */
> + if (cur == NULL)
> + return 0;
> +
> + /* resolution specified, but x or y is missing. report error */
> if (!x || !y)
> - return NULL;
> + return -1;
>
> def = g_new0(virDomainVideoResolutionDef, 1);
>
> if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("cannot parse video x-resolution '%s'"), x);
> - goto cleanup;
> + return -1;
> }
>
> if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("cannot parse video y-resolution '%s'"), y);
> - goto cleanup;
> + return -1;
> }
>
> - cleanup:
> - return g_steal_pointer(&def);
> + if (def->x == 0 || def->y == 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("resolution values must be greater than 0"));
> + return -1;
> + }
> +
> + *res = g_steal_pointer(&def);
> + return 0;
> }
>
This patch is doing two things: converting to the more common error
handling pattern, but also adding this error check. Separating them will
help review.
It's borderline pedantic but this type of error should actually be in
the Validate callback machinery instead. Some drivers will fill in a
virDomainDef manually in code (like virtualbox). If they accidentally
set an x or y value of 0, Validate won't catch it as an error, but the
XML parser will catch it later. Generally the XML parser should only
throw errors about
> static virDomainVideoDriverDefPtr
> @@ -15458,7 +15470,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
> }
>
> def->accel = virDomainVideoAccelDefParseXML(cur);
> - def->res = virDomainVideoResolutionDefParseXML(cur);
> + if (virDomainVideoResolutionDefParseXML(cur, &def->res) < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + "%s", _("Cannot parse video resolution"));
> + goto error;
> + }
> }
Calling virReporError here will overwrite the error raised by
virDomainVideoResolutionDefParseXML. The error reporting machinery
doesn't have any smarts to merge errors or anything like that, it needs
to be done manually. In this case you can just drop the virReportError
entirely
I'll be quicker about reviewing v3.
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2019-11-13 at 13:54 -0500, Cole Robinson wrote:
> I pushed the first three patches. Comments below
>
> On 10/23/19 1:46 PM, Jonathon Jongsma wrote:
> > The current code doesn't properly handle errors when parsing a
> > video
> > device's resolution.
> >
> > This patch changes the parse function signature to return an error
> > when we're missing an 'x' or 'y' parameter or when the 'x' or 'y'
> > parameters are not positive integers. No error is returned when no
> > 'resolution' element is found.
> >
> > Previously we were returning a NULL structure for the case where
> > 'x' or
> > 'y' were missing, but were returning an under-specified structure
> > for
> > the other error cases.
> >
> > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> > ---
> > src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++--------
> > ------
> > 1 file changed, 30 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 269a6ec2c3..b3f32d0b63 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -15338,45 +15338,57 @@ virDomainVideoAccelDefParseXML(xmlNodePtr
> > node)
> > return g_steal_pointer(&def);
> > }
> >
> > -static virDomainVideoResolutionDefPtr
> > -virDomainVideoResolutionDefParseXML(xmlNodePtr node)
> > +static int
> > +virDomainVideoResolutionDefParseXML(xmlNodePtr node,
> > + virDomainVideoResolutionDefPtr
> > *res)
> > {
> > xmlNodePtr cur;
> > g_autofree virDomainVideoResolutionDefPtr def = NULL;
> > g_autofree char *x = NULL;
> > g_autofree char *y = NULL;
> >
> > + *res = NULL;
> > cur = node->children;
> > while (cur != NULL) {
> > - if (cur->type == XML_ELEMENT_NODE) {
> > - if (!x && !y &&
> > - virXMLNodeNameEqual(cur, "resolution")) {
> > - x = virXMLPropString(cur, "x");
> > - y = virXMLPropString(cur, "y");
> > - }
> > + if ((cur->type == XML_ELEMENT_NODE) &&
> > + virXMLNodeNameEqual(cur, "resolution")) {
> > + x = virXMLPropString(cur, "x");
> > + y = virXMLPropString(cur, "y");
> > + break;
> > }
> > cur = cur->next;
> > }
> >
>
> This loop can be removed if you move the virXMLNodeNameEqual to the
> parent function, like how it is handled for parent call of
> virDomainVirtioOptionsParseXML
>
> > + /* resolution not specified */
> > + if (cur == NULL)
> > + return 0;
> > +
> > + /* resolution specified, but x or y is missing. report error
> > */
> > if (!x || !y)
> > - return NULL;
> > + return -1;
> >
> > def = g_new0(virDomainVideoResolutionDef, 1);
> >
> > if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) {
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > _("cannot parse video x-resolution '%s'"),
> > x);
> > - goto cleanup;
> > + return -1;
> > }
> >
> > if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) {
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > _("cannot parse video y-resolution '%s'"),
> > y);
> > - goto cleanup;
> > + return -1;
> > }
> >
> > - cleanup:
> > - return g_steal_pointer(&def);
> > + if (def->x == 0 || def->y == 0) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("resolution values must be greater than
> > 0"));
> > + return -1;
> > + }
> > +
> > + *res = g_steal_pointer(&def);
> > + return 0;
> > }
> >
>
> This patch is doing two things: converting to the more common error
> handling pattern, but also adding this error check. Separating them
> will
> help review.
>
> It's borderline pedantic but this type of error should actually be in
> the Validate callback machinery instead. Some drivers will fill in a
> virDomainDef manually in code (like virtualbox). If they accidentally
> set an x or y value of 0, Validate won't catch it as an error, but
> the
> XML parser will catch it later. Generally the XML parser should only
> throw errors about
It seems that this sentence is unfinished?
>
> > static virDomainVideoDriverDefPtr
> > @@ -15458,7 +15470,11 @@
> > virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
> > }
> >
> > def->accel = virDomainVideoAccelDefParseXML(cur);
> > - def->res =
> > virDomainVideoResolutionDefParseXML(cur);
> > + if (virDomainVideoResolutionDefParseXML(cur, &def-
> > >res) < 0) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + "%s", _("Cannot parse video
> > resolution"));
> > + goto error;
> > + }
> > }
>
> Calling virReporError here will overwrite the error raised by
> virDomainVideoResolutionDefParseXML. The error reporting machinery
> doesn't have any smarts to merge errors or anything like that, it
> needs
> to be done manually. In this case you can just drop the
> virReportError
> entirely
>
> I'll be quicker about reviewing v3.
>
> Thanks,
> Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/13/19 5:41 PM, Jonathon Jongsma wrote:
> On Wed, 2019-11-13 at 13:54 -0500, Cole Robinson wrote:
>> I pushed the first three patches. Comments below
>>
>> On 10/23/19 1:46 PM, Jonathon Jongsma wrote:
>>> The current code doesn't properly handle errors when parsing a
>>> video
>>> device's resolution.
>>>
>>> This patch changes the parse function signature to return an error
>>> when we're missing an 'x' or 'y' parameter or when the 'x' or 'y'
>>> parameters are not positive integers. No error is returned when no
>>> 'resolution' element is found.
>>>
>>> Previously we were returning a NULL structure for the case where
>>> 'x' or
>>> 'y' were missing, but were returning an under-specified structure
>>> for
>>> the other error cases.
>>>
>>> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
>>> ---
>>> src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++--------
>>> ------
>>> 1 file changed, 30 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 269a6ec2c3..b3f32d0b63 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -15338,45 +15338,57 @@ virDomainVideoAccelDefParseXML(xmlNodePtr
>>> node)
>>> return g_steal_pointer(&def);
>>> }
>>>
>>> -static virDomainVideoResolutionDefPtr
>>> -virDomainVideoResolutionDefParseXML(xmlNodePtr node)
>>> +static int
>>> +virDomainVideoResolutionDefParseXML(xmlNodePtr node,
>>> + virDomainVideoResolutionDefPtr
>>> *res)
>>> {
>>> xmlNodePtr cur;
>>> g_autofree virDomainVideoResolutionDefPtr def = NULL;
>>> g_autofree char *x = NULL;
>>> g_autofree char *y = NULL;
>>>
>>> + *res = NULL;
>>> cur = node->children;
>>> while (cur != NULL) {
>>> - if (cur->type == XML_ELEMENT_NODE) {
>>> - if (!x && !y &&
>>> - virXMLNodeNameEqual(cur, "resolution")) {
>>> - x = virXMLPropString(cur, "x");
>>> - y = virXMLPropString(cur, "y");
>>> - }
>>> + if ((cur->type == XML_ELEMENT_NODE) &&
>>> + virXMLNodeNameEqual(cur, "resolution")) {
>>> + x = virXMLPropString(cur, "x");
>>> + y = virXMLPropString(cur, "y");
>>> + break;
>>> }
>>> cur = cur->next;
>>> }
>>>
>>
>> This loop can be removed if you move the virXMLNodeNameEqual to the
>> parent function, like how it is handled for parent call of
>> virDomainVirtioOptionsParseXML
>>
>>> + /* resolution not specified */
>>> + if (cur == NULL)
>>> + return 0;
>>> +
>>> + /* resolution specified, but x or y is missing. report error
>>> */
>>> if (!x || !y)
>>> - return NULL;
>>> + return -1;
>>>
>>> def = g_new0(virDomainVideoResolutionDef, 1);
>>>
>>> if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) {
>>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> _("cannot parse video x-resolution '%s'"),
>>> x);
>>> - goto cleanup;
>>> + return -1;
>>> }
>>>
>>> if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) {
>>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> _("cannot parse video y-resolution '%s'"),
>>> y);
>>> - goto cleanup;
>>> + return -1;
>>> }
>>>
>>> - cleanup:
>>> - return g_steal_pointer(&def);
>>> + if (def->x == 0 || def->y == 0) {
>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> + _("resolution values must be greater than
>>> 0"));
>>> + return -1;
>>> + }
>>> +
>>> + *res = g_steal_pointer(&def);
>>> + return 0;
>>> }
>>>
>>
>> This patch is doing two things: converting to the more common error
>> handling pattern, but also adding this error check. Separating them
>> will
>> help review.
>>
>> It's borderline pedantic but this type of error should actually be in
>> the Validate callback machinery instead. Some drivers will fill in a
>> virDomainDef manually in code (like virtualbox). If they accidentally
>> set an x or y value of 0, Validate won't catch it as an error, but
>> the
>> XML parser will catch it later. Generally the XML parser should only
>> throw errors about
>
> It seems that this sentence is unfinished?
>
I meant to delete that unformed thought because it was going to turn
into more than a sentence and would distract from this patch :)
Generally the XML parser should only raise errors that are specific to
the XML input. Anything that's validating a value after it has already
been parsed is better served in the Validate callbacks. The existing
code is not the best guide for this though, it's just the goal we should
shoot for IMO
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.