Re: [xml] Patch for cleaning up after free() calls - against 2.6.30

[Daniel Veillard <veillard redhat com>]

On Wed, Feb 13, 2008 at 01:58:15PM -0800, Brian Krahmer wrote:
> In doing some testing against a commercial code-checking tool, I found 
> that most (all?) of the free() calls in libxml2 were not being followed 
> by x = NULL.  This was causing crashes in a multi-threaded test program 
> on linux.  Please accept and apply the following patch at your earliest 
> convenience.

okay reviewed it. But your post is unclear, what call crashed, and how ?
There are things you cannot do in 2 parallel threads, so this may be a 
bug or an API use violation, but there is no indication in your mail.
What test program, where did it crash ?

I found that most of the affectations you suggest are actually not needed
they make no sense really, I don't fully understand how you came up with
that list, if it's the tool which provided it, well you probably should not
pay for that commercial code, really. If you manually generated it, well
a bit of analysis would have been better, basically only one change to a 
debug processing loop using only in xmllint --shell is changed after my
review. If some bug was fixed by your patch I can't make sense of it please
provide more details.
   Also note that in libxml2 code, free() is never used (nearly), we always
use xmlFree() which has the same semantic. So if your tool checked only
free(), basically it really makes a poor job of doing the static analysis.
But in general be it free or xmlFree or other freeing routine, systematically
making a NULL affectation after is just useless, it really depend on the
context and only a case by case check in context makes sense.

  In a nutshell, nearly all the changes suggested don't actually make sense
and if you had a crash, the problem remains, and need to be reviewed properly
instead,

  comments in-line below,

  thanks,

Daniel

> diff -ur libxml2-2.6.30.orig/debugXML.c libxml2-2.6.30/debugXML.c
> --- libxml2-2.6.30.orig/debugXML.c    2007-01-03 05:07:52.000000000 -0800
> +++ libxml2-2.6.30/debugXML.c    2008-02-13 12:56:44.000000000 -0800
> @@ -3244,6 +3244,7 @@
>                              "Unknown command %s\n", command);
>          }
>          free(cmdline);          /* not xmlFree here ! */
> +        cmdline = NULL;
>      }

 Not needed in practice. It's a while(1) loop, you can exit only
after aving read a new cmdline, but it's reasonable to add it just for
safety, that loop is fairly complex.

>  #ifdef LIBXML_XPATH_ENABLED
>      xmlXPathFreeContext(ctxt->pctxt);
> @@ -3254,8 +3255,10 @@
>      if (ctxt->filename != NULL)
>          xmlFree(ctxt->filename);
>      xmlFree(ctxt);
> -    if (cmdline != NULL)
> +    if (cmdline != NULL) {
>          free(cmdline);          /* not xmlFree here ! */
> +        cmdline = NULL;
> +    }
>  }

  cmdline is a local variable of the function and we are exiting the function
useless, the affectation would actually be optimized out by any decent compiler.

>  #endif /* LIBXML_XPATH_ENABLED */
> diff -ur libxml2-2.6.30.orig/example/gjobread.c 
> libxml2-2.6.30/example/gjobread.c
> --- libxml2-2.6.30.orig/example/gjobread.c    2007-01-03 
> 05:07:43.000000000 -0800
> +++ libxml2-2.6.30/example/gjobread.c    2008-02-13 13:01:58.000000000 -0800
> @@ -243,6 +243,7 @@
>      if ( cur == 0 ) {
>      xmlFreeDoc(doc);
>      free(ret);
> +   ret = NULL;
>      return ( NULL );
>      }

  Same thing, ret is a local variable of the function, we are calling return
 so obviously the affectation here would be a no-op.

>      if ((xmlStrcmp(cur->name, (const xmlChar *) "Jobs")) || (cur->ns != 
> ns)) {
> @@ -255,6 +256,7 @@
>  #endif /* LIBXML_OUTPUT_ENABLED */
>      xmlFreeDoc(doc);
>      free(ret);
> +   ret = NULL;
>      return(NULL);
>      }

  Same thing, no-op, useless.

> diff -ur libxml2-2.6.30.orig/threads.c libxml2-2.6.30/threads.c
> --- libxml2-2.6.30.orig/threads.c    2007-06-12 01:16:00.000000000 -0700
> +++ libxml2-2.6.30/threads.c    2008-02-13 13:00:46.000000000 -0800
> @@ -191,6 +191,7 @@
>  #elif defined HAVE_BEOS_THREADS
>      if ((tok->sem = create_sem(1, "xmlMutex")) < B_OK) {
>          free(tok);
> +      tok = NULL;
>          return NULL;
>      }

  Same thing, no-op, useless.

