Re: svn commit: r1918258 - /apr/apr/trunk/file_io/unix/pipe.c

[Ruediger Pluem]

On 6/11/24 6:05 PM, jor...@apache.org wrote:
> Author: jorton
> Date: Tue Jun 11 16:05:22 2024
> New Revision: 1918258
> 
> URL: http://svn.apache.org/viewvc?rev=1918258=rev
> Log:
> * file_io/unix/pipe.c (file_pipe_create): Use pipe2(,O_NONBLOCK) by
>   default unless APR_FULL_BLOCK was used; unconditionally set the
>   blocking state later. Saves two syscalls per invocation for both
>   APR_READ_BLOCK and APR_WRITE_BLOCK, no [intended] functional change.

I guess it is me being blind , but how do we save two syscalls for the
APR_READ_BLOCK and APR_WRITE_BLOCK case each? With the code before the patch
we started with a blocking pipe and needed to set one end of the pipe to non 
blocking.
Now we start with a non blocking pipe and need to set the other end to blocking.
Where did I get lost and miss the point?

Regards

Rüdiger

> 
> Github: closes #56
> 
> Modified:
> apr/apr/trunk/file_io/unix/pipe.c
> 
> Modified: apr/apr/trunk/file_io/unix/pipe.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/file_io/unix/pipe.c?rev=1918258=1918257=1918258=diff
> ==
> --- apr/apr/trunk/file_io/unix/pipe.c (original)
> +++ apr/apr/trunk/file_io/unix/pipe.c Tue Jun 11 16:05:22 2024
> @@ -186,7 +186,10 @@ static apr_status_t file_pipe_create(apr
>  apr_interval_time_t fd_timeout;
>  
>  #ifdef HAVE_PIPE2
> -if (blocking == APR_FULL_NONBLOCK) {
> +/* If pipe2() is available, use O_NONBLOCK by default unless
> + * APR_FULL_BLOCK is requested, then set the individual pipe state
> + * as desired later - changing the state uses two syscalls. */
> +if (blocking != APR_FULL_BLOCK) {
>  if (pipe2(filedes, O_NONBLOCK) == -1) {
>  return errno;
>  }
> @@ -239,21 +242,14 @@ static apr_status_t file_pipe_create(apr
>  apr_pool_cleanup_register((*out)->pool, (void *)(*out), 
> apr_unix_file_cleanup,
>   apr_pool_cleanup_null);
>  
> -switch (blocking) {
> -case APR_FULL_BLOCK:
> -break;
> -case APR_READ_BLOCK:
> -apr_file_pipe_timeout_set(*out, 0);
> -break;
> -case APR_WRITE_BLOCK:
> -apr_file_pipe_timeout_set(*in, 0);
> -break;
> -default:
> -/* These are noops for the pipe2() case */
> -apr_file_pipe_timeout_set(*out, 0);
> -apr_file_pipe_timeout_set(*in, 0);
> -break;
> -}
> +/* Set each pipe to the desired O_NONBLOCK state, which may be a
> + * noop depending on whether the pipe2() or pipe() case was
> + * used. (timeout of -1 == blocking) */
> +apr_file_pipe_timeout_set(*in, (blocking == APR_FULL_BLOCK
> +|| blocking == APR_READ_BLOCK) ? -1 : 0);
> +apr_file_pipe_timeout_set(*out, (blocking == APR_FULL_BLOCK
> + || blocking == APR_WRITE_BLOCK) ? -1 : 
> 0);
> +
>  return APR_SUCCESS;
>  }
>  
> 
> 
> 

Re: svn commit: r1917094 [1/3] - in /apr/apr/trunk: ./ build/ docs/ include/ include/private/ ldap/ misc/unix/ test/

[Ruediger Pluem]

On 4/18/24 9:32 AM, minf...@apache.org wrote:
> Author: minfrin
> Date: Thu Apr 18 07:32:13 2024
> New Revision: 1917094
> 
> URL: http://svn.apache.org/viewvc?rev=1917094=rev
> Log:
> apr_ldap: Add the new APR LDAP API.
> 
> - Async API, can be used safely in event driven code.
> - Requests are tied into a pool lifetime, with abandon if the pool is
>   cleaned early.
> - Initial support for SASL bind, compare and search.
> - Backportable to apr-util v1.7.
> - Test case with example implementation.
> - Unmaintained Mozilla/Netware API support removed.
> - Rebind removed pending async re-implementation.
> - Doxygen docs.
> 
> Added:
> apr/apr/trunk/build/ldap.m4
> apr/apr/trunk/include/private/apr_ldap_internal.h   (with props)
> apr/apr/trunk/ldap/
> apr/apr/trunk/ldap/apr_ldap.c   (with props)
> apr/apr/trunk/ldap/apr_ldap_stub.c   (with props)
> apr/apr/trunk/ldap/apr_ldap_url.c   (with props)
> Modified:
> apr/apr/trunk/CHANGES
> apr/apr/trunk/CMakeLists.txt
> apr/apr/trunk/Makefile.in
> apr/apr/trunk/build.conf
> apr/apr/trunk/build/dso.m4
> apr/apr/trunk/configure.in
> apr/apr/trunk/docs/doxygen.conf
> apr/apr/trunk/include/apr.h.in
> apr/apr/trunk/include/apr.hnw
> apr/apr/trunk/include/apr.hw
> apr/apr/trunk/include/apr.hwc
> apr/apr/trunk/include/apu_errno.h
> apr/apr/trunk/misc/unix/errorcodes.c
> apr/apr/trunk/test/Makefile.in
> apr/apr/trunk/test/Makefile.win
> apr/apr/trunk/test/abts_tests.h
> apr/apr/trunk/test/testutil.h
> 

> Added: apr/apr/trunk/build/ldap.m4
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/build/ldap.m4?rev=1917094=auto
> ==
> --- apr/apr/trunk/build/ldap.m4 (added)
> +++ apr/apr/trunk/build/ldap.m4 Thu Apr 18 07:32:13 2024

> +  ])
> +
> +AC_CHECK_HEADERS([sasl.h sasl/sasl.h])
> +
> +AC_SUBST(ldap_h)
> +AC_SUBST(lber_h)
> +AC_SUBST(apu_have_ldap)
> +AC_SUBST(apu_have_ldap_openldap)
> +AC_SUBST(apu_have_ldap_solaris)
> +AC_SUBST(apu_have_ldap_microsoft)
> +AC_SUBST(apu_have_ldap_tivoli)
> +AC_SUBST(apu_have_ldap_zos)
> +AC_SUBST(apu_have_ldap_other)
> +AC_SUBST(LDADD_ldap)
> +
> +])
> +
> +
> 
> Modified: apr/apr/trunk/configure.in
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/configure.in?rev=1917094=1917093=1917094=diff
> ==
> --- apr/apr/trunk/configure.in (original)
> +++ apr/apr/trunk/configure.in Thu Apr 18 07:32:13 2024
> @@ -34,6 +34,7 @@ sinclude(build/dbm.m4)
>  sinclude(build/dbd.m4)
>  sinclude(build/dso.m4)
>  sinclude(build/iconv.m4)
> +sinclude(build/ldap.m4)
>  
>  sinclude(build/ax_prog_cc_for_build.m4)
>  
> @@ -3150,6 +3151,9 @@ APU_CHECK_DBD_SQLITE2
>  APU_CHECK_DBD_ORACLE
>  APU_CHECK_DBD_ODBC
>  
> +dnl Find LDAP backend
> +APU_FIND_LDAP
> +
>  dnl select an XML parser
>  APU_FIND_XML
>  
> @@ -3174,6 +3178,8 @@ if test "$crypt_r" = "1"; then
>APU_CHECK_CRYPT_R_STYLE
>  fi
>  
> +AC_CHECK_HEADERS([sasl.h sasl/sasl.h])
> +

Why do we need AC_CHECK_HEADERS twice? Here and in ldap.m4

>  APRUTIL_LIBNAME="aprutil${libsuffix}"
>  AC_SUBST(APRUTIL_LIBNAME)
>  
> 

Regards

Rüdiger

Re: CMake: Installation directory for include files in apr 1.7.x

[Ruediger Pluem]

On 5/29/24 11:34 PM, Ivan Zhakov wrote:
> On Fri, 26 Apr 2024 at 14:56, Daniel Sahlberg  > wrote:
> 

>  
> 
> What is the state of the 1.8.x branch? Would it make sense to rather do 
> it here and focus on releasing 1.8 with this change?
> 
> Unfortunately the release process is complicated in APR: trunk is 2.0 and has 
> breaking changes. 1.8.x is a branch from 1.7.x with
> many backports from trunk, but as far as I know it misses many backports that 
> merged to 1.7.x branch. So in my opinion it is less
> stable than trunk.

1.8.x should have the same trunk backports that 1.7.x has. If it has not, the 
missing ones need to be backported to 1.8.x as well.

Regards

Rüdiger

Re: svn commit: r1917050 - in /apr/apr/trunk: CHANGES crypto/apr_crypto.c dbd/apr_dbd.c dbm/apr_dbm.c include/private/apu_internal.h util-misc/apu_dso.c

[Ruediger Pluem]

On 4/22/24 4:07 PM, Graham Leggett via dev wrote:
> On 22 Apr 2024, at 14:05, Ruediger Pluem  wrote:
> 
>>>> Now the caller needs to allocate memory even if it is not interested in 
>>>> the error or if there is no error at all.
>>>> Wouldn't it be better to add an apu_err_t **err instead (which can be 
>>>> NULL) and in case of an error allocate an
>>>> apu_err_t from pool and fill it and return it in **err (provided it was 
>>>> not NULL).
>>>
>>> I originally used this pattern and found it overkill - what I do now is 
>>> allocate a app_err_t on the stack and immediately
>>> abandon it.
>>
>> Did you commit this?
> 
> Updated in http://svn.apache.org/viewvc?rev=1917271=rev

But I don't see where you allocate the structure on the stack on the caller 
side for apr_crypto and apr_dbm and only copy it to a
pool based alloc in case of an error and that we pass NULL in case of apr_dbd 
where we don't process it.
Currently we allocate memory for an apu_err_t struct that we don't need in most 
cases as it only matters in case of an error
which I hope is not the typical code path.

Regards

Rüdiger

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

[Ruediger Pluem]

On 4/22/24 2:46 PM, minf...@apache.org wrote:
> Author: minfrin
> Date: Mon Apr 22 12:46:37 2024
> New Revision: 1917266
> 
> URL: http://svn.apache.org/viewvc?rev=1917266=rev
> Log:
> apr_buffer: Redefine size to separate the unsigned size and the
> flag to indicate whether a zero terminated string or not.
> 
> Modified:
> apr/apr/trunk/buffer/apr_buffer.c
> apr/apr/trunk/include/apr_buffer.h
> apr/apr/trunk/test/testbuffer.c
> 
> Modified: apr/apr/trunk/buffer/apr_buffer.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/buffer/apr_buffer.c?rev=1917266=1917265=1917266=diff
> ==
> --- apr/apr/trunk/buffer/apr_buffer.c (original)
> +++ apr/apr/trunk/buffer/apr_buffer.c Mon Apr 22 12:46:37 2024

> @@ -322,7 +287,17 @@ APR_DECLARE(int) apr_buffer_ncmp(const a
>  return 1;
>  }

There was a proposal from Yann to simplify the above block to

if (!src) {
return dst ? 1 : 0;
}
if (!dst) {
return -1;
}

