Re: [PATCH] vfprintf: Remove alloca from string formatting with conversion

[Florian Weimer <fweimer at redhat dot com>]

On 06/27/2017 10:32 PM, Adhemerval Zanella wrote:

> Wouldn't be better to add a wrapper on malloc/malloc-internal for overflow
> check? Something like
> 
> static inline bool
> check_add_overflow_int_t (int left, int right, int *result)
> {
> #if __GNUC__ >= 5
>   return __builtin_add_overflow (left, right, result);
> #else
>   if (INT_MAX - left <= right)
>     return true;
>   *result = left + right;
>   return false;
> #endif
> }
> 
> As we already do for multiplication and for unsigned (from my char_array)?

I'd prefer to wait until we can require GCC 5 as the baseline compiler
and then use the built-ins directly.  Less cognitive load for the
maintainers.

>> +  /* Use a small buffer to combine processing of multiple characters.
>> +     CONVERT_FROM_OTHER_STRING expects the buffer size in (wide)
>> +     characters, and buf_length counts that.  */
>> +#ifdef COMPILE_WPRINTF
>> +  enum { buf_length = 64 };
>> +  wchar_t buf[buf_length];
>> +#else
>> +  enum { buf_length = 256 };
>> +  char buf[buf_length];
>> +  _Static_assert (sizeof (buf) > MB_LEN_MAX,
>> +		  "buffer is large enough for a single multi-byte character");
>> +#endif
> 
> Why different 'buf_length' sizes if you using the required type for array
> creation?

I don't understand the question.  I want to avoid a large stack frame.
Regarding the different constants, see the comment above.

>> +  /* Add the initial padding if needed.  */
>> +  if (width > 0 && !left)
>> +    {
>> +      /* Make a first pass to find the output width, so that we can
>> +	 add the required padding.  */
>> +      memset (&mbstate, 0, sizeof (mbstate_t));
>> +      const OTHER_CHAR_T *src_copy = src;
>> +      size_t total_written;
>> +      if (prec < 0)
>> +	total_written = CONVERT_FROM_OTHER_STRING
>> +	  (NULL, &src_copy, 0, &mbstate);
>> +      else
>> +	{
>> +	  /* The source might not be null-terminated.  Enforce the
>> +	     limit manually, based on the output length.  */
>> +	  total_written = 0;
>> +	  size_t limit = prec;
>> +	  while (limit > 0 && src_copy != NULL)
> 
> I think this comparison using limit as size_t will always be valid,
> shouldn't 'limit' be a 'int'?

No, I think the subtraction …

>> +	    {
>> +	      size_t write_limit = buf_length;
>> +	      if (write_limit > limit)
>> +		write_limit = limit;
>> +	      size_t written = CONVERT_FROM_OTHER_STRING
>> +		(buf, &src_copy, write_limit, &mbstate);
>> +	      if (written == (size_t) -1)
>> +		return -1;
>> +	      if (written == 0)
>> +		break;
>> +	      total_written += written;
>> +	      limit -= written;

here can cause limit to reach zero.

Thanks,
Florian