Re: svn commit: r1917266 - in /apr/apr/trunk: buffer/apr_buffer.c include/apr_buffer.h test/testbuffer.c

On Mon, Apr 22, 2024 at 2:46 PM <[email protected]> wrote:
>
> --- apr/apr/trunk/buffer/apr_buffer.c (original)
> +++ apr/apr/trunk/buffer/apr_buffer.c Mon Apr 22 12:46:37 2024
> @@ -28,7 +28,7 @@
>  #include "apr_strings.h"
>  #include "apr_private.h"
>
> -#define APR_BUFFER_MAX APR_SIZE_MAX/2
> +#define APR_BUFFER_MAX (APR_SIZE_MAX/2-1)

It seems that the ->size does include the terminating NUL byte so
APR_BUFFER_MAX could be APR_SIZE_MAX/2 no? Why -1 here?
Also maybe ->size should be ->len then?

>
> @@ -74,21 +76,25 @@ APR_DECLARE(apr_status_t) apr_buffer_str

Here in apr_buffer_str_set() we still have apr_ssize_t len, please
make it an apr_size_t for the reason explained in the other thread
(also the apr_buffer_mem_*() ones use apr_size_t already so it should
be consistent)?

>      if (!str) {
>          buf->d.str = NULL;
>          buf->size = 0;
> +        buf->zero_terminated = 0;
>      }
>      else if (len < 0) {
>          apr_size_t slen = strlen(str);
>          if (slen <= APR_BUFFER_MAX) {
>              buf->d.str = str;
> -            buf->size = -(apr_off_t)(slen + 1);
> +            buf->size = slen;
> +            buf->zero_terminated = 1;
>          }
>          else {
>              buf->d.str = NULL;
>              buf->size = 0;
> +            buf->zero_terminated = 0;

return APR_EINVAL?

>          }
>      }
>      else {
>          buf->d.str = str;
> -        buf->size = -(len + 1);
> +        buf->size = len;
> +        buf->zero_terminated = 1;

Shouldn't we check that str[len] == '\0' here? Though if it's not the
check might be out of bounds, but OTOH it's not a string buffer and
->zero_terminated is not true..

>      }
>
>      return APR_SUCCESS;

Overall I think it could be simplified as:

APR_DECLARE(apr_status_t) apr_buffer_str_set(apr_buffer_t *buf,
                                             char *str, apr_size_t len)
{
    if (len == APR_BUFFER_STRING) {
        len = str ? strlen(str) : 0;
    }
    else if (str ? str[len] != '\0' : len != 0) {
        return APR_EINVAL;
    }
    if (len > APR_BUFFER_MAX) {
        return APR_EINVAL; /* or APR_ENOSPC? */
    }

    buf->d.str = str;
    buf->size = len;
    buf->zero_terminated = 1;

    return APR_SUCCESS;
}


> @@ -100,25 +106,25 @@ APR_DECLARE(apr_buffer_t *) apr_buffer_s

Likewise apr_ssize_t => apr_size_t len for apr_buffer_str_make(),
APR_BUFFER_STRING and co should be unsigned (e.g. APR_SIZE_MAX) too.

