This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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



On 28/06/2017 15:13, Florian Weimer wrote:
> 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.

This would take at least one or two more releases and I do not see why
add and follow an idea already in place for malloc.

> 
>>> +  /* 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.

If the buffer must be computed as wide characters, why not defined it
using wchar_t regardless:

  wchar_t wbuf[64];
  const size_t buf_length = sizeof (wbuf) / sizeof (CHAR_T); 

?

> 
>>> +  /* 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.

Ack.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]