>      tok->tid = -1;
> @@ -219,6 +220,7 @@
>      delete_sem(tok->sem);
>  #endif
>      free(tok);
> +    tok = NULL;
>  }
>  
>  /**
> @@ -303,6 +305,7 @@
>  #elif defined HAVE_BEOS_THREADS
>      if ((tok->lock = xmlNewMutex()) == NULL) {
>          free(tok);
> +      tok = NULL;
>          return NULL;
>      }

  Same thing, no-op, useless.

>      tok->count = 0;
> @@ -333,6 +336,7 @@
>      xmlFreeMutex(tok->lock);
>  #endif
>      free(tok);
> +    tok = NULL;
>  }

  tok is an argument to the function we return, it's useless to change the
value on the stack

>  /**
> @@ -452,6 +456,7 @@
>       * allocated by this thread. */
>      if (global_init_lock != cs) {
>          free(cs);
> +       cs = NULL;
>      }
>      }

  Same thing local variable not reused any further on the function

> @@ -525,6 +530,7 @@
>      /* free any memory allocated in the thread's xmlLastError */
>      xmlResetError(&(gs->xmlLastError));
>      free(state);
> +    state = NULL;
>  }

  state is an argument to the function

>  /**
> @@ -568,6 +574,7 @@
>      CloseHandle(params->thread);
>      xmlFreeGlobalState(params->memory);
>      free(params);
> +    params = NULL;
>      _endthread();
>  }

  Same thing local variable not reused any further on the function

>  #else /* LIBXML_STATIC && !LIBXML_STATIC_FOR_DLL */
> @@ -846,6 +853,7 @@
>          p = p->next;
>          xmlFreeGlobalState(temp->memory);
>          free(temp);
> +      temp = NULL;
>      }

  temp is a local variable to the block we are using here.

>      cleanup_helpers_head = 0;
>      LeaveCriticalSection(&cleanup_helpers_cs);
> @@ -943,6 +951,7 @@
>                      p->next->prev = p->prev;
>          LeaveCriticalSection(&cleanup_helpers_cs);
>          free(p);
> +      p = NULL;
>          }

  p is a local variable to the block we are using here.

>      }
>      break;
> diff -ur libxml2-2.6.30.orig/xmlcatalog.c libxml2-2.6.30/xmlcatalog.c
> --- libxml2-2.6.30.orig/xmlcatalog.c    2007-01-03 05:07:52.000000000 -0800
> +++ libxml2-2.6.30/xmlcatalog.c    2008-02-13 12:57:37.000000000 -0800
> @@ -122,6 +122,7 @@
>      command[i] = 0;
>      if (i == 0) {
>          free(cmdline);
> +       cmdline = NULL;
>          continue;
>      }

  First thing that the while(1) loop does is reinitialize cmdline, useless

>      nbargs++;
> @@ -301,6 +302,7 @@
>          printf("\texit:  quit the shell\n");
>      }
>      free(cmdline); /* not xmlFree here ! */
> +   cmdline = NULL;
>      }
>  }

  Same thing

> diff -ur libxml2-2.6.30.orig/xmlIO.c libxml2-2.6.30/xmlIO.c
> --- libxml2-2.6.30.orig/xmlIO.c    2007-08-14 06:50:46.000000000 -0700
> +++ libxml2-2.6.30/xmlIO.c    2008-02-13 12:57:11.000000000 -0800
> @@ -1936,6 +1936,7 @@
>          }
>  
>          free( dump_name );
> +      dump_name = NULL;
>          }
>  #endif  /*  DEBUG_HTTP  */

  useless dump_name is a local variable to the block we are exiting

> diff -ur libxml2-2.6.30.orig/xmllint.c libxml2-2.6.30/xmllint.c
> --- libxml2-2.6.30.orig/xmllint.c    2007-04-17 05:28:16.000000000 -0700
> +++ libxml2-2.6.30/xmllint.c    2008-02-13 12:55:48.000000000 -0800
> @@ -2929,6 +2929,7 @@
>      assert(node->_private != NULL);
>      assert(*(long*)node->_private == (long) 0x81726354);
>      free(node->_private);
> +    node->_private = NULL;
>      nbregister--;
>  }

  That's not part of the library this is a callback used only when a node is
deallocated, this really should not change anything

> diff -ur libxml2-2.6.30.orig/xmlmemory.c libxml2-2.6.30/xmlmemory.c
> --- libxml2-2.6.30.orig/xmlmemory.c    2007-01-03 05:07:52.000000000 -0800
> +++ libxml2-2.6.30/xmlmemory.c    2008-02-13 12:55:20.000000000 -0800
> @@ -445,6 +445,7 @@
>      xmlMutexUnlock(xmlMemMutex);
>  
>      free(p);
> +    p = NULL;
>  
>      TEST_POINT

  p is a local variable and we are exiting the function. Moreover that code 
should only be activated if memory debugging has been configured in, which
is really not suggested in applications



-- 
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/