[OPEN-ILS-DEV] Dubious code in mod_xmlbuilder.c

[Scott McKellar]

In mod_xmlbuilder.c, xmlBuilderStartElement() contains the following
fragment of code (condensed here for brevity):

    char* href = strdup(xmlSaxAttr( atts, "href" ));
    if(href) {

        ...snip... 
    }

    if(!node) {
        apacheError("Unable to parse xinclude: %s", href );
        free(href);
        return;
    }
    free(href);

My first thought was: why are we checking the return code of 
strdup() to see if it's NULL?  If strdup() ever returns NULL we 
are almost certainly hosed, and cannot save ourselves simply by
skipping over a short stretch of code.

In fact just after skipping that stretch we may find ourselves
passing the NULL to apacheError(), which probably invokes undefined
behavior.

Upon looking further I realized that xmlSaxAttr() may return NULL,
which we then try to strdup, with unhappy results.  My system
segfaults when I try to strdup a NULL.

My guess is that the test for a NULL from strdup() was intended
conceptually to test for a NULL return from xmlSaxAttr(), but it
just wasn't written right.  However I don't understand the intent
of the code well enough to try to fix it.

Scott McKellar
http://home.swbell.net/mck9/ct/