[PATCH] Allocation buffers for NSS result construction

[Florian Weimer]

On 06/16/2017 04:42 PM, Adhemerval Zanella wrote:

> I would prefer to split the patch in two, one for the alloc_buffer adition
> and another one for its use in NSS result construction.

Okay, continuing with the allocation buffers only for now

>> +/* struct alloc_buffer objects refer to a region of bytes in memory of a
>> +   fixed size.  The functions below can be used to allocate single
>> +   objects and arrays from this memory region, or write to its end.
>> +   On allocation failure (or if an attempt to write beyond the end of
>> +   the buffer with one of the copy functions), the buffer enters a
>> +   failed state.
>> +
>> +   struct alloc_buffer objects can be copied.  The backing buffer will
>> +   be shared, but the current write position will be independent.
>> +
>> +   Conceptually, the memory region consists of a current write pointer
>> +   and a limit, beyond which the write pointer cannot move.  */
> 
> A new line maybe?

What do you mean?
>> +/* Create a new allocation buffer.  The byte range from START to START
>> +   + SIZE - 1 must be valid, and the allocation buffer allocates
>> +   objects from that range.  If START is NULL (so that SIZE must be
>> +   0), the buffer is marked as failed immediately.  */
>> +static inline struct alloc_buffer
>> +alloc_buffer_create (void *start, size_t size)
>> +{
>> +  return (struct alloc_buffer)
>> +    {
>> +      .__alloc_buffer_current = (uintptr_t) start,
>> +      .__alloc_buffer_end = (uintptr_t) start + size
>> +    };
>> +}
> 
> Should we add an overflow test for sanity tests?

I don't think it's worthwhile to do that because the memory range is
already invalid at this point.

>> +/* Internal function.  See alloc_buffer_allocate below.  */
>> +struct alloc_buffer __libc_alloc_buffer_allocate (size_t size);
>> +libc_hidden_proto (__libc_alloc_buffer_allocate)
> 
> I am getting this while trying to build malloc/tst-alloc_buffer.o:
> 
> ../include/alloc_buffer.h:125:1: error: return type defaults to ‘int’ [-Werror=implicit-int]
>  libc_hidden_proto (__libc_alloc_buffer_allocate)
> 
> This is due 7c3018f9 (Suppress internal declarations for most of the
> testsuite) which suppress libc_hidden_proto for testsuite.  I think we
> need either to make them empty macros in this case (move their definition
> outside the _ISOMAC) or move the libc_hidden_proto to another internal
> header.

Yes, fixed in the attached patch.

>> +/* Deallocate the buffer and mark it as failed.  The buffer must be in
>> +   its initial state; if data has been added to it, an invocation of
>> +   alloc_buffer_free results in undefined behavior.  This means that
>> +   callers need to make a copy of the buffer if they need to free it
>> +   later.  Deallocating a failed buffer is allowed; it has no
>> +   effect.  */
>> +static inline void
>> +alloc_buffer_free (struct alloc_buffer *buf)
>> +{
>> +  _Static_assert (__ALLOC_BUFFER_INVALID_POINTER == 0,
>> +		  "free can be called on __ALLOC_BUFFER_INVALID_POINTER");
>> +  free ((void *) buf->__alloc_buffer_current);
>> +  alloc_buffer_mark_failed (buf);
>> +}
> 
> No need to cast to void *.

buf->__alloc_buffer_current is uinptr_t, to help with the alignment
operations.

>> +/* Internal function.  Obtain a pointer to an object.  */
>> +static inline void *
>> +__alloc_buffer_alloc (struct alloc_buffer *buf, size_t size, size_t align)
>> +{
>> +  if (size == 1 && align == 1)
>> +    return alloc_buffer_alloc_bytes (buf, size);
>> +
>> +  size_t current = buf->__alloc_buffer_current;
>> +  size_t aligned = roundup (current, align);
>> +  size_t new_current = aligned + size;
>> +  if (aligned >= current        /* No overflow in align step.  */
>> +      && new_current >= size    /* No overflow in size computation.  */
>> +      && new_current <= buf->__alloc_buffer_end) /* Room in buffer.  */
>> +    {
>> +      buf->__alloc_buffer_current = new_current;
>> +      return (void *) aligned;
>> +    }
>> +  else
>> +    {
>> +      alloc_buffer_mark_failed (buf);
>> +      return NULL;
>> +    }
>> +}
> 
> Maybe use/add the check_add_wrapv_size_t from my char_array patch (also
> for the other occurences)?

Hmm.  I think the comparison idiom is sufficiently common to use it this
way.

>> +
>> +/* Obtain a TYPE * pointer to an object in BUF of TYPE.  Consume these
>> +   bytes from the buffer.  Return NULL and mark the buffer as failed
>> +   if if there is not enough room in the buffer, or if the buffer has
>> +   failed before.  */
>> +#define alloc_buffer_alloc(buf, type)				\
>> +  ((type *) __alloc_buffer_alloc				\
>> +   (buf, __alloc_buffer_assert_size (sizeof (type)),		\
>> +    __alloc_buffer_assert_align (__alignof__ (type))))
> 
> I would prefer to use a static inline function to type check, but we
> can like with it (same for other occurencies).

It would need C++ because TYPE is a type parameter.  Everything that can
be a function already is.

>> +void *
>> +__libc_alloc_buffer_alloc_array (struct alloc_buffer *buf, size_t element_size,
>> +                                 size_t align, size_t count)
>> +{
>> +  size_t current = buf->__alloc_buffer_current;
>> +  /* The caller asserts that align is a power of two.  */
>> +  size_t aligned = (current + align - 1) & ~(align - 1);
> 
> Maybe use ALIGN_UP?

Good idea.

>> +  struct alloc_buffer buf = alloc_buffer_create (dst, dstsiz);
>> +  if (__ns_name_ntop_buffer (&buf, src) == NULL)
>> +    {
>> +      __set_errno (EMSGSIZE);
>> +      return -1;
>> +    }
>> +  return alloc_buffer_next (&buf, void) - (const void *) dst;
> 
> I am not sure how safe is convert a ptrdiff_t to a int at this point.  Does
> it worth add a check for it?

It's a bit ugly, yes.  I should probably check for INT_MAX overflow, too.

In the attached patch, I changed the signature for alloc_buffer_allocate
and removed alloc_buffer_free because the latter encouraged incorrect
use.  Now, alloc_buffer_allocate produces a copy of the initial buffer
pointer which can be used directly with free.

Thanks,
Florian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: alloc_buffer.patch
Type: text/x-patch
Size: 45630 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20170621/7d14bcac/attachment.bin>