>  else {
> -return apr_buffer_cmp(src, dst);
> +
> +apr_size_t slen = apr_buffer_len(src);
> +apr_size_t dlen = apr_buffer_len(dst);
> +
> +if (slen != dlen) {
> +return slen < dlen ? -1 : 1;
> +}
> +else {
> +return memcmp(src->d.mem, dst->d.mem, slen);
> +}
> +
>  }
>  }
>  }
}
> @@ -379,19 +354,19 @@ APR_DECLARE(char *) apr_buffer_pstrncat(
>  strncpy(dst, sep, seplen);
>  dst += seplen;
>  }
> -
> -if (src->size < 0) {
> -strncpy(dst, src->d.str, (apr_size_t)((-src->size) - 1));
> -dst += (-src->size) - 1;
> +
> +if (src->zero_terminated) {
> +strncpy(dst, src->d.str, src->size);
> +dst += src->size;
>  }

Can't we remove the above condition at all and do a

memcpy(dst, src->d.mem, src->size + src->zero_terminated);

below in the if (APR_BUFFER_NONE == flags) block?

>  else {
>  if (APR_BUFFER_NONE == flags) {
> -memcpy(dst, src->d.mem, (apr_size_t)src->size);
> +memcpy(dst, src->d.mem, src->size);
>  }
>  else if (APR_BUFFER_BASE64 == flags) {
>  apr_size_t b64len;
>  
> -if (APR_SUCCESS != apr_encode_base64(dst, src->d.mem, 
> (apr_size_t)src->size,
> +if (APR_SUCCESS != apr_encode_base64(dst, src->d.mem, 
> src->size,
>   APR_ENCODE_NONE, 
> )) {
>  return NULL;
>  }
> 

Regards

Rüdiger

Re: svn commit: r1917050 - in /apr/apr/trunk: CHANGES crypto/apr_crypto.c dbd/apr_dbd.c dbm/apr_dbm.c include/private/apu_internal.h util-misc/apu_dso.c

[Ruediger Pluem]

On 4/17/24 9:30 AM, Graham Leggett via dev wrote:
> On 17 Apr 2024, at 08:07, Ruediger Pluem  wrote:
> 
>>> Modified: apr/apr/trunk/util-misc/apu_dso.c
>>> URL: 
>>> http://svn.apache.org/viewvc/apr/apr/trunk/util-misc/apu_dso.c?rev=1917050=1917049=1917050=diff
>>> ==
>>> --- apr/apr/trunk/util-misc/apu_dso.c (original)
>>> +++ apr/apr/trunk/util-misc/apu_dso.c Tue Apr 16 21:33:58 2024
>>> @@ -131,7 +131,8 @@ apr_status_t apu_dso_load(apr_dso_handle
>>>   apr_dso_handle_sym_t *dsoptr,
>>>   const char *module,
>>>   const char *modsym,
>>> -  apr_pool_t *pool)
>>> +  apr_pool_t *pool,
>>> +  apu_err_t *err)
>>
>> Now the caller needs to allocate memory even if it is not interested in the 
>> error or if there is no error at all.
>> Wouldn't it be better to add an apu_err_t **err instead (which can be NULL) 
>> and in case of an error allocate an
>> apu_err_t from pool and fill it and return it in **err (provided it was not 
>> NULL).
> 
> I originally used this pattern and found it overkill - what I do now is 
> allocate a app_err_t on the stack and immediately abandon it.

Did you commit this?

Regards

Rüdiger

Re: svn commit: r1917082 - /apr/apr/trunk/buffer/apr_buffer.c

[Ruediger Pluem]

On 4/18/24 12:37 AM, minf...@apache.org wrote:
> Author: minfrin
> Date: Wed Apr 17 22:37:07 2024
> New Revision: 1917082
> 
> URL: http://svn.apache.org/viewvc?rev=1917082=rev
> Log:
> apr_buffer: Add explicit casts on all potentially narrowing conversions
> to apr_size_t. Define the maximum buffer size as APR_SIZE_MAX/2.
> 
> Modified:
> apr/apr/trunk/buffer/apr_buffer.c
> 
> Modified: apr/apr/trunk/buffer/apr_buffer.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/buffer/apr_buffer.c?rev=1917082=1917081=1917082=diff
> ==
> --- apr/apr/trunk/buffer/apr_buffer.c (original)
> +++ apr/apr/trunk/buffer/apr_buffer.c Wed Apr 17 22:37:07 2024
> @@ -28,12 +28,13 @@
>  #include "apr_strings.h"
>  #include "apr_private.h"
>  
> +#define APR_BUFFER_MAX APR_SIZE_MAX/2

Why no longer APR_OFF_MAX?

Regards

Rüdiger

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

[Ruediger Pluem]

On 4/17/24 12:02 AM, Graham Leggett via dev wrote:
> Hi all,
> 
>> +/**
>> + * Structure for efficiently tracking a buffer that could contain
>> + * a zero terminated string, or a fixed length non zero string.
>> + */
>> +typedef struct
>> +{
>> +/** pointer to the data, which could be a string or a memory block. */
>> +union {
>> +char *str;
>> +void *mem;
>> +} d;
>> +
>> +/** size of the data. If positive, the data is of fixed size. If
>> +  * negative, the data is zero terminated and the absolute value
>> +  * represents the data length including terminating zero.
>> +  *
>> +  * we use apr_int64_t to make it simple to detect overflow.
>> +  */
>> +apr_int64_t size;
>> +
>> +} apr_buffer_t;
> 
> 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?

Regards

Rüdiger

Re: svn commit: r1917050 - in /apr/apr/trunk: CHANGES crypto/apr_crypto.c dbd/apr_dbd.c dbm/apr_dbm.c include/private/apu_internal.h util-misc/apu_dso.c

[Ruediger Pluem]

On 4/16/24 11:33 PM, minf...@apache.org wrote:
> Author: minfrin
> Date: Tue Apr 16 21:33:58 2024
> New Revision: 1917050
> 
> URL: http://svn.apache.org/viewvc?rev=1917050=rev
> Log:
> Pass details of module loading errors back to the caller through the
> apu_err_t structure.
> 
> Modified:
> apr/apr/trunk/CHANGES
> apr/apr/trunk/crypto/apr_crypto.c
> apr/apr/trunk/dbd/apr_dbd.c
> apr/apr/trunk/dbm/apr_dbm.c
> apr/apr/trunk/include/private/apu_internal.h
> apr/apr/trunk/util-misc/apu_dso.c
> 

> Modified: apr/apr/trunk/util-misc/apu_dso.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/util-misc/apu_dso.c?rev=1917050=1917049=1917050=diff
> ==
> --- apr/apr/trunk/util-misc/apu_dso.c (original)
> +++ apr/apr/trunk/util-misc/apu_dso.c Tue Apr 16 21:33:58 2024
> @@ -131,7 +131,8 @@ apr_status_t apu_dso_load(apr_dso_handle
>apr_dso_handle_sym_t *dsoptr,
>const char *module,
>const char *modsym,
> -  apr_pool_t *pool)
> +  apr_pool_t *pool,
> +  apu_err_t *err)

Now the caller needs to allocate memory even if it is not interested in the 
error or if there is no error at all.
Wouldn't it be better to add an apu_err_t **err instead (which can be NULL) and 
in case of an error allocate an
apu_err_t from pool and fill it and return it in **err (provided it was not 
NULL).


>  {
>  apr_dso_handle_t *dlhandle = NULL;
>  char *pathlist;
> @@ -186,6 +187,7 @@ apr_status_t apu_dso_load(apr_dso_handle
>  apr_cpystrn(eos, module, sizeof(path) - (eos - path));
>  
>  rv = apr_dso_load(, path, global);
> +
>  if (dlhandleptr) {
>  *dlhandleptr = dlhandle;
>  }
> @@ -205,20 +207,27 @@ apr_status_t apu_dso_load(apr_dso_handle
>  apr_cpystrn(eos, module, sizeof(path) - (eos - path));
>  
>  rv = apr_dso_load(, path, global);
> +
>  if (dlhandleptr) {
>  *dlhandleptr = dlhandle;
>  }
> +
>  if (rv == APR_SUCCESS) { /* APR_EDSOOPEN */
>  break;
>  }
>  }
>  }
>  
> -if (rv != APR_SUCCESS) /* APR_ESYMNOTFOUND */
> +if (rv != APR_SUCCESS) { /* APR_ESYMNOTFOUND */
> +err->msg = apr_pstrdup(pool, apr_dso_error(dlhandle, NULL, 0));
> +err->reason = apr_pstrdup(pool, module);
>  return rv;
> +}
>  
>  rv = apr_dso_sym(dsoptr, dlhandle, modsym);
>  if (rv != APR_SUCCESS) { /* APR_ESYMNOTFOUND */
> +err->msg = apr_pstrdup(pool, apr_dso_error(dlhandle, NULL, 0));
> +err->reason = apr_pstrdup(pool, module);
>  apr_dso_unload(dlhandle);
>  }
>  else {
> @@ -231,5 +240,5 @@ apr_status_t apu_dso_load(apr_dso_handle
>  return rv;
>  }
>  
> -#endif /* APR_DSO_BUILD */
> +#endif /* APR_HAVE_MODULAR_DSO */

Regards

Rüdiger

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

[Ruediger Pluem]

On 4/16/24 10:54 PM, minf...@apache.org wrote:
> Author: minfrin
> Date: Tue Apr 16 20:54:51 2024
> New Revision: 1917047
> 
> URL: http://svn.apache.org/viewvc?rev=1917047=rev
> Log:
> Add the apr_buffer API.
> 
> An APR buffer is a structure that can contain a zero terminated string, or
> a non zero terminated block of memory, and allow such structures to be
> passed around and handled in a memory efficient way.
> 
> We allow two buffers to be compared without duplicating strings. Memory
> buffers can be converted to string buffers safely. The contents of buffers
> can be copied into and out of other systems like caches using memory
> allocation callbacks.
> 
> Used by the new LDAP API, where string support has been deprecated in
> favour of fixed sized buffers.
> 
> Added:
> apr/apr/trunk/buffer/
> apr/apr/trunk/buffer/apr_buffer.c   (with props)
> apr/apr/trunk/include/apr_buffer.h   (with props)
> apr/apr/trunk/test/testbuffer.c   (with props)
> Modified:
> apr/apr/trunk/CHANGES
> apr/apr/trunk/build.conf
> apr/apr/trunk/test/Makefile.in
> apr/apr/trunk/test/Makefile.win
> apr/apr/trunk/test/NWGNUaprtest
> apr/apr/trunk/test/abts_tests.h
> apr/apr/trunk/test/testutil.h
> 
> Modified: apr/apr/trunk/CHANGES

> 
> Added: apr/apr/trunk/buffer/apr_buffer.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/buffer/apr_buffer.c?rev=1917047=auto
> ==
> --- 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 @@
> +/*Licensed to the Apache Software Foundation (ASF) under one
> + *or more contributor license agreements.  See the NOTICE file
> + *distributed with this work for additional information
> + *regarding copyright ownership.  The ASF licenses this file
> + *to you under the Apache License, Version 2.0 (the
> + *"License"); you may not use this file except in compliance
> + *with the License.  You may obtain a copy of the License at
> + *
> + *  http://www.apache.org/licenses/LICENSE-2.0
> + *
> + *Unless required by applicable law or agreed to in writing,
> + *software distributed under the License is distributed on an
> + *"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
> + *KIND, either express or implied.  See the License for the
> + *specific language governing permissions and limitations
> + *under the License.
> + */
> +
> +#include "apr.h"
> +#include "apr_lib.h"
> +#if 0
> +#define APR_WANT_STDIO
> +#define APR_WANT_STRFUNC
> +#endif
> +#include "apr_want.h"
> +#include "apr_buffer.h"
> +#include "apr_encode.h"
> +#include "apr_strings.h"
> +#include "apr_private.h"
> +
> +
> +APR_DECLARE(apr_status_t) apr_buffer_mem_set(apr_buffer_t *buf,
> + void *mem, apr_size_t len)
> +{
> +
> +if (len > APR_INT64_MAX) {
> +return APR_EINVAL;
> +}
> +
> +buf->d.mem = mem;
> +buf->size = len;
> +
> +return APR_SUCCESS;
> +}
> +
> +APR_DECLARE(apr_buffer_t *) apr_buffer_mem_make(apr_pool_t *pool,
> +void *mem, apr_size_t len)
> +{
> +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 {
> +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)
> +{
> +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;
> +}

Is the above just a performance shortcut?

> +else if (len < 0) {
> +slen = strlen(str);
> +if (slen <= APR_INT64_MAX) {
> +size = -(slen + 1);
> +}
> +else {
> +return NULL;
> +}
> +}
> +else {
> +size = -(len + 1);
> +}
> +
> +buf = 

Re: svn commit: r1916337 - /apr/apr/trunk/include/apr_general.h

[Ruediger Pluem]

On 3/15/24 2:35 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Fri Mar 15 13:35:04 2024
> New Revision: 1916337
> 
> URL: http://svn.apache.org/viewvc?rev=1916337=rev
> Log:
> apr_general: Use __builtin_offsetof for APR_OFFSETOF if available.  PR 68763.
> 
> Sanitizers might warn about &((type *)0)->field being UB, so use the builtin
> which is meant to avoid it.
> 
> 
> Modified:
> apr/apr/trunk/include/apr_general.h
> 
> Modified: apr/apr/trunk/include/apr_general.h
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/include/apr_general.h?rev=1916337=1916336=1916337=diff
> ==
> --- apr/apr/trunk/include/apr_general.h (original)
> +++ apr/apr/trunk/include/apr_general.h Fri Mar 15 13:35:04 2024
> @@ -106,7 +106,9 @@ typedef enum { APR_WAIT_READ, APR_WAIT_W
>   * @param field  data field within the structure
>   * @return offset
>   */
> -#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.

> +#define APR_OFFSETOF(s_type,field) __builtin_offsetof(s_type,field)
> +#elif 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)
> 
> 
> 

Regards

Rüdiger

Re: svn commit: r1915658 - /apr/apr/trunk/test/testbuckets.c

[Ruediger Pluem]

On 2/8/24 4:49 PM, jor...@apache.org wrote:
> Author: jorton
> Date: Thu Feb  8 15:49:14 2024
> New Revision: 1915658
> 
> URL: http://svn.apache.org/viewvc?rev=1915658=rev
> Log:
> * buckets/apr_brigade.c (apr_brigade_split_line): After finding an LF,
>   only split the bucket if the LF is not the last character in the
>   data.
> 
> * test/testbuckets.c (test_splitline_exactly): New test.
> 
> PR: 64273
> Github: closes #53
> Submitted by: Barnim Dzwillo , jorton
> 
> Modified:
> apr/apr/trunk/test/testbuckets.c

Did you miss to commit the changes to buckets/apr_brigade.c?

Regards

Rüdiger

Re: svn commit: r1914814 - in /apr/apr/trunk: CHANGES encoding/apr_escape.c include/apr_escape.h test/testescape.c tools/gen_test_char.c

[Ruediger Pluem]

On 12/20/23 11:46 PM, minf...@apache.org wrote:
> Author: minfrin
> Date: Wed Dec 20 22:46:22 2023
> New Revision: 1914814
> 
> URL: http://svn.apache.org/viewvc?rev=1914814=rev
> Log:
> apr_escape: Add apr_escape_json() and apr_pescape_json().
> 
> Modified:
> apr/apr/trunk/CHANGES
> apr/apr/trunk/encoding/apr_escape.c
> apr/apr/trunk/include/apr_escape.h
> apr/apr/trunk/test/testescape.c
> apr/apr/trunk/tools/gen_test_char.c
> 

> Modified: apr/apr/trunk/encoding/apr_escape.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/encoding/apr_escape.c?rev=1914814=1914813=1914814=diff
> ==
> --- apr/apr/trunk/encoding/apr_escape.c (original)
> +++ apr/apr/trunk/encoding/apr_escape.c Wed Dec 20 22:46:22 2023
> @@ -36,7 +36,7 @@
>   * char in here and get it to work, because if char is signed then it
>   * will first be sign extended.
>   */
> -#define TEST_CHAR(c, f)(test_char_table[(unsigned)(c)] & (f))
> +#define TEST_CHAR(c, f)(test_char_table[(apr_uint16_t)(c)] & (f))
>  
>  APR_DECLARE(apr_status_t) apr_escape_shell(char *escaped, const char *str,
>  apr_ssize_t slen, apr_size_t *len)
> @@ -1209,6 +1209,271 @@ APR_DECLARE(const char *) apr_pescape_ld
>  }
>  }
>  
> +return src;
> +}
> +
> +APR_DECLARE(apr_status_t) apr_escape_json(char *escaped, const char *str,
> +apr_ssize_t slen, int quote, apr_size_t *len)

Shouldn't we provide a length for the escaped buffer to be able to do some 
sanity checking against overflows?
We could ignore this parameter in case that escaped is NULL.
Otherwise I think we should have a big fat warning in the doxygen documentation 
of this function that it
is extremely dangerous to call this function without providing a buffer of 
sufficient length and that
apr_escape_json should be called with escaped equal NULL before to determine 
this length, just like as
we do in apr_pescape_json.

> +{
> +apr_size_t size = 1;
> +int found = quote;
> +int error = 0;
> +const unsigned char *s = (const unsigned char *) str;
> +unsigned char *d = (unsigned char *) escaped;
> +unsigned c;
> +const char invalid[3] = { 0xEF, 0xBF, 0xBD };
> +
> +if (s) {
> +if (d) {
> +if (quote) {
> +*d++ = '"';
> +size += 1;
> +}
> +while ((c = *s) && slen) {
> +if (TEST_CHAR(c, T_ESCAPE_JSON)) {
> +switch (c) {
> +case '\b':
> +*d++ = '\\';
> +*d++ = 'b';
> +size += 2;
> +found = 1;
> +break;
> +case '\f':
> +*d++ = '\\';
> +*d++ = 'f';
> +size += 2;
> +found = 1;
> +break;
> +case '\n':
> +*d++ = '\\';
> +*d++ = 'n';
> +size += 2;
> +found = 1;
> +break;
> +case '\r':
> +*d++ = '\\';
> +*d++ = 'r';
> +size += 2;
> +found = 1;
> +break;
> +case '\t':
> +*d++ = '\\';
> +*d++ = 't';
> +size += 2;
> +found = 1;
> +break;
> +case '\\':
> +*d++ = '\\';
> +*d++ = '\\';
> +size += 2;
> +found = 1;
> +break;
> +case '"':
> +*d++ = '\\';
> +*d++ = '"';
> +size += 2; 
> +found = 1;
> +break;
> +default:
> +if (c < 0x20) {
> +size += apr_snprintf((char *)d, 6, "\\u%04x", c);
> +d += 5; 
> +found = 1;
> +}
> +else if (((c >> 7) == 0x00)) {

How can this case happen? c's like this should fail the TEST_CHAR(c, 
T_ESCAPE_JSON) test.

> +/* 1 byte */
> +memcpy(d, s, 1);
> +d += 1;
> +size += 1;
> +}
> +else if ((slen < 0 || slen > 1) && ((c >> 5) == 
> 0x06) && s[1]) {
> +/* 2 bytes */
> +if ((slen > 0 && slen < 2) || (s[1] >> 6) != 
> 0x02) {
> +memcpy(d, 

Re: [PATCH] Avoid memcpy-from-NULL in apr_brigade_flatten

[Ruediger Pluem]

On 12/18/23 12:09 PM, Joe Orton wrote:
> On Thu, Dec 14, 2023 at 04:44:47PM -0500, Ben Kallus wrote:
>> memcpy to or from NULL is undefined behavior, even for copies of
>> length 0. See this godbolt link for an example of how this can cause
>> problems: https://gcc.godbolt.org/z/zfvnMMsds
>>
>> This patch avoids calling memcpy for 0-length buckets, so that buckets
>> with NULL data and 0 length don't cause UB when flattened.
>>
>> Addresses this bugzilla report from httpd:
>> https://bz.apache.org/bugzilla/show_bug.cgi?id=68278
> 
> Thanks for the patch, this is an interesting find. Can you say what 
> bucket type was hit here - e.g. print b->type->name in gdb from inside 
> apr_brigade_flatten()?
> 
> It looks like metadata bucket types tend to give NULL on read() so I'm 
> guessing that's the case here. It would trip up other apr_brigade_* 
> functions in similar ways, e.g. split_line looks wrong too. You can make 
> a case that ignoring FLUSH is "safe" but it's probably undesirable, and 
> trying to flatten across an EOS is almost certainly wrong.
> 
> So I'm not sure, maybe a better/safer response is to catch metadata 
> buckets and treat them as end-of-brigade or an error rather than 
> zero-length data buckets. apr_brigade_split_boundary has an API 
> condition for that to explicitly return APR_INCOMPLETE.

Also for the sake of compatibility I would only fail if we encounter
at non metadata bucket after a possible EOS bucket.
Any metadata before EOS I would just ignore when flattening.

Regards

Rüdiger

Possible cache inconsistency in apr_memcache.c

[Ruediger Pluem]

I think the below code from 
memcache/apr_memcache.c::apr_memcache_find_server_hash_default can cause a 
cache inconsistency.
If the server determined by the provided hash value is dead we just choose the 
next one.
If we do this when writing and the previous dead server comes back with its 
cache (cannot happen if it got restarted, but can
happen if there was a temporary network connectivity issue) the next read 
operation might read a stale cache entry from this
resurrected dead server.


do {
ms = mc->live_servers[h % mc->ntotal];
if(ms->status == APR_MC_SERVER_LIVE) {
break;
}
else {
if (curtime == 0) {
curtime = apr_time_now();
}
#if APR_HAS_THREADS
apr_thread_mutex_lock(ms->lock);
#endif
/* Try the dead server, every 5 seconds */
if (curtime - ms->btime >  apr_time_from_sec(5)) {
ms->btime = curtime;
if (mc_version_ping(ms) == APR_SUCCESS) {
make_server_live(mc, ms);
#if APR_HAS_THREADS
apr_thread_mutex_unlock(ms->lock);
#endif
break;
}
}
#if APR_HAS_THREADS
apr_thread_mutex_unlock(ms->lock);
#endif
}
h++;
i++;
} while(i < mc->ntotal);


Regards

Rüdiger

Hard coded time value in apr_memcache.c

[Ruediger Pluem]

I am a little bit bothered by the following hardcoded time in 
memcache/apr_memcache.c::apr_memcache_find_server_hash_default

if (curtime - ms->btime >  apr_time_from_sec(5)) {
ms->btime = curtime;
if (mc_version_ping(ms) == APR_SUCCESS) {
make_server_live(mc, ms);
#if APR_HAS_THREADS
apr_thread_mutex_unlock(ms->lock);
#endif
break;
}
}

I want to make this configurable. I can think of the following three options:

1. Create an apr_memcache_create_ex that allows to set this value.
2. Create a new setter / getter function to allow to set / get this for a 
previously created apr_memcache_t.
3. Encourage people to directly change the field that holds the time 
information in the public struct apr_memcache_t for
   a previously created apr_memcache_t.

What makes most sense from an API point of view?

Regards

Rüdiger

Re: Release APR 1.7.4?

[Ruediger Pluem]

On 4/12/23 5:12 PM, Evgeny Kotkov via dev wrote:
> Hi everyone,
> 
> Unfortunately, one of the fixes in APR 1.7.3 that was authored by me has
> caused a significant regression, where writing to files opened with both
> APR_FOPEN_APPEND and APR_FOPEN_BUFFERED now may not properly append
> the data on Windows.  The issue has been reported in [1].
> 
> Compared to the potential impact of the regression, the original fix seems to
> be of lesser concern, so I reverted the change in r1909088 [2] and backported
> it to 1.7.x in r1909095 [3].
> 
> Would it be feasible to release 1.7.4 with a fix for this regression?

In general I think it is a good idea to do a release to fix this regression.
Unfortunately I don't have the time to do it.

Regards

Rüdiger

Re: [ANNOUNCEMENT] Apache Portable Runtime 1.7.3 Released

[Ruediger Pluem]

On 3/31/23 11:13 AM, rpluem wrote:
>Apache Portable Runtime APR 1.7.3 Released
> 

Should I remove the old APR 1.6.5 files from the releases directory as well?

Regards

Rüdiger

[VOTE] Release apr-1.7.3 [RESULTS]

[Ruediger Pluem]

On 3/27/23 10:17 AM, Ruediger Pluem wrote:
> 1.7.3-rc1 is here:
> 
> https://apr.apache.org/dev/dist/
> 
> For the release of apr-1.7.3
>   [  ]  +1 looks great!
>   [  ]  -1 something is broken
> 
> I will let the vote run through end-of-week.
> 

With more than 90 hours passed I call this vote closed.
Results:

No -1.
4 binding +1: rpluem, ylavic, steffenal, covener

Hence the vote passed and I will continue with the release procedure.

Regards

Rüdiger

Re: [VOTE] Release apr-1.7.3

[Ruediger Pluem]

On 3/27/23 10:17 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!
>   [  ]  -1 something is broken
> 
> I will let the vote run through end-of-week.
> 

Tested on RedHat 8 x86_64 and in conjunctions with httpd 2.4.57-dev and 
APR-UTIL 1.6.3.

Regards

Rüdiger

[VOTE] Release apr-1.7.3

[Ruediger Pluem]

1.7.3-rc1 is here:

https://apr.apache.org/dev/dist/

For the release of apr-1.7.3
  [  ]  +1 looks great!
  [  ]  -1 something is broken

I will let the vote run through end-of-week.

Regards

Rüdiger

Release APR 1.7.3 ?

[Ruediger Pluem]

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.


Regards

Rüdiger

Re: svn commit: r1906889 - in /apr/apr/trunk: ./ build/ include/ include/arch/win32/ test/ threadproc/beos/ threadproc/netware/ threadproc/os2/ threadproc/unix/ threadproc/win32/

[Ruediger Pluem]

On 3/17/23 11:20 AM, Rainer Jung wrote:
> Thanks Yann, configure now detects the missing function.
> 
> 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.

testpoll still works fine on RedHat 7 and RedHat 8 64 bit for me.

Regards

Rüdiger

Re: svn commit: r1908433 - in /apr/apr/trunk: crypto/apr_crypto_openssl.c test/testcrypto.c

[Ruediger Pluem]

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?

> +
>  #include 
>  #include 
>  #include 
{
> @@ -79,8 +124,11 @@ struct apr_crypto_key_t {
>  const apr_crypto_t *f;
>  const apr_crypto_key_rec_t *rec;
>  const EVP_CIPHER *cipher;
> -const EVP_MD *hmac;
> +const EVP_MD *md;
>  EVP_PKEY *pkey;
> +#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) {
^~


>  unsigned char *key;
>  int keyLen;
>  int doPad;

> @@ -106,7 +153,9 @@ struct apr_crypto_digest_t {
>  const apr_crypto_key_t *key;
>  apr_crypto_digest_rec_t *rec;
>  EVP_MD_CTX *mdCtx;
> -int initialised;
> +#if !APR_USE_OPENSSL_PRE_3_0_API
> +EVP_MAC_CTX *macCtx;
> +#endif

Same as above with the mac field:

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) {
  ^~


>  int digestSize;
>  };
>  

Regards

Rüdiger

Re: svn commit: r1908433 - in /apr/apr/trunk: crypto/apr_crypto_openssl.c test/testcrypto.c

[Ruediger Pluem]

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?

Regards

Rüdiger

Re: svn commit: r1907634 - /apr/apr/trunk/include/apr_buckets.h

[Ruediger Pluem]

On 2/14/23 7:28 PM, Christophe JAILLET wrote:
> Le 14/02/2023 à 11:51, Ivan Zhakov via dev a écrit :
>> On Tue, 14 Feb 2023 at 10:58, > > wrote:
>>
>>     Author: jailletc36
>>     Date: Tue Feb 14 07:56:50 2023
>>     New Revision: 1907634
>>
>>     URL: http://svn.apache.org/viewvc?rev=1907634=rev
>>     
>>     Log:
>>     Re-order the fields of 'struct apr_bucket_file' to avoid a hole and
>>     some padding.
>>
>>     Before the patch, pahole states that:
>>
>>     struct apr_bucket_file {
>>          apr_bucket_refcount        refcount;             /*     0   
>>   4 */
>>
>>          /* XXX 4 bytes hole, try to pack */
>>
>>          apr_file_t *               fd;                   /*     8   
>>   8 */
>>          apr_pool_t *               readpool;             /*    16   
>>   8 */
>>          int                        can_mmap;             /*    24   
>>   4 */
>>
>>          /* XXX 4 bytes hole, try to pack */
>>
>>          apr_size_t                 read_size;            /*    32   
>>   8 */
>>
>>          /* size: 40, cachelines: 1, members: 5 */
>>          /* sum members: 32, holes: 2, sum holes: 8 */
>>          /* last cacheline: 40 bytes */
>>     };
>>
>> Nice, but I'd like to note that this change breaks ABI and can be released 
>> only in APR 2.0.
>>
> 
> Sure.
> 
> I was just wondering if such small optimizations make sense or not.
> It potentially makes backport of some other patches more difficult and breaks 
> ABI.

This is true, but I see only a low risk of making backports difficult for 
structures that seem to be set and haven't been touched
for a long time. And even if we need to touch them we can still backport. It is 
just more work then.

> 
> So, does it worth the effort?

I would say yes at least for structures that seem to be set and stable.

> 
> I've spotted a few of them in APR, and would make sure that such patches are 
> welcomed before committing other things.

As always, really appreciate your look on these gory details.

Regards

Rüdiger

Re: apr-1.7.2/Apache shared memory now world readable?

[Ruediger Pluem]

On 2/10/23 2:42 AM, Eric Covener wrote:
>> I think this should be revisited and changed to 600.
> 
> It seems like all the methods use 0644.  After the change, it's just
> accessible in the filesystem rather than in the sysv shm ether.
> 
> It seems like an API gap, APR can't know what the caller expects to do
> with it (other than it's not anonymous).
> Today I guess a caller could run with a more conservative umask, or
> toggle it around calls to apr_shm_create?
> 

I would like to see a more restrictive default, but this cannot be reverted via
umask. Furthermore we are currently inconsistent as we use 600 for SysV SHM, 
but 644
for Posix one.
Maybe time for an

apr_shm_perms_set?


Regards

Rüdiger

Re: apr-1.7.2/Apache shared memory now world readable?

[Ruediger Pluem]

On 2/8/23 11:06 PM, Michael Brunnbauer wrote:
> 
> hi all,
> 
> I upgraded to apr-1.7.2 and apr-util-1.6.3 today and ran into an issue where
> Apache 2.4.55 would not start any more because it could not open the 
> ScoreBoardFile and/or SSLSessionCache. This is apparently caused by this 
> change:
> 
>   *) configure: Prefer posix name-based shared memory over SysV IPC.
>  [Jim Jagielski]
> 
> Some of my systems did not have the /dev/shm directory and on most where it
> existed, it was a normal directory, not a tmpfs. Creating the directory
> fixes the problem but I figured that I better install a tmpfs everywhere.
> But if I do that, Apache creates world readable files:
> 
>  -rw-r--r-- 1 root root 96320 Feb  8 22:56 ShM.5c729b24H72d5072a
> 
> I think this is the ScoreBoardFile from Apache. Should it be world readable?
> If no: Did I make a mistake?

I think the answer to both questions is no.

The decision to use 644 as file permissions in this case is very old though:

http://svn.apache.org/viewvc?view=revision=65135
http://svn.apache.org/viewvc?view=revision=1561260

But it was was already questioned a long time ago here:

http://svn.apache.org/viewvc?view=revision=1561384

I think this should be revisited and changed to 600.

Furthermore I think we should amend

http://svn.apache.org/viewvc?view=revision=1901037

to allow reverting that decision by a switch just like the --enable-posix-shm .

AIX already defaults back to SysV SHM:

http://svn.apache.org/viewvc?view=revision=1906825


Regards

Rüdiger

Re: [VOTE] apr 1.7.2 and apr-util 1.6.3

[Ruediger Pluem]

On 2/1/23 8:32 AM, Ruediger Pluem wrote:
> 
> 
> On 1/31/23 10:22 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 looks great
>>   [  ]  -1 something is broken
>>
> 
> 
> +1 to both.

And BTW: Thanks a ton for actually making the releases happen.

Regards

Rüdiger

Re: [VOTE] apr 1.7.2 and apr-util 1.6.3

[Ruediger Pluem]

On 1/31/23 10:22 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 looks great
>   [  ]  -1 something is broken
> 


+1 to both.

Regards

Rüdiger

Re: [VOTE] release apr-1.6.2-rc3 as APR 1.6.2

[Ruediger Pluem]

On 1/27/23 4:53 PM, Eric Covener wrote:
> On Fri, Jan 27, 2023 at 10:12 AM Ruediger Pluem  wrote:
>>
>>
>>
>> On 1/27/23 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
>>>   [  ]  +1 looks great
>>>   [  ]  -1 something is broken
>>>
>>> I'd like to call a vote in parallel to the discussion around the
>>> suitability for 1.6.x.
>>>
>>> If there is not a passing vote (or is no versioning consensus) in ~72H
>>> I will proceed with rc2 and abandon rc3. Otherwise I will release rc3.
>>>
>>> I will assume +1'es are orthogonal to the versioning issue and will
>>> just gauge the consensus in the two threads.
>>>
>>
>> +1
> 
> Just to be sure, this is +1 for rc3 or just the plan?
> 

+1 for rc3

Regards

Rüdiger

Re: [VOTE] release apr-1.6.2-rc3 as APR 1.6.2

[Ruediger Pluem]

On 1/27/23 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
>   [  ]  +1 looks great
>   [  ]  -1 something is broken
> 
> I'd like to call a vote in parallel to the discussion around the
> suitability for 1.6.x.
> 
> If there is not a passing vote (or is no versioning consensus) in ~72H
> I will proceed with rc2 and abandon rc3. Otherwise I will release rc3.
> 
> I will assume +1'es are orthogonal to the versioning issue and will
> just gauge the consensus in the two threads.
> 

+1

Regards

Rüdiger

Re: [VOTE] release apr-util-1.6.2-rc2 as apr-util 1.6.2

[Ruediger Pluem]

On 1/26/23 3:15 AM, Noel Butler wrote:
> On 24/01/2023 22:08, Graham Leggett via dev wrote:
> 
>> On 24 Jan 2023, at 02:02, Noel Butler > > wrote:
>>>
>>> This is a result of apr-util 1.6 STILL NOT patched for mariadb or mysql 10 
>>> plus 
>>>
>>> https://bz.apache.org/bugzilla/show_bug.cgi?id=61517#c5
>>>
>> If you prepare a patch for 1.6 I can take a look.
>>  
>> Is it possible to backport this to v1.6 given our rules?
>>  
>> Regards,
>> Graham
>>  
> 
> 
> The patch referenced, works on 1.6.1, its the same patch IIRC being applied 
> by RedHat, is applied by PV at Slackware, and probably
> Debian too because modern (past few years of apru using *sql) wont build 
> otherwise.

I would be in favor of hearing further opinions on whether backporting this to 
APR-UTIL 1.6 complies with our versioning rules.
If it does we should do so and release apr-util 1.6.2 with it.
If not we should release APR-UTIL 1.6.2 now, but release APR-UTIL 1.7.0 shortly 
afterwards as soon as we have folded in the MySQL
TLS stuff referred to in the other thread.

Regards

Rüdiger

Re: [VOTE] release apr-util-1.6.2-rc2 as apr-util 1.6.2

[Ruediger Pluem]

On 1/24/23 1:08 PM, Graham Leggett via dev wrote:
> On 24 Jan 2023, at 02:02, Noel Butler  > wrote:
>>
>> This is a result of apr-util 1.6 STILL NOT patched for mariadb or mysql 10 
>> plus 
>>
>> https://bz.apache.org/bugzilla/show_bug.cgi?id=61517#c5
>>
> If you prepare a patch for 1.6 I can take a look.
> 
> Is it possible to backport this to v1.6 given our rules?

Looking at the 1.7.x patch from you in the bz issue I would say yes, it should 
be possible. It does not change or add any API.
Stuff compiled against apr-util 1.6.2 with this stuff will still start with 
apr-util < 1.6.2. Of course this apr-util < 1.6.2
cannot have been compiled against a recent MariaDB / MySQL lib due to this bug. 
Hence there might be indirect issues if the
application requires this outside of APR.

Regards

Rüdiger

Re: VOTE: Release apr-1.7.1-rc2 as 1.7.1

[Ruediger Pluem]

On 1/20/23 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
>   [  ]  +1 looks great!
>   [  ]  -1 something is broken
> 
> I will let the vote run through mid-week.
> 


+1. Tested on RedHat 8.7 x86_64

Regards

Rüdiger

Re: malloc, calloc and APR pools

[Ruediger Pluem]

On 11/13/22 4:23 PM, Jim Jagielski wrote:
> This article (https://vorpus.org/blog/why-does-calloc-exist/)
> looks like it's very applicable to APR, where we do the exact
> malloc-memset trick.
> 
> Prelim testing on my macOS and Linux machines do show appreciable
> improvements. At the very least, maybe a compile-time flag??
> 

I guess the improvements depend on the amount of memory allocated.
If you just allocate just a few pages it shouldn't matter.
Which amounts did you allocate for your tests?

Regards

Rüdiger

Re: svn commit: r1902206 - /apr/apr/trunk/encoding/apr_base64.c

[Ruediger Pluem]

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?

> 
>> +encoded = (char *) apr_palloc(p, apr_base64_encode_len((int)len));
>> +apr_base64_encode(encoded, string, (int)len);
>>  
>>  return encoded;
>>  }
>>
>>
>>
> 

Regards

Rüdiger

Re: svn commit: r1902206 - /apr/apr/trunk/encoding/apr_base64.c

[Ruediger Pluem]

On 6/24/22 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, 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?

Given your explanations about I tend to say yes. Thanks for the details.

Regards

Rüdiger

Re: svn commit: r1902207 - /apr/apr/trunk/json/apr_json_decode.c

[Ruediger Pluem]

On 6/23/22 6:14 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Thu Jun 23 16:14:41 2022
> New Revision: 1902207
> 
> URL: http://svn.apache.org/viewvc?rev=1902207=rev
> Log:
> apr_json_decode: Return APR_ENOSPC if a decoded array is above INT_MAX.
> 
> * json/apr_json_decode.c(apr_json_decode_array):
>   Return APR_ENOSPC should the int counter overflow.
>   
> 
> Modified:
> apr/apr/trunk/json/apr_json_decode.c
> 
> Modified: apr/apr/trunk/json/apr_json_decode.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/json/apr_json_decode.c?rev=1902207=1902206=1902207=diff
> ==
> --- apr/apr/trunk/json/apr_json_decode.c (original)
> +++ apr/apr/trunk/json/apr_json_decode.c Thu Jun 23 16:14:41 2022
> @@ -386,6 +386,10 @@ static apr_status_t apr_json_decode_arra
>  break;
>  }
>  
> +if (count >= APR_INT32_MAX) {

Can INT_MAX and APR_INT32_MAX be different?

> +return APR_ENOSPC;
> +}
> +
>  if (APR_SUCCESS != (status = apr_json_decode_value(self, ))) 
> {
>  return status;
>  }

Regards

Rüdiger

Re: svn commit: r1902206 - /apr/apr/trunk/encoding/apr_base64.c

[Ruediger Pluem]

On 6/23/22 5:12 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Thu Jun 23 15:12:47 2022
> New Revision: 1902206
> 
> URL: http://svn.apache.org/viewvc?rev=1902206=rev
> Log:
> apr_base64: Make sure encoding/decoding lengths fit in an int >= 0.
> 
> The (old) API of apr_base64 functions has always used int for representing
> lengths and it does not return errors. Make sure to abort() if the provided
> data don't fit.
> 
> * encoding/apr_base64.c():
>   #define APR_BASE64_ENCODE_MAX and APR_BASE64_DECODE_MAX as the hard length
>   limits for encoding and decoding respectively.
> 
> * encoding/apr_base64.c(apr_base64_encode_len, apr_base64_encode,
> apr_base64_encode_binary, apr_pbase64_encode):
>   abort() if the given length is above APR_BASE64_ENCODE_MAX.
> 
> * encoding/apr_base64.c(apr_base64_decode_len, apr_base64_decode,
> apr_base64_decode_binary, apr_pbase64_decode):
>   abort() if the given plain buffer length is above APR_BASE64_DECODE_MAX.
> 
> 
> Modified:
> apr/apr/trunk/encoding/apr_base64.c
> 
> Modified: apr/apr/trunk/encoding/apr_base64.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/encoding/apr_base64.c?rev=1902206=1902205=1902206=diff
> ==
> --- apr/apr/trunk/encoding/apr_base64.c (original)
> +++ apr/apr/trunk/encoding/apr_base64.c Thu Jun 23 15:12:47 2022
> @@ -20,11 +20,20 @@
>   * ugly 'len' functions, which is quite a nasty cost.
>   */
>  
> +#undef NDEBUG /* always abort() on assert()ion failure */
> +#include 
> +
>  #include "apr_base64.h"
>  #if APR_CHARSET_EBCDIC
>  #include "apr_xlate.h"
>  #endif   /* APR_CHARSET_EBCDIC */
>  
> +/* 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?

>  /* ck but it's fast and const should make it shared text page. */
>  static const unsigned char pr2six[256] =
>  {

> @@ -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?

> +encoded = (char *) apr_palloc(p, apr_base64_encode_len((int)len));
> +apr_base64_encode(encoded, string, (int)len);
>  
>  return encoded;
>  }
> 
> 
> 

Regards

Rüdiger

Re: svn commit: r1902196 - /apr/apr/trunk/json/apr_json_decode.c

[Ruediger Pluem]

On 6/23/22 1:43 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Thu Jun 23 11:43:05 2022
> New Revision: 1902196
> 
> URL: http://svn.apache.org/viewvc?rev=1902196=rev
> Log:
> apr_json_decode: apr_array_header_t uses ints.
> 
> * json/apr_json_decode.c(apr_json_decode_array):
>   apr_array_make() want an int.
> 
> 
> Modified:
> apr/apr/trunk/json/apr_json_decode.c
> 
> Modified: apr/apr/trunk/json/apr_json_decode.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/json/apr_json_decode.c?rev=1902196=1902195=1902196=diff
> ==
> --- 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?

>  
>  if (self->p >= self->e) {
>  return APR_EOF;

Regards

Rüdiger

Re: svn commit: r1902176 - /apr/apr/trunk/build/xml.m4

[Ruediger Pluem]

On 6/23/22 1:46 AM, cove...@apache.org wrote:
> Author: covener
> Date: Wed Jun 22 23:46:43 2022
> New Revision: 1902176
> 
> URL: http://svn.apache.org/viewvc?rev=1902176=rev
> Log:
> set -lxml2 in non xml2-config case
> 
> Closes #36
> 
> Modified:
> apr/apr/trunk/build/xml.m4
> 
> Modified: apr/apr/trunk/build/xml.m4
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/build/xml.m4?rev=1902176=1902175=1902176=diff
> ==
> --- apr/apr/trunk/build/xml.m4 (original)
> +++ apr/apr/trunk/build/xml.m4 Wed Jun 22 23:46:43 2022
> @@ -177,6 +177,7 @@ AC_ARG_WITH([libxml2],
>  else
>xml2_CPPFLAGS="-I$withval/include/libxml2"
>xml2_LDFLAGS="-L$withval/lib64 -L$withval/lib"
> +  xml2_LIBS="-lxml2"
>  fi
>  
>  APR_ADDTO(CPPFLAGS, [$xml2_CPPFLAGS])
> @@ -186,6 +187,7 @@ AC_ARG_WITH([libxml2],
>  AC_CHECK_HEADERS(libxml/parser.h, AC_CHECK_LIB(xml2, 
> xmlCreatePushParserCtxt, [apu_has_libxml2=1]))
>fi
>], [
> +xml2_LIBS="-lxml2"

Why do we need this and if we need it why don't we need

APR_ADDTO(LIBS, [$xml2_LIBS])

as well?

>  AC_CHECK_HEADERS(libxml/parser.h, AC_CHECK_LIB(xml2, 
> xmlCreatePushParserCtxt, [apu_has_libxml2=1]))
>  ])
>  AC_SUBST(apu_has_libxml2)
> 
> 
> 

Regards

Rüdiger

Re: GitHub Actions CI

[Ruediger Pluem]

On 6/22/22 4:29 PM, 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!

Thanks a bunch from me as well. Did anyone experiment with

https://github.com/marketplace/actions/run-on-architecture

to test on other archs?

Regards

Rüdiger

Re: Possible apr 1.7.1 showstopper from httpd test framework

[Ruediger Pluem]

On 1/14/22 6:47 AM, William A Rowe Jr wrote:
> In addition to a broken release of brotli (where enc/dec don't specify
> -lbrotlicommon,
> even on trunk, for openssl and other consumers to ferret out that binding), 
> and
> lots of fun changes to build flags in curl 7.81 minor release (who does that?)
> there appears to be one test failure with date formatting in httpd 2.4.x 
> branch
> (including release 2.4.52) and apr 1.7.x branch (including release 1.7.0);
> 
> t/modules/include.t . 56/98 # Failed test 64 in
> t/modules/include.t at line 373
> 
> Have not had time to investigate whether this is a change in perl behavior, or
> possibly a regression caused by apr datetime handling in 1.7.x itself., but 
> any
> release apr-side should hold off just a bit to resolve this question.

I cannot reproduce this with APR 1.7.x on RedHat 8 and our Travis builds at 
least for some builds
on Ubuntu use APR 1.7 as well and do not fail.
Is this probably a Windows specific issue? Can anyone reproduce on Windows?

Regards

Rüdiger

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/

[Ruediger Pluem]

On 1/13/22 7:04 PM, Ivan Zhakov wrote:
> [[ sorry for delayed response ]]
> 
> On Fri, 7 Jan 2022 at 17:33, Yann Ylavic  wrote:
>>
>> 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..).
> Yes, it builds now. Thanks!
> 
>>
>>>
>>> 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.
> 
> 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.
> 
> 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.
> 
> If this alternative approach sounds fine, I could try to implement it.

But this could wait for a 1.7.2, correct? I am asking because there is some 
desire to get 1.7.1 out of the door soon.
And yes I would be happy with 1.7.2 that only adds this over 1.7.1 and is 
released soon after 1.7.2.

Regards

Rüdiger

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/

[Ruediger Pluem]

On 1/7/22 3:32 PM, Yann Ylavic wrote:
> 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.

That was also my read of the versioning rules. We only cannot add new stuff. 
For this we need to bump the minor version.

Regards

Rüdiger

Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c unix/filedup.c win32/filedup.c

[Ruediger Pluem]

On 1/7/22 3:02 PM, Yann Ylavic wrote:
> 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.

But if we we setaside on a subpool of readpool where the fd pool is also an 
ancestor of the subpool
like readpool ==  fd->pool, then file_bucket_setaside is a noop, because of the 
first if. Hence I think
it is not made to setaside in subpools, but only to pools which may have an 
unconnected lifeccyle.

>> 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):

Maybe a good idea.

Regards

Rüdiger

Re: Dropping Netware code from Windows apr/trunk

[Ruediger Pluem]

On 12/9/21 9:18 PM, Greg Stein wrote:
> On Thu, Dec 9, 2021 at 1:55 PM Ruediger Pluem  <mailto:rpl...@apache.org>> wrote:
> 
> On 12/9/21 6:10 PM, Greg Stein wrote:
> > On Wed, Dec 8, 2021 at 7:08 AM Ruediger Pluem  <mailto:rpl...@apache.org> <mailto:rpl...@apache.org
> <mailto:rpl...@apache.org>>> wrote:
> >>...
> >
> >     I think our API / ABI rules do not allow this for 1.8, only for 2.0.
> >     But the question is if we are wiling to break them for a platform 
> that is no longer maintained by us and is out of
> vendor support
> >     for many years. Hence it likely will not affect our users if we do 
> this in 1.8.
> >
> >
> > You could totally add APR_FOO in 1.8 as a new name for APU_FOO. Or more 
> precisely: in apr_legacy.h define APU_FOO in terms
> of APR_FOO.
> >
> > Similarly, you could rename aprutil_foo() to apr_foo() and then create 
> a wrapper aprutil_foo() that just calls apr_foo().
> >
> > The ABI would remain the same (all old functions are still present), 
> yet more symbols (apr_foo) are introduced in 1.8. Check.
> > All APU_FOO symbols are present during compilation, and more symbols 
> (APR_FOO) are introduced in 1.8. Check.
> >
> > And clearly, the wrappers and apr_legacy.h would be tossed in 2.0
> 
> I am confused now. My reply above was about dropping Netware support for 
> 1.8. The APR / APU topic was in the
> 
> 
> I think dropping Netware in 1.8 would be prohibited by our versioning 
> guidelines. Elimination of a feature is pretty big.
> 
> That said: I'm +0 on removing it. I doubt anybody would actually complain.
> 
> The rest of email is simply how it could be done for 1.8 while keeping the 
> same API/ABI, to meet the versioning rules.
>  
> 
> Re: Get rid of APU api in favor of APR for apr/turk
> 
> thread.
> But on the APR/APU topic: I guess
> 
> #define apr_foo aprutil_foo
> 
> 
> That might work? For some reason, my spidey-sense is saying there are 
> pitfalls in simply renaming a symbol in the preprocessor.
> Certainly, for sure the debug and stack traces would be wonky. There wouldn't 
> be any true ABI entrypoint named apr_foo() etc.
> (think: dynamic lookup)

Using wrappers and thus creating the symbols would be fine for me as well and 
for the reasons you mention is probably the better
approach.

Regards

Rüdiger

Re: Dropping Netware code from Windows apr/trunk

[Ruediger Pluem]

On 12/9/21 6:10 PM, Greg Stein wrote:
> On Wed, Dec 8, 2021 at 7:08 AM Ruediger Pluem  <mailto:rpl...@apache.org>> wrote:
>>...
> 
> I think our API / ABI rules do not allow this for 1.8, only for 2.0.
> But the question is if we are wiling to break them for a platform that is 
> no longer maintained by us and is out of vendor support
> for many years. Hence it likely will not affect our users if we do this 
> in 1.8.
> 
> 
> You could totally add APR_FOO in 1.8 as a new name for APU_FOO. Or more 
> precisely: in apr_legacy.h define APU_FOO in terms of APR_FOO.
> 
> Similarly, you could rename aprutil_foo() to apr_foo() and then create a 
> wrapper aprutil_foo() that just calls apr_foo().
> 
> The ABI would remain the same (all old functions are still present), yet more 
> symbols (apr_foo) are introduced in 1.8. Check.
> All APU_FOO symbols are present during compilation, and more symbols 
> (APR_FOO) are introduced in 1.8. Check.
> 
> And clearly, the wrappers and apr_legacy.h would be tossed in 2.0

I am confused now. My reply above was about dropping Netware support for 1.8. 
The APR / APU topic was in the


Re: Get rid of APU api in favor of APR for apr/turk

thread.
But on the APR/APU topic: I guess

#define apr_foo aprutil_foo

would be sufficient as users of 1.8 that would use apr_foo in their code, could 
not expect to use 2.0 without recompiling against
2.0, while 1.7 users that use aprutil_foo could expect to run against 1.8 
without recompiling.


Regards

Rüdiger

Re: Dropping Netware code from Windows apr/trunk

[Ruediger Pluem]

On 12/8/21 1:37 PM, Nick Kew wrote:
> 
> 
>> On 7 Dec 2021, at 13:50, Mladen Turk  wrote:
>>
>> There are still bunch of Netware code
>> polluting win32 code in apr/trunk.
>>
>> Since the Netware itself is discontinued for
>> quit some time, I wonder the effective purpose
>> of that code inside apr.
>>
>> I propose to remove all that
>> #ifdef NETWARE from file_io/win32 and others,
>> since IMHO those are totally unusable.
> 
> If you can do that cleanly then great!
> 
> Should we draw a clean line under released branches,
> so anything later than 1.7.x drops Netware, regardless
> of whether it's a 1.8 or a 2.0?

I think our API / ABI rules do not allow this for 1.8, only for 2.0.
But the question is if we are wiling to break them for a platform that is no 
longer maintained by us and is out of vendor support
for many years. Hence it likely will not affect our users if we do this in 1.8.


Regards

Rüdiger

Re: Get rid of APU api in favor of APR for apr/turk

[Ruediger Pluem]

I assumed this as a trunk/2.0 only proposal. Is my assumption wrong?

Regards

Rüdiger

On 12/7/21 9:46 PM, Greg Stein wrote:
> If you're talking 1.7, then as long as an apu_legacy.h header is included by 
> $appropriate with #define symbols mapping APU_FOO to
> APR_FOO. This header could be removed in apr 2.0, and user apps expected to 
> s/APU/APR/ ... the 1.x ABI would need simple apu_foo()
> wrappers around apr_foo()
> 
> Cheers,
> -g
> 
> 
> On Tue, Dec 7, 2021 at 10:50 AM Mladen Turk  > wrote:
> 
> Since apr-util API is now part of APR
> I propose to rename all that legacy APU_FOO to APR_FOO
> 
> 
> 
> Regards
> -- 
> ^TM
> 

Re: Dropping Netware code from Windows apr/trunk

[Ruediger Pluem]

On 12/7/21 2:50 PM, Mladen Turk wrote:
> There are still bunch of Netware code
> polluting win32 code in apr/trunk.
> 
> Since the Netware itself is discontinued for
> quit some time, I wonder the effective purpose
> of that code inside apr.
> 
> I propose to remove all that
> #ifdef NETWARE from file_io/win32 and others,
> since IMHO those are totally unusable.
> 
> Anyone that still needs to support legacy Netware
> can use branches/1.x.x
> 

+1

Regards

Rüdiger

Re: svn commit: r1894917 - /apr/apr/trunk/poll/unix/wakeup.c

[Ruediger Pluem]

On 11/17/21 2:39 AM, Mladen Turk wrote:
> 
> 
> On 16/11/2021 12:00, Yann Ylavic wrote:
>> On Wed, Nov 10, 2021 at 4:19 PM Yann Ylavic  wrote:
>>>
>>> Otherwise I'll revert because I have no way to test it, but I think
>>> that apr_poll_drain_wakeup_pipe() might block on Windows for the same
>>> reason as r1894914 for platforms with poll()able pipes.
>>
>> Reverted in 1.7.x, I'll leave it in trunk for now..
>>
> 
> OK, did some digging.
> 
> First it's an abomination that 512+ threads call apr_pollset_wakeup
> for the same pollset.
> 
> When I wrote that wakeup logic, 512 bytes in drain_wakeup_pipe was a
> quick hack to make it work without adding 'already send wakeup for this 
> pollset'
> 
> Here is the original comment:
> /* Although we write just one byte to the other end of the pipe
>  * during wakeup, multiple treads could call the wakeup.
>  * So simply drain out from the input side of the pipe all
>  * the data.
>  */
> 
> So it seems it's time to make it properly :)
> 
> Basically the 'apr_file_putc(1, pollset->wakeup_pipe[1])' in
> apr_pollset_wakeup should be called only if not already called
> for that pollset.
> 
> pollset should have something like 'send_wakeup' member
> that will be reset by apr_poll_drain_wakeup_pipe call.
> 
> The problem is how to make thread safe.

How about using an int flag set / read with atomic operations e.g. 
apr_atomic_cas32?
Atomic stuff should not add too much performance penalties.

Regards

Rüdiger

Re: svn commit: r1894621 - in /apr/apr/trunk: atomic/unix/builtins.c atomic/unix/builtins64.c atomic/unix/mutex.c configure.in

[Ruediger Pluem]

On 10/29/21 11:10 PM, Yann Ylavic wrote:
> On Fri, Oct 29, 2021 at 8:00 PM Ruediger Pluem  wrote:
>>
>>
>>
>> On 10/29/21 5:09 PM, yla...@apache.org wrote:
>>> Author: ylavic
>>> Date: Fri Oct 29 15:09:13 2021
>>> New Revision: 1894621
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1894621=rev
>>> Log:
>>> apr_atomic: Use __atomic builtins when available.
>>>
>>> Unlike Intel's atomic builtins (__sync_*), the more recent __atomic builtins
>>> provide atomic load and store for weakly ordered architectures like ARM32 or
>>> powerpc[64], so use them when available (gcc 4.6.3+).
>>>
>>>
>>> Modified:
>>> apr/apr/trunk/atomic/unix/builtins.c
>>> apr/apr/trunk/atomic/unix/builtins64.c
>>> apr/apr/trunk/atomic/unix/mutex.c
>>> apr/apr/trunk/configure.in
>>>
>>
>>>
>>> Modified: apr/apr/trunk/atomic/unix/mutex.c
>>> URL: 
>>> http://svn.apache.org/viewvc/apr/apr/trunk/atomic/unix/mutex.c?rev=1894621=1894620=1894621=diff
>>> ==
>>> --- apr/apr/trunk/atomic/unix/mutex.c (original)
>>> +++ apr/apr/trunk/atomic/unix/mutex.c Fri Oct 29 15:09:13 2021
>>> @@ -89,7 +89,7 @@ static APR_INLINE apr_thread_mutex_t *mu
>>>
>>>  APR_DECLARE(apr_status_t) apr_atomic_init(apr_pool_t *p)
>>>  {
>>> -return apr__atomic_generic64_init(p);
>>
>> Why is this no longer needed?
> 
> apr__atomic_generic64_init() is APR_SUCCESS when !APR_HAS_THREADS.

Thanks. I missed this. Does it make sense to keep the current call if in the 
future apr__atomic_generic64_init does something
useful when !APR_HAS_THREADS? Or does this just bloat the code and does not 
make sense and should be tackled when changes to
apr__atomic_generic64_init happen?

Regards

Rüdiger

Re: svn commit: r1894621 - in /apr/apr/trunk: atomic/unix/builtins.c atomic/unix/builtins64.c atomic/unix/mutex.c configure.in

[Ruediger Pluem]

On 10/29/21 5:09 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Fri Oct 29 15:09:13 2021
> New Revision: 1894621
> 
> URL: http://svn.apache.org/viewvc?rev=1894621=rev
> Log:
> apr_atomic: Use __atomic builtins when available.
> 
> Unlike Intel's atomic builtins (__sync_*), the more recent __atomic builtins
> provide atomic load and store for weakly ordered architectures like ARM32 or
> powerpc[64], so use them when available (gcc 4.6.3+).
> 
> 
> Modified:
> apr/apr/trunk/atomic/unix/builtins.c
> apr/apr/trunk/atomic/unix/builtins64.c
> apr/apr/trunk/atomic/unix/mutex.c
> apr/apr/trunk/configure.in
> 

> 
> Modified: apr/apr/trunk/atomic/unix/mutex.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/atomic/unix/mutex.c?rev=1894621=1894620=1894621=diff
> ==
> --- apr/apr/trunk/atomic/unix/mutex.c (original)
> +++ apr/apr/trunk/atomic/unix/mutex.c Fri Oct 29 15:09:13 2021
> @@ -89,7 +89,7 @@ static APR_INLINE apr_thread_mutex_t *mu
>  
>  APR_DECLARE(apr_status_t) apr_atomic_init(apr_pool_t *p)
>  {
> -return apr__atomic_generic64_init(p);

Why is this no longer needed?

> +return APR_SUCCESS;
>  }
>  
>  #endif /* APR_HAS_THREADS */
> 

Regards

Rüdiger

Re: svn commit: r1894551 - in /apr/apr/trunk: buckets/apr_brigade.c configure.in include/apr.h.in include/apr.hnw include/apr.hw include/apr.hwc

[Ruediger Pluem]

On 10/25/21 2:27 PM, Graham Leggett wrote:
> On 25 Oct 2021, at 12:30, Ruediger Pluem  wrote:
> 
>> begin is unused.
> 
> Will fix.
> 
>>> +
>>> +    while ((hay = memchr(hay, *(char *)needle, len))) {
>>
>> Does memchr have a defined behaviour for len == 0? This can happen if
>> *(char *)needle is found at hay[len -1 ] but the memcmp below is not zero.
>> In this case len becomes 1 below and 0 after the --len.
> 
> Code is a simplified version of this:
> 
> https://github.com/apache/apreq/blob/trunk/library/util.c#L92
> 
> I understand this to be the case.
> 
> The BSD man page on MacOS says “Zero-length strings are always identical” and 
> the function is C90.

The comment above is from the memcmp man page. I was talking about memchr. At 
least for MacOS it says:

 The memchr() function returns a pointer to the byte located, or NULL if
 no such byte exists within n bytes.

This could be interpreted in a way that with n == 0 this is not possible and 
hence NULL is returned. This would do the correct
thing as it leaves the while loop and returns NULL. The Linux man page text is 
a little different. Hence I was not sure if we have
a defined behavior in this case, but probably yes.

Regards

Rüdiger

Re: svn commit: r1894551 - in /apr/apr/trunk: buckets/apr_brigade.c configure.in include/apr.h.in include/apr.hnw include/apr.hw include/apr.hwc

[Ruediger Pluem]

On 10/25/21 12:31 PM, minf...@apache.org wrote:
> Author: minfrin
> Date: Mon Oct 25 10:31:32 2021
> New Revision: 1894551
> 
> URL: http://svn.apache.org/viewvc?rev=1894551=rev
> Log:
> apr_brigade_split_boundary: Provide a memmem implementation on platforms that
> do not have one.
> 
> Modified:
> apr/apr/trunk/buckets/apr_brigade.c
> apr/apr/trunk/configure.in
> apr/apr/trunk/include/apr.h.in
> apr/apr/trunk/include/apr.hnw
> apr/apr/trunk/include/apr.hw
> apr/apr/trunk/include/apr.hwc
> 
> Modified: apr/apr/trunk/buckets/apr_brigade.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/buckets/apr_brigade.c?rev=1894551=1894550=1894551=diff
> ==
> --- apr/apr/trunk/buckets/apr_brigade.c (original)
> +++ apr/apr/trunk/buckets/apr_brigade.c Mon Oct 25 10:31:32 2021
> @@ -387,6 +387,36 @@ APR_DECLARE(apr_status_t) apr_brigade_sp
>  return APR_SUCCESS;
>  }
>  
> +#if !APR_HAVE_MEMMEM
> +static const void *
> +memmem(const void *hay, size_t hay_len, const void *needle, size_t 
> needle_len)
> +{
> +
> +if (hay_len < needle_len || !needle_len || !hay_len) {
> +return NULL;
> +}
> +else {
> +
> +apr_size_t len = hay_len - needle_len + 1;
> +const void *end = hay + hay_len;
> +const void *begin = hay;

begin is unused.

> +
> +while ((hay = memchr(hay, *(char *)needle, len))) {

Does memchr have a defined behaviour for len == 0? This can happen if
*(char *)needle is found at hay[len -1 ] but the memcmp below is not zero.
In this case len becomes 1 below and 0 after the --len.

> +len = end - hay - needle_len + 1;
> +
> +if (memcmp(hay, needle, needle_len) == 0 ) {
> +break;
> +}
> +
> +--len;
> +++hay;
> +}
> +
> +return hay;
> +}
> +}
> +#endif
> +
>  APR_DECLARE(apr_status_t) apr_brigade_split_boundary(apr_bucket_brigade 
> *bbOut,
>   apr_bucket_brigade 
> *bbIn,
>   apr_read_type_e block,
> 

Regards

Rüdiger

Re: svn commit: r1894423 - /apr/apr/trunk/buckets/apr_brigade.c

[Ruediger Pluem]

On 10/21/21 12:23 AM, minf...@apache.org wrote:
> Author: minfrin
> Date: Wed Oct 20 22:23:10 2021
> New Revision: 1894423
> 
> URL: http://svn.apache.org/viewvc?rev=1894423=rev
> Log:
> apr_brigade_split_boundary: Rather than shaving one byte from
> a bucket, ignore the byte instead on the next go-round.
> 
> Modified:
> apr/apr/trunk/buckets/apr_brigade.c
> 
> Modified: apr/apr/trunk/buckets/apr_brigade.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/buckets/apr_brigade.c?rev=1894423=1894422=1894423=diff
> ==
> --- apr/apr/trunk/buckets/apr_brigade.c (original)
> +++ apr/apr/trunk/buckets/apr_brigade.c Wed Oct 20 22:23:10 2021

> @@ -644,11 +647,7 @@ skip:
>   *
>   * Bump one byte off, and loop round to search again.
>   */
> -apr_bucket_split(e, 1);
> -APR_BUCKET_REMOVE(e);
> -APR_BRIGADE_INSERT_TAIL(bbOut, e);
> -
> -outbytes++;
> +ignore++;

Don't we need to reset ignore to 0 after each

APR_BUCKET_REMOVE(e);
APR_BRIGADE_INSERT_TAIL(bbOut, e);

in the code above as we start with a new bucket then again and should use the 
what we read from it from the beginning?

Regards

Rüdiger

Re: svn commit: r1894380 - in /apr/apr/trunk: buckets/apr_brigade.c include/apr_buckets.h test/testbuckets.c

[Ruediger Pluem]

On 10/20/21 11:01 PM, Graham Leggett wrote:
> On 20 Oct 2021, at 10:43, Yann Ylavic  wrote:
> 
>> apr_strnstr() maybe, strnstr() is non-standard AFAICT and possibly not
>> available on all platforms (Windows)?
> 
> This needs to be memmem really, as the boundary isn’t always a string.

But I guess in this case you need to replace strncmp with memcmp as well, 
correct?

Regards

Rüdiger

Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c unix/filedup.c win32/filedup.c

[Ruediger Pluem]

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?

> +
> +a->refcount.refcount--;
> +a = b->data = new;
>  }
> -
> -apr_file_setaside(, f, reqpool);
> +else {
> +apr_file_setaside(, f, reqpool);
> +if (!apr_pool_is_ancestor(a->readpool, reqpool)) {
> +a->readpool = reqpool;
> +}
> +}
>  a->fd = fd;
>  return APR_SUCCESS;
>  }


Regards

Rüdiger

Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c unix/filedup.c win32/filedup.c

[Ruediger Pluem]

On 9/10/21 2:55 PM, Yann Ylavic wrote:
> On Fri, Sep 10, 2021 at 1:32 PM Yann Ylavic  wrote:
>>
>> On Fri, Sep 10, 2021 at 11:12 AM Ruediger Pluem  wrote:
>>>
>>> On 9/10/21 11:04 AM, Ruediger Pluem wrote:
>>>>
>>>> On 9/10/21 8:55 AM, Joe Orton wrote:
>>>>> On Fri, Sep 10, 2021 at 12:51:53AM -, yla...@apache.org wrote:
>>>>>>
>>>>>> apr_file_setaside: don't blindly kill the old cleanup and file 
>>>>>> descriptor.
>>>>>>
>>>>>> There is no cleanup with APR_FOPEN_NOCLEANUP, so apr_pool_cleanup_kill() 
>>>>>> can
>>>>>> go in the !(old_file->flags & APR_FOPEN_NOCLEANUP) block.
>>>>>>
>>>>>> The file descriptor can't be invalidated either, the file may be split in
>>>>>> multiple buckets and setting aside one shouldn't invalidate the others.
>>>>>
>>>>> Interesting.  So is the API claim that:
>>>>>
>>>>>  * @remark After calling this function, old_file may not be used
>>>>>
>>>>> not really correct, or needs to be weakened?
>>>>
>>>> I think it is correct, for the reason you state below and the reason I 
>>>> mentioned,
>>>> but I guess we probably need to review in httpd land if we still use the 
>>>> old_file
>>>> afterwards e.g. when buckets have been split.
>>
>> The fix still belongs in APR though, being able to split and setaside
>> file buckets is broken for now.
>>
>>>
>>> I think the old code with setting the file descriptor of old_file to -1, 
>>> should reveal
>>> in a clear way if we still use the old file afterwards. Keeping the file 
>>> descriptor may
>>> lead to subtle failures some time later that are hard to debug.
>>
>> OK, the semantics of apr_file_setaside() are to take ownership
>> (invalidate the old fd) and we probably can't change that.
>>
>> So we need to fix file_bucket_setaside() somehow, like this?
> 
> Well, this rather:
> 
> Index: buckets/apr_buckets_file.c
> ===
> --- buckets/apr_buckets_file.c(revision 1893196)
> +++ buckets/apr_buckets_file.c(working copy)
> @@ -18,6 +18,7 @@
>  #include "apr_general.h"
>  #include "apr_file_io.h"
>  #include "apr_buckets.h"
> +#include "apr_strings.h" /* for apr_pmemdup() */
> 
>  #if APR_HAS_MMAP
>  #include "apr_mmap.h"
> @@ -218,16 +219,34 @@ static apr_status_t file_bucket_setaside(apr_bucke
>  apr_file_t *fd = NULL;
>  apr_file_t *f = a->fd;
>  apr_pool_t *curpool = apr_file_pool_get(f);
> +apr_status_t rv;
> 
>  if (apr_pool_is_ancestor(curpool, reqpool)) {
>  return APR_SUCCESS;
>  }
> 
> +/* If the file is shared/split this bucket can't take exclusive ownership
> + * with apr_file_setaside() (which invalidates the current/old a->fd,) so
> + * we need to dup in this case.
> + */
> +if (a->refcount.refcount > 1) {
> +rv = apr_file_dup(, f, reqpool);
> +if (rv != APR_SUCCESS) {
> +return rv;
> +}
> +a->refcount.refcount--;
> +a = data->data = apr_pmemdup(reqpool, a, sizeof(*a));

Don't you need to create the memory for a / data->data via 
apr_bucket_alloc(sizeof(*a), data->list)
like we do in apr_bucket_file_make for this data structure?

Something like

a = apr_bucket_alloc(sizeof(*a), data->list);
*a = *(data->data);  // or memcpy(a, data->data, sizeof(*a))
data->data = a;

Otherwise looks good.
And you would need to bring back

old_file->filedes = -1

as a partial revert of r1893204?

Regards

Rüdiger

Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c unix/filedup.c win32/filedup.c

[Ruediger Pluem]

On 9/10/21 11:04 AM, Ruediger Pluem wrote:
> 
> 
> On 9/10/21 8:55 AM, Joe Orton wrote:
>> On Fri, Sep 10, 2021 at 12:51:53AM -, yla...@apache.org wrote:
>>> Author: ylavic
>>> Date: Fri Sep 10 00:51:53 2021
>>> New Revision: 1893204
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1893204=rev
>>> Log:
>>> apr_file_setaside: don't blindly kill the old cleanup and file descriptor.
>>>
>>> There is no cleanup with APR_FOPEN_NOCLEANUP, so apr_pool_cleanup_kill() can
>>> go in the !(old_file->flags & APR_FOPEN_NOCLEANUP) block.
>>>
>>> The file descriptor can't be invalidated either, the file may be split in
>>> multiple buckets and setting aside one shouldn't invalidate the others.
>>
>> Interesting.  So is the API claim that:
>>
>>  * @remark After calling this function, old_file may not be used
>>
>> not really correct, or needs to be weakened?
> 
> I think it is correct, for the reason you state below and the reason I 
> mentioned,
> but I guess we probably need to review in httpd land if we still use the 
> old_file
> afterwards e.g. when buckets have been split.

I think the old code with setting the file descriptor of old_file to -1, should 
reveal
in a clear way if we still use the old file afterwards. Keeping the file 
descriptor may
lead to subtle failures some time later that are hard to debug.

Regards

Rüdiger

Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c unix/filedup.c win32/filedup.c

[Ruediger Pluem]

On 9/10/21 8:55 AM, Joe Orton wrote:
> On Fri, Sep 10, 2021 at 12:51:53AM -, yla...@apache.org wrote:
>> Author: ylavic
>> Date: Fri Sep 10 00:51:53 2021
>> New Revision: 1893204
>>
>> URL: http://svn.apache.org/viewvc?rev=1893204=rev
>> Log:
>> apr_file_setaside: don't blindly kill the old cleanup and file descriptor.
>>
>> There is no cleanup with APR_FOPEN_NOCLEANUP, so apr_pool_cleanup_kill() can
>> go in the !(old_file->flags & APR_FOPEN_NOCLEANUP) block.
>>
>> The file descriptor can't be invalidated either, the file may be split in
>> multiple buckets and setting aside one shouldn't invalidate the others.
> 
> Interesting.  So is the API claim that:
> 
>  * @remark After calling this function, old_file may not be used
> 
> not really correct, or needs to be weakened?

I think it is correct, for the reason you state below and the reason I 
mentioned,
but I guess we probably need to review in httpd land if we still use the 
old_file
afterwards e.g. when buckets have been split.

> 
> old_file is also modified/invalidated in the ->buffered case where 
> old_file->thlock is removed.
> 

Regards

Rüdiger

Re: APR 1.7.1 release?

[Ruediger Pluem]

On 9/10/21 10:28 AM, Steffen Land wrote:
> Please be sure that the following two are included in 1.7.1 :
> 
> PR 63491 regression in 1.7, see 
> https://www.apachelounge.com/viewtopic.php?p=39558

r1882155 brought this already to 1.7.

> PR 61165 CPU deadlock under load, see  
> https://github.com/SpiderLabs/ModSecurity/issues/2181

Looks like to me that r1860057 is not backported yet to 1.7.

Thanks for the heads up.

Regards

Rüdiger

Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c unix/filedup.c win32/filedup.c

[Ruediger Pluem]

On 9/10/21 2:51 AM, yla...@apache.org wrote:
> Author: ylavic
> Date: Fri Sep 10 00:51:53 2021
> New Revision: 1893204
> 
> URL: http://svn.apache.org/viewvc?rev=1893204=rev
> Log:
> apr_file_setaside: don't blindly kill the old cleanup and file descriptor.
> 
> There is no cleanup with APR_FOPEN_NOCLEANUP, so apr_pool_cleanup_kill() can
> go in the !(old_file->flags & APR_FOPEN_NOCLEANUP) block.
> 
> The file descriptor can't be invalidated either, the file may be split in
> multiple buckets and setting aside one shouldn't invalidate the others.


Hmm. Is it safe to use the old_file any longer after apr_file_setaside?
The new and the old apr_file_t use the same OS file descriptor. Hence if you 
read from one the position
in the file also changes for the other. This might not matter in the unbuffered 
case, but in the buffered
case I would imagine that the buffer and the file descriptor position of 
apr_file_t which was not used for reading
get out of sync and end up in a mess.

Regards

Rüdiger

> 
> Modified:
> apr/apr/trunk/file_io/os2/filedup.c
> apr/apr/trunk/file_io/unix/filedup.c
> apr/apr/trunk/file_io/win32/filedup.c
> 
> Modified: apr/apr/trunk/file_io/os2/filedup.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/file_io/os2/filedup.c?rev=1893204=1893203=1893204=diff
> ==
> --- apr/apr/trunk/file_io/os2/filedup.c (original)
> +++ apr/apr/trunk/file_io/os2/filedup.c Fri Sep 10 00:51:53 2021
> @@ -113,14 +113,12 @@ APR_DECLARE(apr_status_t) apr_file_setas
>  }
>  
>  if (!(old_file->flags & APR_FOPEN_NOCLEANUP)) {
> +apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
> +  apr_file_cleanup);
>  apr_pool_cleanup_register(p, (void *)(*new_file), 
>apr_file_cleanup,
>apr_file_cleanup);
>  }
>  
> -old_file->filedes = -1;
> -apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
> -  apr_file_cleanup);
> -
>  return APR_SUCCESS;
>  }
> 
> Modified: apr/apr/trunk/file_io/unix/filedup.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/file_io/unix/filedup.c?rev=1893204=1893203=1893204=diff
> ==
> --- apr/apr/trunk/file_io/unix/filedup.c (original)
> +++ apr/apr/trunk/file_io/unix/filedup.c Fri Sep 10 00:51:53 2021
> @@ -179,6 +179,8 @@ APR_DECLARE(apr_status_t) apr_file_setas
>  (*new_file)->fname = apr_pstrdup(p, old_file->fname);
>  }
>  if (!(old_file->flags & APR_FOPEN_NOCLEANUP)) {
> +apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
> +  apr_unix_file_cleanup);
>  apr_pool_cleanup_register(p, (void *)(*new_file), 
>apr_unix_file_cleanup,
>((*new_file)->flags & APR_INHERIT)
> @@ -186,9 +188,6 @@ APR_DECLARE(apr_status_t) apr_file_setas
>   : apr_unix_child_file_cleanup);
>  }
>  
> -old_file->filedes = -1;
> -apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
> -  apr_unix_file_cleanup);
>  #ifndef WAITIO_USES_POLL
>  (*new_file)->pollset = NULL;
>  #endif
> 
> Modified: apr/apr/trunk/file_io/win32/filedup.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/file_io/win32/filedup.c?rev=1893204=1893203=1893204=diff
> ==
> --- apr/apr/trunk/file_io/win32/filedup.c (original)
> +++ apr/apr/trunk/file_io/win32/filedup.c Fri Sep 10 00:51:53 2021
> @@ -211,15 +211,13 @@ APR_DECLARE(apr_status_t) apr_file_setas
>  (*new_file)->fname = apr_pstrdup(p, old_file->fname);
>  }
>  if (!(old_file->flags & APR_FOPEN_NOCLEANUP)) {
> +apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
> +  file_cleanup);
>  apr_pool_cleanup_register(p, (void *)(*new_file), 
>file_cleanup,
>file_cleanup);
>  }
>  
> -old_file->filehand = INVALID_HANDLE_VALUE;
> -apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
> -  file_cleanup);
> -
>  #if APR_FILES_AS_SOCKETS
>  /* Create a pollset with room for one descriptor. */
>  /* ### check return codes */
> 
> 
> 

Re: svn commit: r1887060 - in /apr/apr/trunk: CHANGES include/apr_mmap.h mmap/unix/mmap.c test/data/mmap_large_datafile.txt test/testmmap.c

[Ruediger Pluem]

On 3/2/21 1:54 AM, Yann Ylavic wrote:
> On Tue, Mar 2, 2021 at 1:37 AM  wrote:
>>
>> Author: ylavic
>> Date: Tue Mar  2 00:37:18 2021
>> New Revision: 1887060
>>
>> URL: http://svn.apache.org/viewvc?rev=1887060=rev
>> Log:
>> Align apr_mmap()ing offset to a page boundary.  PR 65158.
> []
>> --- apr/apr/trunk/include/apr_mmap.h (original)
>> +++ apr/apr/trunk/include/apr_mmap.h Tue Mar  2 00:37:18 2021
>> @@ -62,11 +62,10 @@ typedef struct apr_mmap_tapr
>>  struct apr_mmap_t {
>>  /** The pool the mmap structure was allocated out of. */
>>  apr_pool_t *cntxt;
>> -#ifdef BEOS
>> +#if defined(BEOS)
>>  /** An area ID.  Only valid on BeOS */
>>  area_id area;
>> -#endif
>> -#ifdef WIN32
>> +#elif defined(WIN32)
>>  /** The handle of the file mapping */
>>  HANDLE mhandle;
>>  /** The start of the real memory page area (mapped view) */
>> @@ -75,6 +74,8 @@ struct apr_mmap_t {
>>  apr_off_t  pstart;
>>  apr_size_t psize;
>>  apr_off_t  poffset;
>> +#else
>> +apr_off_t poffset;
>>  #endif
>>  /** The start of the memory mapped area */
>>  void *mm;
> 
> By adding this "poffset" member to apr_mmap_t (on unixes), its layout changes.
> Can I backport this to 1.7 or should I find another way to store
> "poffset" there (like some pool/cntxt userdata)?

I don't think that this can be backported this way as it changes a non opaque 
struct on the public API.
So you would need to look for a different area to store this data.

Regards

Rüdiger

Re: svn commit: r1883868 - /apr/apr/trunk/encoding/apr_encode.c

[Ruediger Pluem]

On 11/27/20 5:54 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Fri Nov 27 16:54:50 2020
> New Revision: 1883868
> 
> URL: http://svn.apache.org/viewvc?rev=1883868=rev
> Log:
> apr_encode_base32: fix estimated output *len (when called with src == NULL).


If src == NULL we leave immediately with APR_NOTFOUND. How does the below 
change this?

> 
> Modified:
> apr/apr/trunk/encoding/apr_encode.c
> 
> Modified: apr/apr/trunk/encoding/apr_encode.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/encoding/apr_encode.c?rev=1883868=1883867=1883868=diff
> ==
> --- apr/apr/trunk/encoding/apr_encode.c (original)
> +++ apr/apr/trunk/encoding/apr_encode.c Fri Nov 27 16:54:50 2020
> @@ -665,7 +665,7 @@ APR_DECLARE(apr_status_t) apr_encode_bas
>  }
>  
>  if (len) {
> -*len = ((slen + 2) / 3 * 4) + 1;
> +*len = ((slen + 4) / 5 * 8) + 1;

This creates a much larger result. If slen == 1  e.g. 5 vs 9

>  }
>  
>  return APR_SUCCESS;
> 
> 
> 

Regards

Rüdiger

Re: svn commit: r1883801 - /apr/apr/trunk/memory/unix/apr_pools.c

[Ruediger Pluem]

On 11/25/20 11:45 AM, Yann Ylavic wrote:
> On Wed, Nov 25, 2020 at 10:20 AM Ruediger Pluem  wrote:
>>
>> On 11/24/20 10:12 PM, yla...@apache.org wrote:
>>> Author: ylavic
>>> Date: Tue Nov 24 21:12:37 2020
>>> New Revision: 1883801
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1883801=rev
>>> Log:
>>> apr_pools: follow up to r1883750 and r1883800.
>>>
>>> After r1883800, the mutex of a pool in APR_POOL_DEBUG can't be NULL, so
>>> remove useless NULL checks around locking.
>>
>> I am struggling a bit to see when the mutex could have been NULL before 
>> r1883800.
> 
> It could have been NULL because apr_pool_create_ex_debug() was adding
> the pool to the parent's children list before creating the mutex.
> In this window, apr_pool_walk_tree() (like apr_pool_find() starting
> from the global_pool) could have found the pool with its NULL mutex
> and walked it unlocked.

Ahh. Thanks for explaining. I did not consider this short period of time and 
that another thread doing
apr_pool_walk_tree could then hit the NULL mutex.

Regards

Rüdiger

Re: svn commit: r1883801 - /apr/apr/trunk/memory/unix/apr_pools.c

[Ruediger Pluem]

On 11/24/20 10:12 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Tue Nov 24 21:12:37 2020
> New Revision: 1883801
> 
> URL: http://svn.apache.org/viewvc?rev=1883801=rev
> Log:
> apr_pools: follow up to r1883750 and r1883800.
> 
> After r1883800, the mutex of a pool in APR_POOL_DEBUG can't be NULL, so
> remove useless NULL checks around locking.

I am struggling a bit to see when the mutex could have been NULL before 
r1883800.

Regards

Rüdiger

Re: svn commit: r1883728 - /apr/apr/trunk/network_io/unix/sockaddr.c

[Ruediger Pluem]

On 11/22/20 11:21 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Sun Nov 22 22:21:30 2020
> New Revision: 1883728
> 
> URL: http://svn.apache.org/viewvc?rev=1883728=rev
> Log:
> apr_sockaddr_ip_get[buf]: handle APR_UNIX family.
> 
> For unix sockets, return the path (sun_path).
> 
> Modified:
> apr/apr/trunk/network_io/unix/sockaddr.c
> 
> Modified: apr/apr/trunk/network_io/unix/sockaddr.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/network_io/unix/sockaddr.c?rev=1883728=1883727=1883728=diff
> ==
> --- apr/apr/trunk/network_io/unix/sockaddr.c (original)
> +++ apr/apr/trunk/network_io/unix/sockaddr.c Sun Nov 22 22:21:30 2020
> @@ -118,6 +118,14 @@ static apr_status_t get_remote_addr(apr_
>  APR_DECLARE(apr_status_t) apr_sockaddr_ip_getbuf(char *buf, apr_size_t 
> buflen,
>   apr_sockaddr_t *sockaddr)
>  {
> +#if APR_HAVE_SOCKADDR_UN && 0

Why && 0?

> +if (sockaddr->family == APR_UNIX) {
> +apr_size_t len = (apr_size_t)sockaddr->ipaddr_len;
> +apr_cpystrn(buf, buflen < len ? buflen : len, sockaddr->ipaddr_ptr);
> +return APR_SUCCESS;
> +}
> +#endif
> +
>  if (!apr_inet_ntop(sockaddr->family, sockaddr->ipaddr_ptr, buf, buflen)) 
> {
>  return APR_ENOSPC;
>  }
> 
> 
> 

Regards

Rüdiger

Re: [PATCH] Fix out of bounds write in apr_pbase64_encode() function

[Ruediger Pluem]

On 10/21/20 9:26 PM, Christopher Schultz wrote:
> Denis,
> 
> On 10/21/20 14:53, Denis Kovalchuk wrote:
>> Hello.
>>
>> If I am not mistaken, there is an undefined behavior in apr_pbase64_encode()
>> function:
>>
>> encoded = (char *) apr_palloc(p, apr_base64_encode_len(l));
>> l = apr_base64_encode(encoded, string, l);
>> encoded[l] = '\0'; /* make binary sequence into string */
>>
>> encoded[l] is out of bounds, because apr_base64_encode() returns the length 
>> of
>> the encoded string, including '\0'.
> 
> This is not out-of-bounds. It's just useless. There is no UB here.

The documentation says that apr_base64_encode returns "the length of the 
encoded string", but
in fact it delivers "the length of the encoded string" + 1:

*p++ = '\0';
return (int)(p - encoded);


Hence the result of apr_base64_encode is equal to apr_base64_encode_len and thus
encoded[l] would point to the first byte after the memory reserved by 
apr_palloc(p, apr_base64_encode_len(l))

I guess we either need to fix the documention of apr_base64_encode or change 
the above code to

*p = '\0';
return (int)(p - encoded);

Of course

encoded[l] = '\0';

should still be removed as it is pointless.

> 
>> As far as I understand, there is no need to
>> set '\0' at all, because apr_base64_encode() already appends it. Based on 
>> this,
>> I suggest to get rid of the explicit '\0' setting.
> 
> +1
> 
> It looks like r1780034 fixed the initial patch (r1490248) to remove an
> extra byte from the char array, but failed to see that there was another
> mistake in there.

With r1490248 there were just 2 '\0' at the end. Since r1780034 I think we are 
writing 1 byte after the allocated memory.
Fortunately this seems to be only in trunk.

Regards

Rüdiger

Re: svn commit: r1880286 - /apr/apr/trunk/build/xml.m4

[Ruediger Pluem]

On 7/25/20 11:05 AM, minf...@apache.org wrote:
> Author: minfrin
> Date: Sat Jul 25 09:05:54 2020
> New Revision: 1880286
> 
> URL: http://svn.apache.org/viewvc?rev=1880286=rev
> Log:
> Use the xml2-config tool to configure libxml2. Revert changes to expat build
> that prevented the expat library being propagated to APR.
> 
> Modified:
> apr/apr/trunk/build/xml.m4
> 
> Modified: apr/apr/trunk/build/xml.m4
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/build/xml.m4?rev=1880286=1880285=1880286=diff
> ==
> --- apr/apr/trunk/build/xml.m4 (original)
> +++ apr/apr/trunk/build/xml.m4 Sat Jul 25 09:05:54 2020

> @@ -139,56 +149,57 @@ dnl APU_FIND_LIBXML2: figure out where L
>  dnl
>  AC_DEFUN([APU_FIND_LIBXML2], [
>  
> -save_cppflags="$CPPFLAGS"
> -
>  apu_has_libxml2=0
>  apu_try_libxml2=0
>  
> +old_libs="$LIBS"
> +old_cppflags="$CPPFLAGS"
> +old_ldflags="$LDFLAGS"
> +
>  AC_ARG_WITH([libxml2],
>  [  --with-libxml2=DIR  specify libxml2 location], [
>if test "$withval" = "yes"; then
> -apu_try_libxml2=1
> -APR_ADDTO(INCLUDES, [-I/usr/include/libxml2])
> -  elif test "$withval" != "no"; then
> -apu_try_libxml2=1
> -APR_ADDTO(INCLUDES, [-I$withval/include/libxml2])
> -if test "$withval" != "/usr"; then
> -  APR_ADDTO(LDFLAGS, [-L$withval/lib])
> +AC_PATH_TOOL([XML2_CONFIG],[xml2-config])
> +if test "x$XML2_CONFIG" != 'x'; then
> +  xml2_CPPFLAGS="`$XML2_CONFIG --cflags`"
> +  xml2_LIBS="`$XML2_CONFIG --libs`"
> +
> +  APR_ADDTO(CPPFLAGS, [$xml2_CPPFLAGS])
> +  APR_ADDTO(LIBS, [$xml2_LIBS])
>  fi
> -  fi
> -  if test ${apu_try_libxml2} = "1" ; then
>  
> -#test for libxml2
> -AC_CACHE_CHECK([libxml2], [apu_cv_libxml2], [
> -  apu_libxml2_CPPFLAGS=$CPPFLAGS
> -  apu_libxml2_LIBS="$LIBS -lxml2"
> -  CPPFLAGS="$CPPFLAGS $INCLUDES"
> -  LIBS="$LIBS -lxml2"
> -  AC_TRY_LINK(
> -[#include ],
> -[xmlSAXHandler sax; xmlCreatePushParserCtxt(, NULL, NULL, 0, 
> NULL);],
> -[apu_cv_libxml2=yes],
> -[apu_cv_libxml2=no],
> -  )
> -  CPPFLAGS=$apu_libxml2_CPPFLAGS
> -  LIBS=$apu_libxml2_LIBS
> -])
> -if test $apu_cv_libxml2 = yes ; then
> -  AC_DEFINE([HAVE_LIBXML2_H], 1, [libxml2 found])
> -  apu_has_libxml2=1
> +AC_CHECK_HEADERS(libxml/parser.h, AC_CHECK_LIB(xml2, 
> xmlCreatePushParserCtxt, [apu_has_libxml2=1]))
> +  elif test "$withval" != "no"; then
> +AC_PATH_TOOL([XML2_CONFIG],[xml2-config],,[$withval/bin])
> +if test "x$XML2_CONFIG" != 'x'; then
> +  xml2_CPPFLAGS="`$XML2_CONFIG --cflags`"
> +  xml2_LIBS="`$XML2_CONFIG --libs`"
>  else
> -  apu_has_libxml2=0
> +  xml2_CPPFLAGS="-I$withval/include/libxml2"
> +  xml2_LDFLAGS="-L$withval/lib64 -L$withval/lib"

Where do we use xml2_LDFLAGS and why don't we need to set it in the case that 
we have xml2-config?


>  fi
> +
> +APR_ADDTO(CPPFLAGS, [$xml2_CPPFLAGS])
> +APR_ADDTO(LIBS, [$xml2_LIBS])
> +
> +AC_MSG_NOTICE(checking for libxml2 in $withval)
> +AC_CHECK_HEADERS(libxml/parser.h, AC_CHECK_LIB(xml2, 
> xmlCreatePushParserCtxt, [apu_has_libxml2=1]))
>fi
> +  ], [
> +AC_CHECK_HEADERS(libxml/parser.h, AC_CHECK_LIB(xml2, 
> xmlCreatePushParserCtxt, [apu_has_libxml2=1]))
>  ])
>  AC_SUBST(apu_has_libxml2)
>  
>  if test ${apu_has_libxml2} = "1" ; then
> -  APR_ADDTO(APRUTIL_EXPORT_LIBS, [-lxml2])
> -  APR_ADDTO(LIBS, [-lxml2])
> +  APR_ADDTO(APRUTIL_CPPFLAGS, [$xml2_CPPFLAGS])
> +  APR_ADDTO(APRUTIL_PRIV_INCLUDES, [$xml2_CPPFLAGS])
> +  APR_ADDTO(APRUTIL_LIBS, [$xml2_LIBS])
>  fi
>  
> -CPPFLAGS=$save_cppflags
> +LIBS="$old_libs"
> +CPPFLAGS="$old_cppflags"
> +LDFLAGS="$old_ldflags"
> +
>  ])
>  
>  dnl
> 

Regards

Rüdiger

Re: apr_jose_decode.c gcc warnings

[Ruediger Pluem]

On 3/28/20 11:35 AM, Graham Leggett wrote:
> On 27 Mar 2020, at 21:20, Ruediger Pluem  wrote
> :
>> Why are functions that are supposed to be static in the apr_ namespace?
>> IMHO this is confusing when reading code.
> 
> Are you asking the actual question why, or are you just pointing out that 
> they shouldn’t be in the apr_ namespace?

Both. But my current opinion expressed in the statement may change depending on 
the answer to the question.

Regards

Rüdiger

Re: apr_jose_decode.c gcc warnings

[Ruediger Pluem]

On 3/27/20 12:07 PM, Graham Leggett wrote:
> On 27 Mar 2020, at 10:48, Joe Orton  wrote:
> 
>> I assume all these functions should be declared static but haven't 
>> looked at the code.  branches/1.7.x as at r1875766 -
> 
> If they’re not in the header file they should be static, yes.

Why are functions that are supposed to be static in the apr_ namespace?
IMHO this is confusing when reading code.

Regards

Rüdiger

Re: apr_jose_decode.c gcc warnings

[Ruediger Pluem]

On 3/27/20 9:48 AM, Joe Orton wrote:
> I assume all these functions should be declared static but haven't 
> looked at the code.  branches/1.7.x as at r1875766 -
> 
> jose/apr_jose_decode.c:21:14: warning: no previous prototype for 
> ‘apr_jose_flatten’ [-Wmissing-prototypes]
>21 | apr_status_t apr_jose_flatten(apr_bucket_brigade *bb, apr_jose_text_t 
> *in,
>   |  ^~~~
> jose/apr_jose_decode.c:40:14: warning: no previous prototype for 
> ‘apr_jose_decode_jwk’ [-Wmissing-prototypes]
>40 | apr_status_t apr_jose_decode_jwk(apr_jose_t **jose,
>   |  ^~~
> jose/apr_jose_decode.c:75:14: warning: no previous prototype for 
> ‘apr_jose_decode_jwks’ [-Wmissing-prototypes]
>75 | apr_status_t apr_jose_decode_jwks(apr_jose_t **jose,
>   |  ^~~~
> jose/apr_jose_decode.c:116:14: warning: no previous prototype for 
> ‘apr_jose_decode_jwt’ [-Wmissing-prototypes]
>   116 | apr_status_t apr_jose_decode_jwt(apr_jose_t **jose,
>   |  ^~~
> jose/apr_jose_decode.c:151:14: warning: no previous prototype for 
> ‘apr_jose_decode_data’ [-Wmissing-prototypes]
>   151 | apr_status_t apr_jose_decode_data(apr_jose_t **jose, const char *typ,
>   |  ^~~~
> jose/apr_jose_decode.c:172:14: warning: no previous prototype for 
> ‘apr_jose_decode_jws_signature’ [-Wmissing-prototypes]
>   172 | apr_status_t apr_jose_decode_jws_signature(apr_jose_t **jose,
>   |  ^
> jose/apr_jose_decode.c:272:14: warning: no previous prototype for 
> ‘apr_jose_decode_jwe_recipient’ [-Wmissing-prototypes]
>   272 | apr_status_t apr_jose_decode_jwe_recipient(apr_jose_t **jose,
>   |  ^
> jose/apr_jose_decode.c:389:14: warning: no previous prototype for 
> ‘apr_jose_decode_compact_jws’ [-Wmissing-prototypes]
>   389 | apr_status_t apr_jose_decode_compact_jws(apr_jose_t **jose,
>   |  ^~~
> jose/apr_jose_decode.c:489:14: warning: no previous prototype for 
> ‘apr_jose_decode_compact_jwe’ [-Wmissing-prototypes]
>   489 | apr_status_t apr_jose_decode_compact_jwe(apr_jose_t **jose, const 
> char *left,
>   |  ^~~
> jose/apr_jose_decode.c:636:14: warning: no previous prototype for 
> ‘apr_jose_decode_compact’ [-Wmissing-prototypes]
>   636 | apr_status_t apr_jose_decode_compact(apr_jose_t **jose, const char 
> *typ,
>   |  ^~~
> jose/apr_jose_decode.c:817:14: warning: no previous prototype for 
> ‘apr_jose_decode_json_jws’ [-Wmissing-prototypes]
>   817 | apr_status_t apr_jose_decode_json_jws(apr_jose_t **jose, 
> apr_json_value_t *val,
>   |  ^~~~
> jose/apr_jose_decode.c:1174:14: warning: no previous prototype for 
> ‘apr_jose_decode_json_jwe’ [-Wmissing-prototypes]
>  1174 | apr_status_t apr_jose_decode_json_jwe(apr_jose_t **jose, 
> apr_json_value_t *val,
>   |  ^~~~
> jose/apr_jose_decode.c:1578:14: warning: no previous prototype for 
> ‘apr_jose_decode_json’ [-Wmissing-prototypes]
>  1578 | apr_status_t apr_jose_decode_json(apr_jose_t **jose, const char *typ,
>   |  ^~~~
> 
> 

Hmm. Their names suggests that they are part of an API and hence should have 
prototypes in a header file.

Regards

Rüdiger

Re: Better choice for Linux semaphore than spinlock?

[Ruediger Pluem]

On 10/07/2019 08:40 PM, Branko Čibej wrote:
> On Mon, 7 Oct 2019, 19:47 Doug Robinson,  > wrote:
> 
> Folks:
> 
> I spoke with this user late last week.  They stated that they can only 
> get approximately 400 parallel SVN operations
> before the "system time" consumes all available CPU for an 8-core 
> machine.  Adding more cores won't help because of
> the nature of spin locks (it makes things worse).  Turns out that even 
> with ~100 parallel SVN operations the "system
> time" starts becoming significant/measurable (~10%).  Both HTTP 
> (mod_dav_svn) and "svnserve" protocols participate
> in the lock contention.
> 
> Your help would be greatly appreciated.
> 
> 
> 
> Whew. So. Reducing this issue to "use a more efficient lock" is not going to 
> work, and you provided far too little
> information to even attempt a diagnosis. For starters, I recommend gathering 
> as much info as possible (anonymised of
> course) about the server configuration, everything from httpd an svnserve to 
> the repository config and underlying
> filesystem, if possible. Getting stack traces of the "stuck" threads would be 
> necessary, too. Without knowing exactly
> what is happening, these kinds of problems are extremely hard to understand, 
> let alone fix.

Plus depending on which part of the code requires this lock a different locking 
mechanism that might suit better for
this use case can possibly be chosen via configuration changes (e.g. httpd 
allows this for most of its locking).

Regards

Rüdiger

Re: svn commit: r1862071 - in /apr/apr/trunk: file_io/os2/dir.c file_io/unix/dir.c file_io/win32/dir.c include/apr_file_info.h test/testdir.c

[Ruediger Pluem]

On 08/25/2019 04:04 PM, Branko Čibej wrote:
> On 24.08.2019 16:39, Ivan Zhakov wrote:
>> On Thu, 25 Jul 2019 at 15:58, Ivan Zhakov  wrote:
>>> On Mon, 8 Jul 2019 at 20:05, Ivan Zhakov  wrote:
 On Wed, 3 Jul 2019 at 18:37, Joe Orton  wrote:
> On Wed, Jul 03, 2019 at 02:43:02PM +0300, Ivan Zhakov wrote:
>> They also make the existing old API unusable for many cases thus
>> making a switch to the new API mandatory, even though it doesn't have
>> to be so (see below).
>>
>> APR 1.x did not specify that the result of apr_dir_read() has to be 
>> allocated
>> in dir->pool:
>>
>>   
>> https://apr.apache.org/docs/apr/1.7/group__apr__dir.html#ga3e4ee253e0c712160bee10bfb9c8e4a8
>>
>> This function does not accept a pool argument, and I think that it's 
>> natural
>> to assume limited lifetime in such case. This is what the current major 
>> APR
>> users (httpd, svn) do, and other users should probably be ready for that 
>> too,
>> since on Win32 the subsequent calls to apr_dir_read() already invalidate 
>> the
>> previous apr_finfo_t.
> Are there many examples in the APR API where returned strings are not
> either pool-allocated or constant (process lifetime)?  It seems unusual
> and very surprising to me.
>
> A hypothetical API consumer can have made either assumption:
>
> a) of constant memory (true with limits on Win32, breaks for Unix), or
> b) of pool-allocated string lifetime (works for Unix, breaks for Win32)
>
> neither is documented and both are broken by current implementations. It
> is impossible to satisfy both so anybody can argue that fixing either
> implementation to match the other is a regression.
>
> Doubling down on (a) over (b) seems strongly inferior to me, the API
> cannot support this without introducing runtime overhead for all callers
> (a new iterpool in the dir object), so I'd rather fix this properly with
> a new API.
>
> I'd be fine with leaving Win32 apr_dir_read() behaviour as-is for 1.x at
> the same as introducing _pread() if desired - making the strdup only
> happen for _pread would only be a minor tweak, not a whole new subpool.
>
 There is example of apr_hash_t.ITERATOR that is statically allocated in
 the hash object and returned to the caller.

 Also the native readdir() API behaves the same way [1]:
 [[[
 The returned pointer, and pointers within the structure, might be 
 invalidated
 or the structure or the storage areas might be overwritten by a subsequent 
 call
 to readdir() on the same directory stream.
 ]]]

 From this perspective the current behavior of apr_dir_read on Unix looks
 inconsistent and surprising for the API user because it is impossible to 
 use
 in a loop without unbounded memory usage.

 I strongly agree that the revved version of this function that accepts a 
 POOL
 argument will be good from both usage and implementation terms. I would be 
 +1
 on adding such API. But I also think that unless we fix the original issue
 by using the preallocated buffer/iterpool, the whole thing would still be
 problematic for any existing API user.

 More detailed: if we take this opportunity to choose and document the API 
 to
 consistently return a temporary result, then as the caller I can either 
 process
 the data during iteration or manually put it into a long-living pool if 
 that is
 necessary. I think that all current callers work with the API this way. 
 And I
 can also switch to the new revved function to manually specify the pool and
 better control the allocations. If I would like to support both APR 1.x 
 and 2.x
 the compatibility is also straightforward to achieve.

 If we instead choose an option where the results may be allocated
 in the dir object pool, then as the caller I cannot avoid unbounded memory
 usage when supporting both APR 1.x and 2.x. And this approach also 
 introduces
 the unavoidable the unbounded memory usage for all Win32 users that
 do not update their code.

 [1] 
 https://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html#tag_16_475

>>> Is there any feedback or thoughts on these proposed changes?
>>>
>>> Thanks!
>>>
>> For what it's worth, I'm -1 on the changes done in r1862071 and
>> r1862435 for the reasons listed above.
>>
>> PS: No idea if I can actually veto changes, because while I'm a
>> committer on the APR project, I am not in its PMC. But "-1" properly
>> expresses my technical opinion on this topic.
> 
> 
> I agree with your analysis. I think it would be best if you just
> implemented your proposal, including the revised version of the API. The
> unbounded-memory case you describe is pretty much a showstopper. The
> allocation choices do have 

Re: crash during httpd cleanup when using APR debug library (APR_POOL_DEBUG)

[Ruediger Pluem]

On 07/17/2019 11:43 AM, Rainer Jung wrote:
> Am 17.07.2019 um 10:03 schrieb Ruediger Pluem:
>>
>>
>> On 07/16/2019 11:28 PM, Rainer Jung wrote:
>>> cross-posted to APR+HTTPD
>>>
>>> Crahs happens in #2  0x7faf4c154945 in raise () from /lib64/libc.so.6
>>> #3  0x7faf4c155f21 in abort () from /lib64/libc.so.6
>>> #4  0x7faf4c14d810 in __assert_fail () from /lib64/libc.so.6
>>> #5  0x7faf4c694219 in __pthread_tpp_change_priority () from 
>>> /lib64/libpthread.so.0
>>> #6  0x7faf4c68cd76 in __pthread_mutex_lock_full () from 
>>> /lib64/libpthread.so.0
>>> #7  0x7faf4cd07c29 in apr_thread_mutex_lock (mutex=0x2261fe0) at 
>>> locks/unix/thread_mutex.c:108
>>> #8  0x7faf4cd08603 in apr_pool_walk_tree (pool=0x225a710, 
>>> fn=0x7faf4cd07fc0 , data=0x7faf45777c90)
>>> at memory/unix/apr_pools.c:1515
>>> #9  0x7faf4cd08630 in apr_pool_walk_tree (pool=0x6a3ce0, 
>>> fn=0x7faf4cd07fc0 , data=0x7faf45777c90) at
>>> memory/unix/apr_pools.c:1521
>>> #10 0x7faf4cd08630 in apr_pool_walk_tree (pool=0x6a3770, 
>>> fn=0x7faf4cd07fc0 , data=0x7faf45777c90) at
>>> memory/unix/apr_pools.c:1521
>>> #11 0x7faf4cd08630 in apr_pool_walk_tree (pool=0x6a3110, 
>>> fn=0x7faf4cd07fc0 , data=0x7faf45777c90) at
>>> memory/unix/apr_pools.c:1521
>>> #12 0x7faf4cd086df in apr_pool_num_bytes (pool=0x6d81, recurse=>> optimized out>) at
>>> memory/unix/apr_pools.c:2304
>>> #13 0x7faf4cd0898f in apr_pool_log_event (pool=0x225a710, 
>>> event=0x7faf4cd16e74 "PCALLOC", file_line=0x7faf4cd16d78
>>> "locks/unix/thread_mutex.c:50", deref=-1)
>>>  at memory/unix/apr_pools.c:1543
>>> #14 0x7faf4cd098b8 in apr_pcalloc_debug (pool=0x225a710, size=64, 
>>> file_line=0x7faf4cd16d78
>>> "locks/unix/thread_mutex.c:50") at memory/unix/apr_pools.c:1814
>>> #15 0x7faf4cd07ce5 in apr_thread_mutex_create (mutex=0x225a798, 
>>> flags=1, pool=0x225a710) at
>>> locks/unix/thread_mutex.c:50
>>> #16 0x7faf4cd0a164 in apr_pool_clear_debug (pool=0x225a710, 
>>> file_line=0x488f09 "mpm_fdqueue.c:236") at
>>> memory/unix/apr_pools.c:1911
>>> #17 0x0046c455 in ap_queue_info_push_pool (queue_info=0x22648b0, 
>>> pool_to_recycle=0x225a710) at mpm_fdqueue.c:236
>>> #18 0x7faf4bf18821 in process_lingering_close (cs=0x78d670) at 
>>> event.c:1457
>>> #19 0x7faf4bf196a8 in worker_thread (thd=0x6cae80, dummy=>> optimized out>) at event.c:2083
>>> #20 0x7faf4c68b5f0 in start_thread () from /lib64/libpthread.so.0
>>> #21 0x7faf4c1f684d in clone () from /lib64/libc.so.6
>>>
>>> So it seems a mutex gets created, which allocates memory, which in turn 
>>> triggers debug logging, which walks pools and
>>> finally tries to lock the not yet initialized lock.
>>>
>>> Anyone aware of that? Any ideas how to fix?
>>
>> This is strange. Before apr_thread_mutex_create is called by 
>> apr_pool_clear_debug pool->mutex is set to NULL. So IMHO in
>> frame #7 mutex should be NULL.
>> Which version of APR are you using?
> 
> 1.7 with a few debug patches, that should really not make a difference here 
> (but might offset line numbers a bit).
> 1.7.0, 1.7.x, 1.6.5 and 1.6.x do not differ in apr_pools.c.

I was looking at apr trunk. Maybe r1481186 fixes your issue.

Regards

Rüdiger

Re: crash during httpd cleanup when using APR debug library (APR_POOL_DEBUG)

[Ruediger Pluem]

On 07/16/2019 11:28 PM, Rainer Jung wrote:
> cross-posted to APR+HTTPD
> 
> Crahs happens in #2  0x7faf4c154945 in raise () from /lib64/libc.so.6
> #3  0x7faf4c155f21 in abort () from /lib64/libc.so.6
> #4  0x7faf4c14d810 in __assert_fail () from /lib64/libc.so.6
> #5  0x7faf4c694219 in __pthread_tpp_change_priority () from 
> /lib64/libpthread.so.0
> #6  0x7faf4c68cd76 in __pthread_mutex_lock_full () from 
> /lib64/libpthread.so.0
> #7  0x7faf4cd07c29 in apr_thread_mutex_lock (mutex=0x2261fe0) at 
> locks/unix/thread_mutex.c:108
> #8  0x7faf4cd08603 in apr_pool_walk_tree (pool=0x225a710, 
> fn=0x7faf4cd07fc0 , data=0x7faf45777c90)
> at memory/unix/apr_pools.c:1515
> #9  0x7faf4cd08630 in apr_pool_walk_tree (pool=0x6a3ce0, 
> fn=0x7faf4cd07fc0 , data=0x7faf45777c90) at
> memory/unix/apr_pools.c:1521
> #10 0x7faf4cd08630 in apr_pool_walk_tree (pool=0x6a3770, 
> fn=0x7faf4cd07fc0 , data=0x7faf45777c90) at
> memory/unix/apr_pools.c:1521
> #11 0x7faf4cd08630 in apr_pool_walk_tree (pool=0x6a3110, 
> fn=0x7faf4cd07fc0 , data=0x7faf45777c90) at
> memory/unix/apr_pools.c:1521
> #12 0x7faf4cd086df in apr_pool_num_bytes (pool=0x6d81, recurse= optimized out>) at memory/unix/apr_pools.c:2304
> #13 0x7faf4cd0898f in apr_pool_log_event (pool=0x225a710, 
> event=0x7faf4cd16e74 "PCALLOC", file_line=0x7faf4cd16d78
> "locks/unix/thread_mutex.c:50", deref=-1)
> at memory/unix/apr_pools.c:1543
> #14 0x7faf4cd098b8 in apr_pcalloc_debug (pool=0x225a710, size=64, 
> file_line=0x7faf4cd16d78
> "locks/unix/thread_mutex.c:50") at memory/unix/apr_pools.c:1814
> #15 0x7faf4cd07ce5 in apr_thread_mutex_create (mutex=0x225a798, flags=1, 
> pool=0x225a710) at
> locks/unix/thread_mutex.c:50
> #16 0x7faf4cd0a164 in apr_pool_clear_debug (pool=0x225a710, 
> file_line=0x488f09 "mpm_fdqueue.c:236") at
> memory/unix/apr_pools.c:1911
> #17 0x0046c455 in ap_queue_info_push_pool (queue_info=0x22648b0, 
> pool_to_recycle=0x225a710) at mpm_fdqueue.c:236
> #18 0x7faf4bf18821 in process_lingering_close (cs=0x78d670) at 
> event.c:1457
> #19 0x7faf4bf196a8 in worker_thread (thd=0x6cae80, dummy= out>) at event.c:2083
> #20 0x7faf4c68b5f0 in start_thread () from /lib64/libpthread.so.0
> #21 0x7faf4c1f684d in clone () from /lib64/libc.so.6
> 
> So it seems a mutex gets created, which allocates memory, which in turn 
> triggers debug logging, which walks pools and
> finally tries to lock the not yet initialized lock.
> 
> Anyone aware of that? Any ideas how to fix?

This is strange. Before apr_thread_mutex_create is called by 
apr_pool_clear_debug pool->mutex is set to NULL. So IMHO in
frame #7 mutex should be NULL.
Which version of APR are you using?

Regards

Rüdiger

Re: svn commit: r1862071 - in /apr/apr/trunk: file_io/os2/dir.c file_io/unix/dir.c file_io/win32/dir.c include/apr_file_info.h test/testdir.c

[Ruediger Pluem]

On 07/03/2019 05:37 PM, Joe Orton wrote:
> On Wed, Jul 03, 2019 at 02:43:02PM +0300, Ivan Zhakov wrote:
>> They also make the existing old API unusable for many cases thus 
>> making a switch to the new API mandatory, even though it doesn't have 
>> to be so (see below).
>>
>> APR 1.x did not specify that the result of apr_dir_read() has to be allocated
>> in dir->pool:
>>
>>   
>> https://apr.apache.org/docs/apr/1.7/group__apr__dir.html#ga3e4ee253e0c712160bee10bfb9c8e4a8
>>
>> This function does not accept a pool argument, and I think that it's natural
>> to assume limited lifetime in such case. This is what the current major APR
>> users (httpd, svn) do, and other users should probably be ready for that too,
>> since on Win32 the subsequent calls to apr_dir_read() already invalidate the
>> previous apr_finfo_t.
> 
> Are there many examples in the APR API where returned strings are not 
> either pool-allocated or constant (process lifetime)?  It seems unusual 
> and very surprising to me.
> 
> A hypothetical API consumer can have made either assumption:
> 
> a) of constant memory (true with limits on Win32, breaks for Unix), or
> b) of pool-allocated string lifetime (works for Unix, breaks for Win32)
> 
> neither is documented and both are broken by current implementations. It 
> is impossible to satisfy both so anybody can argue that fixing either 
> implementation to match the other is a regression.  
> 
> Doubling down on (a) over (b) seems strongly inferior to me, the API 
> cannot support this without introducing runtime overhead for all callers 
> (a new iterpool in the dir object), so I'd rather fix this properly with 
> a new API.
> 
> I'd be fine with leaving Win32 apr_dir_read() behaviour as-is for 1.x at 
> the same as introducing _pread() if desired - making the strdup only 
> happen for _pread would only be a minor tweak, not a whole new subpool.
> 
+1

