Awhile back I noticed that calls to xmlNodeGetContent() from libvirt
code were inconsistent in their handling of the returned
pointer. Sometimes we would assume the return was always non-NULL
(dereferencing with wild abandon without concern for the
consequences), sometimes we would interpret NULL as "", and sometimes
as OOM. I sent mail about this to the list last week, wondering (and
doubting) if we could assume that a NULL return would always mean OOM:

https://www.redhat.com/archives/libvir-list/2020-July/msg00333.html

After looking at the libxml code, danpb's determination was that a
NULL return from xmlNodeGetContent *might* mean OOM, but it might also
mean some odd XML that we weren't expecting, so we can't just always
exit on a NULL return. He also agreed with (and expanded on) my
suspicion that there really is no reliable way to tell the reason for
a NULL return from xmlNodeGetContent, and suggested that possibly we
could just examing the xmlNode directly to learn the content instead
of calling xmlNodeGetContent().

This series is a followup to that discussion. The first 4 patches
clean up the code with the result that:

1) a libvirt wrapper function is always called instead of calling
xmlNodeGetContent() directly.

2) that wrapper function logs an error when it gets back NULL from
xmlNodeGetContent().

3) All the callers check for a NULL return, and do a "silent error
return" themselves when there is a NULL.

In the final patch, I try out Dan's idea of looking at the xmlNode
object directly to get the content. It turns out it's not as
straightforward as you would think from just looking at the layout of
the object - a full explanation is in patch 5. I'm mainly sending that
patch as an "FYI" (one step back from an "RFC"), since really all it
changes is that libvirt will exit on OOM, and log (different, but not
any more informative) error messages when the problem isn't
OOM. Unless someone has a strong opinion otherwise, I think just the
first 4 patches should be applied, and users can just "deal" with the
ambiguity in case of error.


Laine Stump (5):
  conf: refactor virDomainBlkioDeviceParseXML to reduce calls to
    xmlNodeGetContent
  util: replace all calls to xmlNodeGetContent with
    virXMLNodeContentString
  util: log an error if virXMLNodeContentString will return NULL
  treat all NULL returns from virXMLNodeContentString() as an error
  util: open code virXMLNodeContentString to access the node object
    directly

 src/conf/domain_conf.c      | 194 ++++++++++++++++++++----------------
 src/conf/network_conf.c     |   7 +-
 src/conf/node_device_conf.c |   6 +-
 src/util/virxml.c           |  53 +++++++++-
 4 files changed, 169 insertions(+), 91 deletions(-)

-- 
2.25.4

On 7/20/20 10:48 PM, Laine Stump wrote:
> Awhile back I noticed that calls to xmlNodeGetContent() from libvirt
> code were inconsistent in their handling of the returned
> pointer. Sometimes we would assume the return was always non-NULL
> (dereferencing with wild abandon without concern for the
> consequences), sometimes we would interpret NULL as "", and sometimes
> as OOM. I sent mail about this to the list last week, wondering (and
> doubting) if we could assume that a NULL return would always mean OOM:
> 
> https://www.redhat.com/archives/libvir-list/2020-July/msg00333.html
> 
> After looking at the libxml code, danpb's determination was that a
> NULL return from xmlNodeGetContent *might* mean OOM, but it might also
> mean some odd XML that we weren't expecting, so we can't just always
> exit on a NULL return. He also agreed with (and expanded on) my
> suspicion that there really is no reliable way to tell the reason for
> a NULL return from xmlNodeGetContent, and suggested that possibly we
> could just examing the xmlNode directly to learn the content instead
> of calling xmlNodeGetContent().
> 
> This series is a followup to that discussion. The first 4 patches
> clean up the code with the result that:
> 
> 1) a libvirt wrapper function is always called instead of calling
> xmlNodeGetContent() directly.
> 
> 2) that wrapper function logs an error when it gets back NULL from
> xmlNodeGetContent().
> 
> 3) All the callers check for a NULL return, and do a "silent error
> return" themselves when there is a NULL.
> 
> In the final patch, I try out Dan's idea of looking at the xmlNode
> object directly to get the content. It turns out it's not as
> straightforward as you would think from just looking at the layout of
> the object - a full explanation is in patch 5. I'm mainly sending that
> patch as an "FYI" (one step back from an "RFC"), since really all it
> changes is that libvirt will exit on OOM, and log (different, but not
> any more informative) error messages when the problem isn't
> OOM. Unless someone has a strong opinion otherwise, I think just the
> first 4 patches should be applied, and users can just "deal" with the
> ambiguity in case of error.
> 
> 
> Laine Stump (5):
>    conf: refactor virDomainBlkioDeviceParseXML to reduce calls to
>      xmlNodeGetContent
>    util: replace all calls to xmlNodeGetContent with
>      virXMLNodeContentString
>    util: log an error if virXMLNodeContentString will return NULL
>    treat all NULL returns from virXMLNodeContentString() as an error
>    util: open code virXMLNodeContentString to access the node object
>      directly
> 
>   src/conf/domain_conf.c      | 194 ++++++++++++++++++++----------------
>   src/conf/network_conf.c     |   7 +-
>   src/conf/node_device_conf.c |   6 +-
>   src/util/virxml.c           |  53 +++++++++-
>   4 files changed, 169 insertions(+), 91 deletions(-)
> 

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal