Re: [PATCH] vfprintf: validate nargs and maybe allocate from heap

[Paul Eggert <eggert at cs dot ucla dot edu>]

On 02/05/2012 10:25 PM, Kees Cook wrote:
> +    args_value = (union printf_arg *) (args_size + nargs);

This isn't right, since the alignment of
args_value[0] is stricter than that of args_size[0],
which means that args_size + nargs might not be aligned
property for args_value.  The code might work on hosts like
the x86 that don't insist on strict alignment, but it won't
work in general.

You can work around this problem by splitting up the
result of alloca in the order (args_value, args_type, args_size)
rather than the order (args_type, args_size, args_value).
This should be guaranteed to work, as args_value's alignment
must be at least that of args_type and args_size.

>  all_done:
> +  if (__builtin_expect (args_malloced, 0))
> +    free (args_type);

Rather than have a boolean 'args_malloced', it's simpler to have
a pointer that is null if malloc hasn't been called, and is
the value that malloc returned otherwise.

One more thing: there shouldn't be any need to cast the pointers involved
in the inserted code.

In summary, you can replace this:

    int args_malloced = 0;
    ...
    if (__libc_use_alloca (nargs * bytes_per_arg))
      args_type = alloca (nargs * bytes_per_arg);
    else if ((args_type = (int *) malloc (nargs * bytes_per_arg)) == NULL)
      {
	done = -1;
	goto all_done;
      }
    else
      args_malloced = 1;
    ...
    args_size = args_type + nargs;
    args_value = (union printf_arg *) (args_size + nargs);
    ...
  all_done:
    if (__builtin_expect (args_malloced, 0))
      free (args_type);

with something like this:  

    void *args_malloced = NULL;
    ...
    if (__libc_use_alloca (nargs * bytes_per_arg))
      args_value = alloca (nargs * bytes_per_arg);
    else
      {
	args_value = args_malloced = malloc (nargs * bytes_per_arg);
	if (! args_value)
	  {
	    done = -1;
	    goto malloc_failed;
	  }
      }
    ...
    args_type = &args_value[nargs].pa_int;
    args_size = &args_type[nargs];
    ...
  all_done:
    if (__builtin_expect (args_malloced != NULL, 0))
      free (args_malloced);