Regards

Rüdiger

Re: svn commit: r1860150 - in /apr/apr/trunk: ./ CHANGES CMakeLists.txt include/apr.hwc xml/apr_xml_xmllite.c

[Ruediger Pluem]

On 2019/05/28 09:12:01, Ruediger Pluem  wrote: 
> 
> 
> On 2019/05/27 17:17:53, Ivan Zhakov  wrote: 
> > On Mon, 27 May 2019 at 20:13,  wrote:
> > >
> > > Author: ivan
> > > Date: Mon May 27 17:13:43 2019
> > > New Revision: 1860150
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1860150=rev
> > > Log:
> > > Windows platform: Provide a native XML parser implementation based on
> > > XmlLite [1].
> > >
> > > Start using it by default if we weren't explicitly told to build with 
> > > either
> > > libxml2 or expat.
> > >
> > > (This is a merge of the `xmllite` branch.)
> > >
> > > Doing so reduces the amount of dependencies required for building and 
> > > using
> > > APR, which I believe to be a good thing in this particular case. And it 
> > > also
> > > means that by default we would be relying on the native component of the 
> > > OS
> > > rather than on a 3rd-party library.
> > >
> > Hey all,
> > 
> > Switching to XmlLite on the Windows platform has been one of those
> > changes that I had in mind for the Win32 part in APR 2.0.
> > 
> > Doing so reduces the amount of dependencies required for building and
> > using APR, which I believe to be a good thing in this particular case.
> > And it also means that by default we would be relying on the native
> > component of the OS rather than on a 3rd-party library.
> > 
> > Please let me know if there would be any objections against this change.
> 
> No objection, but it breaks my build on Linux as it tries to compile 
> apr_xml_xmllite.c despite expat is present (OS provided) and apr_arch_utf8.h 
> is not available:
> 
> /bin/sh /home/pluem/apache/httpd-trunk/srclib/apr/libtool --silent 
> --mode=compile gcc -pthread  -O2 -Wall -g -DHAVE_CONFIG_H  -DLINUX 
> -D_REENTRANT -D_GNU_SOURCE   -I./include 
> -I/home/pluem/apache/httpd-trunk/srclib/apr/include/arch/unix 
> -I./include/arch/unix 
> -I/home/pluem/apache/httpd-trunk/srclib/apr/include/arch/unix 
> -I/home/pluem/apache/httpd-trunk/srclib/apr/include 
> -I/home/pluem/apache/httpd-trunk/srclib/apr/include/private 
> -I/home/pluem/apache/httpd-trunk/srclib/apr/include/private 
> -I/usr/include/nss3 -I/usr/include/nspr4   -I/usr/include/mysql -o 
> xml/apr_xml_xmllite.lo -c xml/apr_xml_xmllite.c && touch 
> xml/apr_xml_xmllite.lo
> xml/apr_xml_xmllite.c:18:27: fatal error: apr_arch_utf8.h: No such file or 
> directory
>  #include "apr_arch_utf8.h"
>^
> compilation terminated.
> make[1]: *** [xml/apr_xml_xmllite.lo] Error 1
> make[1]: Leaving directory `/home/pluem/apache/httpd-trunk/srclib/apr'
> make: *** [all-recursive] Error 1
> 

Was easier to fix than I thought: r1860208

Regards

Rüdiger

Re: svn commit: r1860150 - in /apr/apr/trunk: ./ CHANGES CMakeLists.txt include/apr.hwc xml/apr_xml_xmllite.c

[Ruediger Pluem]

On 2019/05/27 17:17:53, Ivan Zhakov  wrote: 
> On Mon, 27 May 2019 at 20:13,  wrote:
> >
> > Author: ivan
> > Date: Mon May 27 17:13:43 2019
> > New Revision: 1860150
> >
> > URL: http://svn.apache.org/viewvc?rev=1860150=rev
> > Log:
> > Windows platform: Provide a native XML parser implementation based on
> > XmlLite [1].
> >
> > Start using it by default if we weren't explicitly told to build with either
> > libxml2 or expat.
> >
> > (This is a merge of the `xmllite` branch.)
> >
> > Doing so reduces the amount of dependencies required for building and using
> > APR, which I believe to be a good thing in this particular case. And it also
> > means that by default we would be relying on the native component of the OS
> > rather than on a 3rd-party library.
> >
> Hey all,
> 
> Switching to XmlLite on the Windows platform has been one of those
> changes that I had in mind for the Win32 part in APR 2.0.
> 
> Doing so reduces the amount of dependencies required for building and
> using APR, which I believe to be a good thing in this particular case.
> And it also means that by default we would be relying on the native
> component of the OS rather than on a 3rd-party library.
> 
> Please let me know if there would be any objections against this change.

No objection, but it breaks my build on Linux as it tries to compile 
apr_xml_xmllite.c despite expat is present (OS provided) and apr_arch_utf8.h is 
not available:

/bin/sh /home/pluem/apache/httpd-trunk/srclib/apr/libtool --silent 
--mode=compile gcc -pthread  -O2 -Wall -g -DHAVE_CONFIG_H  -DLINUX -D_REENTRANT 
-D_GNU_SOURCE   -I./include 
-I/home/pluem/apache/httpd-trunk/srclib/apr/include/arch/unix 
-I./include/arch/unix 
-I/home/pluem/apache/httpd-trunk/srclib/apr/include/arch/unix 
-I/home/pluem/apache/httpd-trunk/srclib/apr/include 
-I/home/pluem/apache/httpd-trunk/srclib/apr/include/private 
-I/home/pluem/apache/httpd-trunk/srclib/apr/include/private -I/usr/include/nss3 
-I/usr/include/nspr4   -I/usr/include/mysql -o xml/apr_xml_xmllite.lo -c 
xml/apr_xml_xmllite.c && touch xml/apr_xml_xmllite.lo
xml/apr_xml_xmllite.c:18:27: fatal error: apr_arch_utf8.h: No such file or 
directory
 #include "apr_arch_utf8.h"
   ^
compilation terminated.
make[1]: *** [xml/apr_xml_xmllite.lo] Error 1
make[1]: Leaving directory `/home/pluem/apache/httpd-trunk/srclib/apr'
make: *** [all-recursive] Error 1