>      apr_buffer_t *buf;
>      apr_int64_t size;
>      apr_size_t slen;
> +    unsigned int zero_terminated;
>
>      if (!str) {
>          str = NULL;
>          size = 0;
> +        zero_terminated = 0;
>      }
> -    if (APR_BUFFER_STRING == len && !str[0]) {
> -        size = len;
> -    }
> -    else if (len < 0) {
> +    if (APR_BUFFER_STRING == len) {
>          slen = strlen(str);
>          if (slen <= APR_BUFFER_MAX) {
> -            size = -(apr_off_t)(slen + 1);
> +            size = slen;
> +            zero_terminated = 1;
>          }
>          else {
>              return NULL;
>          }
>      }
>      else {
> -        size = -(len + 1);
> +        size = (apr_size_t)len;
>      }
>
>      buf = apr_palloc(pool, sizeof(apr_buffer_t));
> @@ -126,6 +132,7 @@ APR_DECLARE(apr_buffer_t *) apr_buffer_s
>      if (buf) {
>          buf->d.str = str;
>          buf->size = size;
> +        buf->zero_terminated = zero_terminated;
>      }
>
>      return buf;

Since there could be multiple error cases for apr_buffer_str_make()
too, I think we cannot return NULL so rather than a _make() we need a
_create() like:

APR_DECLARE(apr_status_t) apr_buffer_str_create(apr_pool_t *pool,
                                                apr_buffer_t **pbuf,
                                                char *str, apr_size_t len)
{
    *pbuf = apr_palloc(pool, sizeof(apr_buffer_t));

    return *pbuf ? apr_buffer_str_set(*pbuf, str, len) : APR_ENOMEM;
}

?

>
>  APR_DECLARE(apr_size_t) apr_buffer_allocated(const apr_buffer_t *buf)

apr_buffer_size() if ->size is changed to ->len ?

>  {
> -    if (buf->size < 0) {
> -        return (apr_size_t)((-buf->size));
> -    }
> -    else {
> -        return (apr_size_t)buf->size;
> -    }
> +    return buf->size + buf->zero_terminated;
>  }

>
>  APR_DECLARE(void *) apr_buffer_mem(const apr_buffer_t *buf, apr_size_t *size)
> @@ -227,11 +214,12 @@ APR_DECLARE(apr_buffer_t *) apr_buffer_a
>      for (i = 0; i < nelts; i++) {
>
>          /* absolute value is size of mem buffer including optional terminating zero */
> -        apr_size_t size = src->size < 0 ? -src->size : src->size;
> +        apr_size_t size = src->size + src->zero_terminated;
>
>          void *mem = alloc(ctx, size);

Check mem == NULL => APR_ENOMEM ?

>          memcpy(mem, src->d.mem, size);
>
> +        dst->zero_terminated = src->zero_terminated;
>          dst->size = src->size;
>          dst->d.mem = mem;

>
> @@ -256,22 +244,25 @@ APR_DECLARE(apr_buffer_t *) apr_buffer_c
>
>          dst->d.mem = NULL;
>          dst->size = 0;
> +        dst->zero_terminated = 0;
>
>      }
>      else if (!alloc) {
>
>          dst->d.mem = src->d.mem;
>          dst->size = src->size;
> -
> +        dst->zero_terminated = src->zero_terminated;
> +
>      }
>      else {
>
>          /* absolute value is size of mem buffer including optional terminating zero */
> -        apr_size_t size = src->size < 0 ? -src->size : src->size;
> +        apr_size_t size = src->size + src->zero_terminated;
>
>          void *mem = alloc(ctx, size);
>          memcpy(mem, src->d.mem, size);
>
> +        dst->zero_terminated = src->zero_terminated;
>          dst->size = src->size;
>          dst->d.mem = mem;

Maybe we shouldn't check/accept src == NULL (and let it crash), and
alloc == NULL (it's a simple memcpy functionally, the user can do it
already), so just do the deep copy (apr_buffer_copy() =>
apr_buffer_clone() ?)

>
> @@ -280,33 +271,7 @@ APR_DECLARE(apr_buffer_t *) apr_buffer_c
>      return dst;
>  }
>
> -
>  APR_DECLARE(int) apr_buffer_cmp(const apr_buffer_t *src,
> -                                const apr_buffer_t *dst)
> -{
> -    apr_size_t slen = apr_buffer_len(src);
> -    apr_size_t dlen = apr_buffer_len(dst);
> -
> -    if (slen != dlen) {
> -        return slen < dlen ? -1 : 1;
> -    }
> -    else if (src->d.mem == dst->d.mem) {
> -        /* identical data or both NULL */
> -        return 0;
> -    }
> -    else if (!src->d.mem) {
> -        return -1;
> -    }
> -    else if (!dst->d.mem) {
> -        return 1;
> -    }
> -    else {
> -        return memcmp(src->d.mem, dst->d.mem, slen);
> -    }
> -}
> -
> -
> -APR_DECLARE(int) apr_buffer_ncmp(const apr_buffer_t *src,
>                                   const apr_buffer_t *dst)
>  {
>      if (!src || !src->d.mem) {
> @@ -322,7 +287,17 @@ APR_DECLARE(int) apr_buffer_ncmp(const a
>              return 1;
>          }
>          else {
> -            return apr_buffer_cmp(src, dst);
> +
> +            apr_size_t slen = apr_buffer_len(src);
> +            apr_size_t dlen = apr_buffer_len(dst);
> +
> +            if (slen != dlen) {
> +                return slen < dlen ? -1 : 1;
> +            }
> +            else {
> +                return memcmp(src->d.mem, dst->d.mem, slen);
> +            }
> +
>          }
>      }
>  }

APR_DECLARE(int) apr_buffer_cmp(const apr_buffer_t *src,
                                const apr_buffer_t *dst)
{
    apr_size_t slen, dlen;

    if (!src) {
        return dst ? 1 : 0;
    }
    if (!dst) {
        return -1;
    }
    if (src->size != dst->size) {
        return src->size < dst->size ? -1 : 1;
    }

    return memcmp(src->d.mem, dst->d.mem, slen);
}

Though I still don't think that we should handle NULLs here (let it crash..).


> @@ -344,17 +319,17 @@ APR_DECLARE(char *) apr_buffer_pstrncat(
>              size += seplen;
>          }
>
> -        if (src->size < 0) {
> -            size += (apr_size_t)(-src->size) - 1;
> +        if (src->zero_terminated) {
> +            size += src->size;

Why not honor APR_BUFFER_BASE64 if it's a string buffer?

>          }
>          else {
>              if (APR_BUFFER_NONE == flags) {

s/APR_BUFFER_NONE/APR_BUFFER_PLAIN/ ?

> -                size += (apr_size_t)src->size;
> +                size += src->size;
>              }
>              else if (APR_BUFFER_BASE64 == flags) {
>                  apr_size_t b64len;
>
> -                if (APR_SUCCESS != apr_encode_base64(NULL, src->d.mem, (apr_size_t)src->size,
> +                if (APR_SUCCESS != apr_encode_base64(NULL, src->d.mem, src->size,
>                                                       APR_ENCODE_NONE, &b64len)) {
>                      return NULL;
>                  }
> @@ -379,19 +354,19 @@ APR_DECLARE(char *) apr_buffer_pstrncat(
>              strncpy(dst, sep, seplen);
>              dst += seplen;
>          }
> -
> -        if (src->size < 0) {
> -            strncpy(dst, src->d.str, (apr_size_t)((-src->size) - 1));
> -            dst += (-src->size) - 1;
> +
> +        if (src->zero_terminated) {
> +            strncpy(dst, src->d.str, src->size);
> +            dst += src->size;
>          }
>          else {
>              if (APR_BUFFER_NONE == flags) {
> -                memcpy(dst, src->d.mem, (apr_size_t)src->size);
> +                memcpy(dst, src->d.mem, src->size);
>              }
>              else if (APR_BUFFER_BASE64 == flags) {
>                  apr_size_t b64len;
>
> -                if (APR_SUCCESS != apr_encode_base64(dst, src->d.mem, (apr_size_t)src->size,
> +                if (APR_SUCCESS != apr_encode_base64(dst, src->d.mem, src->size,
>                                                       APR_ENCODE_NONE, &b64len)) {
>                      return NULL;
>                  }

>
> --- apr/apr/trunk/include/apr_buffer.h (original)
> +++ apr/apr/trunk/include/apr_buffer.h Mon Apr 22 12:46:37 2024
> @@ -73,13 +73,17 @@ typedef struct
>          void *mem;
>      } d;
>
> -    /** size of the data. If positive, the data is of fixed size. If
> -      * negative, the data is zero terminated and the absolute value
> -      * represents the data length including terminating zero.
> -      *
> -      * we use apr_off_t to make it simple to detect overflow.
> -      */
> -    apr_off_t size;
> +    /** is the data zero terminated */
> +    unsigned int zero_terminated:1;
> +
> +    /** size of the data, excluding terminating zero */
> +#if defined(SIZE_MAX) && SIZE_MAX == APR_UINT64_MAX
> +    apr_size_t size:63;
> +#elif defined(SIZE_MAX) && SIZE_MAX == APR_UINT32_MAX
> +    apr_size_t size:31;
> +#else
> +#error sizeof size_t is neither 64 nor 32 bit (SIZE_MAX not defined)
> +#endif

Is sizeof(apr_buffer_t) still 8 (32bit) or 16 (64bit) with
->zero_terminated and ->size in two bitmaps of different type/width
(unsigned int for the former, apr_size_t for the latter)?
It would be simpler to have both use apr_size_t IMHO.


Regards;
Yann.