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/
[EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
_______________________________________________
xml mailing list, project page http://xmlsoft.org/
[email protected]
http://mail.gnome.org/mailman/listinfo/xml