Regards

Rüdiger

Re: [vote] Win32 Decision Point

[Ruediger Pluem]

On 04/24/2019 09:31 PM, William A Rowe Jr wrote:
> Some 17 years later we are at a crossroads, because the win32 code
> is somewhat illegible and harder to maintain due to the ANSI-vs-UTF8,
> Win9x-vs-NT code paths.
> 
> NT won. The only remaining question is how many apr consumers are
> leveraging ANSI-specific builds for local code page semantics, vs how
> many are willing to treat all system resources as utf-8 names, and for
> ANSI, willing to live on the 1.x branch in perpetuity? These are builds
> that explicitly toggle ANSI in spite of whatever OS the binary runs on.
> 
> So the vote is pretty simple, I propose to strip all ANSI 8-bit logic from
> the apr (2.0) trunk/ and leave only the utf8->wide char logic remaining.
> Committers and community both, please choose one below,
> 
> [ ] Please retain the ANSI logic in APR 2.0 on Win32
> 
> [ X ] Please drop 8-bit and focus only on utf-8 resource names on Win32.
> 
> Will leave this question open a full 10 days to get the widest sampling 
> of opinions.
> 
> 


Regards

Rüdiger

Re: Verifying printf() formatting of types

[Ruediger Pluem]

On 03/20/2019 11:00 AM, Stefan Sperling wrote:
> On Tue, Mar 19, 2019 at 07:30:09PM -0500, William A Rowe Jr wrote:
>> According to my observations, apr_time_t should match the APR_TIME_T_FMT
>> token in every case. Please inspect that line of httpd code to see how some
>> non-apr_time_t value was passed in APR_TIME_T_FMT formatting.
> 
> Indeed, this value is not a time_t, it's an apr_int64_t, i.e. long.
> 
> The problematic format string is in this bit code from proxy_util.c
> starting at line 3176:
> 
> ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(00959)  
>  
> "ap_proxy_connect_backend disabling worker for (%s) for 
> %" 
> APR_TIME_T_FMT "s",
> worker->s->hostname_ex, apr_time_sec(worker->s->retry)); 
> 
> This assumes apr_time_sec returns apr_time_t, but in fact apr_time_sec is
> a macro. So the expression returns the type of the variable passed in,
> which in this case is apr_interval_time_t.

