Re: [xml] Regarding Malloc Failure Handling--Patch for Parser.c

[Daniel Veillard <veillard redhat com>]

On Sat, Mar 29, 2008 at 09:32:46AM +0530, Ashwin wrote:
> >  Yes memory allocation failures should be handled, at least be reported,
> > and of course not leaking or crashing. However it's really hard to
> perfectly
> >handle out of memory failures.
> >  We really need to fix leaks or crashes and report the error though the
> > API, anything beyond that is nice but less critical. If the application >
> chose to ignore the memory error, nothing can be done to save it anyway.
> Hi,
> I am attaching a patch for some memory leak fixes in malloc failure
> scenarios, also adding checks where malloc return is not checked for
> NULL(parser.c). Please let me know if the changes are ok.

  it's right in general, except on need to use xmlFree() and not free()
directly in the library

>   	    goto mem_error;
>   	defaults->nbAttrs = 0;
>   	defaults->maxAttrs = 4;
> ! 	if(xmlHashUpdateEntry2(ctxt->attsDefault, name, prefix, defaults, NULL) < 0){
> !          free(defaults);

  xmlFree()

> !          goto mem_error;
> !          }
>       } else if (defaults->nbAttrs >= defaults->maxAttrs) {
>           xmlDefAttrsPtr temp;
>   
> *************** xmlParserHandlePEReference(xmlParserCtxt
> *** 2289,2297 ****
>   #define growBuffer(buffer) {						\
>       xmlChar *tmp;							\
>       buffer##_size *= 2;							\
> !     tmp = (xmlChar *)							\
> !     		xmlRealloc(buffer, buffer##_size * sizeof(xmlChar));	\
> !     if (tmp == NULL) goto mem_error;					\
>       buffer = tmp;							\
>   }
>   
> --- 2296,2306 ----
>   #define growBuffer(buffer) {						\
>       xmlChar *tmp;							\
>       buffer##_size *= 2;							\
> !     tmp = (xmlChar *) xmlRealloc(buffer, buffer##_size * sizeof(xmlChar)); \
> !     if (tmp == NULL) {						\
> ! 	xmlFree(buffer);						\
> ! 	goto mem_error;							\
> !     }									\
>       buffer = tmp;							\
>   }

  That looks wrong to me, I would expect the code to free up the buffer
in the mem_error code path, no do that in the macro, i did that in the 2
functions.

> *************** xmlStringLenDecodeEntities(xmlParserCtxt
> *** 2321,2326 ****
> --- 2330,2336 ----
>       int buffer_size = 0;
>   
>       xmlChar *current = NULL;
> +     xmlChar *rep = NULL;
[...]
>   
>   mem_error:
> +     if (rep != NULL)
> +     {
> +         xmlFree(rep);
> +     }
>       xmlErrMemory(ctxt, NULL);
>       return(NULL);
>   }

  hum, right the rep/mem_error goto was a problem, good catch

> *************** xmlSplitQName(xmlParserCtxtPtr ctxt, con
> *** 2634,2640 ****
>   		tmp = (xmlChar *) xmlRealloc(buffer,
>   						max * sizeof(xmlChar));
>   		if (tmp == NULL) {
> ! 		    xmlFree(tmp);
>   		    xmlErrMemory(ctxt, NULL);
>   		    return(NULL);
>   		}
> --- 2650,2656 ----
>   		tmp = (xmlChar *) xmlRealloc(buffer,
>   						max * sizeof(xmlChar));
>   		if (tmp == NULL) {
> ! 		    xmlFree(buffer);
>   		    xmlErrMemory(ctxt, NULL);
>   		    return(NULL);
>   		}

  right !

> *************** static xmlChar *
> *** 3197,3202 ****
> --- 3213,3219 ----
>   xmlParseAttValueComplex(xmlParserCtxtPtr ctxt, int *attlen, int normalize) {
>       xmlChar limit = 0;
>       xmlChar *buf = NULL;
> +     xmlChar *rep = NULL;

  Same problem, yes

> *************** xmlParseElementChildrenContentDecl (xmlP
> *** 5471,5476 ****
> --- 5504,5510 ----
>   		return(NULL);
>   	    }
>   	    last = xmlNewDocElementContent(ctxt->myDoc, elem, XML_ELEMENT_CONTENT_ELEMENT);
> +         if (last == NULL) return NULL;
>   	    if (RAW == '?') {
>   		last->ocur = XML_ELEMENT_CONTENT_OPT;
>   		NEXT;

  i think you may leak ret here, so I changed that accordingly

> *************** xmlParseVersionNum(xmlParserCtxtPtr ctxt
> *** 8980,8985 ****
> --- 9014,9020 ----
>   	    size *= 2;
>   	    tmp = (xmlChar *) xmlRealloc(buf, size * sizeof(xmlChar));
>   	    if (tmp == NULL) {
> + 	    xmlFree(buf);
>   		xmlErrMemory(ctxt, NULL);
>   		return(NULL);
>   	    }

  Right,

  so I applied manually all changes, modifying a few things, might be worth
re-checking from SVN head (rev 3720),

  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/