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/