Possibly stupid idea, but what if the macro does a cast to apr_time_t? Would 
that solve the issue?

Regards

Rüdiger

Re: svn commit: r1841078 - in /apr/apr/trunk: CHANGES apr.dsp atomic/unix/builtins64.c atomic/unix/mutex64.c atomic/win32/apr_atomic64.c include/apr_atomic.h include/arch/unix/apr_arch_atomic.h test/t

[Ruediger Pluem]

On 09/18/2018 04:02 PM, Jim Jagielski wrote:
> 
> 
>> On Sep 18, 2018, at 9:53 AM, Jim Jagielski > <mailto:j...@jagunet.com>> wrote:
>>
>>
>>
>>> On Sep 18, 2018, at 9:33 AM, Ruediger Pluem >> <mailto:rpl...@apache.org>> wrote:
>>>
>>>
>>>
>>> On 09/17/2018 05:50 PM, j...@apache.org <mailto:j...@apache.org> wrote:
>>>> Author: jim
>>>> Date: Mon Sep 17 15:50:19 2018
>>>> New Revision: 1841078
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1841078=rev
>>>> Log:
>>>> Add in Atomics for 64bit ints
>>>>
>>>> Added:
>>>>   apr/apr/trunk/atomic/unix/builtins64.c   (with props)
>>>>   apr/apr/trunk/atomic/unix/mutex64.c   (with props)
>>>>   apr/apr/trunk/atomic/win32/apr_atomic64.c   (with props)
>>>> Modified:
>>>>   apr/apr/trunk/CHANGES
>>>>   apr/apr/trunk/apr.dsp
>>>>   apr/apr/trunk/include/apr_atomic.h
>>>>   apr/apr/trunk/include/arch/unix/apr_arch_atomic.h
>>>>   apr/apr/trunk/test/testatomic.c
>>>>
>>>
>>> Hm, don't we miss some configure magic here?
>>
>>
>> How so? All configure does is see if __sync_val_compare_and_swap, et.al. 
>> exists
> 
> If it does, then it adds the 64bit versions. This is for Unix. For other 
> platforms, either we know they do have 64bit
> atomics (eg: Windows) or else we use the portable versions of 64bit atomics.

