Re: [PATCH] Fix leaks and check for newer libraries in Kerberos related helpers

2013-08-27 Thread Alex Rousskov 
On 08/26/2013 03:38 PM, Markus Moeller wrote:
> Here is the update patch.

> -if (pp && pp->next) {
> -xfree(pp->next);
> -pp->next = NULL;
> -}
> +safe_free(pp->next);

This change will cause crashes on single-entry lists where pp is NULL.


> -if (p == gdsp) {
> -xfree(gdsp);
> -gdsp = NULL;
> -}
> +safe_free(gdsp);
>  p = gdsp;

This change will prevent cleanup of all entries except the very first
one because the outer p-loop will terminate with p=gdsp making p NULL.

Similar problems in another, similar part of the patch.


You can completely remove an if-statement when using safe_free, but only
where the if guard is the same as the one provided by safe_free:

  if (p) {
  xfree(p);
  p = NULL;
  }

can be replaced with

  safe_free(p);


However,

  if (something && p) {
  xfree(p);
  p = NULL;
  }

can only be replaced with

  if (something)
  safe_free(p);


Similarly,

  if (something) {
  xfree(p);
  p = NULL;
  }

can only be replaced with

  if (something)
  safe_free(p);


HTH,

Alex.

Re: [PATCH] Fix leaks and check for newer libraries in Kerberos related helpers

2013-08-27 Thread Markus Moeller 

Thank you for pointing this out. I was too radical :-(.
Markus

"Alex Rousskov"  wrote in message 
news:521d0b73.3090...@measurement-factory.com...

On 08/26/2013 03:38 PM, Markus Moeller wrote:

Here is the update patch.



-if (pp && pp->next) {
-xfree(pp->next);
-pp->next = NULL;
-}
+safe_free(pp->next);


This change will cause crashes on single-entry lists where pp is NULL.



-if (p == gdsp) {
-xfree(gdsp);
-gdsp = NULL;
-}
+safe_free(gdsp);
 p = gdsp;


This change will prevent cleanup of all entries except the very first
one because the outer p-loop will terminate with p=gdsp making p NULL.

Similar problems in another, similar part of the patch.


You can completely remove an if-statement when using safe_free, but only
where the if guard is the same as the one provided by safe_free:

 if (p) {
 xfree(p);
 p = NULL;
 }

can be replaced with

 safe_free(p);


However,

 if (something && p) {
 xfree(p);
 p = NULL;
 }

can only be replaced with

 if (something)
 safe_free(p);


Similarly,

 if (something) {
 xfree(p);
 p = NULL;
 }

can only be replaced with

 if (something)
 safe_free(p);


HTH,

Alex.