Re: svn commit: r1902731 - /httpd/httpd/trunk/server/util_pcre.c

[Ruediger Pluem]

On 7/28/22 9:25 PM, Ruediger Pluem wrote:
> 
> 
> On 7/15/22 12:36 PM, yla...@apache.org wrote:
>> Author: ylavic
>> Date: Fri Jul 15 10:36:24 2022
>> New Revision: 1902731
>>
>> URL: http://svn.apache.org/viewvc?rev=1902731=rev

>>  
>> -state->buf_used += APR_ALIGN_DEFAULT(size);
>> +#if APREG_USE_THREAD_LOCAL
>> +if (state->thd) {
>> +struct match_thread_state *ts = thread_state;
>> +void *p;
>> +
>> +if (!ts) {
>> +apr_pool_t *tp = apr_thread_pool_get(state->thd);
>> +ts = apr_pcalloc(tp, sizeof(*ts));
>> +apr_pool_create(>pool, tp);
>> +thread_state = state->ts = ts;
>> +}
>> +else if (!state->ts) {
>> +ts->heap_used = 0;
>> +state->ts = ts;
>> +}
>>  
>> +avail = ts->heap_size - ts->heap_used;
>> +if (avail >= size) {
>> +size = APR_ALIGN_DEFAULT(size);
>> +if (size > avail) {
>> +size = avail;
>> +}
>> +p = ts->heap + ts->heap_used;
>> +}
>> +else {
>> +ts->heap_size *= 2;
>> +size = APR_ALIGN_DEFAULT(size);
>> +if (ts->heap_size < size) {
>> +ts->heap_size = size;
>> +}
>> +if (ts->heap_size < AP_PCRE_STACKBUF_SIZE * 2) {
>> +ts->heap_size = AP_PCRE_STACKBUF_SIZE * 2;
>> +}
>> +ts->heap = apr_palloc(ts->pool, ts->heap_size);
>> +ts->heap_used = 0;
>> +p = ts->heap;
> 
> I think that apr_palloc should be efficient enough for allocating memory 
> quickly and
> that we don't need to reserve bigger memory chunks manage them here locally 
> that looks
> like what apr_palloc already does.

Already done by r1902858 :-)

Regards

Rüdiger

Re: svn commit: r1902731 - /httpd/httpd/trunk/server/util_pcre.c

[Ruediger Pluem]

On 7/15/22 12:36 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Fri Jul 15 10:36:24 2022
> New Revision: 1902731
> 
> URL: http://svn.apache.org/viewvc?rev=1902731=rev
> Log:
> util_pcre: Add a thread local subpool cache for when stack does not suffice.
> 
> When AP_HAS_THREAD_LOCAL is available, use a thread-local match_thread_state 
> to
> save per-thread data in a subpool of the thread's pool.
> 
> If private_malloc() gets out of the stack buffer and the current thread has a
> pool (i.e. ap_thread_current() != NULL), it will apr_palloc()ate and return
> memory from the subpool.
> 
> When the match is complete and the match_data are freed, the thread subpool is
> cleared thus giving back the memory to the allocator, which itself will give
> back the memory or recycle it depending on its max_free setting.
> 
> * util_pcre.c:
>   Restore POSIX_MALLOC_THRESHOLDsince this is part of the user API.
> 
> * util_pcre.c(match_data_pt):
>   Type not used (explicitely) anymore, axe.
>   
> * util_pcre.c(struct match_data_state):
>   Put the stack buffer there to simplify code (the state is allocated on
>   stack anyway).
>   If APREG_USE_THREAD_LOCAL, add the apr_thread_t* and match_thread_state*
>   fields that track the thread local data for the match.
> 
> * util_pcre.c(alloc_match_data, free_match):
>   Renamed to setup_state() and cleanup_state(), simplified (no stack buffer
>   parameters anymore).
>   cleanup_state() now clears the thread local subpool if used during the 
> match.
>   setup_state() set state->thd to ap_thread_current(), thus NULL if it's not a
>   suitable thread for using thread local data.
> 
> * util_pcre.c(private_malloc):
>   Fix a possible buf_used overflow (size <= avail < APR_ALIGN_DEFAULT(size)).
>   Create the thread local subpool (once per thread) and allocate from there
>   when stack space is missing and state->thd != NULL, otherwise fall back to
>   malloc() still.
> 
> * util_pcre.c(private_free):
>   Do nothing for thread local subpool memory, will be freed in cleanup_state
>   eventually.
> 
> 
> Modified:
> httpd/httpd/trunk/server/util_pcre.c
> 
> Modified: httpd/httpd/trunk/server/util_pcre.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_pcre.c?rev=1902731=1902730=1902731=diff
> ==
> --- httpd/httpd/trunk/server/util_pcre.c (original)
> +++ httpd/httpd/trunk/server/util_pcre.c Fri Jul 15 10:36:24 2022

> @@ -257,105 +268,201 @@ AP_DECLARE(int) ap_regcomp(ap_regex_t *
>   *  Match a regular expression   *
>   */
>  
> -/* Unfortunately, PCRE requires 3 ints of working space for each captured
> +/* Unfortunately, PCRE1 requires 3 ints of working space for each captured
>   * substring, so we have to get and release working store instead of just 
> using
>   * the POSIX structures as was done in earlier releases when PCRE needed 
> only 2
>   * ints. However, if the number of possible capturing brackets is small, use 
> a
>   * block of store on the stack, to reduce the use of malloc/free. The 
> threshold
> - * is in a macro that can be changed at configure time.
> - * Yet more unfortunately, PCRE2 wants an opaque context by providing the API
> - * to allocate and free it, so to minimize these calls we maintain one opaque
> - * context per thread (in Thread Local Storage, TLS) grown as needed, and 
> while
> - * at it we do the same for PCRE1 ints vectors. Note that this requires a 
> fast
> - * TLS mechanism to be worth it, which is the case of 
> apr_thread_data_get/set()
> - * from/to ap_thread_current() when AP_HAS_THREAD_LOCAL; otherwise we'll do
> - * the allocation and freeing for each ap_regexec().
> + * is in POSIX_MALLOC_THRESHOLD macro that can be changed at configure time.
> + * PCRE2 takes an opaque match context and lets us provide the callbacks to
> + * manage the memory needed during the match, so we can still use a small 
> stack
> + * space that'll suffice for regexes up to POSIX_MALLOC_THRESHOLD captures 
> (and
> + * not too complex), above that either use some thread local subpool cache 
> (if
> + * AP_HAS_THREAD_LOCAL) or fall back to malloc()/free().
>   */
>  
> +#if AP_HAS_THREAD_LOCAL && !defined(APREG_NO_THREAD_LOCAL)
> +#define APREG_USE_THREAD_LOCAL 1
> +#else
> +#define APREG_USE_THREAD_LOCAL 0
> +#endif
> +
>  #ifdef HAVE_PCRE2
> -typedef pcre2_match_data* match_data_pt;
> -typedef size_t*   match_vector_pt;
> +typedef PCRE2_SIZE* match_vector_pt;
>  #else
> -typedef int*  match_data_pt;
> -typedef int*  match_vector_pt;
> +typedef int*match_vector_pt;
>  #endif
>  
> -struct match_data_state
> -{
> -match_data_pt data;
> -char *buf;
> -apr_size_t buf_len;
> +#if APREG_USE_THREAD_LOCAL
> +struct match_thread_state {
> +char *heap;
> +apr_size_t heap_size;
> +apr_size_t heap_used;
> +apr_pool_t *pool;
> 

Re: svn commit: r1902909 - /httpd/httpd/trunk/server/util.c

[Ruediger Pluem]

On 7/21/22 1:21 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Thu Jul 21 11:21:30 2022
> New Revision: 1902909
> 
> URL: http://svn.apache.org/viewvc?rev=1902909=rev
> Log:
> core: Follow up to r1902728 and r1902906: simplify for APR-1.8+.
> 
> apr_threadattr_max_free_set() is now in APR-1.8.x.
> 
> 
> Modified:
> httpd/httpd/trunk/server/util.c
> 
> Modified: httpd/httpd/trunk/server/util.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1902909=1902908=1902909=diff
> ==
> --- httpd/httpd/trunk/server/util.c (original)
> +++ httpd/httpd/trunk/server/util.c Thu Jul 21 11:21:30 2022

> @@ -3355,19 +3355,6 @@ AP_DECLARE(apr_status_t) ap_thread_main_
>  return rv;
>  }
>  
> -#if APR_VERSION_AT_LEAST(1,8,0) && !APR_VERSION_AT_LEAST(2,0,0)
> -/* Don't let the thread's pool allocator with no limits, though there
> - * is possibly no allocator with APR <= 1.7 and APR_POOL_DEBUG.
> - */
> -{
> -apr_pool_t *tp = apr_thread_pool_get(*thread);
> -apr_allocator_t *ta = apr_pool_allocator_get(tp);
> -if (ta) {
> -apr_allocator_max_free_set(ta, ap_max_mem_free);
> -}
> -}
> -#endif
> -

Why don't we do the above for APR <= 1.7? The code is now NULL safe for 
APR_POOL_DEBUG.

>  apr_pool_cleanup_register(pool, *thread, main_thread_cleanup,
>apr_pool_cleanup_null);
>  return APR_SUCCESS;
> @@ -3398,12 +3385,12 @@ AP_DECLARE(apr_status_t) ap_thread_curre
>  abort_fn(rv);
>  return rv;
>  }
> +apr_allocator_max_free_set(ta, ap_max_mem_free);
>  rv = apr_pool_create_unmanaged_ex(, abort_fn, ta);
>  if (rv != APR_SUCCESS) {
>  return rv;
>  }
>  /* Don't let the thread's pool allocator with no limits */

The comment should move as well.

> -apr_allocator_max_free_set(ta, ap_max_mem_free);
>  apr_allocator_owner_set(ta, p);
>  
>  osthd = apr_os_thread_current();
> 
> 
> 

Regards

Rüdiger

Re: svn commit: r1902409 - in /httpd/httpd/trunk: changes-entries/h2_trailers.txt modules/http2/h2_stream.c modules/http2/h2_stream.h

[Ruediger Pluem]

On 7/2/22 11:39 AM, ic...@apache.org wrote:
> Author: icing
> Date: Sat Jul  2 09:39:22 2022
> New Revision: 1902409
> 
> URL: http://svn.apache.org/viewvc?rev=1902409=rev
> Log:
>   *) mod_http2: fixed trailer handling. Empty response bodies
>  prevented trailers from being sent to a client. See
>   for how
>  this affected gRPC use.
> 
> 
> Added:
> httpd/httpd/trunk/changes-entries/h2_trailers.txt
> Modified:
> httpd/httpd/trunk/modules/http2/h2_stream.c
> httpd/httpd/trunk/modules/http2/h2_stream.h
> 

> Modified: httpd/httpd/trunk/modules/http2/h2_stream.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_stream.c?rev=1902409=1902408=1902409=diff
> ==
> --- httpd/httpd/trunk/modules/http2/h2_stream.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_stream.c Sat Jul  2 09:39:22 2022

> @@ -1406,60 +1412,81 @@ static ssize_t stream_data_cb(nghttp2_se
>  }
>  
>  /* How much data do we have in our buffers that we can write? */
> -buf_len = output_data_buffered(stream, );
> -if (buf_len < length && !eos) {
> +check_and_receive:
> +buf_len = output_data_buffered(stream, , _blocked);
> +while (buf_len < length && !eos && !header_blocked) {
>  /* read more? */
>  ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c1,
>H2_SSSN_STRM_MSG(session, stream_id,
>"need more (read len=%ld, %ld in buffer)"),
>(long)length, (long)buf_len);
>  rv = buffer_output_receive(stream);
> -/* process all headers sitting at the buffer head. */
> -while (APR_SUCCESS == rv && !eos && !stream->sent_trailers) {
> -rv = buffer_output_process_headers(stream);
> -if (APR_SUCCESS != rv && APR_EAGAIN != rv) {
> -ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c1,
> -  H2_STRM_LOG(APLOGNO(10300), stream,
> -  "data_cb, error processing headers"));
> -return NGHTTP2_ERR_CALLBACK_FAILURE;
> -}
> -buf_len = output_data_buffered(stream, );
> +if (APR_EOF == rv) {
> +eos = 1;
> +rv = APR_SUCCESS;
>  }
>  
> -if (APR_SUCCESS != rv && !APR_STATUS_IS_EAGAIN(rv)) {
> +if (APR_SUCCESS == rv) {
> +/* re-assess */
> +buf_len = output_data_buffered(stream, , _blocked);
> +}
> +else if (APR_STATUS_IS_EAGAIN(rv)) {
> +/* currently, no more is available */
> +break;
> +}
> +else if (APR_SUCCESS != rv) {

The if is always true as if APR_SUCCESS == rv we hit the first block.


>  ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c1,
>H2_STRM_LOG(APLOGNO(02938), stream, "data_cb, 
> reading data"));
>  return NGHTTP2_ERR_CALLBACK_FAILURE;
>  }
> +}
>  
> -if (stream->sent_trailers) {
> -AP_DEBUG_ASSERT(eos);
> -AP_DEBUG_ASSERT(buf_len == 0);
> -return NGHTTP2_ERR_DEFERRED;
> +if (buf_len == 0 && header_blocked) {
> +/* we are blocked from having data to send by a HEADER bucket sitting
> + * at buffer start. Send it and check again what DATA we can send. */
> +rv = buffer_output_process_headers(stream);
> +if (APR_SUCCESS == rv) {
> +goto check_and_receive;
> +}
> +else if (APR_STATUS_IS_EAGAIN(rv)) {
> +/* unable to send the HEADER at this time. */
> +eos = 0;
> +goto cleanup;
> +}
> +else {
> +ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c1,
> +  H2_STRM_LOG(APLOGNO(10300), stream,
> +  "data_cb, error processing headers"));
> +return NGHTTP2_ERR_CALLBACK_FAILURE;
>  }
>  }
>  
>  if (buf_len > (apr_off_t)length) {
> -eos = 0;
> +eos = 0;  /* Any EOS we have in the buffer does not apply yet */
>  }
>  else {
>  length = (size_t)buf_len;
>  }
> +
> +if (stream->sent_trailers) {
> +/* We already sent trailers and will/can not send more DATA. */
> +eos = 0;
> +}
> +
>  if (length) {
>  ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c1,
>H2_STRM_MSG(stream, "data_cb, sending len=%ld, 
> eos=%d"),
>(long)length, eos);
>  *data_flags |=  NGHTTP2_DATA_FLAG_NO_COPY;
>  }
> -else if (!eos) {
> -/* no data available and output is not closed, need to suspend */
> +else if (!eos && !stream->sent_trailers) {
> +/* We have not reached the end of DATA yet, DEFER sending */
>  ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c1,
>

Re: svn commit: r1902317 - /httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c

[Ruediger Pluem]

On 6/28/22 3:05 PM, gbec...@apache.org wrote:
> Author: gbechis
> Date: Tue Jun 28 13:05:20 2022
> New Revision: 1902317
> 
> URL: http://svn.apache.org/viewvc?rev=1902317=rev
> Log:
> check apr_sockaddr_info_get() return value
> bz #66135
> 
> Modified:
> httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
> 
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c?rev=1902317=1902316=1902317=diff
> ==
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c Tue Jun 28 13:05:20 2022
> @@ -1555,7 +1555,12 @@ static int proxy_ftp_handler(request_rec
>  }
>  
>  /* make the connection */
> -apr_sockaddr_info_get(_addr, apr_psprintf(p, 
> "%d.%d.%d.%d", h3, h2, h1, h0), connect_addr->family, pasvport, 0, p);
> +err = apr_sockaddr_info_get(_addr, apr_psprintf(p, 
> "%d.%d.%d.%d", h3, h2, h1, h0), connect_addr->family, pasvport, 0, p);
> +if (APR_SUCCESS != err) {
> +return ap_proxyerror(r, HTTP_BAD_GATEWAY, apr_pstrcat(p,
> +"DNS lookup failure for: ",
> +connectname, NULL));

I think this needs to be ftp_proxyerror instead of ap_proxyerror.

> +}
>  rv = apr_socket_connect(data_sock, pasv_addr);
>  if (rv != APR_SUCCESS) {
>  ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, 
> APLOGNO(01048)
> @@ -1598,7 +1603,12 @@ static int proxy_ftp_handler(request_rec
>  #endif  /* _OSD_POSIX */
>  }
>  
> -apr_sockaddr_info_get(_addr, local_ip, APR_UNSPEC, local_port, 
> 0, r->pool);
> +err = apr_sockaddr_info_get(_addr, local_ip, APR_UNSPEC, 
> local_port, 0, r->pool);
> +if (APR_SUCCESS != err) {
> +return ap_proxyerror(r, HTTP_BAD_GATEWAY, apr_pstrcat(p,
> +"DNS lookup failure for: ",
> +connectname, NULL));

I think this needs to be ftp_proxyerror instead of ap_proxyerror.

> +}
>  
>  if ((rv = apr_socket_bind(local_sock, local_addr)) != APR_SUCCESS) {
>  ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01051)
> 
> 
> 

Regards

Rüdiger