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

[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 <[email protected]> wrote:
>
> On 24 Apr 2024, at 14:03, Yann Ylavic <[email protected]> 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.