Ahh, now I get it. Thanks. If the existing test for the compiler atomics 
succeeds then 32bit *and* 64bit atomics are
offered by the compiler on a 64 bit platform, correct?

Regards

Rüdiger

Re: svn commit: r1841078 - in /apr/apr/trunk: CHANGES apr.dsp atomic/unix/builtins64.c atomic/unix/mutex64.c atomic/win32/apr_atomic64.c include/apr_atomic.h include/arch/unix/apr_arch_atomic.h test/t

[Ruediger Pluem]

On 09/17/2018 05:50 PM, j...@apache.org wrote:
> Author: jim
> Date: Mon Sep 17 15:50:19 2018
> New Revision: 1841078
> 
> URL: http://svn.apache.org/viewvc?rev=1841078=rev
> Log:
> Add in Atomics for 64bit ints
> 
> Added:
> apr/apr/trunk/atomic/unix/builtins64.c   (with props)
> apr/apr/trunk/atomic/unix/mutex64.c   (with props)
> apr/apr/trunk/atomic/win32/apr_atomic64.c   (with props)
> Modified:
> apr/apr/trunk/CHANGES
> apr/apr/trunk/apr.dsp
> apr/apr/trunk/include/apr_atomic.h
> apr/apr/trunk/include/arch/unix/apr_arch_atomic.h
> apr/apr/trunk/test/testatomic.c
> 

