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 wrote:
>
> On 24 Apr 2024, at 14:03, Yann Ylavic 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&
Re: svn commit: r1917266 - in /apr/apr/trunk: buffer/apr_buffer.c include/apr_buffer.h test/testbuffer.c
On Wed, Apr 24, 2024 at 3:03 PM Yann Ylavic 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
On Mon, Apr 22, 2024 at 2:46 PM 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 =
Re: svn commit: r1917047 - in /apr/apr/trunk: CHANGES buffer/ buffer/apr_buffer.c build.conf include/apr_buffer.h test/Makefile.in test/Makefile.win test/NWGNUaprtest test/abts_tests.h test/testbuffer
On Tue, Apr 16, 2024 at 10:54 PM wrote:
>
> --- apr/apr/trunk/buffer/apr_buffer.c (added)
> +++ apr/apr/trunk/buffer/apr_buffer.c Tue Apr 16 20:54:51 2024
> @@ -0,0 +1,408 @@
> +
> +APR_DECLARE(apr_buffer_t *) apr_buffer_mem_make(apr_pool_t *pool,
> +void *mem, apr_size_t len)
Two different possibilities for returning NULL, maybe an
apr_buffer_mem_create() instead returning APR_EINVAL/ENOMEM
accordingly.
> +{
> +apr_buffer_t *buf;
> +
> +if (len > APR_INT64_MAX) {
> +return NULL;
> +}
> +
> +buf = apr_palloc(pool, sizeof(apr_buffer_t));
> +
> +if (buf) {
> +buf->d.mem = mem;
> +buf->size = len;
> +}
> +
> +return buf;
> +}
> +
> +APR_DECLARE(apr_status_t) apr_buffer_str_set(apr_buffer_t *buf,
> + char *str, apr_ssize_t len)
> +{
> +
> +if (len > APR_INT64_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_INT64_MAX) {
> +buf->d.str = str;
> +buf->size = -(slen + 1);
> +}
> +else {
return APR_EINVAL?
> +buf->d.str = NULL;
> +buf->size = 0;
> +}
> +}
> +else {
> +buf->d.str = str;
> +buf->size = -(len + 1);
> +}
> +
> +return APR_SUCCESS;
> +}
> +
> +APR_DECLARE(apr_buffer_t *) apr_buffer_str_make(apr_pool_t *pool,
> +char *str, apr_ssize_t len)
Likewise several possibilities for returning NULL, apr_buffer_str_create()?
> +{
> +apr_buffer_t *buf;
> +apr_int64_t size;
> +apr_size_t slen;
> +
> +if (!str) {
> +str = NULL;
> +size = 0;
> +}
> +if (APR_BUFFER_STRING == len && !str[0]) {
> +size = len;
> +}
> +else if (len < 0) {
> +slen = strlen(str);
> +if (slen <= APR_INT64_MAX) {
> +size = -(slen + 1);
> +}
> +else {
> +return NULL;
> +}
> +}
> +else {
> +size = -(len + 1);
> +}
> +
> +buf = apr_palloc(pool, sizeof(apr_buffer_t));
> +
> +if (buf) {
> +buf->d.str = str;
> +buf->size = size;
> +}
> +
> +return buf;
> +}
> +
> +APR_DECLARE(apr_buffer_t *) apr_buffer_null_make(apr_pool_t *pool)
> +{
> +return apr_pcalloc(pool, sizeof(apr_buffer_t));
> +}
> +
> +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;
> +}
> +}
> +
> +APR_DECLARE(apr_size_t) apr_buffer_allocated(const apr_buffer_t *buf)
Same as apr_buffer_len()?
> +{
> +if (buf->size < 0) {
> +return (apr_size_t)((-buf->size));
> +}
> +else {
> +return (apr_size_t)buf->size;
> +}
> +}
> +
> +APR_DECLARE(void *) apr_buffer_pmemdup(apr_pool_t *pool, const apr_buffer_t
> *buf, apr_size_t *size)
> +{
> +apr_size_t len = apr_buffer_len(buf);
> +
> +if (size) {
> +size[0] = len;
> +}
> +
> +return apr_pmemdup(pool, buf->d.mem, len);
Should pmemdup() include the '\0' for a "string" buffer?
> +}
> +
> +APR_DECLARE(apr_buffer_t *) apr_buffer_arraydup(const apr_buffer_t *src,
> +apr_buffer_alloc alloc, void
> *ctx,
> +int nelts)
apr_size_t or at least unsigned int for nelts?
> +{
> +apr_buffer_t *dst = alloc(ctx, nelts * sizeof(apr_buffer_t));
Maybe check for nelts < APR_SIZE_MAX / sizeof(apr_buffer_t) before
allocating, and fail with APR_EINVAL otherwise?
Also, like apr_buffer_cpy() below, we could allow for allow == NULL
here (and if so maybe move the nelts argument before the alloc and
ctx).
> +apr_buffer_t *d = dst;
> +
> +int i;
> +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;
> +
> +void *mem = alloc(ctx, size);
> +memcpy(mem, src->d.mem, size);
> +
> +dst->size = src->size;
> +dst->d.mem = mem;
> +
> +src++;
> +dst++;
> +}
> +
> +return d;
> +}
> +
> +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) {
> +
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 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 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: r1917047 - in /apr/apr/trunk: CHANGES buffer/ buffer/apr_buffer.c build.conf include/apr_buffer.h test/Makefile.in test/Makefile.win test/NWGNUaprtest test/abts_tests.h test/testbuffer
On Wed, Apr 17, 2024 at 9:36 AM Graham Leggett via dev
wrote:
>
> On 17 Apr 2024, at 08:12, Ruediger Pluem wrote:
>
> >> I need some advice on handling Windows 32 bit. apr_int64_t for size is too
> >> big, and tests are failing.
> >>
> >> Technically apr_ssize_t is the right size, but the definition of ssize_t
> >> is [-1, SSIZE_MAX]. I need a signed very big number. Given we define
> >> apr_ssize_t, is it ok to use apr_ssize_t anyway?
> >
> > How about off_t / apr_off_t instead?
>
> Perfect, will fix.
Does it work on 32bit systems since apr_off_t is always 64bitin in apr (IIRC)?
I think the right type is apr_size_t because a buffer length just
can't be negative.
That'd also avoid signed integers arithmetic games like:
+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;
+}
+}
If you want a bit for string vs non-string buffers just limit the size
to SIZE_MAX/2 and use the high bit of an apr_size_t. The user can't
use buf->size directly still and must use apr_buffer_len() but it's no
different from the current implementation.
Likewise for the "apr_ssize_t len" parameter of apr_buffer_str_set()
and apr_buffer_str_make(), passing an sise_t/strlen() is just
undefined behaviour in C so it does not really help the user. With
apr_size_t one could pass APR_BUFFER_STRING for a nul-terminated
string and we could check the SIZE_MAX/2 limit there still.
Regards;
Yann.
PS: we have the very problem in the apr_encode/apr_escape interfaces.
Re: svn commit: r1916332 - /apr/apr/trunk/test/testescape.c
On Mon, Mar 18, 2024 at 2:11 PM Joe Orton wrote: > > On Fri, Mar 15, 2024 at 10:26:27AM -, yla...@apache.org wrote: > > Author: ylavic > > Date: Fri Mar 15 10:26:27 2024 > > New Revision: 1916332 > > > > URL: http://svn.apache.org/viewvc?rev=1916332=rev > > Log: > > testescape: missing ';' in entity escaping. > > The test now fails... did you intend to commit the fix too, something > like this? Argh yes, I had another (similar) change for apr_escape_json() and wanted to commit them separately, but this got lost when proceeding with further changes.. Now in r1916390 (plus backports to 1.[78]). Thanks! Yann.
Re: svn commit: r1916337 - /apr/apr/trunk/include/apr_general.h
On Mon, Mar 18, 2024 at 10:00 AM Ruediger Pluem wrote:
>
> On 3/15/24 2:35 PM, yla...@apache.org wrote:
> >
> > -#if defined(offsetof) && !defined(__cplusplus)
> > +#if defined(__has_builtin) && __has_builtin(__builtin_offsetof)
>
> This causes an
>
> ./include/apr_general.h:109:45: error: missing binary operator before token
> "("
>
> with gcc 8.5.0 for me. Following
> https://gcc.gnu.org/onlinedocs/cpp/_005f_005fhas_005fbuiltin.html
> and changing it to
>
> Index: include/apr_general.h
> ===
> --- include/apr_general.h (revision 1916382)
> +++ include/apr_general.h (working copy)
> @@ -106,13 +106,18 @@
> * @param field data field within the structure
> * @return offset
> */
> -#if defined(__has_builtin) && __has_builtin(__builtin_offsetof)
> +#if defined(__has_builtin)
> +#if __has_builtin(__builtin_offsetof)
> #define APR_OFFSETOF(s_type,field) __builtin_offsetof(s_type,field)
> -#elif defined(offsetof) && !defined(__cplusplus)
> +#endif
> +#endif
> +#if !defined(APR_OFFSETOF)
> +#if defined(offsetof) && !defined(__cplusplus)
> #define APR_OFFSETOF(s_type,field) offsetof(s_type,field)
> #else
> #define APR_OFFSETOF(s_type,field) APR_OFFSET(s_type*,field)
> #endif
> +#endif
>
>
> Looks like to me that __has_builtin does not like to be used within an '&&'
> operation for whatever reason.
oO well, ok: r1916385 (plus backports to 1.[78])
Thanks;
Yann.
APR_POOL_DEBUG in apr-util (was: APR_POOL_DEBUG usage question)
dev@ team,
I'm wondering if we could/should remove APR_POOL_DEBUG specifics in
apr-util 1.6.x and 1.7.x, otherwise it's not possible run
non-pool-debug apr-util with pool-debug apr.
Fortunately the only dependency on APR_POOL_DEBUG for apr-util seems
to be the apr_bucket code below.
On Thu, Feb 1, 2024 at 2:38 PM Simon Walter wrote:
>
> I had partial success with '--enable-pool-debug=yes' and
> '--enable-pool-debug=verbose'. Then I ran into something else regarding
> apr-util. I see there are pre-processor conditions based on APR_POOL_DEBUG.
>
> In apr_bucket_alloc_create():
>
> #if APR_POOL_DEBUG
> /* may be NULL for debug mode. */
> if (allocator == NULL) {
> if (apr_allocator_create() != APR_SUCCESS) {
> apr_abortfunc_t fn = apr_pool_abort_get(p);
> if (fn)
> (fn)(APR_ENOMEM);
> abort();
> }
> }
> #endif
>
> Indeed it segfaults in allocator_alloc() because the allocator is null.
Here, if apr was compiled with APR_POOL_DEBUG but apr-util without, it
crashes because the allocator is/remains NULL.
I think this code works without the compile time "#if APR_POOL_DEBUG"
checks because there is no way for apr_pool_allocator_get() to return
NULL besides APR_POOL_DEBUG mode, and of course both apr_pool_t and
apr_bucket_alloc_t are opaque which prevents anyone from playing with
their ->allocator.
It shouldn't either cause too much overhead if running this code for
!APR_POOL_DEBUG.
So don't you think we could/should just remove the "#if
APR_POOL_DEBUG" in buckets/apr_buckets_alloc.c?
Regards;
Yann.
Re: APR_POOL_DEBUG usage question
On Thu, Feb 1, 2024 at 2:38 PM Simon Walter wrote: > > I had partial success with '--enable-pool-debug=yes' and > '--enable-pool-debug=verbose'. Then I ran into something else regarding > apr-util. I see there are pre-processor conditions based on APR_POOL_DEBUG. Yes, you need to both apr and apr-util consistently with or without --enable-pool-debug. Regards; Yann.
Re: APR_POOL_DEBUG usage question
On Wed, Jan 31, 2024 at 7:44 PM Simon Walter wrote:
>
> My question about how to debuging should be done. abort() is called on
> apr_initialize():
>
> Program received signal SIGABRT, Aborted.
Does the attached patch fix the issue with apr_initialize()?
Regards;
Yann.
Index: memory/unix/apr_pools.c
===
--- memory/unix/apr_pools.c (revision 1913753)
+++ memory/unix/apr_pools.c (working copy)
@@ -2029,6 +2029,13 @@ APR_DECLARE(apr_status_t) apr_pool_create_ex_debug
pool->file_line = file_line;
#if APR_HAS_THREADS
+pool->owner = apr_os_thread_current();
+#endif /* APR_HAS_THREADS */
+#ifdef NETWARE
+pool->owner_proc = (apr_os_proc_t)getnlmhandle();
+#endif /* defined(NETWARE) */
+
+#if APR_HAS_THREADS
if (parent == NULL || parent->allocator != allocator) {
apr_status_t rv;
@@ -2067,13 +2074,6 @@ APR_DECLARE(apr_status_t) apr_pool_create_ex_debug
pool->ref = NULL;
}
-#if APR_HAS_THREADS
-pool->owner = apr_os_thread_current();
-#endif /* APR_HAS_THREADS */
-#ifdef NETWARE
-pool->owner_proc = (apr_os_proc_t)getnlmhandle();
-#endif /* defined(NETWARE) */
-
#if (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE)
apr_pool_log_event(pool, "CREATE", file_line, 1);
#endif /* (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE) */
Re: APR_POOL_DEBUG usage question
On Thu, Feb 1, 2024 at 1:56 PM Yann Ylavic wrote: > > On Wed, Jan 31, 2024 at 7:44 PM Simon Walter wrote: > > > > Should I use '--enable-pool-debug=yes' or '--enable-pool-debug=verbose'? > > I'd suggest using simple --enable-pool-debug[=yes] and ASan (Address > Sanitizer, i.e. "-fsanitize=address -fno-sanitize-recover=address" in > $CFLAGS). I mean, ASan for compiling the APR eventually, but it should be used to compile your program too for a full leaks/use-after-free coverage. > > Regards, > Yann.
Re: APR_POOL_DEBUG usage question
On Wed, Jan 31, 2024 at 7:44 PM Simon Walter wrote: > > Should I use '--enable-pool-debug=yes' or '--enable-pool-debug=verbose'? I'd suggest using simple --enable-pool-debug[=yes] and ASan (Address Sanitizer, i.e. "-fsanitize=address -fno-sanitize-recover=address" in $CFLAGS). Regards, Yann.
Re: Out of tree build broken for APR 1.7.x and 1.8.x
On Mon, Oct 16, 2023 at 11:41 AM Rainer Jung wrote: > > This problem is in 1.7.x and 1.8.x. I did not check apr trunk. Looks like apr_private_common.h does not exist in trunk (anymore?). > > Unfortunately I have no idea how to make autoheader to use > > #include "arch/apr_private_common.h" > > instead of > > #include "../apr_private_common.h" > > in the generated arch/unix/apr_private.h.in. > > Maybe someone has an idea? The below seems to do it (and pass `make check` for me): Index: configure.in === --- configure.in(revision 1910349) +++ configure.in(working copy) @@ -100,7 +100,7 @@ AH_BOTTOM([ /* * Include common private declarations. */ -#include "../apr_private_common.h" +#include "arch/apr_private_common.h" #endif /* APR_PRIVATE_H */ ]) Index: include/arch/netware/apr_private.h === --- include/arch/netware/apr_private.h(revision 1910349) +++ include/arch/netware/apr_private.h(working copy) @@ -199,7 +199,7 @@ void* getStatCache(); /* * Include common private declarations. */ -#include "../apr_private_common.h" +#include "arch/apr_private_common.h" #endif /*APR_PRIVATE_H*/ #endif /*NETWARE*/ Index: include/arch/win32/apr_private.h === --- include/arch/win32/apr_private.h(revision 1910349) +++ include/arch/win32/apr_private.h(working copy) @@ -169,7 +169,7 @@ APR_DECLARE_DATA int errno; /* * Include common private declarations. */ -#include "../apr_private_common.h" +#include "arch/apr_private_common.h" #endif /*APR_PRIVATE_H*/ #endif /*WIN32*/ -- Regards; Yann.
Re: svn commit: r1912607 - in /apr/apr/trunk: ./ .github/workflows/ build/ dbm/ include/ include/private/ test/
On Fri, Sep 29, 2023 at 3:39 PM wrote:
>
> Author: jorton
> Date: Fri Sep 29 13:39:11 2023
> New Revision: 1912607
[]
> --- apr/apr/trunk/dbm/apr_dbm_lmdb.c (added)
> +++ apr/apr/trunk/dbm/apr_dbm_lmdb.c Fri Sep 29 13:39:11 2023
[]
> +static apr_status_t vt_lmdb_open(apr_dbm_t **pdb, const char *pathname,
> + apr_int32_t mode, apr_fileperms_t perm,
> + apr_pool_t *pool)
> +{
[]
> +{
> +int dberr;
> +file.txn = NULL;
> +file.cursor = NULL;
> +
> +if ((dberr = mdb_env_create()) == 0) {
> +//XXX: properly set db size
> +if ((dberr = mdb_env_set_mapsize(file.env, UINT32_MAX)) == 0){
> +if ((dberr = mdb_env_open(file.env, pathname, dbmode |
> DEFAULT_ENV_FLAGS, apr_posix_perms2mode(perm))) == 0) {
> +if ((dberr = mdb_txn_begin(file.env, NULL, dbmode,
> )) == 0){
> +if ((dberr = mdb_dbi_open(file.txn, NULL,
> dbi_open_flags, )) != 0){
> +/* close the env handler */
> +mdb_env_close(file.env);
> +}
> +}
> +}
> +}
> +}
> +
> +if (truncate){
> +if ((dberr = mdb_drop(file.txn, file.dbi, 0)) != 0){
> +mdb_env_close(file.env);
> +}
> +}
> +
> +if (dberr != 0)
> +return db2s(dberr);
> +}
This seems to potentially double-close or leak "file.env" on some
errors, maybe write it like the below instead?
dberr = mdb_env_create();
if (dberr != 0) {
return db2s(dberr);
}
dberr = ...;
if (dberr == 0) {
dberr = ...;
}
dberr = ...;
if (dberr == 0) {
dberr = ...;
}
...
if (dberr == 0) {
dberr = mdb_dbi_open(file.txn, NULL, dbi_open_flags, );
if (dberr == 0 && truncate) {
dberr = mdb_drop(file.txn, file.dbi, 0);
if (dberr != 0) {
mdb_dbi_close(file.env, file.dbi);
}
}
}
if (dberr != 0) {
mdb_env_close(file.env);
return db2s(dberr);
}
> +
> +/* we have an open database... return it */
> +*pdb = apr_pcalloc(pool, sizeof(**pdb));
> +(*pdb)->pool = pool;
> +(*pdb)->type = _dbm_type_lmdb;
> +(*pdb)->file = apr_pmemdup(pool, , sizeof(file));
> +
> +/* ### register a cleanup to close the DBM? */
> +
> +return APR_SUCCESS;
> +}
> +
> +static void vt_lmdb_close(apr_dbm_t *dbm)
> +{
> +real_file_t *f = dbm->file;
> +
> +if (f->cursor){
> +mdb_cursor_close(f->cursor);
> +f->cursor = NULL;
> +}
> +
> +if (f->txn){
> + mdb_txn_commit(f->txn);
> + f->txn = NULL;
> +}
Shouldn't we abort the transaction here if there was no explicit
commit (store/del) previously?
In any case, the docs[1] for commit/abort seem to say that both
invalidate the ->cursor, so maybe we want to write this like:
if (f->txn){
mdb_txn_{commit,abort}(f->txn);
f->txn = NULL;
f->cursor = NULL;
}
if (f->cursor){
mdb_cursor_close(f->cursor);
f->cursor = NULL;
}
?
[1] http://www.lmdb.tech/doc/group__mdb.html#ga846fbd6f46105617ac9f4d76476f6597
> +
> +mdb_dbi_close(f->env, f->dbi);
> +mdb_env_close(f->env);
> +
> +f->env = NULL;
> +f->dbi = 0;
> +}
[]
> +static apr_status_t vt_lmdb_store(apr_dbm_t *dbm, apr_datum_t key,
> + apr_datum_t value)
> +{
> +real_file_t *f = dbm->file;
> +int rv;
> +MDB_val ckey = { 0 };
> +MDB_val cvalue = { 0 };
> +
> +ckey.mv_data = key.dptr;
> +ckey.mv_size = key.dsize;
> +
> +cvalue.mv_data = value.dptr;
> +cvalue.mv_size = value.dsize;
> +
> +if ((rv = mdb_put(f->txn, f->dbi, , , 0)) == 0) {
> +/* commit transaction */
> +if (((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS)
> +&& ((rv = mdb_txn_begin(f->env, NULL, 0, >txn)) ==
> MDB_SUCCESS)) {
> +f->cursor = NULL;
> +}
AIUI, f->cursor should be reset whether or not mdb_txn_begin()
succeeds here, and f->txn if any of _commit() or _begin() fails?
> +}
> +
> +/* store any error info into DBM, and return a status code. */
> +return set_error(dbm, rv);
> +}
> +
> +static apr_status_t vt_lmdb_del(apr_dbm_t *dbm, apr_datum_t key)
> +{
> +real_file_t *f = dbm->file;
> +int rv;
> +MDB_val ckey = { 0 };
> +
> +ckey.mv_data = key.dptr;
> +ckey.mv_size = key.dsize;
> +
> +if ((rv = mdb_del(f->txn, f->dbi, , NULL)) == 0) {
> +/* commit transaction */
> +if (((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS)
> +&& ((rv = mdb_txn_begin(f->env, NULL, 0, >txn)) ==
> MDB_SUCCESS)) {
> +f->cursor = NULL;
> +}
Likewise for f->cursor and f->txn resetting.
> +}
> +
> +/* store any error info into DBM, and
Re: [VOTE] Release apr-1.7.4-rc1 as apr-1.7.4
On Thu, Apr 13, 2023 at 1:08 PM Yann Ylavic wrote: > > On Thu, Apr 13, 2023 at 1:02 PM Eric Covener wrote: > > > > New for this release we are only toggling it on the release tag (sic), > > similar to httpd release process. > > Maybe a bump (to 1.7.5 here) is missing in 1.7.x after tagging now? > Currently apr_version.h is still at 1.7.4-dev it seems. Nevermind, this probably happens when the release is accepted..
Re: [VOTE] Release apr-1.7.4-rc1 as apr-1.7.4
On Thu, Apr 13, 2023 at 4:25 AM Eric Covener wrote: > > I would like to call a VOTE over the next few days to release > this candidate tarball apr-1.7.4-rc1 as 1.7.4: [X] +1: It's not just good, it's good enough! All tests pass on Debian 11 and 12, sums/sigs/version/dirname OK. Thanks Eric for RMing!
Re: [VOTE] Release apr-1.7.4-rc1 as apr-1.7.4
On Thu, Apr 13, 2023 at 1:02 PM Eric Covener wrote: > > New for this release we are only toggling it on the release tag (sic), > similar to httpd release process. Maybe a bump (to 1.7.5 here) is missing in 1.7.x after tagging now? Currently apr_version.h is still at 1.7.4-dev it seems.
Re: [VOTE] Release apr-1.7.4-rc1 as apr-1.7.4
On Thu, Apr 13, 2023 at 12:16 PM SteffenAL wrote: > > it's me. I build from svn, and there is : > > #define APR_IS_DEV_VERSION Yeah, this is restored immediately after tagging to not let a branch in a "release" state. You could checkout tags/apr-1.7.4-rc1 rather than branches/1.7.x for testing, the vote is about the tarballs but I suppose testing the tag is fine too if it helps (maybe related with Windows' CRLF vs tarball's LF in the source files in your case..). Regards; Yann.
Re: Release APR 1.7.4?
On Thu, Apr 13, 2023 at 12:15 PM Evgeny Kotkov wrote: > > Yann Ylavic writes: > > > Do we want to include r1909117 in 1.7.4 (to call abort() instead of > > assert() in apr_base64 when NDEBUG)? > > Sorry for the lateness, but should there be an -rc2 (or in any case..). > > If I recall correctly, APR 1.7.x doesn't have apr_base64, because it is a > part of APR-util in that timeline. Ah yes sorry for the noise, I keep forgetting which ones are in apr or in apr-util :) Regards; Yann.
Re: Release APR 1.7.4?
On Thu, Apr 13, 2023 at 10:47 AM Evgeny Kotkov via dev wrote: > > Eric Covener writes: > > > I will RM but it might be a bit piecemeal between now and the weekend. > > At the mercy of a lot of inflexible $bigco stuff. > > > > Hoping to improve a little this time by porting more of icings amazing > > httpd release work. > > Awesome, thanks! Do we want to include r1909117 in 1.7.4 (to call abort() instead of assert() in apr_base64 when NDEBUG)? Sorry for the lateness, but should there be an -rc2 (or in any case..). Regards; Yann.
Re: [VOTE] Release apr-1.7.4-rc1 as apr-1.7.4
On Thu, Apr 13, 2023 at 9:38 AM SteffenAL wrote: > > It is identified as 1.7.4-dev > > In apr_version.h dev version is defined. I don't see this in the release candidate tarballs (.gz or .bz2), apr_version.h contains: /* #undef APR_IS_DEV_VERSION */ so it should not include the "-dev" suffix in APR_VERSION_STRING. Likewise here: https://github.com/apache/apr/blob/1.7.4-rc1-candidate/include/apr_version.h#L72 Could it be from a local /D of yours or something? Regards; Yann.
Re: svn commit: r1902206 - /apr/apr/trunk/encoding/apr_base64.c
On Wed, Mar 29, 2023 at 1:04 PM Evgeny Kotkov via dev
wrote:
>
> Yann Ylavic writes:
>
> > +#undef NDEBUG /* always abort() on assert()ion failure */
> > +#include
>
> Sorry for being late to the party, but I think there are a few problems with
> the "#undef NDEBUG" line:
>
> 1) The debug implementation of an assert() may print a diagnostic message,
> for example to stderr. A caller of the library function may not be ready for
> this to happen when using a non-debug version of the library.
>
> 2) The actual destination of the message seems to be implementation-defined.
> For example, in Windows-based applications this may show a message box [1],
> which is probably even more unexpected for the user of the library.
>
> 3) Undefining NDEBUG before other headers may silently cause unexpected
> effects if any of those headers make some decisions based on the NDEBUG value,
> which isn't an entirely unreasonable thing to expect.
Fair points, the system printing (the faulting code) or not by default
before abort() should be #ifdef'd then? Depend on NDEBUG or an
APR_ABORT_USE_STDERR alike?
I'm not sure what the right default is though, ISTM that using system
printing before abort() for a portable [system] lib is not delusional.
This works well for users that can afford to let std outputs go or
handle/redirect them (e.g. httpd including when running as a Windows
service where the error could go to a journal/eventlog?), and an
abort() can be hard to diagnose without a message (and without a
coredump/backtrace).
Using a build time toggle does not necessarily help by the way when
all you have is an apr built by a distro/vendor (no NDEBUG set in
Debian it seems for instance).
Faulting silently by default looks harsh to me, there is no precedent
on apr abort()ing explicitly so there is no legacy behaviour to
preserve IMO. But not a strong opinion provided there is a #define'd.
>
> So, depending on whether we want the checks to soft- or hard-fault, maybe we
> could either remove the "#undef NDEBUG" line, or switch to a custom check that
> explicitly calls an abort()?
If we let some user-defined NDEBUG fly through apr_base64.c the code
will never abort(), which we always want for this int/size overflow
UB.
So possibly something like the below:
Index: encoding/apr_base64.c
===
--- encoding/apr_base64.c(revision 1908764)
+++ encoding/apr_base64.c(working copy)
@@ -20,8 +20,27 @@
* ugly 'len' functions, which is quite a nasty cost.
*/
-#undef NDEBUG /* always abort() on assert()ion failure */
+/* An APR__ASSERT()ion failure will abort() with or without printing
+ * the faulting condition (and code location) depending on NDEBUG.
+ */
+#ifndef NDEBUG /* use assert()/system's printing before abort() mechanism */
#include
+#define APR__ASSERT(cond) assert(cond)
+#else /* just abort() */
+#if APR_HAVE_STDLIB_H
+#include
+#endif
+#if HAVE___BUILTIN_EXPECT
+#define APR__UNLIKELY(cond) __builtin_expect(!!(cond), 0)
+#else
+#define APR__UNLIKELY(cond) (!!(cond))
+#endif
+#define APR__ASSERT(cond) do { \
+if (APR__UNLIKELY(!(cond))) { \
+abort(); \
+} \
+} while (0)
+#endif
#include "apr_base64.h"
#if APR_CHARSET_EBCDIC
@@ -124,7 +143,7 @@ APR_DECLARE(int) apr_base64_decode_len(const char
bufin = (const unsigned char *) bufcoded;
while (pr2six[*(bufin++)] <= 63);
nprbytes = (bufin - (const unsigned char *) bufcoded) - 1;
-assert(nprbytes <= APR_BASE64_DECODE_MAX);
+APR__ASSERT(nprbytes <= APR_BASE64_DECODE_MAX);
return (int)(((nprbytes + 3u) / 4u) * 3u + 1u);
}
@@ -161,7 +180,7 @@ APR_DECLARE(int) apr_base64_decode_binary(unsigned
bufin = (const unsigned char *) bufcoded;
while (pr2six[*(bufin++)] <= 63);
nprbytes = (bufin - (const unsigned char *) bufcoded) - 1;
-assert(nprbytes <= APR_BASE64_DECODE_MAX);
+APR__ASSERT(nprbytes <= APR_BASE64_DECODE_MAX);
nbytesdecoded = (int)(((nprbytes + 3u) / 4u) * 3u);
bufout = (unsigned char *) bufplain;
@@ -206,7 +225,7 @@ static const char basis_64[] =
APR_DECLARE(int) apr_base64_encode_len(int len)
{
-assert(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
+APR__ASSERT(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
return ((len + 2) / 3 * 4) + 1;
}
@@ -219,7 +238,7 @@ APR_DECLARE(int) apr_base64_encode(char *encoded,
int i;
char *p;
-assert(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
+APR__ASSERT(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
p = encoded;
for (i = 0; i < len - 2; i += 3) {
@@ -258,7 +277,7 @@ APR_DECLARE(int) apr_base64_encode_binary(char *en
int i;
char *p;
-assert(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
+APR__ASSERT(len >= 0 && len &
Re: svn commit: r1908503 - in /apr/apr/trunk: build/crypto.m4 crypto/apr_crypto_openssl.c
Hi Rainer; On Mon, Mar 27, 2023 at 2:02 PM Rainer Jung wrote: > > Am 18.03.23 um 17:33 schrieb yla...@apache.org: > > > > @@ -113,15 +112,13 @@ AC_DEFUN([APU_CHECK_CRYPTO_OPENSSL], [ > > AC_MSG_NOTICE(checking for openssl in $withval) > > AC_CHECK_HEADERS(openssl/x509.h, [openssl_have_headers=1]) > > AC_CHECK_LIB(crypto, EVP_CIPHER_CTX_new, openssl_have_libs=1) > > - if test "$openssl_have_headers" != "0" && test "$openssl_have_libs" > > != "0"; then > > + if test "$openssl_have_headers" = "1" && test "$openssl_have_libs" = > > "1"; then > > apu_have_openssl=1 > > -APR_ADDTO(LDFLAGS, [-L$withval/lib]) > > -APR_ADDTO(INCLUDES, [-I$withval/include]) > > fi > > - > > - AC_CHECK_DECLS([EVP_PKEY_CTX_new, OPENSSL_init_crypto], [], [], > > - [#include ]) > > - > > +fi > > +if test "$apu_have_openssl" = "1"; then > > +AC_CHECK_LIB(crypto, OPENSSL_init_crypto) > > +AC_CHECK_FUNCS([OPENSSL_init_crypto]) > > fi > > Hmmm, for me this dropped "APR_ADDTO(INCLUDES,..." breaks the apr trunk > build. The path to the OpenSSL header files gets no longer added to any > INCLUDES variable for the build. Ah yes, I thought both LDFLAGS and INCLUDES were already set above but it is LDFLAGS and CPPFLAGS actually. Restored in r1908749. Thanks! Yann.
Re: [VOTE] Release apr-1.7.3
On Mon, Mar 27, 2023 at 10:18 AM Ruediger Pluem wrote: > > 1.7.3-rc1 is here: > > https://apr.apache.org/dev/dist/ > > For the release of apr-1.7.3 [X] +1 looks great! Tested on Debian 11 and 12. All good with: apr_lock_method=USE_PROC_PTHREAD_SERIALIZE --enable-nonportable-atomics=yes --enable-posix-shm Digests and sigs are OK, and directory name is w/o the -rc. Thanks Rüdiger for RMing!
Re: Release APR 1.7.3 ?
On Fri, Mar 24, 2023 at 3:56 PM Ruediger Pluem wrote: > > Is someone opposed if I put an APR 1.7.x release for vote on the beginning of > next week? > It would be my first APR release, but I think I should be able to do it with > the instructions in release.sh (thanks Eric). > Otherwise I will ask here for help. > > We had a regression in apr-1-config that is now fixed and we have some nice > fixes / improvements for Windows. > I know that people think of releasing 1.8., but another 1.7 release seems to > be the lower hanging fruit. +1, the atomic64 changes are worth shipping ASAP too. Regards; Yann.
Re: svn commit: r1906889 - in /apr/apr/trunk: ./ build/ include/ include/arch/win32/ test/ threadproc/beos/ threadproc/netware/ threadproc/os2/ threadproc/unix/ threadproc/win32/
On Tue, Mar 21, 2023 at 2:24 PM Yann Ylavic wrote: > > Is there something I can #ifdef (or check at configure/run time) for > SLELS11 or the kernel? This would document in the test that a minimal > timeout is usually expected, but on this platform/kernel. Or I'll use > 190ms for everyone, which looks "unfair". Does r1908616 work for you? > > Regards; > Yann.
Re: svn commit: r1906889 - in /apr/apr/trunk: ./ build/ include/ include/arch/win32/ test/ threadproc/beos/ threadproc/netware/ threadproc/os2/ threadproc/unix/ threadproc/win32/
Hi Rainer; On Mon, Mar 20, 2023 at 10:31 PM Rainer Jung wrote: > > Am 17.03.23 um 15:11 schrieb Yann Ylavic: > > > > Hm OK, these commits were meant to address that precisely.. > > It looks like SLES has some special epoll_wait() implementation that > > can return before the timeout. Could you printf the t2 - t1 diff in > > the justsleep() test after apr_pollset_poll()? It could be interesting > > to know if it's almost 200ms or close to zero (the latter could mean > > epoll_wait() does not block if there is no fd in the pollset, which is > > the case in this test). > > The observed time delta on this platform is between 197 and 198.5 ms > instead of somewhat above 200ms. I should note it is an old patch level > of SLES 11 using kernel 2.6.32.12-0.7. > > There are some old informations about the rounding of the timeout: > > https://linux-fsdevel.vger.kernel.narkive.com/9jp0o6FA/bug-epoll-wait-timeout-is-shorter-than-requested Thanks for looking. Well, it seems that there is no guarantee of a minimal timeout on this system. There is something fishy though, the only change w.r.t. before is the millisecond round _up_.. > > On this system HZ is 100, so a jiffie is 10ms and probably this means it > should suffice to accept 200-10=190ms as the minimum sleep time. Is there something I can #ifdef (or check at configure/run time) for SLELS11 or the kernel? This would document in the test that a minimal timeout is usually expected, but on this platform/kernel. Or I'll use 190ms for everyone, which looks "unfair". Regards; Yann.
Re: svn commit: r1908433 - in /apr/apr/trunk: crypto/apr_crypto_openssl.c test/testcrypto.c
On Fri, Mar 17, 2023 at 9:23 AM Ruediger Pluem wrote:
>
> On 3/16/23 1:43 PM, yla...@apache.org wrote:
> > Author: ylavic
> > Date: Thu Mar 16 12:43:17 2023
> > New Revision: 1908433
> >
> > URL: http://svn.apache.org/viewvc?rev=1908433=rev
> > Log:
> > apr_crypto_openssl: Compatibility with OpenSSL 3+
> >
> > Modified:
> > apr/apr/trunk/crypto/apr_crypto_openssl.c
> > apr/apr/trunk/test/testcrypto.c
> >
> > Modified: apr/apr/trunk/crypto/apr_crypto_openssl.c
> > URL:
> > http://svn.apache.org/viewvc/apr/apr/trunk/crypto/apr_crypto_openssl.c?rev=1908433=1908432=1908433=diff
> > ==
> > --- apr/apr/trunk/crypto/apr_crypto_openssl.c (original)
> > +++ apr/apr/trunk/crypto/apr_crypto_openssl.c Thu Mar 16 12:43:17 2023
> > @@ -32,6 +32,10 @@
> >
> > #if APU_HAVE_CRYPTO
> >
> > +#ifndef OPENSSL_API_COMPAT
> > +#define OPENSSL_API_COMPAT 0x1010L /* for ENGINE API */
> > +#endif
>
> On RedHat 8 with openssl 1.1.1k this causes openssl/err.h which is included
> openssl/engine.h to
> no longer define the noop macro ERR_free_strings and thus causing the
> compilation to fail.
> Removing the above makes this go away. Why do we need to set it?
Hm, ERR_free_strings() shouldn't be used with openssl-1.1.1
(HAVE_DECL_OPENSSL_INIT_CRYPTO)?
OPENSSL_API_COMPAT is needed to avoid the warnings for the ENGINE api
with openssl >= 3, and moving to the new API (providers) is quite some
work FWICT. I thought it was a good compromise for now (and plan to do
the same thing for httpd FWIW), using the new API for what can be done
easily enough and setting OPENSSL_API_COMPAT for the rest (that is
mainly ENGINE..).
>
> > +#if !APR_USE_OPENSSL_PRE_3_0_API
> > +EVP_MAC *mac;
> > +#endif
>
> It looks like the usage of this field is not appropriately #If ed later on as
> I get compilation failures like
>
> crypto/apr_crypto_openssl.c: In function ‘crypto_key_cleanup’:
> crypto/apr_crypto_openssl.c:301:12: error: ‘apr_crypto_key_t’ {aka ‘struct
> apr_crypto_key_t’} has no member named ‘mac’
> if (key->mac) {
[]
>
> crypto/apr_crypto_openssl.c: In function ‘crypto_digest_cleanup’:
> crypto/apr_crypto_openssl.c:355:14: error: ‘apr_crypto_digest_t’ {aka ‘struct
> apr_crypto_digest_t’} has no member named ‘macCtx’;
> did you mean ‘mdCtx’?
> if (ctx->macCtx) {
Should be good with now r1908448.
Regards;
Yann.
Re: svn commit: r1908433 - in /apr/apr/trunk: crypto/apr_crypto_openssl.c test/testcrypto.c
On Thu, Mar 16, 2023 at 5:53 PM Ruediger Pluem wrote: > > On 3/16/23 1:43 PM, yla...@apache.org wrote: > > Author: ylavic > > Date: Thu Mar 16 12:43:17 2023 > > New Revision: 1908433 > > > > URL: http://svn.apache.org/viewvc?rev=1908433=rev > > Log: > > apr_crypto_openssl: Compatibility with OpenSSL 3+ > > > > Modified: > > apr/apr/trunk/crypto/apr_crypto_openssl.c > > apr/apr/trunk/test/testcrypto.c > > Why don't we need to call crypto_digest_cleanup any longer? Does the pool > cleanup take of this now? Cleanups were a bit scattered over normal/error exit codes while a cleanup was always registered. I missed that apr_crypto_block_t or apr_crypto_digest_t could be reused in _init() though, so removing some cleanups here was leaky on reuse. I find it simpler/cleaner to do the cleanup or alloc+register in _init() depending on reuse or not (respectively), so I did that in r1908453 rather than restoring the cleanups as before. Regards; Yann.
Re: svn commit: r1906889 - in /apr/apr/trunk: ./ build/ include/ include/arch/win32/ test/ threadproc/beos/ threadproc/netware/ threadproc/os2/ threadproc/unix/ threadproc/win32/
On Fri, Mar 17, 2023 at 2:30 PM Rainer Jung wrote: > > Am 17.03.23 um 14:12 schrieb Yann Ylavic: > > On Fri, Mar 17, 2023 at 11:20 AM Rainer Jung > > wrote: > >> > >> But: testpoll fails: > >> > >> testpoll: Line 897: apr_pollset_poll() didn't sleep > >> > >> Unfortunately I don't know when it started. Any idea, what I should > >> investigate? > >> > >> All this is on SLES11, haven't tried the recent APR trunk with newer > >> Linuxes, but r1908005 worked on them including testpoll tests. > > > > I suppose SLES11 is using epoll implementation, so r1902236 and > > r1902258 may help. > > Does your APR contain those? > > > > Regards; > > Yann. > > Yes, it contained everything until r1908442. Hm OK, these commits were meant to address that precisely.. It looks like SLES has some special epoll_wait() implementation that can return before the timeout. Could you printf the t2 - t1 diff in the justsleep() test after apr_pollset_poll()? It could be interesting to know if it's almost 200ms or close to zero (the latter could mean epoll_wait() does not block if there is no fd in the pollset, which is the case in this test). Regards; Yann.
Re: svn commit: r1906889 - in /apr/apr/trunk: ./ build/ include/ include/arch/win32/ test/ threadproc/beos/ threadproc/netware/ threadproc/os2/ threadproc/unix/ threadproc/win32/
On Fri, Mar 17, 2023 at 11:20 AM Rainer Jung wrote: > > But: testpoll fails: > > testpoll: Line 897: apr_pollset_poll() didn't sleep > > Unfortunately I don't know when it started. Any idea, what I should > investigate? > > All this is on SLES11, haven't tried the recent APR trunk with newer > Linuxes, but r1908005 worked on them including testpoll tests. I suppose SLES11 is using epoll implementation, so r1902236 and r1902258 may help. Does your APR contain those? Regards; Yann.
Re: svn commit: r1906889 - in /apr/apr/trunk: ./ build/ include/ include/arch/win32/ test/ threadproc/beos/ threadproc/netware/ threadproc/os2/ threadproc/unix/ threadproc/win32/
On Thu, Mar 16, 2023 at 2:53 PM Ivan Zhakov wrote: > > I have very little knowledge about autoconf stuff. The autoconf part was > suggest by Yann in [1] > > Yann: do you have any ideas why it does not work? Does r1908438 fix it for you Rainer? Regards; Yann.
Re: Problems with atomics in 1.7.2
Hi Stefan; On Sun, Feb 5, 2023 at 9:44 PM Stefan Fritsch wrote: > > > On powerpc we got a segfault: [] > Fixed in http://svn.apache.org/viewvc?view=revision=1907442 . [] > > On armel, we got a link error: > > /usr/bin/ld: ../.libs/libapr-1.so: undefined reference to > `__atomic_fetch_sub_8' > /usr/bin/ld: ../.libs/libapr-1.so: undefined reference to `__atomic_load_8' > /usr/bin/ld: ../.libs/libapr-1.so: undefined reference to > `__atomic_exchange_8' > /usr/bin/ld: ../.libs/libapr-1.so: undefined reference to `__atomic_store_8' > /usr/bin/ld: ../.libs/libapr-1.so: undefined reference to > `__atomic_fetch_add_8' > /usr/bin/ld: ../.libs/libapr-1.so: undefined reference to > `__atomic_compare_exchange_8' > > Build log at > https://buildd.debian.org/status/logs.php?pkg=apr=armel=sid > Fixed in http://svn.apache.org/viewvc?view=revision=1907441 Thanks! > > > However, even with both fixes, there still seem to be problems. On > powerpc, there was the same test failure as in PR 66457: > testatomic: Line 908: Unexpected value > > Log at > https://buildd.debian.org/status/fetch.php?pkg=apr=powerpc=1.7.2-2=1675512084=0 Fixed in http://svn.apache.org/r1907541 > > If I am not mistaken, on 32 bit architectures you always need to ensure > manually that both halves of a 64 bit value are written atomically. So > code like this > > apr_atomic_set64() > ... > *mem = val; > > seems wrong. > > Also, this construct > > #if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__) > #define WEAK_MEMORY_ORDERING 1 > #else > #define WEAK_MEMORY_ORDERING 0 > #endif > > seems to be very fragile. I think we have several issues here: 1. we should set WEAK_MEMORY_ORDERING for anything but __i386__, __x86_64__, __s390__ and __s390x__ 2. we should not use direct 64bit load/store on 32bit systems for atomic operations 3. on 32bit systems the alignment for an uint64_t is usually 4 (not 8 like on a 64bit systems), which hurts (at best) or prevents atomicity even with builtins. For 1. and 2. the generic/mutex case was addressed in r1907541 (apr_atomic_set64() was doing the right thing already), and the builtins case is addressed in r1907637 (hopefully). Windows code is also concerned but I believe it's doing the right thing already by checking _M_X64 (x86_64 CPU) or should we check for _WIN64 too (or APR_SIZEOF_VOIDP >= 8) for WIN32 running on x64? What about the alignment of uint64_t on WIN32, the Interlocked*64() functions seem to require 8-byte alignment so should we fall back to generic/mutex on WIN32 too? For 3., r1907642 should now take care of the alignment to define (or not) HAVE_ATOMIC_BUILTINS64 and HAVE__ATOMIC_BUILTINS64, with a fallback to USE_ATOMICS_GENERIC for cases where it matters. This was tested by looking at the (32bit) assembly generated for different compilers/options/builtins here: https://godbolt.org/z/r4daf6b4v (comment in/out some "__attribute__((aligned(8)))" and/or "&& 0" to see what helps or not). Notably, while both gcc and clang seem to implement the same kind of compare-and-swap loop for _read64 and _set64 with -m32 and the legacy __sync_* builtins, they seem to disagree on whether __atomic builtins should defer to libatomic (thus possibly mutex) when an uint64_t is not 8-byte aligned. gcc will use some x87/FPU instructions (fild/fistp) regardless of the alignment whereas clang will issue calls to libatomic whenever apr_uint64_t is not forcibly 8-byte aligned. Dunno who's correct here (whether x87 instructions work with 4-byte alignment, CPU cacheline crossing and so on..), but this seems to beg for a forced "typedef uint64_t apr_uint64_t __attribute__((aligned(8)));" (an ABI change) or a new API for 64bit atomics (with properly aligned apr_atomic_u64_t) on the APR side anyway to do the right thing on 32bit systems.. For now (after r1907642) this means that compiling a 32bit APR will USE_ATOMICS_BUILTINS64 with gcc and NEED_ATOMICS_GENERIC64 with clang. Does this all work for you? Finally it seems that both -march=i[56]86 (but not i[34]86) provide the same atomic builtins too (with gcc), so maybe we could use them instead of the forced generic/mutex implementation (currently) if it's how distros build 32bit APR (per https://bz.apache.org/bugzilla/show_bug.cgi?id=66457). So I think something like this would be fine: Index: configure.in === --- configure.in(revision 1907642) +++ configure.in(working copy) @@ -566,6 +566,9 @@ if test "$ap_cv_atomic_builtins" = "yes" -o "$ap_c if test "$ap_cv__atomic_builtins" = "yes"; then AC_DEFINE(HAVE__ATOMIC_BUILTINS, 1, [Define if compiler provides 32bit __atomic builtins]) fi +has_atomic_builtins=yes +else +has_atomic_builtins=no fi AC_CACHE_CHECK([whether the compiler provides 64bit atomic builtins], [ap_cv_atomic_builtins64], @@ -829,10 +832,15 @@ AC_ARG_ENABLE(nonportable-atomics, force_generic_atomics=yes fi ], -[case $host_cpu in -
Re: [VOTE] apr 1.7.2 and apr-util 1.6.3
On Tue, Jan 31, 2023 at 10:23 PM Eric Covener wrote: > > I hosed 1.7.1/1.6.2 and the archives have -rcX in them at the top > level. I would like to call for an expedited vote to replace them > with version bumps. I will proceed once we get 3 binding +1. > > I have re-tagged because I think the consensus will be that updating > the tarballs and signatures is too confusing. > > candidates are here: > > https://apr.apache.org/dev/dist/ > > For the release of apr-1.7.2 AND apr-util-1.6.3 +1 to both, big thanks Eric! Regards; Yann.
Re: VOTE: Release apr-1.7.1-rc2 as 1.7.1
Argh, this vote was actually meant for apr-util-1.6.2-rc2 :/ Nevermind, my +1 for apr-util-1.6.2-rc3 on the other thread anyway. On Wed, Jan 25, 2023 at 4:10 PM Yann Ylavic wrote: > > On Fri, Jan 20, 2023 at 1:44 AM Eric Covener wrote: > > > > 1.7.1-rc1 is here: > > > > https://apr.apache.org/dev/dist/ > > > > For the release of apr-1.7.1 > > [X] +1 looks great! > > Testing on Debian 11 and 12+ (bookwork/sid) using apr-1.7.1(-rc2). > > No test failure, checksum/signature ok. > > Thanks Eric! > > Regards; > Yann.
Re: [VOTE] release apr-1.6.2-rc3 as APR 1.6.2
On Fri, Jan 27, 2023 at 2:42 PM Eric Covener wrote: > > 1.6.2-rc3 is here: > > https://apr.apache.org/dev/dist/ > > For the release of apr-util-1.6.2 [X] +1 looks great Debian 11 & 12. Did not test the new mariadb bits specifically. Thanks Eric. Regards; Yann.
Re: VOTE: Release apr-1.7.1-rc2 as 1.7.1
On Fri, Jan 20, 2023 at 1:44 AM Eric Covener wrote: > > 1.7.1-rc1 is here: > > https://apr.apache.org/dev/dist/ > > For the release of apr-1.7.1 [X] +1 looks great! Testing on Debian 11 and 12+ (bookwork/sid) using apr-1.7.1(-rc2). No test failure, checksum/signature ok. Thanks Eric! Regards; Yann.
Re: VOTE: Release apr-1.7.1-rc2 as 1.7.1
On Fri, Jan 20, 2023 at 1:44 AM Eric Covener wrote: > > For the release of apr-1.7.1 [X] +1 looks great! apr-1.7.1-rc2 tested on Debian 11 and 12+ (bookwork/sid). All good with: apr_lock_method=USE_PROC_PTHREAD_SERIALIZE --enable-nonportable-atomics=yes --enable-posix-shm Thanks a lot Eric for RMing! Regards; Yann.
Re: svn commit: r1902206 - /apr/apr/trunk/encoding/apr_base64.c
On Mon, Jul 25, 2022 at 12:43 PM Ruediger Pluem wrote:
>
>
>
> On 6/23/22 8:49 PM, Ruediger Pluem wrote:
> >
> >
> > On 6/23/22 5:12 PM, yla...@apache.org wrote:
> >> Author: ylavic
> >> Date: Thu Jun 23 15:12:47 2022
> >> New Revision: 1902206
> >
> >
> >> @@ -275,16 +284,17 @@ APR_DECLARE(int) apr_base64_encode_binar
> >> }
> >>
> >> *p++ = '\0';
> >> -return (int)(p - encoded);
> >> +return (unsigned int)(p - encoded);
> >> }
> >>
> >> APR_DECLARE(char *) apr_pbase64_encode(apr_pool_t *p, const char *string)
> >> {
> >> char *encoded;
> >> -int l = strlen(string);
> >> +apr_size_t len = strlen(string);
> >>
> >> -encoded = (char *) apr_palloc(p, apr_base64_encode_len(l));
> >> -apr_base64_encode(encoded, string, l);
> >> +assert(len <= (apr_size_t)APR_INT32_MAX);
> >
> > Shouldn't this be INT_MAX or APR_BASE64_ENCODE_MAX?
>
> Any update on this comment? I guess INT_MAX or APR_INT32_MAX is mood given
> the result of the discussion in this thread, but it
> probably should be APR_BASE64_ENCODE_MAX?
Good point, r1904666.
>
> >
> >> +encoded = (char *) apr_palloc(p, apr_base64_encode_len((int)len));
> >> +apr_base64_encode(encoded, string, (int)len);
> >>
> >> return encoded;
> >> }
Regards;
Yann.
Re: Issues with shipping apr_thread_current() in APR 1.8.0 and an alternative approach for PCRE2 allocations in HTTPD
On Tue, Jul 12, 2022 at 12:46 PM Ivan Zhakov wrote:
>
> On Wed, 29 Jun 2022 at 01:28, Yann Ylavic wrote:
> >
> > On Tue, Jun 28, 2022 at 6:08 PM Ivan Zhakov wrote:
> > > >
> > > I meant a memory leak for native threads. For example, with an MPM
> > > that uses native Windows threadpool for worker threads.
> >
> > That's not mpm_winnt right? (it does not use native threads anymore
> > and plays no apr_thread_current_create() game, it just uses
> > apr_thread_create() so apr_thread_current() works automatically
> > there).
>
> No, it's not mpm_winnt, but still a possible real-world example. And there
> are multiple other users of APR besides the HTTP Server.
So we have a hypothetical issue for what exactly? Someone will be
using the new ap_thread API (or future APR one) for something that it
can't be used for?
>
> >
> > Probably the thread pool lets you pass the thread function? So you
> > could "apr_thread_current_create()" when entering the
> > function and "apr_pool_destroy(apr_thread_pool_get(mythread))" before
> > leaving?
> > That'd still leak if the thread calls TerminateThread() explicitly at
> > some point, but it seems that the native Windows thread API is not
> > very helpful for that since there is way to register a destructor for
> > when the thread terminates by any mean (like the pthread_key_create()
> > API's destructor for instance). If the native API does not help I
> > don't think that APR can do much better..
> >
> This sounds problematic to me, because the result is a memory leak unless
> the application code is changed (and the APR consumer has to somehow be aware
> of that).
>
> Also, native Windows thread pool works in a different way: it invokes a
> callback on the random thread created by OS. So there is no easy way to
> track the thread exit.
You call something that does not exist as problematic. If some native
threads can call TerminateThread() at any uncontrollable point then
don't use ap_thread_current_create(), and if this is in an MPM then
don't use that MPM at all if you are worried about leaks.
But threadpools usually let you provide the thread function, so you
have both an entry and exit point between which you are not supposed
to kill the thread (underneath the threadpool), right?
> >
> > As said above, this is how it works already when apr_thread_current() is
> > NULL.
> > If it's not NULL, you get the nice behaviour that it reuses
> > efficiently the same vector (PCRE1) or context (PCRE2) for each call
> > on the same thread, with no allocation/free overhead or memory
> > fragmentation.
> >
>
> The code in server/util_pcre.c:342 was doing the following:
> [[[
> current = ap_thread_current();
> if (!current) {
> *to_free = 1;
> return alloc_match_data(size, small_vector);
> }
> ]]]
>
> Here alloc_match_data() was calling pcre2_match_data_create(), resulting
> in a malloc().
Yes OK, with the previous implementation there was no cache/stack
provided for PCRE2 for non APR threads (or !AP_HAS_THREAD_LOCAL).
You implemented that, but now what's missing is a per-thread cache
that works for all the patterns and is reused across the calls to
ap_regexec(), and for that you need
AP_THREAD_LOCAL/ap_thread_current().
So how about we still use the stack buffer only if it's enough, or use
some AP_THREAD_LOCAL/ap_thread_current() cache if available, or use
malloc() finally otherwise. Best of both worlds, should there be
regexes with many captures or PCRE2's START_FRAMES_SIZE be smaller for
some libpcre2 build or pcre2_match_data_create_with_frames() be
available on a future PCRE2 version (all of those cases would put more
pressure on private_malloc() with more frequent and/or larger calls
from pcre2_match()), then systems with
AP_THREAD_LOCAL/ap_thread_current() will still work efficiently.
Patch attached, thoughts?
Regards;
Yann.
PS: the patch also includes the fix from
https://bz.apache.org/bugzilla/show_bug.cgi?id=66119
Index: server/util_pcre.c
===
--- server/util_pcre.c (revision 1902674)
+++ server/util_pcre.c (working copy)
@@ -56,6 +56,11 @@ POSSIBILITY OF SUCH DAMAGE.
#include "apr_strings.h"
#include "apr_tables.h"
+#if AP_HAS_THREAD_LOCAL
+#include "apr_thread_proc.h"
+#include "mpm_common.h" /* for ap_max_mem_free */
+#endif
+
#ifdef HAVE_PCRE2
#define PCRE2_CODE_UNIT_WIDTH 8
#include "pcre2.h"
@@ -280,9 +285,24 @@ typedef int* match_data_pt;
typedef int* match_vector_pt;
#endif
+#if AP_HAS_THREAD_LOCAL && !defined(APREG_NO_THREAD_LOCAL)
+#define APREG_USE_THREAD_LOCAL 1
+#else
+#define APREG
Re: svn commit: r1902375 - in /apr/apr/branches/1.8.x: ./ CHANGES file_io/win32/readwrite.c test/testfile.c
On Fri, Jul 1, 2022 at 1:35 PM Evgeny Kotkov
wrote:
>
> Yann Ylavic writes:
>
> > Why would buffered reads always be full reads (i.e. until the given
> > size of EOF)?
> >
> > For apr_file_read() I don't think this is expected, users might want
> > read() to return what is there when it's there (i.e. they handle short
> > reads by themselves), so implicit full reads will force them to use
> > small buffers artificially and they'll lose in throughput. On the
> > other hand, users that want full reads can do that already/currently
> > in their own code (by calling read() multiple times).
> >
> > For apr_file_gets() this is the same issue, why when some read returns
> > the \n would we need to continue reading to fill the buffer? In a pipe
> > scenario the peer would apr_file_write("Hello\n") and the consumer's
> > apr_file_read() would not return that to the user immediately?
> >
> > This has nothing to do with buffered vs non-buffered IMHO, there are
> > apr_file_{read,write}_full() already for that.
>
> One comment I have here is that this commit does not change the existing
> behavior of apr_file_read() for buffered files. The read_buffered() helper
> isn't new by itself, as it has been merely factored out from apr_file_read().
>
> So, doing full reads for buffered files has been the current behavior
> for both unix and win32 and for a long time, I think. For example, see
> unix/readwrite.c:file_read_buffered(). This applies to the unix version
> of apr_file_gets() as well.
Oh, and the loop seems to have been removed later in r1828509
"apr_file_read: Optimize large reads from buffered files on Windows"
(backported to 2.4.x in r1902379 too).
So Windows is doing the right thing now (not unix), right?
Sorry for the noise, and thanks for the fix/info :)
Regards;
Yann.
Re: svn commit: r1902378 - in /apr/apr/branches/1.8.x: ./ CHANGES file_io/win32/readwrite.c test/testfile.c
On Fri, Jul 1, 2022 at 11:14 AM Ivan Zhakov wrote:
>
> On Fri, 1 Jul 2022 at 01:10, Yann Ylavic wrote:
> >
> > On Fri, Jul 1, 2022 at 12:00 AM Yann Ylavic wrote:
> > >
> > > On Thu, Jun 30, 2022 at 7:28 PM wrote:
> > > >
> > > > Author: ivan
> > > > Date: Thu Jun 30 17:28:50 2022
> > > > New Revision: 1902378
> > > >
> > > > URL: http://svn.apache.org/viewvc?rev=1902378=rev
> > > > Log:
> > > > On 1.8.x branch: Merge r1806299, r1806301, r1806308, r1806610:
> > > > *) apr_file_write: Optimize large writes to buffered files on
> > > > Windows.
> > > []
> > > >
> > > > --- apr/apr/branches/1.8.x/file_io/win32/readwrite.c (original)
> > > > +++ apr/apr/branches/1.8.x/file_io/win32/readwrite.c Thu Jun 30
> > > > 17:28:50 2022
> > > > @@ -247,6 +247,91 @@ APR_DECLARE(apr_status_t) apr_file_read(
> > > > return rv;
> > > > }
> > > >
> > > > +/* Helper function that adapts WriteFile() to apr_size_t instead
> > > > + * of DWORD. */
> > > > +static apr_status_t write_helper(HANDLE filehand, const char *buf,
> > > > + apr_size_t len, apr_size_t *pwritten)
> > > > +{
> > > > +apr_size_t remaining = len;
> > > > +
> > > > +*pwritten = 0;
> > > > +do {
> > > > +DWORD to_write;
> > > > +DWORD written;
> > > > +
> > > > +if (remaining > APR_DWORD_MAX) {
> > > > +to_write = APR_DWORD_MAX;
> > > > +}
> > > > +else {
> > > > +to_write = (DWORD)remaining;
> > > > +}
> > > > +
> > > > +if (!WriteFile(filehand, buf, to_write, , NULL)) {
> > > > +*pwritten += written;
> > > > +return apr_get_os_error();
> > > > +}
> > > > +
> > > > +*pwritten += written;
> > > > +remaining -= written;
> > > > +buf += written;
> > > > +} while (remaining);
> > >
> > > So there's no writev() like syscall on Windows (something that
> > > provides atomicity)?
> >
> > I found WriteFileGather
> > (https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-writefilegather).
> > Maybe it can help with some logic à la apr_socket_sendv()?
> >
> [[[
> Each buffer must be at least the size of a system memory page and must be
> aligned on a system memory page size boundary. The system writes one system
> memory page of data from each buffer.
> ]]]
> [[[
> The total number of bytes to be written. Each element of aSegmentArray
> contains a one-page chunk of this total. Because the file must be opened with
> FILE_FLAG_NO_BUFFERING, the number of bytes must be a multiple of the sector
> size of the file system where the file is located.
> ]]]
OK, I said "found it" not "looked at it in detail" :)
Too restrictive it seems (reminds me of O_DIRECT on unixes, we don't
want to use it here).
But the short-write argument stands, if Windows never short-writes
then the implicit full-write is unnecessary, but if it does
short-write then apr_file_write() has to return it too (for the user
to know), so in both cases the loop has to stop when written <
to_write (and probably return success if written > 0).
Users that don't expect short-writes should use apr_file_write_full().
Same on the apr_file_read() side.
Regards;
Yann.
Re: svn commit: r1902378 - in /apr/apr/branches/1.8.x: ./ CHANGES file_io/win32/readwrite.c test/testfile.c
On Fri, Jul 1, 2022 at 12:00 AM Yann Ylavic wrote:
>
> On Thu, Jun 30, 2022 at 7:28 PM wrote:
> >
> > Author: ivan
> > Date: Thu Jun 30 17:28:50 2022
> > New Revision: 1902378
> >
> > URL: http://svn.apache.org/viewvc?rev=1902378=rev
> > Log:
> > On 1.8.x branch: Merge r1806299, r1806301, r1806308, r1806610:
> > *) apr_file_write: Optimize large writes to buffered files on
> > Windows.
> []
> >
> > --- apr/apr/branches/1.8.x/file_io/win32/readwrite.c (original)
> > +++ apr/apr/branches/1.8.x/file_io/win32/readwrite.c Thu Jun 30 17:28:50
> > 2022
> > @@ -247,6 +247,91 @@ APR_DECLARE(apr_status_t) apr_file_read(
> > return rv;
> > }
> >
> > +/* Helper function that adapts WriteFile() to apr_size_t instead
> > + * of DWORD. */
> > +static apr_status_t write_helper(HANDLE filehand, const char *buf,
> > + apr_size_t len, apr_size_t *pwritten)
> > +{
> > +apr_size_t remaining = len;
> > +
> > +*pwritten = 0;
> > +do {
> > +DWORD to_write;
> > +DWORD written;
> > +
> > +if (remaining > APR_DWORD_MAX) {
> > +to_write = APR_DWORD_MAX;
> > +}
> > +else {
> > +to_write = (DWORD)remaining;
> > +}
> > +
> > +if (!WriteFile(filehand, buf, to_write, , NULL)) {
> > +*pwritten += written;
> > +return apr_get_os_error();
> > +}
> > +
> > +*pwritten += written;
> > +remaining -= written;
> > +buf += written;
> > +} while (remaining);
>
> So there's no writev() like syscall on Windows (something that
> provides atomicity)?
I found WriteFileGather
(https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-writefilegather).
Maybe it can help with some logic à la apr_socket_sendv()?
> I would stop the loop on written < to_write at least, apr_file_write()
> should be prepared to get a short/partial write (for whatever reason),
> or otherwise use apr_file_write_full().
>
> > +
> > +return APR_SUCCESS;
> > +}
> > +
> > +static apr_status_t write_buffered(apr_file_t *thefile, const char *buf,
> > + apr_size_t len, apr_size_t *pwritten)
> > +{
> > +apr_status_t rv;
> > +
> > +if (thefile->direction == 0) {
> > +/* Position file pointer for writing at the offset we are
> > logically reading from */
> > +apr_off_t offset = thefile->filePtr - thefile->dataRead +
> > thefile->bufpos;
> > +DWORD offlo = (DWORD)offset;
> > +LONG offhi = (LONG)(offset >> 32);
> > +if (offset != thefile->filePtr)
> > +SetFilePointer(thefile->filehand, offlo, , FILE_BEGIN);
> > +thefile->bufpos = thefile->dataRead = 0;
> > +thefile->direction = 1;
> > +}
> > +
> > +*pwritten = 0;
> > +
> > +while (len > 0) {
> > +if (thefile->bufpos == thefile->bufsize) { /* write buffer is full
> > */
> > +rv = apr_file_flush(thefile);
> > +if (rv) {
> > +return rv;
> > +}
> > +}
> > +/* If our buffer is empty, and we cannot fit the remaining chunk
> > + * into it, write the chunk with a single syscall and return.
> > + */
> > +if (thefile->bufpos == 0 && len > thefile->bufsize) {
> > +apr_size_t written;
> > +
> > +rv = write_helper(thefile->filehand, buf, len, );
> > +thefile->filePtr += written;
> > +*pwritten += written;
> > +return rv;
> > +}
> > +else {
> > +apr_size_t blocksize = len;
> > +
> > +if (blocksize > thefile->bufsize - thefile->bufpos) {
> > +blocksize = thefile->bufsize - thefile->bufpos;
> > +}
> > +memcpy(thefile->buffer + thefile->bufpos, buf, blocksize);
> > +thefile->bufpos += blocksize;
> > +buf += blocksize;
> > +len -= blocksize;
> > +*pwritten += blocksize;
> > +}
> > +}
>
> Likewise.
>
> > +
> > +return APR_SUCCESS;
> > +}
>
>
> Regards;
> Yann.
Re: svn commit: r1902378 - in /apr/apr/branches/1.8.x: ./ CHANGES file_io/win32/readwrite.c test/testfile.c
On Thu, Jun 30, 2022 at 7:28 PM wrote:
>
> Author: ivan
> Date: Thu Jun 30 17:28:50 2022
> New Revision: 1902378
>
> URL: http://svn.apache.org/viewvc?rev=1902378=rev
> Log:
> On 1.8.x branch: Merge r1806299, r1806301, r1806308, r1806610:
> *) apr_file_write: Optimize large writes to buffered files on
> Windows.
[]
>
> --- apr/apr/branches/1.8.x/file_io/win32/readwrite.c (original)
> +++ apr/apr/branches/1.8.x/file_io/win32/readwrite.c Thu Jun 30 17:28:50 2022
> @@ -247,6 +247,91 @@ APR_DECLARE(apr_status_t) apr_file_read(
> return rv;
> }
>
> +/* Helper function that adapts WriteFile() to apr_size_t instead
> + * of DWORD. */
> +static apr_status_t write_helper(HANDLE filehand, const char *buf,
> + apr_size_t len, apr_size_t *pwritten)
> +{
> +apr_size_t remaining = len;
> +
> +*pwritten = 0;
> +do {
> +DWORD to_write;
> +DWORD written;
> +
> +if (remaining > APR_DWORD_MAX) {
> +to_write = APR_DWORD_MAX;
> +}
> +else {
> +to_write = (DWORD)remaining;
> +}
> +
> +if (!WriteFile(filehand, buf, to_write, , NULL)) {
> +*pwritten += written;
> +return apr_get_os_error();
> +}
> +
> +*pwritten += written;
> +remaining -= written;
> +buf += written;
> +} while (remaining);
So there's no writev() like syscall on Windows (something that
provides atomicity)?
I would stop the loop on written < to_write at least, apr_file_write()
should be prepared to get a short/partial write (for whatever reason),
or otherwise use apr_file_write_full().
> +
> +return APR_SUCCESS;
> +}
> +
> +static apr_status_t write_buffered(apr_file_t *thefile, const char *buf,
> + apr_size_t len, apr_size_t *pwritten)
> +{
> +apr_status_t rv;
> +
> +if (thefile->direction == 0) {
> +/* Position file pointer for writing at the offset we are logically
> reading from */
> +apr_off_t offset = thefile->filePtr - thefile->dataRead +
> thefile->bufpos;
> +DWORD offlo = (DWORD)offset;
> +LONG offhi = (LONG)(offset >> 32);
> +if (offset != thefile->filePtr)
> +SetFilePointer(thefile->filehand, offlo, , FILE_BEGIN);
> +thefile->bufpos = thefile->dataRead = 0;
> +thefile->direction = 1;
> +}
> +
> +*pwritten = 0;
> +
> +while (len > 0) {
> +if (thefile->bufpos == thefile->bufsize) { /* write buffer is full */
> +rv = apr_file_flush(thefile);
> +if (rv) {
> +return rv;
> +}
> +}
> +/* If our buffer is empty, and we cannot fit the remaining chunk
> + * into it, write the chunk with a single syscall and return.
> + */
> +if (thefile->bufpos == 0 && len > thefile->bufsize) {
> +apr_size_t written;
> +
> +rv = write_helper(thefile->filehand, buf, len, );
> +thefile->filePtr += written;
> +*pwritten += written;
> +return rv;
> +}
> +else {
> +apr_size_t blocksize = len;
> +
> +if (blocksize > thefile->bufsize - thefile->bufpos) {
> +blocksize = thefile->bufsize - thefile->bufpos;
> +}
> +memcpy(thefile->buffer + thefile->bufpos, buf, blocksize);
> +thefile->bufpos += blocksize;
> +buf += blocksize;
> +len -= blocksize;
> +*pwritten += blocksize;
> +}
> +}
Likewise.
> +
> +return APR_SUCCESS;
> +}
Regards;
Yann.
Re: svn commit: r1902375 - in /apr/apr/branches/1.8.x: ./ CHANGES file_io/win32/readwrite.c test/testfile.c
On Thu, Jun 30, 2022 at 6:19 PM wrote:
>
> Author: ivan
> Date: Thu Jun 30 16:18:51 2022
> New Revision: 1902375
>
> URL: http://svn.apache.org/viewvc?rev=1902375=rev
> Log:
> On 1.8.x branch: Merge r1785072, r1788930 from trunk:
> *) apr_file_gets: Optimize for buffered files on Windows.
> [Evgeny Kotkov ]
[]
>
> --- apr/apr/branches/1.8.x/file_io/win32/readwrite.c (original)
> +++ apr/apr/branches/1.8.x/file_io/win32/readwrite.c Thu Jun 30 16:18:51 2022
> @@ -141,6 +141,56 @@ static apr_status_t read_with_timeout(ap
> return rv;
> }
>
> +static apr_status_t read_buffered(apr_file_t *thefile, void *buf, apr_size_t
> *len)
> +{
> +apr_status_t rv;
> +char *pos = (char *)buf;
> +apr_size_t blocksize;
> +apr_size_t size = *len;
> +
> +if (thefile->direction == 1) {
> +rv = apr_file_flush(thefile);
> +if (rv != APR_SUCCESS) {
> +return rv;
> +}
> +thefile->bufpos = 0;
> +thefile->direction = 0;
> +thefile->dataRead = 0;
> +}
> +
> +rv = 0;
> +while (rv == 0 && size > 0) {
> +if (thefile->bufpos >= thefile->dataRead) {
> +apr_size_t read;
> +rv = read_with_timeout(thefile, thefile->buffer,
> + thefile->bufsize, );
> +if (read == 0) {
> +if (rv == APR_EOF)
> +thefile->eof_hit = TRUE;
> +break;
> +}
> +else {
> +thefile->dataRead = read;
> +thefile->filePtr += thefile->dataRead;
> +thefile->bufpos = 0;
> +}
> +}
> +
> +blocksize = size > thefile->dataRead - thefile->bufpos ?
> thefile->dataRead - thefile->bufpos : size;
> +memcpy(pos, thefile->buffer + thefile->bufpos, blocksize);
> +thefile->bufpos += blocksize;
> +pos += blocksize;
> +size -= blocksize;
> +}
Why would buffered reads always be full reads (i.e. until the given
size of EOF)?
For apr_file_read() I don't think this is expected, users might want
read() to return what is there when it's there (i.e. they handle short
reads by themselves), so implicit full reads will force them to use
small buffers artificially and they'll lose in throughput. On the
other hand, users that want full reads can do that already/currently
in their own code (by calling read() multiple times).
For apr_file_gets() this is the same issue, why when some read returns
the \n would we need to continue reading to fill the buffer? In a pipe
scenario the peer would apr_file_write("Hello\n") and the consumer's
apr_file_read() would not return that to the user immediately?
This has nothing to do with buffered vs non-buffered IMHO, there are
apr_file_{read,write}_full() already for that.
I don't know if nonblocking reads take this path too, but if so this
also causes a second/useless read (because nonblocking are often short
reads) that is almost sure to return EAGAIN (or windows equivalent),
and the call would return EGAIN with *len > 0?
> +
> +*len = pos - (char *)buf;
> +if (*len) {
> +rv = APR_SUCCESS;
> +}
> +
> +return rv;
> +}
Re: svn commit: r1887497 - in /apr/apr/branches/1.7.x: ./ CHANGES mmap/unix/mmap.c test/data/mmap_large_datafile.txt test/testfnmatch.c test/testmmap.c
On Wed, Jun 29, 2022 at 4:49 PM William A Rowe Jr wrote: > > The eventual change still is present for apr > 1.8 adopters. > > Thoughts? ylavic are you willing to revert? It seems that no one is using apr_mmap_create() with an offset != 0 or the bug would have been reported (PR 65158 proved to be a cifs filesystem bug finally). Well, apr_bucket_file reads can use an mmap offset if planets misalign still, and that's possibly why we had quite some issues for "EnableMMap on" in httpd (IIRC), but since it was never nailed to this bug it usually ends with "please EnableMMap => off".. So let's revert, if something comes up around this, it's probably another case where I'll first ask: can you reproduce with apr-1.8? For 1.8.x then (we can fix it there right?), I'd rather we revert this change and r1887508 to merge trunk's r1887060 and r1887506 directly (i.e. without the API workaround-try of this 1.7.x merge). That is, add the "poffset" field to (the end of) apr_mmap_t and be done with no tricks. But since I'm not sure what we can or not do with a minor bump I'll ask, can we do that? Regards; Yann.
Re: svn commit: r1884103 - in /apr/apr/branches/1.7.x: ./ include/arch/beos/ include/arch/netware/ include/arch/unix/ include/arch/win32/ memory/unix/ threadproc/beos/ threadproc/netware/ threadproc/o
On Wed, Jun 29, 2022 at 6:09 PM Yann Ylavic wrote: > > By making such impracticable policies for revision changes (semver > sense), we are going to simply not fix anything but trivial changes > without bumping minor, and no one would/should care about the old > (though still "maintained") minor versions. What's the point of > releasing 1.7.1 then? Let's go with 1.8.0 only, simpler (less > maintenance) and better for everybody.. OK, by re-reading this I realize I was a bit harsh, a bit of frustration here (fixes that don't get in..), sorry about that. After a bit of diving into svn history, this commit was mainly to address UAF with apr_thread_pool code when it was using detached threads, which is not the case anymore (r1884110, r1889219). So the scope of the fix might be for user detached thread usage "only" (which may not be common), and APR_POOL_DEBUG (which is mainly never by devs only, though something happening in APR_POOL_DEBUG mode might happen without when the planets are no longer aligned). I'll try to revert and see how it goes for, e.g., httpd's ci (where mod_watchdog uses thread pools). Anyway, if we get bug/crash reports for 1.7.1 and some (time consuming) invertigations shows that it's fixed in 1.8, I'm going to be a bit frustated too ;) > > Regards; > Yann.
Re: svn commit: r1884103 - in /apr/apr/branches/1.7.x: ./ include/arch/beos/ include/arch/netware/ include/arch/unix/ include/arch/win32/ memory/unix/ threadproc/beos/ threadproc/netware/ threadproc/o
On Wed, Jun 29, 2022 at 4:47 PM William A Rowe Jr wrote: > > This change does introduce an API change, and a somewhat possibly dangerous > structure size alteration, and seems to be out of bounds for apr 1.7.x branch. AFAICT, this change only touches internal/opaque structures.. Are you talking about apr__pool_unmanage()? If so it was later removed in r1897210. About this change particularly, it fixes serious bugs in APR threads, just run the tests with r1884102 (before this commit), APR_POOL_DEBUG (to ease reproduction) and under ASan (Address Sanitizer, -fsanitize=address), or maybe it was httpd's tests. I can't remember the details and travis does not seem to keep builds history that far (Dec 2020) so I can't point you to the stack traces / reports. I do remember that period where we started to run ASan builds though (in httpd and later APR), this led to a series of fixes in APR, including this one.. > > Anyone using a build crossing runtimes between apr 1.7.0 and 1.7.1 may > experience > unintended side-effects. Anyone running apr-1.7.x without these fixes and using threads is kind of in UB land. > > While it's true that include/arch and include/private are not > considered generally > supported, folks are known to use them, and macros may cause the library to > bleed internals. Really? We don't install these headers, and they are opaque precisely so that we can change them without breaking users. If users copy private headers for their use cases, shouldn't they take the burden of maintaining their codebase (depending on that) when those headers change? This can't be on us.. By making such impracticable policies for revision changes (semver sense), we are going to simply not fix anything but trivial changes without bumping minor, and no one would/should care about the old (though still "maintained") minor versions. What's the point of releasing 1.7.1 then? Let's go with 1.8.0 only, simpler (less maintenance) and better for everybody.. Regards; Yann.
Re: svn commit: r1863234 - in /apr/apr/branches/1.7.x: ./ include/apr_pools.h memory/unix/apr_pools.c
On Wed, Jun 29, 2022 at 4:41 PM William A Rowe Jr wrote: > > This is the first commit which appeared to break APR in such a way > that code compiled > against apr 1.7.1 would be unable to run against apr 1.7.0 binaries. I think it has been reverted already by http://svn.apache.org/r1902022 Regards; Yann.
Re: Issues with shipping apr_thread_current() in APR 1.8.0 and an alternative approach for PCRE2 allocations in HTTPD
On Tue, Jun 28, 2022 at 6:08 PM Ivan Zhakov wrote: > > On Tue, 28 Jun 2022 at 00:24, Yann Ylavic wrote: > > > > Hi Ivan, > > > > On Mon, Jun 27, 2022 at 8:19 PM Ivan Zhakov wrote: > > > > > > For the longer version, let me first outline a few problems with the > > > current apr_thread_current() API: > > > > > > 1) apr_thread_current_create() leaks the memory allocated in > > > alloc_thread() after the thread exits. > > > > Hm, alloc_thread() creates an unmanaged pool (thread->pool) and > > allocates everything for the thread on that pool (including the thread > > itself). > > This pool is destroyed when the thread is joined (for attached > > threads), or for detached threads when apr_thread_exit() is called > > explicitly otherwise when the thread function actually exits. > > So which memory leaks precisely? apr_thread_current_create() for the > > process' main thread may leak the thread pool on exit because it's > > detached, it has not been apr_thread_create()d and is usually not > > apr_thread_exit()ed, but when the main thread exits (hence the > > process) nothing really leaks.. Should it matter we could also have an > > explicit apr_thread_current_unleak(). > > > > Or do you mean the apr_threadattr_t that can be passed to > > apr_thread_current_create()? That one is on the caller's > > responsibility, and it can be reused for multiple threads if needed. > > > I meant a memory leak for native threads. For example, with an MPM > that uses native Windows threadpool for worker threads. That's not mpm_winnt right? (it does not use native threads anymore and plays no apr_thread_current_create() game, it just uses apr_thread_create() so apr_thread_current() works automatically there). Probably the thread pool lets you pass the thread function? So you could "apr_thread_current_create()" when entering the function and "apr_pool_destroy(apr_thread_pool_get(mythread))" before leaving? That'd still leak if the thread calls TerminateThread() explicitly at some point, but it seems that the native Windows thread API is not very helpful for that since there is way to register a destructor for when the thread terminates by any mean (like the pthread_key_create() API's destructor for instance). If the native API does not help I don't think that APR can do much better.. Also finally, if you want to leave native threads alone (i.e. not APR-ize them), just do that, ap_regex will still work. The ap_regex code is prepared to get apr_thread_current() == NULL, because it could be called by a non-APR thread, and in this case it works exactly how you propose it should (small stack with fall back to malloc/free for PCRE1, or with PCRE2 using its native exec-context alloc/free since it's opaque so you can't put it on the stack, but that'd be for every ap_regexec() call). Note that httpd could (hypothetically) later assert/assume, for some other code, that it's never called from a non-APR thread and enforce/omit the NULL checks, that's up to a "main" program to decide since it has the full picture (httpd probably won't do that in a 2.x version since it could break third-party MPMs creating native worker threads only, but who knows if for 3.x it proves to be a nice/simpler way to handle things in core/modules, then httpd-3.x could require that the MPMs create APR worker threads only, or APR-ize them before entering core/modules processing since there is an API for that..). > > > > > > 2) apr_thread_current_create() requires passing in the apr_threadattr_t > > > value that may be unknown to the caller, so the result may not correspond > > > to the actual thread attributes. > > > > This threadattr is for the users who know (attached vs detached > > mainly), but if they don't know/care maybe they shouldn't use > > apr_thread_current_create()? > > It really matters if > > apr_thread_detach()/apr_thread_join()/apr_thread_exit() is to be > > called later anyway, but users that do this usually create their > > thread with apr_thread_create(), and for these threads > > apr_thread_current_create() is moot anyway (apr_thread_current() works > > automagically for them). > > The main usage of apr_thread_current_create() is for the main thread, > > and this one is always detached per definition. But if users know the > > nature/lifetime of the native threads they want to APR-ize, > > apr_thread_current_create() can be useful too. > > > I think the problem is that in the general case the API user still > needs to pass in the proper apr_threadattr_t to receive the proper > current apr_thread_t object (even if there's a shortcut for some > cases). This is the case
Re: svn commit: r1902297 - in /apr/apr/branches/thread-name: include/ include/arch/win32/ test/ threadproc/beos/ threadproc/netware/ threadproc/os2/ threadproc/unix/ threadproc/win32/
On Tue, Jun 28, 2022 at 9:24 PM Eric Covener wrote: > > Ran out of time but I added a crude check to the branch in 1902326. Argh, I missed your message (which showed up after I sent mine..). Well, two solutions are better than none :)
Re: svn commit: r1902297 - in /apr/apr/branches/thread-name: include/ include/arch/win32/ test/ threadproc/beos/ threadproc/netware/ threadproc/os2/ threadproc/unix/ threadproc/win32/
On Tue, Jun 28, 2022 at 8:06 PM Ivan Zhakov wrote:
>
> On Tue, 28 Jun 2022 at 14:45, Yann Ylavic wrote:
> >
> > On Tue, Jun 28, 2022 at 1:13 AM wrote:
> > >
> > > Author: ivan
> > > Date: Mon Jun 27 23:13:52 2022
> > > New Revision: 1902297
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1902297=rev
> > > Log:
> > > On 'thread-name' branch: Add apr_thread_name_get() and
> > > apr_thread_name_set()
> > > API to get/set thread name.
> > >
> > []
> > > --- apr/apr/branches/thread-name/threadproc/unix/thread.c (original)
> > > +++ apr/apr/branches/thread-name/threadproc/unix/thread.c Mon Jun 27
> > > 23:13:52 2022
> > []
> > > @@ -277,6 +282,48 @@ APR_DECLARE(apr_thread_t *) apr_thread_c
> > > #endif
> > > }
> > >
> > > +APR_DECLARE(apr_status_t) apr_thread_name_set(const char *name,
> > > + apr_thread_t *thread,
> > > + apr_pool_t *pool)
> > > +{
> > > +pthread_t td;
> > > +
> > > +size_t name_len;
> > > +if (!name) {
> > > +return APR_BADARG;
> > > +}
> > > +
> > > +if (thread) {
> > > +td = *thread->td;
> > > +}
> > > +else {
> > > +td = pthread_self();
> > > +}
> > > +
> > > +name_len = strlen(name);
> > > +if (name_len >= TASK_COMM_LEN) {
> > > +name = name + name_len - TASK_COMM_LEN + 1;
> > > +}
> > > +
> > > +return pthread_setname_np(td, name);
> >
> > We probably need to check for HAVE_PTHREAD_SETNAME_NP since it's _np
> > (non-portable).
> Thanks for the suggestion!
>
> I'm not so familiar with autoconf stuff (that's why I created branch):
> can I just check for HAVE_PTHREAD_SETNAME_NP or do I need some
> autoconf magic to set it?
Maybe something like the attached patch, and then check for
HAVE_PTHREAD_GETNAME_SETNAME_NP for both functions (which pair
anyway).
Regards;
Yann.
Index: configure.in
===
--- configure.in (revision 1902267)
+++ configure.in (working copy)
@@ -2401,11 +2401,16 @@ APR_CHECK_DEFINE(SEM_UNDO, sys/sem.h)
APR_CHECK_DEFINE_FILES(POLLIN, poll.h sys/poll.h)
if test "$threads" = "1"; then
+AC_CHECK_HEADERS([pthread.h])
APR_CHECK_DEFINE(PTHREAD_PROCESS_SHARED, pthread.h)
-AC_CHECK_FUNCS(pthread_mutex_timedlock pthread_mutexattr_setpshared)
+AC_CHECK_FUNCS(pthread_mutex_timedlock pthread_mutexattr_setpshared dnl
+ pthread_getname_np pthread_setname_np)
APR_IFALLYES(header:pthread.h func:pthread_mutex_timedlock,
have_pthread_mutex_timedlock="1", have_pthread_mutex_timedlock="0")
AC_SUBST(have_pthread_mutex_timedlock)
+APR_IFALLYES(header:pthread.h func:pthread_getname_np func:pthread_setname_np,
+ have_pthread_getname_setname_np="1", have_pthread_getname_setname_np="0")
+AC_SUBST(have_pthread_getname_setname_np)
# Some systems have setpshared and define PROCESS_SHARED, but don't
# really support PROCESS_SHARED locks. So, we must validate that we
# can go through the steps without receiving some sort of system error.
Re: svn commit: r1902312 - /apr/apr/trunk/network_io/win32/sendrecv.c
On Tue, Jun 28, 2022 at 1:53 PM Ivan Zhakov wrote: > > On Tue, 28 Jun 2022 at 14:08, wrote: > > > > Author: ylavic > > Date: Tue Jun 28 11:08:32 2022 > > New Revision: 1902312 > > > > URL: http://svn.apache.org/viewvc?rev=1902312=rev > > Log: > > apr_socket_sendv: WIN32: Limit the number of WSABUFs allocated for a single > > call. > > > > * network_io/win32/sendrecv.c(): > > Define WSABUF_ON_HEAP to 500. > > > > * network_io/win32/sendrecv.c(apr_socket_sendv): > > Do not sendv more than WSABUF_ON_HEAP WSABUFs. > > > This commit changes the behavior for blocking sockets: now > apr_socket_sendv() may result in a partial write. It's always been like that for sockets, write[v]() has non-partial guarantees on some systems only (Linux, Windows?) *and* for regular files (and, IIRC, pipes up to PIPE_BUF). There can't be any guarantee for communications with a negotiated window size or datagrams, if you try to send 10GB of data in a single call on a TCP socket it won't block until all the data have been sent (unless you tune the SND_BUF?), not to talk about UDP. > > As far I see apr_socket_sendv() does not explicitly document whether > partial writes are allowed for *blocking* sockets. Posix send() seems > to send all data [1]: > [[ > When the message does not fit into the send buffer of the socket, > send() normally blocks, unless the socket has been placed in > nonblocking I/O mode. In nonblocking mode it would fail with the error > EAGAIN or EWOULDBLOCK in this case. The select(2) call may be used to > determine when it is possible to send more data. > ]] This does not say it sends all data, whenever the send buffer gets some space again the system will send what it can and return that (as opposed to buffering the remaining data and loop). > > Also even APR code assumes that apr_socket_sendv() doesn't allow > partial writes. See apr_memcache_delete() for example: > https://demo-server.visualsvn.com/!/#asf/view/r1902313/apr/apr/trunk/memcache/apr_memcache.c?line=923 I don't think that the change breaks this case where nvec == 3 (given that WSABUF_ON_HEAP == 500). > > Not too sure that limiting the size of the malloc() is worth it in > this case. What are the reasons to limit it? On a 64bit system, iov_len is a size_t (64bit) while WSABUF->len is a DWORD (32bit), so we could create a really huge number of WSABUFs (~ 2^32) with a single iovec. So I first had: #define WSABUF_ON_HEAP (APR_SIZE_MAX / sizeof(WSABUF)) which would have limited nvec to the hard limits, and let malloc(APR_SIZE_MAX) fail for pathological cases. But I then thought someone calling apr_socket_sendv() with more than 500 iovecs (an arbitrary value admittedly, but that's 8KB for the WSABUFs already) kind of deserves implicit partial writes.. > > Another approach would be to split writes to multiple WSASend calls > when iovec count is more than WSABUF_ON_STACK, but this would break > implicit assumption that apr_socket_sendv() is atomic like writev [2]. There is no atomicity guarantee besides regular files and pipes (both probably up to PIPE_BUF only). > > PS: Do we have a test for such apr_socket_sendv() call? Looks like we don't, besides some calls in testmemcache and testredis.. > > [1] https://linux.die.net/man/2/send > [2] https://linux.die.net/man/2/writev Regards; Yann.
Re: svn commit: r1902297 - in /apr/apr/branches/thread-name: include/ include/arch/win32/ test/ threadproc/beos/ threadproc/netware/ threadproc/os2/ threadproc/unix/ threadproc/win32/
On Tue, Jun 28, 2022 at 1:13 AM wrote:
>
> Author: ivan
> Date: Mon Jun 27 23:13:52 2022
> New Revision: 1902297
>
> URL: http://svn.apache.org/viewvc?rev=1902297=rev
> Log:
> On 'thread-name' branch: Add apr_thread_name_get() and apr_thread_name_set()
> API to get/set thread name.
>
[]
> --- apr/apr/branches/thread-name/threadproc/unix/thread.c (original)
> +++ apr/apr/branches/thread-name/threadproc/unix/thread.c Mon Jun 27 23:13:52
> 2022
[]
> @@ -277,6 +282,48 @@ APR_DECLARE(apr_thread_t *) apr_thread_c
> #endif
> }
>
> +APR_DECLARE(apr_status_t) apr_thread_name_set(const char *name,
> + apr_thread_t *thread,
> + apr_pool_t *pool)
> +{
> +pthread_t td;
> +
> +size_t name_len;
> +if (!name) {
> +return APR_BADARG;
> +}
> +
> +if (thread) {
> +td = *thread->td;
> +}
> +else {
> +td = pthread_self();
> +}
> +
> +name_len = strlen(name);
> +if (name_len >= TASK_COMM_LEN) {
> +name = name + name_len - TASK_COMM_LEN + 1;
> +}
> +
> +return pthread_setname_np(td, name);
We probably need to check for HAVE_PTHREAD_SETNAME_NP since it's _np
(non-portable).
Some systems seem to implement "prctl(PR_SET_NAME, ...)" too, but
that's for pthread_self() only..
> +}
> +
> +APR_DECLARE(apr_status_t) apr_thread_name_get(char **name,
> + apr_thread_t *thread,
> + apr_pool_t *pool)
> +{
> +pthread_t td;
> +if (thread) {
> +td = *thread->td;
> +}
> +else {
> +td = pthread_self();
> +}
> +
> +*name = apr_pcalloc(pool, TASK_COMM_LEN);
> +return pthread_getname_np(td, *name, TASK_COMM_LEN);
Likewise for HAVE_PTHREAD_GETNAME_NP/prctl(PR_GET_NAME).
> +}
Re: Issues with shipping apr_thread_current() in APR 1.8.0 and an alternative approach for PCRE2 allocations in HTTPD
Hi Ivan,
On Mon, Jun 27, 2022 at 8:19 PM Ivan Zhakov wrote:
>
> For the longer version, let me first outline a few problems with the current
> apr_thread_current() API:
>
> 1) apr_thread_current_create() leaks the memory allocated in alloc_thread()
> after the thread exits.
Hm, alloc_thread() creates an unmanaged pool (thread->pool) and
allocates everything for the thread on that pool (including the thread
itself).
This pool is destroyed when the thread is joined (for attached
threads), or for detached threads when apr_thread_exit() is called
explicitly otherwise when the thread function actually exits.
So which memory leaks precisely? apr_thread_current_create() for the
process' main thread may leak the thread pool on exit because it's
detached, it has not been apr_thread_create()d and is usually not
apr_thread_exit()ed, but when the main thread exits (hence the
process) nothing really leaks.. Should it matter we could also have an
explicit apr_thread_current_unleak().
Or do you mean the apr_threadattr_t that can be passed to
apr_thread_current_create()? That one is on the caller's
responsibility, and it can be reused for multiple threads if needed.
>
> 2) apr_thread_current_create() requires passing in the apr_threadattr_t value
> that may be unknown to the caller, so the result may not correspond to the
> actual thread attributes.
This threadattr is for the users who know (attached vs detached
mainly), but if they don't know/care maybe they shouldn't use
apr_thread_current_create()?
It really matters if
apr_thread_detach()/apr_thread_join()/apr_thread_exit() is to be
called later anyway, but users that do this usually create their
thread with apr_thread_create(), and for these threads
apr_thread_current_create() is moot anyway (apr_thread_current() works
automagically for them).
The main usage of apr_thread_current_create() is for the main thread,
and this one is always detached per definition. But if users know the
nature/lifetime of the native threads they want to APR-ize,
apr_thread_current_create() can be useful too.
>
> 3) apr_thread_current() allows for situations where it can return NULL, such
> as with !APR_THREAD_LOCAL, making the API easy to misuse.
Maybe we shouldn't define apr_thread_current{,_create}() when
!APR_THREAD_LOCAL? I find it useful though to determine if the current
thread is an APR one or not.
>
> 4) apr_thread_current_after_fork() callback has to be called by the API user
> in the appropriate moments of time, allowing for a hard-to-spot API misuse.
Again, this one is a helper for native threads, if apr_fork() is used
there is nothing to be done on the user side (httpd could do that in
the unix threaded MPMs for instance to avoid the calls to
apr_thread_current_after_fork()).
>
> Since apr_thread_current() was added to implement the specific allocation
> scheme for PCRE2,
I personally find the API qui useful per se, but admittedly I'm a bit
biased too..
> I think that we could try an alternative approach for that part of the
> problem. The alternative approach would have the same characteristics as the
> approach that had been used for PCRE1:
>
> -- Supply PCRE2 with a custom context with malloc and free implementations.
> -- Those implementations would work by either using a stack buffer for small
> allocations or by doing a malloc().
> -- This allocation scheme would have the same properties as the existing
> scheme used when compiling with PCRE1.
For PCRE2, you would malloc()/free() for every ap_regexec(), which can
be quite costly depending on the configuration/modules. This work was
done precisely for this concern, for the switch from PCRE1 to PCRE2 to
be as little costly as possible. The current implementation reuses the
same PCRE2 context per thread for the lifetime of httpd..
Same for PCRE1, besides the stack buffer for small vectors (which is
still there currently btw).
>
> With that in mind, the overall plan would be as follows:
>
> A) Implement the above approach for PCRE2 allocations in httpd.
> B) Remove the apr_thread_current() and APR_THREAD_LOCAL from APR trunk and
> 1.8.x.
> C) Release APR 1.8.0.
Frankly, I find it hard to shoot down an API that works because it
could be misused when playing with native threads, whereas it works
transparently when using the APR threads directly.
Isn't somewhere:
apr_thread_data_set(my_data, my_key, NULL, apr_thread_current());
And elsewhere:
apr_thread_data_get(_data, my_key, apr_thread_current());
quite nice compared to creating a native threadkey (whose number is
usually limited) for each entry?
Regards;
Yann.
Re: svn commit: r1902285 - /apr/apr/trunk/test/testencode.c
On Mon, Jun 27, 2022 at 8:35 PM Ivan Zhakov wrote: > > On Mon, 27 Jun 2022 at 21:14, Yann Ylavic wrote: >> >> On Mon, Jun 27, 2022 at 8:08 PM Ivan Zhakov wrote: >> > >> > On Mon, 27 Jun 2022 at 21:01, wrote: >> >> >> >> +#ifdef WIN32 >> >> +typedef apr_status_t (__stdcall *encdec_fn)(char*, const char*, >> >> apr_ssize_t, int, apr_size_t*); >> >> +#else >> >> +typedef apr_status_t (*encdec_fn)(char*, const char*, apr_ssize_t, int, >> >> apr_size_t*); >> >> +#endif >> >> >> > This uses assumptions about calling convention used on the Win32 platform. >> > I don't think it's a good thing. Do we really need an array of callbacks? >> > May just inline all these calls? >> >> APR_DECLARE() seems to always __stdcall, as opposed to >> APR_DECLARE_NONSTD()'s __cdecl. >> But I barely understand these Windows subtleties, so yeah I can inline.. >> > > Yes, *currently* it is always a stdcall, but it could change so it's better > do not have such assumptions. r1902286. Thanks; Yann.
Re: svn commit: r1902285 - /apr/apr/trunk/test/testencode.c
On Mon, Jun 27, 2022 at 8:08 PM Ivan Zhakov wrote: > > On Mon, 27 Jun 2022 at 21:01, wrote: >> >> +#ifdef WIN32 >> +typedef apr_status_t (__stdcall *encdec_fn)(char*, const char*, >> apr_ssize_t, int, apr_size_t*); >> +#else >> +typedef apr_status_t (*encdec_fn)(char*, const char*, apr_ssize_t, int, >> apr_size_t*); >> +#endif >> > This uses assumptions about calling convention used on the Win32 platform. I > don't think it's a good thing. Do we really need an array of callbacks? May > just inline all these calls? APR_DECLARE() seems to always __stdcall, as opposed to APR_DECLARE_NONSTD()'s __cdecl. But I barely understand these Windows subtleties, so yeah I can inline.. Regards; Yann.
Re: svn commit: r1902281 [1/2] - in /apr/apr/trunk: encoding/apr_encode.c encoding/apr_escape.c include/apr_encode.h include/private/apr_encode_private.h test/testencode.c
On Mon, Jun 27, 2022 at 6:14 PM Yann Ylavic wrote:
>
> On Mon, Jun 27, 2022 at 5:26 PM wrote:
> >
> > Author: ylavic
> > Date: Mon Jun 27 15:26:09 2022
> > New Revision: 1902281
> >
> > URL: http://svn.apache.org/viewvc?rev=1902281=rev
> > Log:
> > encoding: Better check inputs of apr_{encode,decode}_* functions.
>
> This commit seems to cause a segfault on Windows 32bit:
> https://github.com/apache/apr/runs/7075975923?check_suite_focus=true#step:5:35
> Any stacktrace/hint someone?
Thanks Ivan, r1902283 helped ;)
>
> Regards;
> Yann.
Re: svn commit: r1902281 [1/2] - in /apr/apr/trunk: encoding/apr_encode.c encoding/apr_escape.c include/apr_encode.h include/private/apr_encode_private.h test/testencode.c
On Mon, Jun 27, 2022 at 5:26 PM wrote:
>
> Author: ylavic
> Date: Mon Jun 27 15:26:09 2022
> New Revision: 1902281
>
> URL: http://svn.apache.org/viewvc?rev=1902281=rev
> Log:
> encoding: Better check inputs of apr_{encode,decode}_* functions.
This commit seems to cause a segfault on Windows 32bit:
https://github.com/apache/apr/runs/7075975923?check_suite_focus=true#step:5:35
Any stacktrace/hint someone?
Regards;
Yann.
Re: svn commit: r1902219 - in /apr/apr/branches/1.8.x: ./ test/testlock.c
On Sat, Jun 25, 2022 at 6:34 PM Christophe JAILLET wrote: > > Le 24/06/2022 à 11:06, yla...@apache.org a écrit : > > > > --- apr/apr/branches/1.8.x/test/testlock.c (original) > > +++ apr/apr/branches/1.8.x/test/testlock.c Fri Jun 24 09:06:05 2022 > > @@ -111,6 +111,7 @@ static void *APR_THREAD_FUNC thread_mute > > break; > > } > > return NULL; > > +return NULL; > > ??? Just in case :) Fixed in r1902254, thanks. Regards; Yann.
apr_atomic_casptr2() in apr-1.8.x ?
Daniel proposed in [1] that we introduce apr_atomic_casptr2() to
resolve the defect in the apr_atomic_casptr() API.
Namely the correct:
1. APR_DECLARE(void*) apr_atomic_casptr(void *volatile *mem, ...);
Versus the broken:
2. APR_DECLARE(void*) apr_atomic_casptr(volatile void **mem, ...);
The problem with 2 is that it does not prevent the compiler from
caching *mem into a local variable, which would break atomicity.
This is fixed in trunk already where we could change the API to 1, but
not in 1.x.
Anyone opposed to the attached patch (for 1.8.x only then)?
Regards;
Yann.
[1] https://bz.apache.org/bugzilla/show_bug.cgi?id=50731#c2
Index: atomic/netware/apr_atomic.c
===
--- atomic/netware/apr_atomic.c (revision 1902233)
+++ atomic/netware/apr_atomic.c (working copy)
@@ -72,6 +72,11 @@ APR_DECLARE(void *) apr_atomic_casptr(volatile voi
return (void*)atomic_cmpxchg((unsigned long *)mem,(unsigned long)cmp,(unsigned long)with);
}
+APR_DECLARE(void *) apr_atomic_casptr2(void *volatile *mem, void *with, const void *cmp)
+{
+return (void*)atomic_cmpxchg((unsigned long *)mem,(unsigned long)cmp,(unsigned long)with);
+}
+
APR_DECLARE(void*) apr_atomic_xchgptr(volatile void **mem, void *with)
{
return (void*)atomic_xchg((unsigned long *)mem,(unsigned long)with);
Index: atomic/os390/atomic.c
===
--- atomic/os390/atomic.c (revision 1902233)
+++ atomic/os390/atomic.c (working copy)
@@ -94,6 +94,15 @@ void *apr_atomic_casptr(volatile void **mem_ptr,
_ptr);
return (void *)cmp_ptr;
}
+void *apr_atomic_casptr2(void *volatile *mem_ptr,
+ void *swap_ptr,
+ const void *cmp_ptr)
+{
+ __cs1(_ptr, /* automatically updated from mem on __cs1 failure */
+ mem_ptr, /* set from swap when __cs1 succeeds*/
+ _ptr);
+ return (void *)cmp_ptr;
+}
#elif APR_SIZEOF_VOIDP == 8
void *apr_atomic_casptr(volatile void **mem_ptr,
void *swap_ptr,
@@ -104,6 +113,15 @@ void *apr_atomic_casptr(volatile void **mem_ptr,
_ptr);
return (void *)cmp_ptr;
}
+void *apr_atomic_casptr2(void *volatile *mem_ptr,
+ void *swap_ptr,
+ const void *cmp_ptr)
+{
+ __csg(_ptr, /* automatically updated from mem on __csg failure */
+ mem_ptr, /* set from swap when __csg succeeds*/
+ _ptr);
+ return (void *)cmp_ptr;
+}
#else
#error APR_SIZEOF_VOIDP value not supported
#endif /* APR_SIZEOF_VOIDP */
Index: atomic/unix/builtins.c
===
--- atomic/unix/builtins.c (revision 1902234)
+++ atomic/unix/builtins.c (working copy)
@@ -121,6 +121,16 @@ APR_DECLARE(void*) apr_atomic_casptr(volatile void
#endif
}
+APR_DECLARE(void*) apr_atomic_casptr2(void *volatile *mem, void *ptr, const void *cmp)
+{
+#if HAVE__ATOMIC_BUILTINS
+__atomic_compare_exchange_n(mem, (void *), ptr, 0, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);
+return (void *)cmp;
+#else
+return (void *)__sync_val_compare_and_swap(mem, (void *)cmp, ptr);
+#endif
+}
+
APR_DECLARE(void*) apr_atomic_xchgptr(volatile void **mem, void *ptr)
{
#if HAVE__ATOMIC_BUILTINS
Index: atomic/unix/ia32.c
===
--- atomic/unix/ia32.c (revision 1902233)
+++ atomic/unix/ia32.c (working copy)
@@ -111,6 +111,24 @@ APR_DECLARE(void*) apr_atomic_casptr(volatile void
return prev;
}
+APR_DECLARE(void*) apr_atomic_casptr2(void *volatile *mem, void *with, const void *cmp)
+{
+void *prev;
+#if APR_SIZEOF_VOIDP == 4
+asm volatile ("lock; cmpxchgl %2, %1"
+ : "=a" (prev), "=m" (*mem)
+ : "r" (with), "m" (*mem), "0" (cmp));
+#elif APR_SIZEOF_VOIDP == 8
+asm volatile ("lock; cmpxchgq %q2, %1"
+ : "=a" (prev), "=m" (*mem)
+ : "r" ((unsigned long)with), "m" (*mem),
+"0" ((unsigned long)cmp));
+#else
+#error APR_SIZEOF_VOIDP value not supported
+#endif
+return prev;
+}
+
APR_DECLARE(void*) apr_atomic_xchgptr(volatile void **mem, void *with)
{
void *prev;
Index: atomic/unix/mutex.c
===
--- atomic/unix/mutex.c (revision 1902233)
+++ atomic/unix/mutex.c (working copy)
@@ -190,6 +190,21 @@ APR_DECLARE(void*) apr_atomic_casptr(volatile void
return prev;
}
+APR_DECLARE(void*) apr_atomic_casptr(void *volatile *mem, void *with, const void *cmp)
+{
+void *prev;
+DECLARE_MUTEX_LOCKED(mutex, *mem);
+
+prev = *(void **)mem;
+if (prev == cmp) {
+*mem = with;
+}
+
+MUTEX_UNLOCK(mutex);
+
+return prev;
+}
+
APR_DECLARE(void*) apr_atomic_xchgptr(volatile void **mem, void *with)
{
void *prev;
Re: GitHub Actions CI
On Wed, Jun 22, 2022 at 2:26 PM Ivan Zhakov via dev wrote: > > Suggestions and improvements are welcome. I don't see any warning left in trunk and 1.8.x, possibly we'd have a Windows /Werror build (or whatever appropriate msvc toggle is) to better check the ones that would be introduced? Regards; Yann.
Re: svn commit: r1902206 - /apr/apr/trunk/encoding/apr_base64.c
On Fri, Jun 24, 2022 at 11:56 AM Yann Ylavic wrote:
>
> On Thu, Jun 23, 2022 at 8:50 PM Ruediger Pluem wrote:
> >
> > On 6/23/22 5:12 PM, yla...@apache.org wrote:
> > >
> > > +/* Above APR_BASE64_ENCODE_MAX length the encoding can't fit in an int
> > > >= 0 */
> > > +#define APR_BASE64_ENCODE_MAX 1610612733
> > > +
> > > +/* Above APR_BASE64_DECODE_MAX length the decoding can't fit in an int
> > > >= 0 */
> > > +#define APR_BASE64_DECODE_MAX 2863311524u
> > > +
> >
> > Doesn't this depend on the storage size of int on the respective
> > architecture and thus
> > should be derived from INT_MAX?
>
> There is no APR_INT_MAX unfortunately
By the way, I don't see much value in the
APR_{U,}INT{8,16,32,64}_{MIN,MAX} definitions we have, those values
have well known min/max (i.e. fixed).
I'd rather we define APR_{U,}{CHAR,SHORT,INT,LONG,LONGLONG}_MAX ones,
but without C99 (stdint.h, though most if not all platforms should
have this header now) it may be tricky (if not impossible) to define
them portably..
>
> Regards;
> Yann.
Re: svn commit: r1902206 - /apr/apr/trunk/encoding/apr_base64.c
On Thu, Jun 23, 2022 at 8:50 PM Ruediger Pluem wrote: > > On 6/23/22 5:12 PM, yla...@apache.org wrote: > > > > +/* Above APR_BASE64_ENCODE_MAX length the encoding can't fit in an int >= > > 0 */ > > +#define APR_BASE64_ENCODE_MAX 1610612733 > > + > > +/* Above APR_BASE64_DECODE_MAX length the decoding can't fit in an int >= > > 0 */ > > +#define APR_BASE64_DECODE_MAX 2863311524u > > + > > Doesn't this depend on the storage size of int on the respective architecture > and thus > should be derived from INT_MAX? There is no APR_INT_MAX unfortunately, I could do something like: #if APR_HAVE_STDINT_H /* C99, but maintainer-mode is C89 */ #include #define APR_BASE64_LEN_MAX INT_MAX #else #define APR_BASE64_LEN_MAX APR_INT32_MAX #endif and use APR_BASE64_LEN_MAX instead of APR_INT32_MAX here and there, but I doubt there are any architectures (we care about) where sizeof(int) != 4. I don't think we support < 32bit archs, do we? For >= 32bit archs, of the 5 data models (LP32, ILP32, ILP64, LLP64 and LP64), only LP32 (i.e. WIN16 API, Apple Macintosh) and ILP64 ([1] mentions HAL Computer Systems port of Solaris to the SPARC64) don't have 32bit ints, and I don't think we care about those either. So we should be safe assuming ints are 32bit? Regards; Yann. [1] https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models
Re: svn commit: r1902196 - /apr/apr/trunk/json/apr_json_decode.c
On Thu, Jun 23, 2022 at 5:57 PM Ruediger Pluem wrote:
>
> On 6/23/22 1:43 PM, yla...@apache.org wrote:
> >
> > --- apr/apr/trunk/json/apr_json_decode.c (original)
> > +++ apr/apr/trunk/json/apr_json_decode.c Thu Jun 23 11:43:05 2022
> > @@ -353,7 +353,7 @@ static apr_status_t apr_json_decode_arra
> > apr_json_value_t * array)
> > {
> > apr_status_t status = APR_SUCCESS;
> > -apr_size_t count = 0;
> > +int count = 0;
>
> Wouldn't we need to guard against overflow then?
Probably yes, r1902207.
Thanks for the review;
Yann.
Re: GitHub Actions CI
On Thu, Jun 23, 2022 at 1:55 AM Eric Covener wrote: > > On Wed, Jun 22, 2022 at 7:53 PM Yann Ylavic wrote: > > > > On Thu, Jun 23, 2022 at 1:14 AM Eric Covener wrote: > > > > > > On Wed, Jun 22, 2022 at 10:30 AM Yann Ylavic wrote: > > > > > > > > On Wed, Jun 22, 2022 at 2:26 PM Ivan Zhakov via dev > > > > wrote: > > > > > > > > > > As far I understand Apache Infra recommends to migrate Continuous > > > > > Integration builds from Travis to GitHub Actions. > > > > > > > > > > I've added initial configuration for GitHub Actions for APR project: > > > > > https://svn.apache.org/repos/asf/apr/apr/trunk/.github/workflows/ > > > > > > > > > > GitHub supports Linux, macOS and Windows runners. > > > > > > > > Nice work Ivan, thanks a bunch! > > > > > > > > Regards; > > > > Yann. > > > > > > > > PS: now the macos guys have some work > > > > (https://github.com/apache/apr/runs/7004338300?check_suite_focus=true) > > > > > > I think on mac we do not have xml2-config and build/xml.m4 does not > > > actually set xml2_LIBS to anything when xml2 is auto-detected. > > > I think a similar path is wrong when --with-xml2 passes a dir instead > > > of a path to xml2-config. > > > > > > I am testing this which seemed to just work for the no xml2 args at > > > all case w/ no xml2-config: > > > > Looks good to me, though I can't test on macos.. > > > > apr_file_datasync() is failing in test_datasync_on_stream() with errno=45: > > https://github.com/apache/apr/runs/7014421531?check_suite_focus=true#step:6:297 > > We already ignore EINVAL which is returned by Linux (meaning ENOTIMPL > > there IIRC), any idea what errno=45 is on macos? > > ENOTSUP > 45: Operation not supported Thanks, looks like we can ignore that one too (r1902180).
Re: GitHub Actions CI
On Thu, Jun 23, 2022 at 1:14 AM Eric Covener wrote: > > On Wed, Jun 22, 2022 at 10:30 AM Yann Ylavic wrote: > > > > On Wed, Jun 22, 2022 at 2:26 PM Ivan Zhakov via dev > > wrote: > > > > > > As far I understand Apache Infra recommends to migrate Continuous > > > Integration builds from Travis to GitHub Actions. > > > > > > I've added initial configuration for GitHub Actions for APR project: > > > https://svn.apache.org/repos/asf/apr/apr/trunk/.github/workflows/ > > > > > > GitHub supports Linux, macOS and Windows runners. > > > > Nice work Ivan, thanks a bunch! > > > > Regards; > > Yann. > > > > PS: now the macos guys have some work > > (https://github.com/apache/apr/runs/7004338300?check_suite_focus=true) > > I think on mac we do not have xml2-config and build/xml.m4 does not > actually set xml2_LIBS to anything when xml2 is auto-detected. > I think a similar path is wrong when --with-xml2 passes a dir instead > of a path to xml2-config. > > I am testing this which seemed to just work for the no xml2 args at > all case w/ no xml2-config: Looks good to me, though I can't test on macos.. apr_file_datasync() is failing in test_datasync_on_stream() with errno=45: https://github.com/apache/apr/runs/7014421531?check_suite_focus=true#step:6:297 We already ignore EINVAL which is returned by Linux (meaning ENOTIMPL there IIRC), any idea what errno=45 is on macos? Regards; Yann.
Re: Spurious testreslist failures on Windows (was Re: GitHub Actions CI)
On Wed, Jun 22, 2022 at 5:57 PM Ivan Zhakov wrote: > > On Wed, 22 Jun 2022 at 17:29, Yann Ylavic wrote: > >> >> PS: now the macos guys have some work >> (https://github.com/apache/apr/runs/7004338300?check_suite_focus=true) >> :) > > > Also testreslist fails sometimes on Windows: > https://github.com/apache/apr/runs/7003387964?check_suite_focus=true > [[[ > 48/77 Test #48: testreslist ..***Failed 1.05 sec > testreslist : Line 258: expected <10>, but saw <11> > Line 258: expected <10>, but saw <12> > FAILED 2 of 3 > Failed Tests Total Fail Failed % > === > testreslist 3 2 66.67% > ]]] > > Sometimes it works as expected: > https://github.com/apache/apr/actions/runs/2542369041 > > Anyone have any ideas why it may happen? Possibly fixed in r1902169. AIUI it was apr_sleep(RESLIST_TTL / RESLIST_HMAX), i.e. apr_sleep(17) thus Sleep(0) on Windows, which is not enough for the reslist entries to expire. By rounding up to Sleep(1) instead, the last build passed, we'll see the next ones.. unless you can reproduce the failure locally with r1902169 already? Regards; Yann. > > -- > Ivan Zhakov
Re: GitHub Actions CI
On Wed, Jun 22, 2022 at 2:26 PM Ivan Zhakov via dev wrote: > > As far I understand Apache Infra recommends to migrate Continuous Integration > builds from Travis to GitHub Actions. > > I've added initial configuration for GitHub Actions for APR project: > https://svn.apache.org/repos/asf/apr/apr/trunk/.github/workflows/ > > GitHub supports Linux, macOS and Windows runners. Nice work Ivan, thanks a bunch! Regards; Yann. PS: now the macos guys have some work (https://github.com/apache/apr/runs/7004338300?check_suite_focus=true) :)
Re: svn commit: r1902143 - /apr/apr/trunk/.github/workflows/linux.yml
On Wed, Jun 22, 2022 at 12:35 AM Ivan Zhakov wrote: > > I see two runs for every commit, but one is for Windows and another is for > Linux. Do I miss something? No, brain fart all along on my side.. Regards; Yann.
Re: svn commit: r1902143 - /apr/apr/trunk/.github/workflows/linux.yml
On Tue, Jun 21, 2022 at 6:21 PM Ivan Zhakov wrote: > > On Tue, 21 Jun 2022 at 19:16, Yann Ylavic wrote: >> >> On Tue, Jun 21, 2022 at 5:58 PM Ivan Zhakov wrote: >> > >> > On Tue, 21 Jun 2022 at 18:33, Yann Ylavic wrote: >> >> >> >> Hi Ivan, >> >> >> >> I'm responding randomly to one of your ci commits, first to say >> >> thanks, really nice and valuable work! >> >> >> >> I have a question too :) Sorry if it's premature, but I'm also >> >> wondering why we don't see the outputs of the jobs in the github UI, >> >> for instance in [1] for both succeeding and failing jobs the details >> >> of the steps shows "Error:" only without more output (i.e. something >> >> that could help us fix errors). >> >> Do you know if there is a way to show the stdout/stderr of the >> >> commands/scripts instead? >> >> >> > I see error output in testlog: >> >> Ah yes, I actually expected the outputs to be viewable by developing >> the failing step (with the arrow), but the logs are actually available >> via "View raw logs" in the configuration like icon.. >> > I see outputs when click on the failing step. See screenshot in attachement. Argh, sorry for the noise and your time, the blame is on a javascript filter on my firefox :/ Well, I'm hesitating for my second remark now, but I'm going for it anyway :p It seems that setting both branches like you did in this commit (r1902143): -branches: [ "trunk" ] +branches: [ "trunk", "1.8.x" ] is causing two runs to be launched by gh actions whenever either apr-trunk or apr-1.8.x is modified. Don't we want "trunk" only in "trunk/.github/workflows/linux.yml" and "1.8.x" only in "1.8.x/.github/workflows/linux.yml" instead? I'm double checking but https://github.com/apache/apr/actions really seem to show this behaviour, and somehow one of the build is failing.. Regards; Yann.
Re: svn commit: r1902143 - /apr/apr/trunk/.github/workflows/linux.yml
On Tue, Jun 21, 2022 at 5:58 PM Ivan Zhakov wrote: > > On Tue, 21 Jun 2022 at 18:33, Yann Ylavic wrote: >> >> Hi Ivan, >> >> I'm responding randomly to one of your ci commits, first to say >> thanks, really nice and valuable work! >> >> I have a question too :) Sorry if it's premature, but I'm also >> wondering why we don't see the outputs of the jobs in the github UI, >> for instance in [1] for both succeeding and failing jobs the details >> of the steps shows "Error:" only without more output (i.e. something >> that could help us fix errors). >> Do you know if there is a way to show the stdout/stderr of the >> commands/scripts instead? >> > I see error output in testlog: Ah yes, I actually expected the outputs to be viewable by developing the failing step (with the arrow), but the logs are actually available via "View raw logs" in the configuration like icon.. Thanks!
Re: svn commit: r1902143 - /apr/apr/trunk/.github/workflows/linux.yml
Hi Ivan, I'm responding randomly to one of your ci commits, first to say thanks, really nice and valuable work! I have a question too :) Sorry if it's premature, but I'm also wondering why we don't see the outputs of the jobs in the github UI, for instance in [1] for both succeeding and failing jobs the details of the steps shows "Error:" only without more output (i.e. something that could help us fix errors). Do you know if there is a way to show the stdout/stderr of the commands/scripts instead? Regards; Yann. [1] https://github.com/apache/apr/runs/6982048399?check_suite_focus=true
Re: svn commit: r1902120 - in /apr/apr/branches/1.8.x: ./ test/testfile.c
On Mon, Jun 20, 2022 at 11:57 PM wrote: > > Author: ivan > Date: Mon Jun 20 21:57:13 2022 > New Revision: 1902120 > > URL: http://svn.apache.org/viewvc?rev=1902120=rev > [] > --- apr/apr/branches/1.8.x/test/testfile.c (original) > +++ apr/apr/branches/1.8.x/test/testfile.c Mon Jun 20 21:57:13 2022 [] > @@ -1298,8 +1366,7 @@ abts_suite *testfile(abts_suite *suite) > abts_run_test(suite, test_fail_read_flush, NULL); > abts_run_test(suite, test_buffer_set_get, NULL); > abts_run_test(suite, test_xthread, NULL); > -abts_run_test(suite, test_datasync_on_file, NULL); > -abts_run_test(suite, test_datasync_on_stream, NULL); Were these two removed intentionally? This causes the below warnings at least currently: testfile.c:1243:13: error: ‘test_datasync_on_stream’ defined but not used [-Werror=unused-function] testfile.c:1222:13: error: ‘test_datasync_on_file’ defined but not used [-Werror=unused-function] > +abts_run_test(suite, test_append, NULL); > > return suite; > } Regards; Yann.
Re: Need Pointers on Cores.
Hi, On Mon, Mar 28, 2022 at 2:50 PM Ramapandi Muniyasamy -X (ramapmun - WIPRO LIMITED at Cisco) wrote: > > I am getting a crash when I am newly migrating to openssl 1.1.1l with version > 2.4.39 You need to re-build httpd with the new openssl (./configure --with-openssl=/path/to/openssl 1.1.1l ...), and verify that httpd is linked to the new openssl at runtime (`ldd modules/mod_ssl.so` should show the new paths for openssl, and/or LD_LIBRARY_PATH should be used). Regards; Yann.
Re: svn commit: r1897207 - in /apr/apr/trunk: include/apr_thread_proc.h threadproc/beos/thread.c threadproc/netware/thread.c threadproc/os2/thread.c threadproc/unix/thread.c threadproc/win32/thread.c
On Tue, Feb 8, 2022 at 7:56 PM Evgeny Kotkov
wrote:
>
> Yann Ylavic writes:
>
> > +APR_DECLARE(apr_status_t) apr_thread_current_create(apr_thread_t **current,
> > +apr_threadattr_t *attr,
> > +apr_pool_t *pool)
> > +{
> > +apr_status_t stat;
> > +
> > +*current = apr_thread_current();
> > +if (*current) {
> > +return APR_EEXIST;
> > +}
> > +
> > +stat = alloc_thread(current, attr, NULL, NULL, pool);
> > +if (stat != APR_SUCCESS) {
> > +return stat;
> > +}
> > +
> > +if (!(attr && attr->detach)) {
> > +(*new)->td = apr_os_thread_current();
> > +}
>
> Hi Yann,
>
> It looks like this hunk doesn't compile on Windows:
>
> …\threadproc\win32\thread.c(193): error C2065: 'new': undeclared identifier
> …\threadproc\win32\thread.c(193): error C2100: illegal indirection
> …\threadproc\win32\thread.c(193): error C2223: left of '->td' must point
> to struct/union
Thanks, hopefully fixed in r1897879.
Regards;
Yann.
Re: svn commit: r1897209 - in /apr/apr/branches/1.7.x: ./ include/apr_pools.h memory/unix/apr_pools.c
On Thu, Jan 20, 2022 at 4:37 PM Yann Ylavic wrote: > > On Thu, Jan 20, 2022 at 3:56 PM William A Rowe Jr wrote: > > > > On Thu, Jan 20, 2022 at 8:50 AM Eric Covener wrote: > > > > > > > Code that will compile on 1.7.1 release > > > > still won't compile on 1.7.0, unless I misunderstand something. > > > > > > Is it a requirement in this direction? > > > https://apr.apache.org/versioning.html says "later versions" > > > > That's why I raise the question, I don't have the authoritative answer. > > > > Our SVN participants who adopted and recommended strongly that > > APR follow the semantic versioning [1] approach so they could -also- > > adopt it probably feel more strongly about this. > > It was proposed in another thread to add the APU macros to 1.7.x to > ease later migration to 2.x, I don't think that adding these apr_pool_ > macros can have more compat issues.. My bad, re-reading the thread the proposal was a compat apu_ -> apr_ layer in APR *util* 1.7.x, yet since apr-util 1.7.0 is released already it would be the same kind of compat issue. > > > Cheers; > Yann.
Re: svn commit: r1897209 - in /apr/apr/branches/1.7.x: ./ include/apr_pools.h memory/unix/apr_pools.c
On Thu, Jan 20, 2022 at 3:56 PM William A Rowe Jr wrote: > > On Thu, Jan 20, 2022 at 8:50 AM Eric Covener wrote: > > > > > Code that will compile on 1.7.1 release > > > still won't compile on 1.7.0, unless I misunderstand something. > > > > Is it a requirement in this direction? > > https://apr.apache.org/versioning.html says "later versions" > > That's why I raise the question, I don't have the authoritative answer. > > Our SVN participants who adopted and recommended strongly that > APR follow the semantic versioning [1] approach so they could -also- > adopt it probably feel more strongly about this. It was proposed in another thread to add the APU macros to 1.7.x to ease later migration to 2.x, I don't think that adding these apr_pool_ macros can have more compat issues.. Cheers; Yann.
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
On Thu, Jan 13, 2022 at 7:04 PM Ivan Zhakov wrote: > > On Fri, 7 Jan 2022 at 17:33, Yann Ylavic wrote: > > > > I think that most if not all of the changes to 1.7.x since 1.7.0 are > > fixes for bugs that were there before 1.7 already, not regressions > > introduced by 1.7.0. > > Agreed on the bugfix/regressions part. I have misunderstood that > r1896510 is a bugfix, perhaps, due to its size, and was thinking that > it adds new functionality. So after reverting r1896510, the reported bug in [1] is still addressed by r1894916, but only on unixes then. [1] https://lists.apache.org/thread/mgosgwpqpxqrhh1q06z9okqgfqr46q24
Re: svn commit: r1897021 - /apr/apr/branches/1.8.x/
On Fri, Jan 14, 2022 at 1:59 AM William A Rowe Jr wrote: > > A quick review of nm against libapr-1.so between 1.7.0 and 1.7.x > indicates that 1.7.1 > isn't releasable as-is. Any hints? > > +T apr_pool_find > +T apr_pool_join > +T apr_pool_lock > +T apr_pool_num_bytes These should have disappeared with r1897209. > +T apr__pool_unmanage And this one with r1897210. Cheers; Yann.
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
On Fri, Jan 14, 2022 at 4:20 PM Ivan Zhakov wrote: > > 1. Revert this change from 1.7.x Done in r1897222. Regards; Yann.
Re: svn commit: r1897208 - in /apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation: file_io/win32/ include/arch/unix/ include/arch/win32/ poll/unix/
On Wed, Jan 19, 2022 at 5:42 PM wrote:
>
> Author: ivan
> Date: Wed Jan 19 16:41:59 2022
> New Revision: 1897208
>
> URL: http://svn.apache.org/viewvc?rev=1897208=rev
> Log:
> On 'win32-pollset-wakeup-no-file-socket-emulation' branch:
>
> Windows: For the pollset wakeup, use apr_socket_t directly instead of using a
> socket disguised as an apr_file_t.
[]
>
> -apr_status_t apr_file_socket_pipe_create(apr_file_t **in,
> - apr_file_t **out,
> +apr_status_t apr_file_socket_pipe_create(apr_socket_t **in,
> + apr_socket_t **out,
> apr_pool_t *p)
> {
> apr_status_t rv;
> SOCKET rd;
> SOCKET wr;
>
> +*in = NULL;
> +*out = NULL;
> +
> if ((rv = create_socket_pipe(, )) != APR_SUCCESS) {
> return rv;
> }
> -(*in) = (apr_file_t *)apr_pcalloc(p, sizeof(apr_file_t));
> -(*in)->pool = p;
> -(*in)->fname = NULL;
> -(*in)->ftype = APR_FILETYPE_SOCKET;
> -(*in)->timeout = 0; /* read end of the pipe is non-blocking */
> -(*in)->ungetchar = -1;
> -(*in)->eof_hit = 0;
> -(*in)->filePtr = 0;
> -(*in)->bufpos = 0;
> -(*in)->dataRead = 0;
> -(*in)->direction = 0;
> -(*in)->pOverlapped = NULL;
> -(*in)->filehand = (HANDLE)rd;
> -
> -(*out) = (apr_file_t *)apr_pcalloc(p, sizeof(apr_file_t));
> -(*out)->pool = p;
> -(*out)->fname = NULL;
> -(*out)->ftype = APR_FILETYPE_SOCKET;
> -(*out)->timeout = -1;
> -(*out)->ungetchar = -1;
> -(*out)->eof_hit = 0;
> -(*out)->filePtr = 0;
> -(*out)->bufpos = 0;
> -(*out)->dataRead = 0;
> -(*out)->direction = 0;
> -(*out)->pOverlapped = NULL;
> -(*out)->filehand = (HANDLE)wr;
> +apr_os_sock_put(in, , p);
> +apr_os_sock_put(out, , p);
I think that the *in apr_socket and the underlying socket descriptor
are not aligned w.r.t. non-blocking.
We probably need to call apr_socket_timeout_set(*in, 0) here to make
that happen, thus remove the last ioctlsocket() in
create_socket_pipe() that sets the descriptor non-blocking before
leaving (which would be useless now).
Something like the below on top of your branch?
Index: file_io/win32/pipe.c
===
--- file_io/win32/pipe.c(revision 1897212)
+++ file_io/win32/pipe.c(working copy)
@@ -381,14 +381,7 @@ static apr_status_t create_socket_pipe(SOCKET *rd,
goto cleanup;
}
if (nrd == (int)sizeof(uid) && memcmp(iid, uid, sizeof(uid)) == 0) {
-/* Got the right identifier, put the poll()able read side of
- * the pipe in nonblocking mode and return.
- */
-bm = 1;
-if (ioctlsocket(*rd, FIONBIO, ) == SOCKET_ERROR) {
-rv = apr_get_netos_error();
-goto cleanup;
-}
+/* Got the right identifier, return. */
break;
}
closesocket(*rd);
@@ -438,6 +431,9 @@ apr_status_t apr_file_socket_pipe_create(apr_socke
apr_os_sock_put(in, , p);
apr_os_sock_put(out, , p);
+/* read end of the pipe is non-blocking */
+apr_socket_timeout_set(*in, 0);
+
apr_pool_cleanup_register(p, (void *)(*in), socket_pipe_cleanup,
apr_pool_cleanup_null);
apr_pool_cleanup_register(p, (void *)(*out), socket_pipe_cleanup,
--
Regards;
Yann.
Re: svn commit: r1895508 - in /apr/apr/trunk: dso/win32/ file_io/win32/ locks/win32/ misc/unix/ misc/win32/ network_io/unix/ network_io/win32/ passwd/ shmem/win32/ threadproc/win32/ time/win32/ user/w
On Thu, Dec 2, 2021 at 10:21 PM wrote:
>
> Author: mturk
> Date: Thu Dec 2 21:21:18 2021
> New Revision: 1895508
>
> URL: http://svn.apache.org/viewvc?rev=1895508=rev
> Log:
> Stage 3 in dismantling _WIN32_WCE ... cleanup code
>
[]
> Modified: apr/apr/trunk/threadproc/win32/thread.c
> URL:
> http://svn.apache.org/viewvc/apr/apr/trunk/threadproc/win32/thread.c?rev=1895508=1895507=1895508=diff
> ==
> --- apr/apr/trunk/threadproc/win32/thread.c (original)
> +++ apr/apr/trunk/threadproc/win32/thread.c Thu Dec 2 21:21:18 2021
> @@ -138,21 +138,12 @@ APR_DECLARE(apr_status_t) apr_thread_cre
> /* Use 0 for default Thread Stack Size, because that will
> * default the stack to the same size as the calling thread.
> */
> -#ifndef _WIN32_WCE
> if ((handle = (HANDLE)_beginthreadex(NULL,
> (DWORD) (attr ? attr->stacksize : 0),
> (unsigned int (APR_THREAD_FUNC *)(void
> *))dummy_worker,
> (*new), 0, )) == 0) {
> return APR_FROM_OS_ERROR(_doserrno);
> }
> -#else
> - if ((handle = CreateThread(NULL,
> -attr && attr->stacksize > 0 ? attr->stacksize : 0,
> -(unsigned int (APR_THREAD_FUNC *)(void
> *))dummy_worker,
> -(*new), 0, )) == 0) {
> -return apr_get_os_error();
> -}
> -#endif
I'm a bit surprised that _WIN32_WCE used CreateThread() while more
recent ones use _beginthreadex(), shouldn't it be the opposite?
CreateThread() aligns more with the usual/modern naming convention on Windows..
Regards;
Yann.
Re: apr/trunk netware and os2
On Tue, Jan 18, 2022 at 2:08 AM Yann Ylavic wrote: > > On Tue, Nov 30, 2021 at 11:46 PM Mladen Turk wrote: > > > > According to Wikipedia NetWare was discontinued in 2009 > > and OS/2 in 2001, so if anyone can explain why we have > > those in trunk/apr-2 (that will be hopefully released one day). > > What about BeOS? We have some specifics for that too. Ditto for os390, also present in include/arch. > > Regards; > Yann.
Re: apr/trunk netware and os2
On Tue, Nov 30, 2021 at 11:46 PM Mladen Turk wrote: > > According to Wikipedia NetWare was discontinued in 2009 > and OS/2 in 2001, so if anyone can explain why we have > those in trunk/apr-2 (that will be hopefully released one day). What about BeOS? We have some specifics for that too. Regards; Yann.
Re: svn commit: r1897021 - /apr/apr/branches/1.8.x/
On Sat, Jan 15, 2022 at 2:00 AM William A Rowe Jr wrote: > > On Fri, Jan 14, 2022 at 5:07 AM Yann Ylavic wrote: > > > > On Fri, Jan 14, 2022 at 1:59 AM William A Rowe Jr wrote: > > > > > > A quick review of nm against libapr-1.so between 1.7.0 and 1.7.x > > > indicates that 1.7.1 > > > isn't releasable as-is. Any hints? > > > > > > +T apr_pool_find > > > +T apr_pool_join > > > +T apr_pool_lock > > > +T apr_pool_num_bytes > > > > Those come from r1863234 [1], possibly we can let them as empty macros > > in 1.7.x and still add the ones for apr_pool_join() and > > apr_pool_num_bytes() which were missing for !APR_POOL_DEBUG? > > Yet it might make apps using them from 1.7.1 (w/o #ifdef > > APR_POOL_DEBUG) not compilable with 1.7.0 (though still ABI compatible > > with all 1.7.x), but no one would be able to use them as function > > pointers for instance, which would be a ABI/runtime breakage. Could > > that hurt actually? > > Worse, a macro -> function without a major bump? That's awkward. Remember > the .so remains loadable throughout apr-1.so lifespan. Yes I see your point for macro -> function. > > Afraid this has to be rejected outright on apr-1.7. There are contracts with > our > developers that they can load the 1.7.0 .so in place of a binary built > against 1.7.2 > and **nothing bad can happen** (other than the 'whoops - you wanted > that bug fix.) > The binaries won't fail to load. This sets us up for failure, and why put > macro > stubs here if they will be no-ops? What purpose is this serving to break what > has been a pretty reliable solution in developers' hands? What I'm thinking is to revert r1863234 in 1.7.x, thus back to r1863233's apr_pools.h code do: Index: include/apr_pools.h === --- include/apr_pools.h(revision 1863233) +++ include/apr_pools.h(working copy) @@ -799,6 +799,16 @@ APR_DECLARE(void) apr_pool_lock(apr_pool_t *pool, #endif #define apr_pool_join(a,b) +#ifdef apr_pool_find +#undef apr_pool_find +#endif +#define apr_pool_find(mem) (NULL) + +#ifdef apr_pool_num_bytes +#undef apr_pool_num_bytes +#endif +#define apr_pool_num_bytes(p, rec) ((apr_size_t)0) + #ifdef apr_pool_lock #undef apr_pool_lock #endif -- So yes, that's two new macros so any (new) code using them when !APR_POOL_DEBUG would build with 1.7.1 but not 1.7.0, though it would _run_ with both apr-1.7.[01].so (no ABI change). > > See https://apr.apache.org/versioning.html > > What's the actual reservation about 1.8.0? (After a prompt 1.7.1)? No reservation if we are able to release faster than we used to lately.. I also advocate for people who work on the APR code, for their changes to be integrated (as much/reasonable as possible) in the version they actually use. > > > > > +T apr__pool_unmanage > > > > That one is for internal use only (from [2]), not APR_DECLAREd so > > should be fine? > > Still, exported on linux, curious how to avoid that. Even as an > internal, not an issue > in 1.8.0, an issue on 1.7.x branch now. That's no different than other arch/private yet exported functions, but yes introducing it in a patch release makes an app build with 1.7.1 not runnable with apr-1.7.0.so. This function serves a real bugfix though, apr threads had real pool lifetime issues before that (raised by the httpd tests suite built/run under AddressSanitizer). I don't remember all the details of the change but possibly for this particular apr__pool_unmanage() introduction it's only about apr_thread_detach() correctness, so the other option might be to APR_ENOTIMPL there in 1.7.1, or leave a known defect, not sure it helps more.. At this point I question the relevance of releasing 1.7.1 at all, I think most people will be interested in 1.8.0 and I'd advise distros to upgrade to that too. Who will be using 1.7.1? What is the rationale for releasing both and how confusing will that be? Hopefully Ivan can address the Windows apr_file_t (as suitable) soon enough, in this case could we release 1.8.0 only? Cheers; Yann.
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
On Fri, Jan 14, 2022 at 4:40 PM Yann Ylavic wrote: > > On Fri, Jan 14, 2022 at 4:20 PM Ivan Zhakov wrote: > > > > 1. Revert this change from 1.7.x > > 2. Release 1.7.1 > > 3. Rework this code on trunk without changing the apr_file_t's behavior > > 4. Backport it to 1.7.x/1.8.x > > > > And if this plan makes sense, I am ready to proceed with steps (1), (3) and > > (4). > > +1, not sure we'd later release both 1.7.2 and 1.8.0 later on, > probably only the latter. Btw, I don't think the _behaviour_ of apr_file_t changed, the implementation of apr_file_socket_pipe did but it should be transparent for the user. > > Cheers; > Yann.
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
On Fri, Jan 14, 2022 at 4:20 PM Ivan Zhakov wrote: > > 1. Revert this change from 1.7.x > 2. Release 1.7.1 > 3. Rework this code on trunk without changing the apr_file_t's behavior > 4. Backport it to 1.7.x/1.8.x > > And if this plan makes sense, I am ready to proceed with steps (1), (3) and > (4). +1, not sure we'd later release both 1.7.2 and 1.8.0 later on, probably only the latter. Cheers; Yann.
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Hi Ivan;
On Thu, Jan 13, 2022 at 7:04 PM Ivan Zhakov wrote:
>
> On Fri, 7 Jan 2022 at 17:33, Yann Ylavic wrote:
> >
> > I was missing backport of r1895178, does r1896808 compile now?
> > (Sorry, no Windows at hand..).
> Yes, it builds now. Thanks!
Great, thanks for testing.
> >
> > I think that most if not all of the changes to 1.7.x since 1.7.0 are
> > fixes for bugs that were there before 1.7 already, not regressions
> > introduced by 1.7.0.
>
> Agreed on the bugfix/regressions part. I have misunderstood that
> r1896510 is a bugfix, perhaps, due to its size, and was thinking that
> it adds new functionality. But even with that in mind, I still think
> that the size of the change might be just too large for it to be an
> appropriate fit for a patch release.
The bugfix part is quite probable though still theoretical on the
httpd side. As you may know mpm_event makes heavy use of the wakeup
pipe with potentially plenty of worker threads waking up one listener
thread, and that should not block for obvious event scheme scalability
reasons. But there is actually no bug report about this since blocking
would not result in a deadlock and httpd users have not tested the new
implementation performance wise, yet. Theoretically it may happen..
So since mpm_event is unix(es) only, this change is mainly aimed at
fixing the unix implementation of the wakeup pipe (write a single byte
atomically/once until it's consumed), but that can't happen portably
on the APR side without adapting the Windows implementation given that
there is no synchronous/nonblocking ReadFile().
Using {Read,Write}File() on a SOCKET seems to be supported by Windows,
but if the SOCKET is set nonblocking it does not behave accordingly
(from what I could read on the subject), hence the switch to
WSA{Recv,Send}(). Note that there was already a special "pipe"
handling in Windows' apr_file_t, the "socket" one is just a third
case.
I don't find it too cumbersome personally, but if you have a better
option I'm all for it!
>
> Speaking of the change itself, I think that there might be an
> alternative to making the apr_file_t also handle sockets on Windows.
> It might be better to specifically change the pollset implementation
> so that on Windows it would add a socket and use it for wakeup,
> instead of using the socket disguised as a file.
AIUI, that's already what OS/2 does (using an UDP socket), having its
own apr_pollset_wakeup() implementation and its own draining directly
in apr_pollset_poll() instead of reusing the common
apr_poll_drain_wakeup_pipe(), which is the one performing the
apr_file_read() currently.
That's certainly an option for Windows too, and in that area it may be
interesting to look at an IOCB implementation for polling rather than
select (but that's probably another story given the different
semantics of IOCB vs epoll/kqueue..).
>
> If this alternative approach sounds fine, I could try to implement it.
Looks good to me, not sure it should block the current implementation
in 1.7.x though because AFAIK it passes the tests suite on Windows too
(which Mladen Turk made work and tried IIRC). The changes are not
trivial but quite straightforward given the previous/existing socket
as pipe implementation in apr_file_t.
But I don't want to push anything, mpm_event has worked like this for
quite some time now and it can probably wait for 1.8.x for this
supposed fix/improvement.
Regards;
Yann.
Re: svn commit: r1897021 - /apr/apr/branches/1.8.x/
[plus [1] and [2] links..] On Fri, Jan 14, 2022 at 12:06 PM Yann Ylavic wrote: > > On Fri, Jan 14, 2022 at 1:59 AM William A Rowe Jr wrote: > > > > A quick review of nm against libapr-1.so between 1.7.0 and 1.7.x > > indicates that 1.7.1 > > isn't releasable as-is. Any hints? > > > > +T apr_pool_find > > +T apr_pool_join > > +T apr_pool_lock > > +T apr_pool_num_bytes > > Those come from r1863234 [1], possibly we can let them as empty macros > in 1.7.x and still add the ones for apr_pool_join() and > apr_pool_num_bytes() which were missing for !APR_POOL_DEBUG? > Yet it might make apps using them from 1.7.1 (w/o #ifdef > APR_POOL_DEBUG) not compilable with 1.7.0 (though still ABI compatible > with all 1.7.x), but no one would be able to use them as function > pointers for instance, which would be a ABI/runtime breakage. Could > that hurt actually? > > > +T apr__pool_unmanage > > That one is for internal use only (from [2]), not APR_DECLAREd so > should be fine? [1] https://svn.apache.org/r1863234 [2] https://svn.apache.org/r1884103
Re: svn commit: r1897021 - /apr/apr/branches/1.8.x/
On Fri, Jan 14, 2022 at 1:59 AM William A Rowe Jr wrote: > > A quick review of nm against libapr-1.so between 1.7.0 and 1.7.x > indicates that 1.7.1 > isn't releasable as-is. Any hints? > > +T apr_pool_find > +T apr_pool_join > +T apr_pool_lock > +T apr_pool_num_bytes Those come from r1863234 [1], possibly we can let them as empty macros in 1.7.x and still add the ones for apr_pool_join() and apr_pool_num_bytes() which were missing for !APR_POOL_DEBUG? Yet it might make apps using them from 1.7.1 (w/o #ifdef APR_POOL_DEBUG) not compilable with 1.7.0 (though still ABI compatible with all 1.7.x), but no one would be able to use them as function pointers for instance, which would be a ABI/runtime breakage. Could that hurt actually? > +T apr__pool_unmanage That one is for internal use only (from [2]), not APR_DECLAREd so should be fine? Regards; Yann.
Re: APR 1.7.1 release?
On Thu, Dec 23, 2021 at 7:37 AM William A Rowe Jr wrote: > > On Fri, Dec 17, 2021 at 10:09 AM Yann Ylavic wrote: > > > > On Fri, Sep 3, 2021 at 3:44 AM William A Rowe Jr > > wrote: > > > > > > But I'd be happier co-RM'ing this with a newer committer/PMC > > > participant who wants to learn the ropes. Any volunteers? > > > > \o_ thanks for helping! Anytime for, maybe in early 2022 days? > > 1. Is that an offer? Sure it is :) I'm happy to assist you in the release process and learn about it. I looked at the release.sh script which is quite simple and the preliminar tagging process from the previous release, but following someone who knows is always better.. > > In any case I see us shipping a minimal APR 1.7.x with the fixes at hand and > for > Windows FS that frustrated some svn users. The scope of the unix domain socket > enablement are probably late into January/early Feb. Unsure what other folks > are > working on that fit into the 1.8 bump. I already backported the unix socket changes to 1.7.x, though Ivan objected already given the non trivial changes. I'd like it to be in 1.7.1 (mainly because of the new atomic/once wakeup which is useful for httpd's mpm_event usage), but not a strong opinion either so I could revert it's an uncomfortable change. Besides, current 1.7.x is not a minimal change already w.r.t. 1.7.0, some not-so-trivial backports are to address issues raised by running ASAN built APR and httpd through their test suites (namely apr_pool's r1884100, apr_thread's r1884103, apr_thread_pool's r1884110). Those have landed for quite some time now, but more eyes are always welcome. > > We all need to review APR-util 1.7.0-dev, to ensure it's ready. That > could happen > before year end, or early next year, depending on how stable it is. +1 > > So yes, I'd be grateful for your help, and more than happy to help you :) Great, let's go whenever you have the time for it ;) Cheers; Yann.
Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c unix/filedup.c win32/filedup.c
On Fri, Jan 7, 2022 at 3:58 PM Ruediger Pluem wrote: > > On 1/7/22 3:02 PM, Yann Ylavic wrote: > > > > Should we be consistent here then, and always use the passed in reqpool? > > I.e. something like the below (on the current code): > > Maybe a good idea. r1896812. Thanks; Yann.
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Hi Ivan, On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov wrote: > > This change does not compile on Windows in APR 1.7.x: > [[[ > file_io\win32\readwrite.c(325): error C2065: 'file': undeclared identifier > file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must > point to struct/union I was missing backport of r1895178, does r1896808 compile now? (Sorry, no Windows at hand..). > > I also have a high-level objection against backporting this change to > APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only > regression fixes should be backported to the stable branch. r1896510 > is a significant change and as far as I understand it's not a > regression fix. So I think it would be better to revert r1896510 and > release it as part of APR 2.0 (or 1.8.x). I think that most if not all of the changes to 1.7.x since 1.7.0 are fixes for bugs that were there before 1.7 already, not regressions introduced by 1.7.0. But I'm fine with switching to 1.8.x and releasing 1.8.0 if that's the policy, not sure that 1.7.1 is needed then (and we'd need to revert quite some changes in 1.7.x..). Regards; Yann.
Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c unix/filedup.c win32/filedup.c
On Fri, Sep 10, 2021 at 5:49 PM Yann Ylavic wrote:
>
> On Fri, Sep 10, 2021 at 4:44 PM Ruediger Pluem wrote:
> >
> > On 9/10/21 4:04 PM, Yann Ylavic wrote:
> > > Index: buckets/apr_buckets_file.c
> > > ===
> > > --- buckets/apr_buckets_file.c(revision 1893196)
> > > +++ buckets/apr_buckets_file.c(working copy)
> >
> > > @@ -223,11 +223,33 @@ APR_DECLARE(apr_status_t) apr_bucket_file_set_buf_
> > > return APR_SUCCESS;
> > > }
> > >
> > > -if (!apr_pool_is_ancestor(a->readpool, reqpool)) {
> > > -a->readpool = reqpool;
> > > +/* If the file is shared/split accross multiple buckets, this bucket
> > > can't
> > > + * take exclusive ownership with apr_file_setaside() (thus
> > > invalidating the
> > > + * current/old a->fd), let's apr_file_dup() in this case instead.
> > > + */
> > > +if (a->refcount.refcount > 1) {
> > > +apr_bucket_file *new;
> > > +apr_status_t rv;
> > > +
> > > +rv = apr_file_dup(, f, reqpool);
> > > +if (rv != APR_SUCCESS) {
> > > +return rv;
> > > +}
> > > +
> > > +new = apr_bucket_alloc(sizeof(*new), b->list);
> > > +memcpy(new, a, sizeof(*new));
> > > +new->refcount.refcount = 1;
> > > +new->readpool = reqpool;
> >
> > Why is the above no longer conditional on apr_pool_is_ancestor(a->readpool,
> > reqpool) like in the else branch?
>
> Good question..
> Since we created a new apr_bucket_file and apr_file_t above with the
> given reqpool's lifetime, I thought the reads would use it too just
> like apr_bucket_file_make() uses the given pool.
>
> But I don't really understand in the first place why we need to keep
> the existing ->readpool if it's an ancestor of the given reqpool.
> It's been so from the very beginning of the reqpool parameter
> (r58312!), but if one wants to setaside on a subpool it may not be
> thread-safe to keep using the parent pool.
> While calling apr_file_setaside (or apr_file_dup now) makes sure that
> the (new) file has the requested lifetime, why use the parent pool for
> mmaping or !XTHREAD reopening the file?
Should we be consistent here then, and always use the passed in reqpool?
I.e. something like the below (on the current code):
Index: buckets/apr_buckets_file.c
===
--- buckets/apr_buckets_file.c(revision 1896509)
+++ buckets/apr_buckets_file.c(working copy)
@@ -239,7 +239,6 @@ static apr_status_t file_bucket_setaside(apr_bucke
new = apr_bucket_alloc(sizeof(*new), b->list);
memcpy(new, a, sizeof(*new));
new->refcount.refcount = 1;
-new->readpool = reqpool;
a->refcount.refcount--;
a = b->data = new;
@@ -246,11 +245,9 @@ static apr_status_t file_bucket_setaside(apr_bucke
}
else {
apr_file_setaside(, f, reqpool);
-if (!apr_pool_is_ancestor(a->readpool, reqpool)) {
-a->readpool = reqpool;
-}
}
a->fd = fd;
+a->readpool = reqpool;
return APR_SUCCESS;
}
>
> Regards;
> Yann.
Re: APR 1.7.1 release?
Hi Bill, On Fri, Sep 3, 2021 at 3:44 AM William A Rowe Jr wrote: > > I'm willing to RM APR and APR-util 1.7 releases. Any news on this? > > But I'd be happier co-RM'ing this with a newer committer/PMC > participant who wants to learn the ropes. Any volunteers? \o_ thanks for helping! Anytime for, maybe in early 2022 days? Cheers; Yann.
