Re: [xml] Coverity Code Checker Problem

[Daniel Veillard <veillard redhat com>]

On Tue, Jun 03, 2008 at 10:42:09AM -0700, Price.Derrick wrote:
> Hello,
>
>  
>
> We run the Coverity Prevent product on our code and since we use LibXML2
> there are a number of issues reported in LibXML2 as well.  I thought you
> would be interested in knowing about these items.  Here are a few of the
> issues reported by Coverity.  It includes the line number, the code, and
> the events and assumptions made by the checker.

  What version of libxml2 are you using ? Line numbers when I could find a 
match are off by huge values. Could you rechec with the latest version ?
Also if you're concerned by the security/quality of your product, i really
suggest you follow what's happening upstream, I think there have been 2 
CVE security announcement emitted and fixed since the version you seems to
be using.

> 1)       Coverity Check Type: UNUSED_VALUE - catalog.c
>
> 2728                       sgml = xmlCatalogSGMLResolve(catal, NULL,
> URI);
>
> At conditional (5): "sgml != 0" taking true path
>
> 2729                       if (sgml != NULL)
>
> Event returned_pointer: Pointer "sgml" returned from "xmlStrdup" is
> never used
>
> 2730                                              sgml =
> xmlStrdup(sgml);

  I guess it's in xmlACatalogResolveURI(), it should be
     ret = xmlStrdup(sgml);

> 2)       Coverity Check Type: UNUSED_VALUE - catalog.c
>
> 2430            if (ret != NULL)
> 2431                return(ret);
> At conditional (4): "sysID != 0" taking true path
> 2432            if (sysID != NULL)
> Event returned_pointer: Pointer "ret" returned from
> "xmlCatalogGetSGMLSystem" is never used
> 2433                ret = xmlCatalogGetSGMLSystem(catal->sgml, sysID);
> 2434            return(NULL);


  i guess it's in xmlCatalogSGMLResolve() , missing a
    if (ret != NULL)
            return(ret);

> 3)       Coverity Check Type: Time-of-check-time-of-use (TOCTOU) -
> catalog.c
>
> 898                 if (filename == NULL)
>
> 899                     return (NULL);
>
> 900             
>
> 901             #ifdef HAVE_STAT
>
> Event fs_check_call: Called "stat" to perform check on "filename"
>
> At conditional (1): "stat < 0" taking false path
>
> 902                 if (stat(filename, &info) < 0)
>
> 903                     return (NULL);
>
> 904             #endif
>
> 905             
>
> 906             #ifdef HAVE_STAT
>
> Event toctou: Called use function "open" on "filename" after a check
> function. This can cause a time-of-check, time-of-use race condition.
>
> 907                 if ((fd = open(filename, O_RDONLY)) < 0)

  yes looks like a race, but IMHO not a big deal, the fd result is checked

> 4)       Coverity Check Type: REVERSE_INULL - catalog.c.  REVERSE_INULL
> checker finds many instances of NULL checks after dereferences.
>
> 3054               cur = pathss;
>
> Event check_after_deref: Pointer "cur" dereferenced before NULL check
>
> 3055               while ((cur != NULL) && (*cur != 0)) {
>
> 3056                       while (xmlIsBlank_ch(*cur)) cur++;
>
> 3057                       if (*cur != 0) {
>
> 3058                           paths = cur;
>
> 3059                           while ((*cur != 0) && (*cur != ':') &&
> (!xmlIsBlank_ch(*cur)))
>
> 3060                                   cur++;
>
> 3061                           path = xmlStrndup((const xmlChar *)paths,
> cur - paths);
>
> 3062                           if (path != NULL) {
>
> 3063                                   xmlLoadCatalog((const char *)
> path);
>
> 3064                                   xmlFree(path);
>
> 3065                           }
>
> 3066                       }
> Event deref_ptr: Directly dereferenced pointer "cur"
>
> At conditional (1): "*cur == 58" taking false path
>
> 3067                       while (*cur == ':')
>
> 3068                           cur++;
>
>  

  I don't understand the problem and the line numbers are completely off...
i don't think we can have cur == NULL here.

>  
>
> 5)       Coverity Check Type: DEAD_CODE - catalog.c.
>
> Event const: After this line, the value of "base" is equal to 0
>
> Event assignment: Assigning "0" to "base"
>
> 1088               xmlChar *base = NULL;

  i couldn't fine any 'xmlChar *base = NULL;' in the current code base.
So no idea ...

  thanks,

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/