Hm, don't we miss some configure magic here?

Regards

Rüdiger

Re: Hashes for apr downloads

[Ruediger Pluem]

On 09/18/2018 02:52 PM, William A Rowe Jr wrote:
> Note that in moderation of annou...@apache.org , 
> I received the following response;
> 
>> MD5 and SHA1 hashes have been deprecated for some time on download pages (*)
>> 
>> Please update the download page(s) to remove these.
>> 
>> (*) http://www.apache.org/dev/release-distribution#sigs-and-sums
> 
> Are we concerned with retaining either-or MD5 or SHA1 for legacy architecture 
> users? As we integrate to openssl 0.9.8+,
> and those all have an `openssl sha256` facility, it seems like the concern is 
> pretty obscure.

Sounds reasonable.


> 
> Do we have an opinion on offering both sha256 + sha512? Only one or the 
> other, and if so, which?

Does offering both create additional work?

Regards

Rüdiger

Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c

[Ruediger Pluem]

On 04/16/2018 12:04 PM, Yann Ylavic wrote:
> On Mon, Apr 16, 2018 at 8:45 AM, Ruediger Pluem <rpl...@apache.org> wrote:
>>
>> On 04/14/2018 02:32 AM, Yann Ylavic wrote:
>>>
>>> IOW, this simple patch would work equally for me (and could go in any 
>>> version):
>>>
>>> Index: util-misc/apr_reslist.c
>>> ===
>>> --- util-misc/apr_reslist.c(revision 1829106)
>>> +++ util-misc/apr_reslist.c(working copy)
>>> @@ -61,13 +61,13 @@ struct apr_reslist_t {
>>>  };
>>>
>>>  /**
>>> - * Grab a resource from the front of the resource list.
>>> + * Grab a resource from the back of the resource list.
>>>   * Assumes: that the reslist is locked.
>>>   */
>>>  static apr_res_t *pop_resource(apr_reslist_t *reslist)
>>>  {
>>>  apr_res_t *res;
>>> -res = APR_RING_FIRST(>avail_list);
>>> +res = APR_RING_LAST(>avail_list);
>>>  APR_RING_REMOVE(res, link);
>>>  reslist->nidle--;
>>>  return res;
>>> --
>>
>> Hm, but this would change this to become a fifo list instead of the current 
>> lifo list or do I miss something?
> 
> Yes clearly, I suggested that reslists be changed to FIFO
> unconditionnaly, because I find that LIFO and expiry don't mix well
> w.r.t. starvation..
> 
> That would be the simpler patch too (not that
> apr_reslist_acquire_fifo() is hard to implement, but I wonder who

I think there are scenarios where LIFO is useful, but the question is if these 
are frequent enough to warrant LIFO /
FIFO as an option.

> needs LIFO in the firtst place with such a structure...).

There seems to be no promise in the API documentation that this is LIFO, but of 
course switching to FIFO changes the
behavior of the structure for the better (as you assume) or for the worse (for 
people who build on the current
implementation detail that it is LIFO).
So the question is: Can we do that in 1.6.x or even in 1.7.x from our 
guarantees we give point of view?

Regards

Rüdiger

Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c

[Ruediger Pluem]

On 04/14/2018 02:32 AM, Yann Ylavic wrote:
> On Sat, Apr 14, 2018 at 2:18 AM, Yann Ylavic  wrote:
>>
>> when the ttl is to be
>> checked against the resource we should always peek it as LIFO (i.e.
>> s/fifo/1/ in the first peek_resource() of reslist_acquire() in my
>> patch).
>> This would prevent starvation, and we should possibly do that in 1.6.x
>> too (where apr_reslist_acquire_fifo() can't land).
>> If we do that unconditionnaly, this patch is moot. After all,
>> apr_reslist_maintain() and/hence apr_reslist_release() are already
>> LIFO for recycling resources.
> 
> IOW, this simple patch would work equally for me (and could go in any 
> version):
> 
> Index: util-misc/apr_reslist.c
> ===
> --- util-misc/apr_reslist.c(revision 1829106)
> +++ util-misc/apr_reslist.c(working copy)
> @@ -61,13 +61,13 @@ struct apr_reslist_t {
>  };
> 
>  /**
> - * Grab a resource from the front of the resource list.
> + * Grab a resource from the back of the resource list.
>   * Assumes: that the reslist is locked.
>   */
>  static apr_res_t *pop_resource(apr_reslist_t *reslist)
>  {
>  apr_res_t *res;
> -res = APR_RING_FIRST(>avail_list);
> +res = APR_RING_LAST(>avail_list);
>  APR_RING_REMOVE(res, link);
>  reslist->nidle--;
>  return res;
> --

Hm, but this would change this to become a fifo list instead of the current 
lifo list or do I miss something?

Regards

Rüdiger

Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c

[Ruediger Pluem]

On 04/11/2018 01:26 PM, Yann Ylavic wrote:
> On Fri, Apr 6, 2018 at 10:18 AM, Yann Ylavic <ylavic@gmail.com> wrote:
>> On Fri, Apr 6, 2018 at 8:57 AM, Ruediger Pluem <rpl...@apache.org> wrote:
>>>
>>> I don't think that a remark is strong enough here. I would like to
>>> see this parameter either added to a new apr_reslist_create_ex or if
>>> we want to stick with apr_reslist_fifo_set it should return an error
>>> if elements are already in the reslist.
>>
>> r1828492, I kept it as a helper rather than _create_ex() because:
>> - it's already how settings seem to be added there (timeout, ...)
>> - _create() has a substantial number of args already :/
>> - _create_ex() are definitive extensions or may end up in endless
>> discussions for the next function name when/should an _ex_ex() is/be
>> needed :)
> 
> Actually it's not that easy.
> While writing the unit test for apr_reslist_fifo_set(), I realized
> that if min > 0 is configured the reslist is immediately filled in
> upon creation, so there is no way to call apr_reslist_fifo_set()
> afterward...
> 
> This brings us to either apr_reslist_create_ex() as you suggested, or
> disable the non-empty check in apr_reslist_fifo_set() as the original
> commit (and rely on the user to read docs...).

Other options:

1. Destroy all reslist elements when apr_reslist_fifo_set is called.
2. Reverse the order in the ring when apr_reslist_fifo_set is called.

Of course both options require a lock.

> Setting fifo just after create() with min > 0 is not an issue per se,
> it'll really matter at acquire time anyway...

I guess this is true.

> So I tend to prefer the documentation only over the _ex() (potential
> _ex++() later...), but I could live the the latter too :)
> 
> Thoughts?
> 

Regards

Rüdiger

Re: svn commit: r1828492 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c

[Ruediger Pluem]

On 04/06/2018 09:53 AM, yla...@apache.org wrote:
> Author: ylavic
> Date: Fri Apr  6 07:53:02 2018
> New Revision: 1828492
> 
> URL: http://svn.apache.org/viewvc?rev=1828492=rev
> Log:
> reslist: follow up to r1828289: enfore empty list requirement when setting 
> fifo.
> 
> The doxygen remark wasn't enough as noted by Ruediger.
> 
> 
> Modified:
> apr/apr/trunk/include/apr_reslist.h
> apr/apr/trunk/util-misc/apr_reslist.c
> 

> 
> Modified: apr/apr/trunk/util-misc/apr_reslist.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/util-misc/apr_reslist.c?rev=1828492=1828491=1828492=diff
> ==
> --- apr/apr/trunk/util-misc/apr_reslist.c (original)
> +++ apr/apr/trunk/util-misc/apr_reslist.c Fri Apr  6 07:53:02 2018
> @@ -445,9 +445,15 @@ APR_DECLARE(void) apr_reslist_timeout_se
>  reslist->timeout = timeout;
>  }
>  
> -APR_DECLARE(void) apr_reslist_fifo_set(apr_reslist_t *reslist, int fifo)
> +APR_DECLARE(apr_status_t) apr_reslist_fifo_set(apr_reslist_t *reslist,
> +   int fifo)
>  {
> +if (!APR_RING_EMPTY(>avail_list, apr_res_t, link)) {

Don't we need to lock if want to do this or is this sufficiently atomic?
Further on don't we need to make the check and the setting of reslist->fifo 
atomic, hence both done while we hold the lock?

> +return APR_EBUSY;
> +}
> +
>  reslist->fifo = fifo;
> +return APR_SUCCESS;
>  }
>  

