On Mon, Apr 22, 2024 at 2:46 PM <minf...@apache.org> 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.
