Re: svn commit: r1917082 - /apr/apr/trunk/buffer/apr_buffer.c
On 18 Apr 2024, at 17:07, Yann Ylavic wrote:
> If so maybe:
>
> typedef struct
> {
>union {
>char *str;
>void *mem;
>} d;
> #if APR_SIZEOF_VOIDP == 8
> # define APR_BUFFER_SIZE_WIDTH 63
> #else
> # define APR_BUFFER_SIZE_WIDTH 31
> #endif
> apr_size_t size:APR_BUFFER_SIZE_WIDTH,
> zero_terminated:1;
> } apr_buffer_t;
>
> One could still use buf->size directly.
This is way simpler, thank you - I have made it so.
http://svn.apache.org/viewvc?view=revision=1917266
Regards,
Graham
--
Re: svn commit: r1917082 - /apr/apr/trunk/buffer/apr_buffer.c
On Thu, Apr 18, 2024 at 3:58 PM Graham Leggett wrote:
>
> On 18 Apr 2024, at 13:15, Yann Ylavic wrote:
>
>> But let me plead again for a much simpler ->size of type apr_size_t,
>> checked against APR_BUFFER_MAX=APR_SIZE_MAX/2 wherever an apr_buffer_t
>> is initialized, using the high bit of ->size for string vs plain
>> buffer, and then getting rid of off_t/ssize_t plus all the fancy
>> signed arithmetics in the apr_buffer code (we don't care about the
>> sizeof(off_t) or anything like that anymore)..
>
> I looked at it, and it was more confusing.
There is also another completely obvious alternative which is:
typedef struct
{
union {
char *str;
void *mem;
} d;
apr_size_t size;
unsigned int flags;
} apr_buffer_t;
which also makes ->size directly usable without a helper, but you want
the type to be to words only?
If so maybe:
typedef struct
{
union {
char *str;
void *mem;
} d;
#if APR_SIZEOF_VOIDP == 8
# define APR_BUFFER_SIZE_WIDTH 63
#else
# define APR_BUFFER_SIZE_WIDTH 31
#endif
apr_size_t size:APR_BUFFER_SIZE_WIDTH,
zero_terminated:1;
} apr_buffer_t;
One could still use buf->size directly.
Regards;
Yann.
Re: svn commit: r1917082 - /apr/apr/trunk/buffer/apr_buffer.c
On 18 Apr 2024, at 15:57, Yann Ylavic wrote:
>> could be:
>> 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;
>>}
>>if (len > APR_BUFFER_MAX) {
>>return APR_EINVAL;
>>}
>>
>>if (!str) {
>>buf->d.str = NULL;
>>buf->size = 0;
>>}
>>else {
>>buf->d.str = str;
>>buf->size = len;
>>}
>>buf->size |= ~APR_BUFFER_MAX;
>
> The above conditional is not even needed, just:
> buf->d.str = str;
> buf->size = len | ~APR_BUFFER_MAX;
> works too.
Hmmm…
In the current implementation apr_buffer_alloc() is the simple case (absolute
value of buf->size) and apr_buffer_len() is the complicated case.
Swapping them round as you’ve suggested makes a lot of sense, as we len far
more often than we alloc.
We still have the case that alloc is one larger than len, so the real limit is
SIZE_MAX/2-1. We can probably detect this with a simple mask, let me ponder.
All of this is a private implementation detail, your point this needs to be
emphasized is spot on.
The only thing public about apr_buffer_t is sizeof, nothing else.
Regards,
Graham
—
Re: svn commit: r1917082 - /apr/apr/trunk/buffer/apr_buffer.c
On Thu, Apr 18, 2024 at 4:41 PM Yann Ylavic wrote:
>
> could be:
> 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;
> }
> if (len > APR_BUFFER_MAX) {
> return APR_EINVAL;
> }
>
> if (!str) {
> buf->d.str = NULL;
> buf->size = 0;
> }
> else {
> buf->d.str = str;
> buf->size = len;
> }
> buf->size |= ~APR_BUFFER_MAX;
The above conditional is not even needed, just:
buf->d.str = str;
buf->size = len | ~APR_BUFFER_MAX;
works too.
>
> return APR_SUCCESS;
> }
Re: svn commit: r1917082 - /apr/apr/trunk/buffer/apr_buffer.c
On Thu, Apr 18, 2024 at 3:58 PM Graham Leggett wrote:
>
> On 18 Apr 2024, at 13:15, Yann Ylavic wrote:
>
>> Indeed at least APR_BUFFER_MAX and buf->size of should be of the same
>> signedness.
>
>
> APR_BUFFER_MAX represents the size limit visible outside the API. This is
> always positive.
I meant the same signedness of the type, not the value itself.
If APR_BUFFER_MAX is APR_SIZE_MAX/2 it's a size_t (thus unsigned),
while buf->size is an apr_off_t (thus signed), not the same types.
>
> buf->size is an internal implementation detail. This is invisible.
>
> The only reason we can see inside apr_buffer_t is because we want to allocate
> them on the stack, and make arrays of them, so code outside needs to know the
> size.
Fair enough, though there probably should be big warning a the struct
apr_buffer_t definition to not use ->size directly.
>
>> But let me plead again for a much simpler ->size of type apr_size_t,
>> checked against APR_BUFFER_MAX=APR_SIZE_MAX/2 wherever an apr_buffer_t
>> is initialized, using the high bit of ->size for string vs plain
>> buffer, and then getting rid of off_t/ssize_t plus all the fancy
>> signed arithmetics in the apr_buffer code (we don't care about the
>> sizeof(off_t) or anything like that anymore)..
>
>
> I looked at it, and it was more confusing.
>
> Right now, positive is a memory buffer, negative is a string. I don't know
> why we would need to make it more complicated than that.
I think we can hardly make more complicated than:
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;
}
}
...
With an apr_size_t ->size it could be:
APR_DECLARE(apr_size_t) apr_buffer_len(const apr_buffer_t *buf)
{
return buf->size & APR_BUFFER_MAX;
}
which I find much more simpler (and efficient) personally.
Another exemple:
APR_DECLARE(apr_status_t) apr_buffer_str_set(apr_buffer_t *buf,
char *str, apr_ssize_t len)
{
if (len > APR_BUFFER_MAX) {
return APR_EINVAL;
}
if (!str) {
buf->d.str = NULL;
buf->size = 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);
}
else {
buf->d.str = NULL;
buf->size = 0;
}
}
else {
buf->d.str = str;
buf->size = -(len + 1);
}
return APR_SUCCESS;
}
could be:
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;
}
if (len > APR_BUFFER_MAX) {
return APR_EINVAL;
}
if (!str) {
buf->d.str = NULL;
buf->size = 0;
}
else {
buf->d.str = str;
buf->size = len;
}
buf->size |= ~APR_BUFFER_MAX;
return APR_SUCCESS;
}
Should be the same kind of simplification everywhere I suppose.
>
>> Currently apr_buffer_str_make(mystring, strlen(mystring)) is UB, the
>> API is just broken IMHO.
>
> Can you clarify why? What, specifically, is broken, so I can fix it?
Passing/converting an unsigned value to a signed one where the
unsigned value cannot be represented in the signed one is undefined
behaviour in C.
So with:
APR_DECLARE(apr_status_t) apr_buffer_str_set(apr_buffer_t *buf,
char *str, apr_ssize_t len);
one cannot simply do (w/o UB):
apr_size_t len = strlen(mystr);
apr_buffer_str_set(buf, mystr, len);
without first checking that len <= APR_BUFFER_MAX, which is not really
user friendly.
Whereas with the proposed:
APR_DECLARE(apr_status_t) apr_buffer_str_set(apr_buffer_t *buf,
char *str, apr_size_t len);
the will return EINVAL if len > APR_BUFFER_MAX without the user having
to do the check to avoid UB.
Regards;
Yann.
Re: svn commit: r1917082 - /apr/apr/trunk/buffer/apr_buffer.c
On 18 Apr 2024, at 13:15, Yann Ylavic wrote: > Indeed at least APR_BUFFER_MAX and buf->size of should be of the same > signedness. APR_BUFFER_MAX represents the size limit visible outside the API. This is always positive. buf->size is an internal implementation detail. This is invisible. The only reason we can see inside apr_buffer_t is because we want to allocate them on the stack, and make arrays of them, so code outside needs to know the size. > But let me plead again for a much simpler ->size of type apr_size_t, > checked against APR_BUFFER_MAX=APR_SIZE_MAX/2 wherever an apr_buffer_t > is initialized, using the high bit of ->size for string vs plain > buffer, and then getting rid of off_t/ssize_t plus all the fancy > signed arithmetics in the apr_buffer code (we don't care about the > sizeof(off_t) or anything like that anymore).. I looked at it, and it was more confusing. Right now, positive is a memory buffer, negative is a string. I don't know why we would need to make it more complicated than that. > Currently apr_buffer_str_make(mystring, strlen(mystring)) is UB, the > API is just broken IMHO. Can you clarify why? What, specifically, is broken, so I can fix it? Regards, Graham --
Re: svn commit: r1917082 - /apr/apr/trunk/buffer/apr_buffer.c
On 18 Apr 2024, at 12:14, Ruediger Pluem wrote: >> +#define APR_BUFFER_MAX APR_SIZE_MAX/2 > > Why no longer APR_OFF_MAX? As was pointed out, apr_off_t is always 64 bits would still break on Windows. Inside the API, we need a very big, signed, value, and apr_off_t is perfect. Outside the API, we need apr_size_t, halved. Regards, Graham --
Re: svn commit: r1917082 - /apr/apr/trunk/buffer/apr_buffer.c
On Thu, Apr 18, 2024 at 1:16 PM Ruediger Pluem wrote: > > On 4/18/24 12:37 AM, minf...@apache.org wrote: > > Author: minfrin > > Date: Wed Apr 17 22:37:07 2024 > > New Revision: 1917082 > > > > URL: http://svn.apache.org/viewvc?rev=1917082=rev > > Log: > > apr_buffer: Add explicit casts on all potentially narrowing conversions > > to apr_size_t. Define the maximum buffer size as APR_SIZE_MAX/2. > > > > Modified: > > apr/apr/trunk/buffer/apr_buffer.c > > > > Modified: apr/apr/trunk/buffer/apr_buffer.c > > URL: > > http://svn.apache.org/viewvc/apr/apr/trunk/buffer/apr_buffer.c?rev=1917082=1917081=1917082=diff > > == > > --- apr/apr/trunk/buffer/apr_buffer.c (original) > > +++ apr/apr/trunk/buffer/apr_buffer.c Wed Apr 17 22:37:07 2024 > > @@ -28,12 +28,13 @@ > > #include "apr_strings.h" > > #include "apr_private.h" > > > > +#define APR_BUFFER_MAX APR_SIZE_MAX/2 > > Why no longer APR_OFF_MAX? Indeed at least APR_BUFFER_MAX and buf->size of should be of the same signedness. But let me plead again for a much simpler ->size of type apr_size_t, checked against APR_BUFFER_MAX=APR_SIZE_MAX/2 wherever an apr_buffer_t is initialized, using the high bit of ->size for string vs plain buffer, and then getting rid of off_t/ssize_t plus all the fancy signed arithmetics in the apr_buffer code (we don't care about the sizeof(off_t) or anything like that anymore).. Currently apr_buffer_str_make(mystring, strlen(mystring)) is UB, the API is just broken IMHO. Regards; Yann.
Re: svn commit: r1917082 - /apr/apr/trunk/buffer/apr_buffer.c
On 4/18/24 12:37 AM, minf...@apache.org wrote: > Author: minfrin > Date: Wed Apr 17 22:37:07 2024 > New Revision: 1917082 > > URL: http://svn.apache.org/viewvc?rev=1917082=rev > Log: > apr_buffer: Add explicit casts on all potentially narrowing conversions > to apr_size_t. Define the maximum buffer size as APR_SIZE_MAX/2. > > Modified: > apr/apr/trunk/buffer/apr_buffer.c > > Modified: apr/apr/trunk/buffer/apr_buffer.c > URL: > http://svn.apache.org/viewvc/apr/apr/trunk/buffer/apr_buffer.c?rev=1917082=1917081=1917082=diff > == > --- apr/apr/trunk/buffer/apr_buffer.c (original) > +++ apr/apr/trunk/buffer/apr_buffer.c Wed Apr 17 22:37:07 2024 > @@ -28,12 +28,13 @@ > #include "apr_strings.h" > #include "apr_private.h" > > +#define APR_BUFFER_MAX APR_SIZE_MAX/2 Why no longer APR_OFF_MAX? Regards Rüdiger
