Re: [patch] Cleanup: don't call free(NULL) unnecessarily

["Carlos O'Donell" <carlos at redhat dot com>]

On 03/05/2013 02:18 PM, Paul Pluzhnikov wrote:
> Greetings,
> 
> This patch eliminates calls to free(NULL) from within vfprintf.
> 
> This is desirable because e.g. fprintf(stderr, ...) is often called from
> signal handlers, and while technically illegal, this tends to work.
> 
> Glibc free(NULL) also works in signal context, but end user could supply
> his own malloc/free, which may not.

I agree that this is a sufficiently useful thing to do that
a couple of instructions for a function that is not performance
critical will make no difference.
 
> The same check was already done on e.g. line 1648.

This was added by Ulrich in 2002, with the rest of the free (NULL)'s
being added in 2007. It doesn't look like there was any particular
reason for not checking for NULL, so I agree that your patch is good
given that it makes vfprintf consistent (and fixes a potential crash).
 
> I also added missing check for malloc failure.

Good catch.

> Tested on Linux/x86_64 -- no new failures.
> 
> --
> Paul Pluzhnikov
> 
> 2013-03-05  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	* stdio-common/vfprintf.c (vfprintf): Check malloc return; don't
>           call free(NULL).

Incorrectly formatted, please clean this up before committing.

It should be:

{tab}* stdio-common/vfprintf.c (vfprintf): Check malloc return; don't
{tab}call free (NULL).

It looks like your second line has all space, no tab, and aligns after
the *.
 
> diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
> index 89126d2..7042090 100644
> --- a/stdio-common/vfprintf.c
> +++ b/stdio-common/vfprintf.c
> @@ -1691,7 +1691,8 @@ do_positional:
>      /* Just a counter.  */
>      size_t cnt;
>  
> -    free (workstart);
> +    if (__builtin_expect (workstart != NULL, 0))
> +      free (workstart);

OK.

>      workstart = NULL;
>  
>      if (grouping == (const char *) -1)
> @@ -1944,6 +1945,11 @@ do_positional:
>  	      {
>  		workstart = (CHAR_T *) malloc ((MAX (prec, width) + 32)
>  					       * sizeof (CHAR_T));
> +		if (workstart == NULL)
> +		  {
> +		    done = -1;
> +		    goto all_done;
> +		  }


OK. Good catch.

>  		workend = workstart + (MAX (prec, width) + 32);
>  	      }
>  	  }
> @@ -2021,7 +2027,8 @@ do_positional:
>  	    break;
>  	  }
>  
> -	free (workstart);
> +	if (__builtin_expect (workstart != NULL, 0))
> +	  free (workstart);

OK.

>  	workstart = NULL;
>  
>  	/* Write the following constant string.  */
> @@ -2032,8 +2039,10 @@ do_positional:
>    }
>  
>  all_done:
> -  free (args_malloced);
> -  free (workstart);
> +  if (__builtin_expect (args_malloced != NULL, 0))
> +    free (args_malloced);
> +  if (__builtin_expect (workstart != NULL, 0))
> +    free (workstart);

OK.

>    /* Unlock the stream.  */
>    _IO_funlockfile (s);
>    _IO_cleanup_region_end (0);

OK to checkin with your ChangeLog fixes.

Cheers,
Carlos.