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/17 22:37:08 UTC
svn commit: r1917082 - /apr/apr/trunk/buffer/apr_buffer.c
Author: minfrin
Date: Wed Apr 17 22:37:07 2024
New Revision: 1917082
URL: http://svn.apache.org/viewvc?rev=1917082&view=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&r1=1917081&r2=1917082&view=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
APR_DECLARE(apr_status_t) apr_buffer_mem_set(apr_buffer_t *buf,
void *mem, apr_size_t len)
{
- if (len > APR_OFF_MAX) {
+ if (len > APR_BUFFER_MAX) {
return APR_EINVAL;
}
@@ -48,7 +49,7 @@ APR_DECLARE(apr_buffer_t *) apr_buffer_m
{
apr_buffer_t *buf;
- if (len > APR_OFF_MAX) {
+ if (len > APR_BUFFER_MAX) {
return NULL;
}
@@ -66,7 +67,7 @@ APR_DECLARE(apr_status_t) apr_buffer_str
char *str, apr_ssize_t len)
{
- if (len > APR_OFF_MAX) {
+ if (len > APR_BUFFER_MAX) {
return APR_EINVAL;
}
@@ -76,9 +77,9 @@ APR_DECLARE(apr_status_t) apr_buffer_str
}
else if (len < 0) {
apr_size_t slen = strlen(str);
- if (slen <= APR_OFF_MAX) {
+ if (slen <= APR_BUFFER_MAX) {
buf->d.str = str;
- buf->size = -(slen + 1);
+ buf->size = -(apr_off_t)(slen + 1);
}
else {
buf->d.str = NULL;
@@ -109,8 +110,8 @@ APR_DECLARE(apr_buffer_t *) apr_buffer_s
}
else if (len < 0) {
slen = strlen(str);
- if (slen <= APR_OFF_MAX) {
- size = -(slen + 1);
+ if (slen <= APR_BUFFER_MAX) {
+ size = -(apr_off_t)(slen + 1);
}
else {
return NULL;
@@ -188,10 +189,10 @@ 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, -(buf->size + 1));
+ return apr_pstrmemdup(pool, buf->d.str, (apr_size_t)(-(buf->size + 1)));
}
else {
- return apr_pstrmemdup(pool, buf->d.str, buf->size);
+ return apr_pstrmemdup(pool, buf->d.str, (apr_size_t)buf->size);
}
}
@@ -226,7 +227,7 @@ 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_int64_t size = src->size < 0 ? -src->size : src->size;
+ apr_size_t size = src->size < 0 ? -src->size : src->size;
void *mem = alloc(ctx, size);
memcpy(mem, src->d.mem, size);
@@ -266,7 +267,7 @@ APR_DECLARE(apr_buffer_t *) apr_buffer_c
else {
/* absolute value is size of mem buffer including optional terminating zero */
- apr_int64_t size = src->size < 0 ? -src->size : src->size;
+ apr_size_t size = src->size < 0 ? -src->size : src->size;
void *mem = alloc(ctx, size);
memcpy(mem, src->d.mem, size);
@@ -344,16 +345,16 @@ APR_DECLARE(char *) apr_buffer_pstrncat(
}
if (src->size < 0) {
- size += (-src->size) - 1;
+ size += (apr_size_t)(-src->size) - 1;
}
else {
if (APR_BUFFER_NONE == flags) {
- size += src->size;
+ size += (apr_size_t)src->size;
}
else if (APR_BUFFER_BASE64 == flags) {
apr_size_t b64len;
- if (APR_SUCCESS != apr_encode_base64(NULL, src->d.mem, src->size,
+ if (APR_SUCCESS != apr_encode_base64(NULL, src->d.mem, (apr_size_t)src->size,
APR_ENCODE_NONE, &b64len)) {
return NULL;
}
@@ -380,17 +381,17 @@ APR_DECLARE(char *) apr_buffer_pstrncat(
}
if (src->size < 0) {
- strncpy(dst, src->d.str, (-src->size) - 1);
+ strncpy(dst, src->d.str, (apr_size_t)((-src->size) - 1));
dst += (-src->size) - 1;
}
else {
if (APR_BUFFER_NONE == flags) {
- memcpy(dst, src->d.mem, src->size);
+ memcpy(dst, src->d.mem, (apr_size_t)src->size);
}
else if (APR_BUFFER_BASE64 == flags) {
apr_size_t b64len;
- if (APR_SUCCESS != apr_encode_base64(dst, src->d.mem, src->size,
+ if (APR_SUCCESS != apr_encode_base64(dst, src->d.mem, (apr_size_t)src->size,
APR_ENCODE_NONE, &b64len)) {
return NULL;
}
Re: svn commit: r1917082 - /apr/apr/trunk/buffer/apr_buffer.c
Posted by Graham Leggett via dev <de...@apr.apache.org>.
On 18 Apr 2024, at 12:14, Ruediger Pluem <rp...@apache.org> 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
Posted by Graham Leggett via dev <de...@apr.apache.org>.
On 18 Apr 2024, at 17:07, Yann Ylavic <yl...@gmail.com> 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&revision=1917266
Regards,
Graham
--
Re: svn commit: r1917082 - /apr/apr/trunk/buffer/apr_buffer.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Apr 18, 2024 at 3:58 PM Graham Leggett <mi...@sharp.fm> wrote:
>
> On 18 Apr 2024, at 13:15, Yann Ylavic <yl...@gmail.com> 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
Posted by Graham Leggett via dev <de...@apr.apache.org>.
On 18 Apr 2024, at 15:57, Yann Ylavic <yl...@gmail.com> 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
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Apr 18, 2024 at 4:41 PM Yann Ylavic <yl...@gmail.com> 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
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Apr 18, 2024 at 3:58 PM Graham Leggett <mi...@sharp.fm> wrote:
>
> On 18 Apr 2024, at 13:15, Yann Ylavic <yl...@gmail.com> 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
Posted by Graham Leggett via dev <de...@apr.apache.org>.
On 18 Apr 2024, at 13:15, Yann Ylavic <yl...@gmail.com> 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
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Apr 18, 2024 at 1:16 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 4/18/24 12:37 AM, minfrin@apache.org wrote:
> > Author: minfrin
> > Date: Wed Apr 17 22:37:07 2024
> > New Revision: 1917082
> >
> > URL: http://svn.apache.org/viewvc?rev=1917082&view=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&r1=1917081&r2=1917082&view=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
Posted by Ruediger Pluem <rp...@apache.org>.
On 4/18/24 12:37 AM, minfrin@apache.org wrote:
> Author: minfrin
> Date: Wed Apr 17 22:37:07 2024
> New Revision: 1917082
>
> URL: http://svn.apache.org/viewvc?rev=1917082&view=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&r1=1917081&r2=1917082&view=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