Regards

Rüdiger

Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c

[Ruediger Pluem]

On 04/04/2018 07:43 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Wed Apr  4 17:43:46 2018
> New Revision: 1828369
> 
> URL: http://svn.apache.org/viewvc?rev=1828369=rev
> Log:
> reslist: follow up to r1828289: adjust maintenance top too.
> 
> Also, clarify in doxygen when apr_reslist_fifo_set() should be called.
> 
> 
> Modified:
> apr/apr/trunk/include/apr_reslist.h
> apr/apr/trunk/util-misc/apr_reslist.c
> 
> Modified: apr/apr/trunk/include/apr_reslist.h
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/include/apr_reslist.h?rev=1828369=1828368=1828369=diff
> ==
> --- apr/apr/trunk/include/apr_reslist.h (original)
> +++ apr/apr/trunk/include/apr_reslist.h Wed Apr  4 17:43:46 2018
> @@ -134,11 +134,17 @@ APR_DECLARE(apr_status_t) apr_reslist_re
>   */
>  APR_DECLARE(void) apr_reslist_timeout_set(apr_reslist_t *reslist,
>apr_interval_time_t timeout);
> +
>  /**
>   * Set whether the reslist reuses resources as FIFO (First In First Out) or
>   * LIFO (Last In First Out).
>   * @param reslist The resource list.
>   * @param fifo Set as FIFO (non zero) or LIFO (zero).
> + * @remark This function should be called before any resource is in the
> + * the reslist, otherwise maintenance optimizations based on the expiration
> + * time relative to the order of insertion (i.e. position in the list) won't
> + * work as expected.
> + *

I don't think that a remark is strong enough here. I would like to see this 
parameter either added to a new
apr_reslist_create_ex or if we want to stick with apr_reslist_fifo_set it 
should return an error if elements
are already in the reslist.

Regards

Rüdiger

Re: svn commit: r1822315 - /apr/apr/trunk/buckets/apr_buckets_alloc.c

[Ruediger Pluem]

On 01/26/2018 10:42 PM, Yann Ylavic wrote:
> On Fri, Jan 26, 2018 at 4:24 PM,   wrote:
>>
>> Modified: apr/apr/trunk/buckets/apr_buckets_alloc.c
>> URL: 
>> http://svn.apache.org/viewvc/apr/apr/trunk/buckets/apr_buckets_alloc.c?rev=1822315=1822314=1822315=diff
>> ==
>> --- apr/apr/trunk/buckets/apr_buckets_alloc.c (original)
>> +++ apr/apr/trunk/buckets/apr_buckets_alloc.c Fri Jan 26 15:24:40 2018
>> @@ -45,12 +45,21 @@ struct apr_bucket_alloc_t {
>>  static apr_status_t alloc_cleanup(void *data)
>>  {
>>  apr_bucket_alloc_t *list = data;
>> +#if APR_POOL_DEBUG
>> +apr_allocator_t *allocator = NULL;
>> +#endif
>> +
>> +#if APR_POOL_DEBUG
>> +if (list->pool && list->allocator != 
>> apr_pool_allocator_get(list->pool)) {
>> +allocator = list->allocator;
>> +}
>> +#endif
>>
>>  apr_allocator_free(list->allocator, list->blocks);
>>
>>  #if APR_POOL_DEBUG
>> -if (list->pool && list->allocator != 
>> apr_pool_allocator_get(list->pool)) {
>> -apr_allocator_destroy(list->allocator);
>> +if (allocator) {
>> +apr_allocator_destroy(allocator);
>>  }
>>  #endif
> 
> Since apr_allocator_destroy() will free all its nodes, maybe we can simply:
> 
> #if APR_POOL_DEBUG
> if (list->pool && list->allocator != apr_pool_allocator_get(list->pool)) {
> apr_allocator_destroy(allocator);
> }
> else
> #endif
> apr_allocator_free(list->allocator, list->blocks);

This should be possible as well, but I wanted to keep the code of the debug 
mode as close as possible
to the non debug mode.

Regards

Rüdiger

Re: svn commit: r1816527 - in /apr/apr/trunk: CHANGES configure.in include/apr_network_io.h include/arch/win32/apr_private.h network_io/unix/sockaddr.c test/testsock.c

[Ruediger Pluem]

On 11/28/2017 09:53 AM, jor...@apache.org wrote:
> Author: jorton
> Date: Tue Nov 28 08:53:13 2017
> New Revision: 1816527
> 
> URL: http://svn.apache.org/viewvc?rev=1816527=rev
> Log:
> Support IPv6 link-local address scope/zone mapping.
> 
> * network_io/unix/sockaddr.c (apr_sockaddr_zone_set,
>   apr_sockaddr_zone_get): New functions.
>   (apr_sockaddr_ip_getbuf): Append %scope for link-local address.
>   (apr_sockaddr_equal): Compare link-local address with different
>   scopes as not equal.
> 
> * include/apr_network_io.h: Add function declarations.
> 
> * configure.in: Test for if_indextoname and if_nametoindex.
> 
> * test/testsock.c (test_zone): New test case.
> 
> * include/arch/win32/apr_private.h: Assume Windows supports
>   if_nametoindex and if_indextoname.
> 
> Modified:
> apr/apr/trunk/CHANGES
> apr/apr/trunk/configure.in
> apr/apr/trunk/include/apr_network_io.h
> apr/apr/trunk/include/arch/win32/apr_private.h
> apr/apr/trunk/network_io/unix/sockaddr.c
> apr/apr/trunk/test/testsock.c
> 

> Modified: apr/apr/trunk/test/testsock.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/test/testsock.c?rev=1816527=1816526=1816527=diff
> ==
> --- apr/apr/trunk/test/testsock.c (original)
> +++ apr/apr/trunk/test/testsock.c Tue Nov 28 08:53:13 2017
> @@ -640,6 +640,98 @@ static void test_freebind(abts_case *tc,
>  #endif
>  }
>  
> +#define TEST_ZONE_ADDR "fe80::1"
> +
> +#ifdef __linux__
> +/* Reasonable bet that "lo" will exist. */
> +#define TEST_ZONE_NAME "lo"
> +/* ... fill in other platforms here */
> +#endif
> +
> +#ifdef TEST_ZONE_NAME 
> +#define TEST_ZONE_FULLADDR TEST_ZONE_ADDR "%" TEST_ZONE_NAME
> +#endif
> +
> +static void test_zone(abts_case *tc, void *data)
> +{
> +#if APR_HAVE_IPV6
> +apr_sockaddr_t *sa;
> +apr_status_t rv;
> +const char *name = NULL;
> +apr_uint32_t id = 0;
> +
> +/* RFC 5737 address */
> +rv = apr_sockaddr_info_get(, "127.0.0.1", APR_INET, 8080, 0, p);
> +APR_ASSERT_SUCCESS(tc, "Problem generating sockaddr", rv);
> +
> +/* Fail for an IPv4 address! */
> +ABTS_INT_EQUAL(tc, APR_EBADIP,
> +   apr_sockaddr_zone_set(sa, "1"));
> +ABTS_INT_EQUAL(tc, APR_EBADIP,
> +   apr_sockaddr_zone_get(sa, , , p));
> +
> +rv = apr_sockaddr_info_get(, TEST_ZONE_ADDR, APR_INET6, 8080, 0, p);
> +APR_ASSERT_SUCCESS(tc, "Problem generating sockaddr", rv);
> +
> +rv = apr_sockaddr_info_get(, TEST_ZONE_ADDR, APR_INET6, 8080, 0, p);
> +APR_ASSERT_SUCCESS(tc, "Problem generating sockaddr", rv);

Why do we do the above test twice?

> +
> +ABTS_INT_EQUAL(tc, APR_EBADIP, apr_sockaddr_zone_get(sa, , , p));
> +

Regards

Rüdiger

Re: [POLL] (Was Re: Et resurrexit tertia die.)

[Ruediger Pluem]

On 05/19/2017 07:14 AM, William A Rowe Jr wrote:
> On Mon, Apr 10, 2017 at 4:40 AM, Nick Kew  wrote:
>> I think we've done most of 1.6.0, modulo a couple of questionmarks.
>>
>> Potentially open issues are (in no particular order):
>> 1.  Mark timedlocks experimental
> 
> The underlying question which we haven't resolved, and which our
> discussion didn't draw out enough opinions/voices, boils down to this
> simple question...
> 
> [  ] Release 1.x may include experimental features, disabled by default
> [  ] Release 1.x may include experimental features, enabled by default
> [ X ] Releases don't include experimental features

But some words of explanation:

The risk I see with experimental features is that it might turn out during 
stabilization phase, so on the way out of
experimental, that the API isn't right. And this is something we cannot change 
then afterwards until a new major release.
On the other hand it is sometimes tough to get enough test audience for new 
features when they are not contained in a
release. But I like to play it safe in this case. Hence my vote.

Regards

Rüdiger

Re: 1.6.0 release candidates

[Ruediger Pluem]

On 04/20/2017 09:03 AM, Nick Kew wrote:
> On Wed, 2017-04-19 at 13:43 -0500, William A Rowe Jr wrote:
>> With patches now accounted for I can pre-test and report back today on
>> AIX, HPUX and propose a fix to silence the win32 32 -> 16 bit warnings
>> (after division is done.)
> 
> Excellent.  I look forward to your findings.
> 
>> But my inclination is to defer this new feature to a later 1.7+2.0
>> release. Shipping a feature that devs would expect to need when it may
>> be largely unavailable seems a disservice to our end users and these
>> devs.
> 
> I'd be fine with that.  Or with the intermediate solution of
> switching from a --disable to a --enable option so it's
> disabled by default but minimal change to enable in 1.6.later.
> Anyone else have strong views?
> 

No strong view, but if we defer to 1.7.x or 2.0, we are still free to do API 
changes or behavior changes
if it turns out that they are needed. If we are confident that the API contract 
is stable as such and
that we just need to fix bugs in the implementation on these platforms then go 
with 1.6.x with a default of off.

Regards

Rüdiger

Re: svn commit: r1790490 - in /apr/apr/branches/1.6.x: ./ include/ include/arch/unix/ locks/beos/ locks/netware/ locks/os2/ locks/unix/ locks/win32/ test/

[Ruediger Pluem]

On 04/07/2017 04:31 PM, Yann Ylavic wrote:
> On Fri, Apr 7, 2017 at 3:39 PM, Ruediger Pluem <rpl...@apache.org> wrote:
>>
>>
>> On 04/07/2017 03:33 PM, Ruediger Pluem wrote:
>>>
>>>
>>> On 04/07/2017 10:37 AM, Yann Ylavic wrote:
>>>> On Fri, Apr 7, 2017 at 10:21 AM, Ruediger Pluem <rpl...@apache.org> wrote:
>>>>>
>>>>>
>>>>> On 04/07/2017 02:11 AM, yla...@apache.org wrote:
>>>>>> Author: ylavic
>>>>>> Date: Fri Apr  7 00:11:27 2017
>>>>>> New Revision: 1790490
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=1790490=rev
>>>>>> Log:
>>>>>> Merge r1790488 from trunk:
>>>>>>
>>>>>> locks: follow up to r1667900.
>>>>>>
>>>>>> Axe the 'absolute' argument of apr_{thread,proc,global}_mutex_timedlock()
>>>>>> which was confusing, hence 'timeout' is always relative now.
>>>>>
>>>>> Hm. Doesn't that violate the APR versioning rules? IMHO you can change an 
>>>>> existing public API only in a major release
>>>>> aka. 2.0 in our case.
>>>>
>>>> Was never released (new to 1.6.x), does the rule apply?
>>>>
>>>
>>> IMHO that does not matter. Apps that run with 1.5.x are expected to run 
>>> with 1.6.x. This wouldn't be the case here.
>>>
>>
>> Or are apr_{thread,proc,global}_mutex_timedlock() new to APR 1.6 and are not 
>> part of 1.5.x and before?
> 
> Yes that's the case (new to 1.6, not in 1.5), so no possible
> regression but for the braves running 1.6.x :)
> 

Ah ok. Then it is fine of course.

Regards

Rüdiger

Re: svn commit: r1790490 - in /apr/apr/branches/1.6.x: ./ include/ include/arch/unix/ locks/beos/ locks/netware/ locks/os2/ locks/unix/ locks/win32/ test/

[Ruediger Pluem]

On 04/07/2017 03:33 PM, Ruediger Pluem wrote:
> 
> 
> On 04/07/2017 10:37 AM, Yann Ylavic wrote:
>> On Fri, Apr 7, 2017 at 10:21 AM, Ruediger Pluem <rpl...@apache.org> wrote:
>>>
>>>
>>> On 04/07/2017 02:11 AM, yla...@apache.org wrote:
>>>> Author: ylavic
>>>> Date: Fri Apr  7 00:11:27 2017
>>>> New Revision: 1790490
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1790490=rev
>>>> Log:
>>>> Merge r1790488 from trunk:
>>>>
>>>> locks: follow up to r1667900.
>>>>
>>>> Axe the 'absolute' argument of apr_{thread,proc,global}_mutex_timedlock()
>>>> which was confusing, hence 'timeout' is always relative now.
>>>
>>> Hm. Doesn't that violate the APR versioning rules? IMHO you can change an 
>>> existing public API only in a major release
>>> aka. 2.0 in our case.
>>
>> Was never released (new to 1.6.x), does the rule apply?
>>
> 
> IMHO that does not matter. Apps that run with 1.5.x are expected to run with 
> 1.6.x. This wouldn't be the case here.
> 

Or are apr_{thread,proc,global}_mutex_timedlock() new to APR 1.6 and are not 
part of 1.5.x and before?

Regards

Rüdiger

Re: svn commit: r1790490 - in /apr/apr/branches/1.6.x: ./ include/ include/arch/unix/ locks/beos/ locks/netware/ locks/os2/ locks/unix/ locks/win32/ test/

[Ruediger Pluem]

On 04/07/2017 10:37 AM, Yann Ylavic wrote:
> On Fri, Apr 7, 2017 at 10:21 AM, Ruediger Pluem <rpl...@apache.org> wrote:
>>
>>
>> On 04/07/2017 02:11 AM, yla...@apache.org wrote:
>>> Author: ylavic
>>> Date: Fri Apr  7 00:11:27 2017
>>> New Revision: 1790490
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1790490=rev
>>> Log:
>>> Merge r1790488 from trunk:
>>>
>>> locks: follow up to r1667900.
>>>
>>> Axe the 'absolute' argument of apr_{thread,proc,global}_mutex_timedlock()
>>> which was confusing, hence 'timeout' is always relative now.
>>
>> Hm. Doesn't that violate the APR versioning rules? IMHO you can change an 
>> existing public API only in a major release
>> aka. 2.0 in our case.
> 
> Was never released (new to 1.6.x), does the rule apply?
> 

IMHO that does not matter. Apps that run with 1.5.x are expected to run with 
1.6.x. This wouldn't be the case here.

Regards

Rüdiger

Re: svn commit: r1790490 - in /apr/apr/branches/1.6.x: ./ include/ include/arch/unix/ locks/beos/ locks/netware/ locks/os2/ locks/unix/ locks/win32/ test/

[Ruediger Pluem]

On 04/07/2017 02:11 AM, yla...@apache.org wrote:
> Author: ylavic
> Date: Fri Apr  7 00:11:27 2017
> New Revision: 1790490
> 
> URL: http://svn.apache.org/viewvc?rev=1790490=rev
> Log:
> Merge r1790488 from trunk:
> 
> locks: follow up to r1667900.
> 
> Axe the 'absolute' argument of apr_{thread,proc,global}_mutex_timedlock()
> which was confusing, hence 'timeout' is always relative now.

Hm. Doesn't that violate the APR versioning rules? IMHO you can change an 
existing public API only in a major release
aka. 2.0 in our case.

Regards

Rüdiger