You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by mi...@apache.org on 2024/04/22 12:46:37 UTC
svn commit: r1917266 - in /apr/apr/trunk: buffer/apr_buffer.c include/apr_buffer.h test/testbuffer.c
Author: minfrin
Date: Mon Apr 22 12:46:37 2024
New Revision: 1917266
URL: http://svn.apache.org/viewvc?rev=1917266&view=rev
Log:
apr_buffer: Redefine size to separate the unsigned size and the
flag to indicate whether a zero terminated string or not.
Modified:
apr/apr/trunk/buffer/apr_buffer.c
apr/apr/trunk/include/apr_buffer.h
apr/apr/trunk/test/testbuffer.c
Modified: apr/apr/trunk/buffer/apr_buffer.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/buffer/apr_buffer.c?rev=1917266&r1=1917265&r2=1917266&view=diff
==============================================================================
--- 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)
APR_DECLARE(apr_status_t) apr_buffer_mem_set(apr_buffer_t *buf,
void *mem, apr_size_t len)
@@ -40,6 +40,7 @@ APR_DECLARE(apr_status_t) apr_buffer_mem
buf->d.mem = mem;
buf->size = len;
+ buf->zero_terminated = 0;
return APR_SUCCESS;
}
@@ -58,6 +59,7 @@ APR_DECLARE(apr_buffer_t *) apr_buffer_m
if (buf) {
buf->d.mem = mem;
buf->size = len;
+ buf->zero_terminated = 0;
}
return buf;
@@ -74,21 +76,25 @@ APR_DECLARE(apr_status_t) apr_buffer_str
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;
}
}
else {
buf->d.str = str;
- buf->size = -(len + 1);
+ buf->size = len;
+ buf->zero_terminated = 1;
}
return APR_SUCCESS;
@@ -100,25 +106,25 @@ APR_DECLARE(apr_buffer_t *) apr_buffer_s
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;
@@ -138,22 +145,12 @@ APR_DECLARE(apr_buffer_t *) apr_buffer_n
APR_DECLARE(apr_size_t) apr_buffer_len(const apr_buffer_t *buf)
{
- if (buf->size < 0) {
- return (apr_size_t)((-buf->size) - 1);
- }
- else {
- return (apr_size_t)buf->size;
- }
+ return buf->size;
}
APR_DECLARE(apr_size_t) apr_buffer_allocated(const apr_buffer_t *buf)
{
- 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(int) apr_buffer_is_null(const apr_buffer_t *buf)
@@ -168,17 +165,12 @@ APR_DECLARE(int) apr_buffer_is_null(cons
APR_DECLARE(int) apr_buffer_is_str(const apr_buffer_t *buf)
{
- if (buf->size < 0) {
- return 1;
- }
- else {
- return 0;
- }
+ return buf->zero_terminated;
}
APR_DECLARE(char *) apr_buffer_str(const apr_buffer_t *buf)
{
- if (buf->size < 0) {
+ if (buf->zero_terminated) {
return buf->d.str;
}
else {
@@ -188,12 +180,7 @@ APR_DECLARE(char *) apr_buffer_str(const
APR_DECLARE(char *) apr_buffer_pstrdup(apr_pool_t *pool, const apr_buffer_t *buf)
{
- if (buf->size < 0) {
- return apr_pstrmemdup(pool, buf->d.str, (apr_size_t)(-(buf->size + 1)));
- }
- else {
- return apr_pstrmemdup(pool, buf->d.str, (apr_size_t)buf->size);
- }
+ return apr_pstrmemdup(pool, buf->d.str, buf->size);
}
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);
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;
@@ -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);
+ }
+
}
}
}
@@ -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;
}
else {
if (APR_BUFFER_NONE == flags) {
- 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;
}
Modified: apr/apr/trunk/include/apr_buffer.h
URL: http://svn.apache.org/viewvc/apr/apr/trunk/include/apr_buffer.h?rev=1917266&r1=1917265&r2=1917266&view=diff
==============================================================================
--- 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
} apr_buffer_t;
@@ -344,23 +348,6 @@ APR_DECLARE(apr_buffer_t *) apr_buffer_c
__attribute__((nonnull(1)));
/**
- * Compare two buffers for equality.
- *
- * Each buffer can be either a string or memory buffer.
- *
- * A string buffer and a memory buffer are considered equal if the length
- * excluding any trailing zero is equal, and the contents without the trailing
- * zero are the same.
- * @param dst The first buffer
- * @param src The second buffer
- * @return Positive, negative, or zero, depending on whether b1 is greater
- * than, less than, or equal to b2.
- */
-APR_DECLARE(int) apr_buffer_cmp(const apr_buffer_t *dst,
- const apr_buffer_t *src)
- __attribute__((nonnull(1,2)));
-
-/**
* Compare two possibly NULL buffers for equality.
*
* Each buffer can be either a string or memory buffer, or NULL.
@@ -372,11 +359,11 @@ APR_DECLARE(int) apr_buffer_cmp(const ap
* zero are the same.
* @param dst The first buffer
* @param src The second buffer
- * @return Positive, negative, or zero, depending on whether b1 is greater
- * than, less than, or equal to b2.
+ * @return Positive, negative, or zero, depending on whether src is greater
+ * than, less than, or equal to dst.
*/
-APR_DECLARE(int) apr_buffer_ncmp(const apr_buffer_t *dst,
- const apr_buffer_t *src);
+APR_DECLARE(int) apr_buffer_cmp(const apr_buffer_t *dst,
+ const apr_buffer_t *src);
/**
* Concatenate multiple buffers and return a string.
Modified: apr/apr/trunk/test/testbuffer.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/test/testbuffer.c?rev=1917266&r1=1917265&r2=1917266&view=diff
==============================================================================
--- apr/apr/trunk/test/testbuffer.c (original)
+++ apr/apr/trunk/test/testbuffer.c Mon Apr 22 12:46:37 2024
@@ -179,29 +179,29 @@ static void test_compare_buffers(abts_ca
small = NULL;
large = NULL;
ABTS_ASSERT(tc, "NULL equals NULL",
- !apr_buffer_ncmp(small, large));
+ !apr_buffer_cmp(small, large));
small = apr_buffer_null_make(pool);
ABTS_ASSERT(tc, "null buffer equals NULL",
- !apr_buffer_ncmp(small, large));
+ !apr_buffer_cmp(small, large));
large = apr_buffer_null_make(pool);
ABTS_ASSERT(tc, "null buffer equals null buffer",
- !apr_buffer_ncmp(small, large));
+ !apr_buffer_cmp(small, large));
small = NULL;
ABTS_ASSERT(tc, "NULL equals null buffer",
- !apr_buffer_ncmp(small, large));
+ !apr_buffer_cmp(small, large));
small = apr_buffer_str_make(pool, same, APR_BUFFER_STRING);
large = apr_buffer_str_make(pool, same, APR_BUFFER_STRING);
ABTS_ASSERT(tc, "pointer equals same pointer",
- !apr_buffer_ncmp(small, large));
+ !apr_buffer_cmp(small, large));
small = apr_buffer_str_make(pool, "same", APR_BUFFER_STRING);
large = apr_buffer_str_make(pool, "same", APR_BUFFER_STRING);
ABTS_ASSERT(tc, "'same' equals 'same'",
- !apr_buffer_ncmp(small, large));
+ !apr_buffer_cmp(small, large));
small = apr_buffer_str_make(pool, "short", APR_BUFFER_STRING);
large = apr_buffer_str_make(pool, "l o n g", APR_BUFFER_STRING);
Re: svn commit: r1917266 - in /apr/apr/trunk: buffer/apr_buffer.c include/apr_buffer.h test/testbuffer.c
Posted by Graham Leggett via dev <de...@apr.apache.org>.
On 22 Apr 2024, at 14:13, Ruediger Pluem <rp...@apache.org> wrote:
>> @@ -322,7 +287,17 @@ APR_DECLARE(int) apr_buffer_ncmp(const a
>> return 1;
>> }
>
> There was a proposal from Yann to simplify the above block to
>
> if (!src) {
> return dst ? 1 : 0;
> }
> if (!dst) {
> return -1;
> }
apr_buffer_ncmp has been removed. It was originally a null supporting comparison
function alongside a non null supporting comparison function, but now that the
buffer could contain NULL, it became overkill.
>> @@ -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;
>> }
>
> Can't we remove the above condition at all and do a
>
> memcpy(dst, src->d.mem, src->size + src->zero_terminated);
>
> below in the if (APR_BUFFER_NONE == flags) block?
Alas no, the encoding is only applied if the buffer contains binary data.
I did replace the strncpy, as the terminating zero is written at the end.
http://svn.apache.org/viewvc?rev=1917268&view=rev
>
>> 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;
>> }
>>
Regards,
Graham
--
Re: svn commit: r1917266 - in /apr/apr/trunk: buffer/apr_buffer.c include/apr_buffer.h test/testbuffer.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 4/22/24 2:46 PM, minfrin@apache.org wrote:
> Author: minfrin
> Date: Mon Apr 22 12:46:37 2024
> New Revision: 1917266
>
> URL: http://svn.apache.org/viewvc?rev=1917266&view=rev
> Log:
> apr_buffer: Redefine size to separate the unsigned size and the
> flag to indicate whether a zero terminated string or not.
>
> Modified:
> apr/apr/trunk/buffer/apr_buffer.c
> apr/apr/trunk/include/apr_buffer.h
> apr/apr/trunk/test/testbuffer.c
>
> Modified: apr/apr/trunk/buffer/apr_buffer.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/buffer/apr_buffer.c?rev=1917266&r1=1917265&r2=1917266&view=diff
> ==============================================================================
> --- apr/apr/trunk/buffer/apr_buffer.c (original)
> +++ apr/apr/trunk/buffer/apr_buffer.c Mon Apr 22 12:46:37 2024
> @@ -322,7 +287,17 @@ APR_DECLARE(int) apr_buffer_ncmp(const a
> return 1;
> }
There was a proposal from Yann to simplify the above block to
if (!src) {
return dst ? 1 : 0;
}
if (!dst) {
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);
> + }
> +
> }
> }
> }
}
> @@ -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;
> }
Can't we remove the above condition at all and do a
memcpy(dst, src->d.mem, src->size + src->zero_terminated);
below in the if (APR_BUFFER_NONE == flags) block?
> 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;
> }
>
Regards
Rüdiger
Re: svn commit: r1917266 - in /apr/apr/trunk: buffer/apr_buffer.c include/apr_buffer.h test/testbuffer.c
Posted by Yann Ylavic <yl...@gmail.com>.
[Graham please use plain text emails or at least fix your MUA so that
one does not need to re-quote/format your messages when replying in
plain text]
On Wed, Apr 24, 2024 at 7:51 PM Graham Leggett <mi...@sharp.fm> wrote:
>
> On 24 Apr 2024, at 14:03, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> 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)?
>
> The apr_ssize_t is intended in this case.
>
> As per the docs, an apr_ssize_t of -1 means compute the length:
>
> https://github.com/apache/apr/blob/trunk/include/apr_buffer.h#L46
>
> /**
> * When passing a string to apr_buffer_str_make, this value can be
> * passed to indicate a string with unknown length, and have apr_buffer_str_make
> * compute the length automatically.
> */
> #define APR_BUFFER_STRING (-1)
I've got that, but it's still a valid value to pass in a size_t, which
with sign extension will become (apr_size_t)-1, i.e. APR_SIZE_MAX.
If len is a size_t it's still perfectly defined and works to pass it -1, though:
#define APR_BUFFER_STRING ((apr_size_t)-1) /* or APR_SIZE_MAX */
would make it an unsigned value and is probably cleaner.
>
> https://github.com/apache/apr/blob/trunk/include/apr_buffer.h#L125
>
> * @param len The length of the string without terminating zero, or
> * APR_BUFFER_STRING to have the length calculated.
>
> The definition of ssize_t is "unsigned length or -1", so we're using it correctly.
The definition is not really "unsigned length or -1", it's _at least_
greater or equal to -1, but it has to be a signed type to be able to
represent -1 (in practice it's the same as ptrdiff_t, no real
restriction/trap when below -1).
But anyway, the point is that you cannot pass strlen() or any size_t
to a ssize_t without first checking that it's representable in a
ssize_t, so if len is defined as ssize_t the onus is on the user to
verify that the passed in value (possibly and likely an
unsigned/size_t which is what users use for sizes) fits in a ssize_t
*before* calling apr_buffer_str_set/make(). And then the checks in
apr_buffer_str_set/make() for len > APR_BUFFER_MAX are just useless,
the compiler might just have assumed that since it's undefined to pass
an unrepresentable value then len > APR_BUFFER_MAX just cannot happen
and it simply removes/optimizes out the test..
>
> The purpose of the apr_buffer is to make a stark and impossible to mix up distinction between a string and a memory area, apr_buffer_src() and apr_buffer_mem() definitely shouldn't be the same.
>>>
>>> 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?
>
> A NULL buffer is valid and intended.
But here it becomes NULL because len > APR_BUFFER_MAX, not because the
user passed NULL, so it's an error?
>>
>> 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;
>> }
>
> This breaks when src is NULL.
What breaks? If src (actually str) is NULL then ->d.str is NULL but
it's what you want AFAICT (plus the above also enforces that len is
zero in this case).
Or do you mean ->zero_termined is broken? All right, so how about:
buf->zero_terminated = (str != NULL);
?
>>
>> 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.
>
> We already use -1 to mean "compute the length" in various parts of APR, such as apr_hash. We should keep the behaviour the same as the rest of APR.
>
> If "compute the length" was APR_SIZE_MAX, we couldn't distinguish between "compute the length" and "too big".
I don't object to -1, I object to the signed type. APR_BUFFER_STRING =
-1/APR_SIZE_MAX is just a special value just like APR_HASH_KEY_STRING.
I which we didn't use apr_ssize_t for the apr_hash or apr_encode APIs
but that's where we are, let's not introduce a new one?
>>>
>>> APR_DECLARE(apr_size_t) apr_buffer_allocated(const apr_buffer_t *buf)
>>
>> apr_buffer_size() if ->size is changed to ->len ?
>
> apr_buffer_allocated() means physical size, apr_buffer_len() means logical length. One you care about when you're interested in the logical data, the other you care about when you're copying the physical data, for example when you're writing it to shared memory or some other system with special requirements.
All right, my (non-english native) thinking is that "physical size"
can just be called "size" (of the string/mem) and that "logical
length" just "length" (of the string/mem), thus that the ->size member
of the buffer is actually a length (not including the trailing NUL for
string buffers) and should be called ->len. But it's quite nitpicking
admittedly.
>>
>> 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() ?)
>
> We need the ability to store a NULL in a buffer, that's a valid value.
I understand that you want the ability for buf->d.str/mem to be NULL,
what I mean is that buf should not be NULL.
If it is, no defensive programming, just let it crash. For me src ==
NULL does not mean I want dst->d.mem = NULL.
>>
>> 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..).
>
> NULL support is intended.
Same response as above, it should be allowed for str/mem but not for
src/dst IMHO.
>>>
>>> @@ -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?
>
> Base64 is something you do to binary data, not strings
It's possibly what you do, but I could want to base64 a string too.
> - type safety is the point behind the api.
Yes! So let's move to apr_size_t len :)
>
> The LDAP api returns data as either a string, or a binary blob. When you're trying to log buffers, you want the string to stay a string, and the binary blob to become a string safely.
>
> When you're tasked with creating an LDAP filter, you want your strings to stay strings, and your binary blobs to be base64 encoded.
>
> The plan is to support the whole range of encodings, because we can.
I don't think the apr_buffer API should be bound to LDAP, though I'm
not opposed to it being helpful for LDAP too with the
apr_buffer_pstrncat() flags. But if users ask for APR_BUFFER_BASE64 we
should not ignore them because the buffer is a string.
>>>
>>> + /** 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.
>
> I went looking for the formal definition of this, and all I could find is that bits are bits.
>
> In theory there should be no need for a 1 bit wide field and a 31 bit wide field to have the same type, both will pack to a total of 32 bits.
It seems that gcc and probably other compilers do indeed pack
bit-fields regardless of the adjacent types (per [1]), but from
reading here and there it seems to be mostly compiler/implementation
defined, at least not formally/strictly standard defined.
So I would go with a single type for all the bits, an apr_size_t since
the whole is meant to fit in a single one (and I don't see what
unsigned int buys us here).
[1] https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Bit-Field-Packing.html
Regards;
Yann.
Re: svn commit: r1917266 - in /apr/apr/trunk: buffer/apr_buffer.c include/apr_buffer.h test/testbuffer.c
Posted by Graham Leggett via dev <de...@apr.apache.org>.
On 24 Apr 2024, at 14:03, Yann Ylavic <yl...@gmail.com> wrote:
>> -#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?
The size doesn't include the terminating NUL byte, that went away when the internal size became unsigned.
Thinking about this more, the size restriction on size only applies inside apr_buffer, no such restriction exists on the full sized apr_size_t passed to apr_palloc().
The -1 isn't necessary in this case.
>> @@ -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)?
The apr_ssize_t is intended in this case.
As per the docs, an apr_ssize_t of -1 means compute the length:
https://github.com/apache/apr/blob/trunk/include/apr_buffer.h#L46
/**
* When passing a string to apr_buffer_str_make, this value can be
* passed to indicate a string with unknown length, and have apr_buffer_str_make
* compute the length automatically.
*/
#define APR_BUFFER_STRING (-1)
https://github.com/apache/apr/blob/trunk/include/apr_buffer.h#L125
* @param len The length of the string without terminating zero, or
* APR_BUFFER_STRING to have the length calculated.
The definition of ssize_t is "unsigned length or -1", so we're using it correctly.
The purpose of the apr_buffer is to make a stark and impossible to mix up distinction between a string and a memory area, apr_buffer_src() and apr_buffer_mem() definitely shouldn't be the same.
>
>> 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?
A NULL buffer is valid and intended.
https://github.com/apache/apr/blob/trunk/include/apr_buffer.h#L162
/**
* Does the buffer contain a NULL buffer.
*
* If the internal buffer is NULL, 1 is returned, otherwise 0.
*
* @param buf The buffer.
* @return Returns 1 if buffer is null, otherwise 0.
*/
APR_DECLARE(int) apr_buffer_is_null(const apr_buffer_t *buf)
__attribute__((nonnull(1)));
>> }
>> }
>> 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..
Yes, we should.
It's a great chance to catch bogus strings.
>> }
>>
>> 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;
> }
This breaks when src is NULL.
>> @@ -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.
We already use -1 to mean "compute the length" in various parts of APR, such as apr_hash. We should keep the behaviour the same as the rest of APR.
If "compute the length" was APR_SIZE_MAX, we couldn't distinguish between "compute the length" and "too big".
>> 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;
> }
>
> ?
Hmmm... we have both APR_ENOMEM and a possible APR_EINVAL if a non-zero-terminated-string is passed in, so this makes sense.
>>
>> APR_DECLARE(apr_size_t) apr_buffer_allocated(const apr_buffer_t *buf)
>
> apr_buffer_size() if ->size is changed to ->len ?
apr_buffer_allocated() means physical size, apr_buffer_len() means logical length. One you care about when you're interested in the logical data, the other you care about when you're copying the physical data, for example when you're writing it to shared memory or some other system with special requirements.
>> {
>> - 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 ?
Definitely yes.
>> 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() ?)
We need the ability to store a NULL in a buffer, that's a valid value.
>> @@ -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..).
NULL support is intended.
>> @@ -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?
Base64 is something you do to binary data, not strings - type safety is the point behind the api.
The LDAP api returns data as either a string, or a binary blob. When you're trying to log buffers, you want the string to stay a string, and the binary blob to become a string safely.
When you're tasked with creating an LDAP filter, you want your strings to stay strings, and your binary blobs to be base64 encoded.
The plan is to support the whole range of encodings, because we can.
>
>> }
>> else {
>> if (APR_BUFFER_NONE == flags) {
>
> s/APR_BUFFER_NONE/APR_BUFFER_PLAIN/ ?
Hmmm... this does make sense.
>> - 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.
I went looking for the formal definition of this, and all I could find is that bits are bits.
In theory there should be no need for a 1 bit wide field and a 31 bit wide field to have the same type, both will pack to a total of 32 bits.
Regards,
Graham
--
Re: svn commit: r1917266 - in /apr/apr/trunk: buffer/apr_buffer.c include/apr_buffer.h test/testbuffer.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Apr 24, 2024 at 3:03 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> 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..).
I meant:
APR_DECLARE(int) apr_buffer_cmp(const apr_buffer_t *src,
const apr_buffer_t *dst)
{
apr_size_t slen, dlen;
if (!src.d.mem) {
return dst.d.mem ? 1 : 0;
}
if (!dst.d.mem) {
return -1;
}
if (src->size != dst->size) {
return src->size < dst->size ? -1 : 1;
}
return memcmp(src->d.mem, dst->d.mem, slen);
}
Re: svn commit: r1917266 - in /apr/apr/trunk: buffer/apr_buffer.c include/apr_buffer.h test/testbuffer.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Apr 22, 2024 at 2:46 PM <mi...@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.