Re: MS-WDV (was Re: Help with buckets)

2022-12-04 Thread Ruediger Pluem



On 12/2/22 4:42 PM, Emmanuel Dreyfus wrote:
> On Fri, Dec 02, 2022 at 03:17:05PM +, Joe Orton wrote:
>> I think this might need to do something more complex, maybe running the 
>> PROPFIND in a subrequest properly and capturing (buffering) the output 
>> in a custom filter, rather than using the mod_dav internal API directly. 
>> Have you tried using ap_sub_req_method_uri()? Not sure this has been 
>> tried before with mod_dav so might well be something I'm missing.
> 
> I can try that, but whatever the method is, we need to produce the
> propfind data before sending its size.
> 
> I see two unsatisfying alternatives:
> 
> 1) produce propfind data in a buffer, output the size, then the buffer
> 
> 2) produce propfind data, discarding it as it comes but coutning its size, 
> then output the size, and produce the propfind data a second time.
> 
> First approach wastes memory, second approach wastes CPU. And second approach
> needs a mechanism to ensure propfind data does not change between the two
> times it is produced. I am not sure that can be guaranteed.

I think we cannot guarantee this, which leaves us with 1.

Regards

Rüdiger



Re: svn commit: r1905646 - /httpd/httpd/trunk/include/ap_mmn.h

2022-11-30 Thread Ruediger Pluem



On 11/30/22 3:44 PM, j...@apache.org wrote:
> Author: jim
> Date: Wed Nov 30 14:44:05 2022
> New Revision: 1905646
> 
> URL: http://svn.apache.org/viewvc?rev=1905646=rev
> Log:
> Bump mmn
> 

Maybe it is my missing coffee this morning, but I only see a change in 
comments, not a bump itself.

Regards

Rüdiger

> Modified:
> httpd/httpd/trunk/include/ap_mmn.h
> 
> Modified: httpd/httpd/trunk/include/ap_mmn.h
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/include/ap_mmn.h?rev=1905646=1905645=1905646=diff
> ==
> --- httpd/httpd/trunk/include/ap_mmn.h (original)
> +++ httpd/httpd/trunk/include/ap_mmn.h Wed Nov 30 14:44:05 2022
> @@ -436,7 +436,7 @@
>   * 20130702.1 (2.5.0-dev)  Restore AUTH_HANDLED to mod_auth.h
>   * 20130702.2 (2.5.0-dev)  Add ap_log_data(), ap_log_rdata(), etc.
>   * 20130702.3 (2.5.0-dev)  Add util_fcgi.h, FastCGI protocol support
> - * 20130903.0 (2.5.0-dev)  Changes sizeof(worker_score) in scoreboard 
> + * 20130903.0 (2.5.0-dev)  Changes sizeof(worker_score) in scoreboard
>   * 20130924.0 (2.5.0-dev)  Add ap_errorlog_provider
>   * 20130924.1 (2.5.0-dev)  Add ap_proxy_connection_reusable()
>   * 20131112.0 (2.5.0-dev)  Add parse_errorlog_arg to ap_errorlog_provider
> @@ -457,7 +457,7 @@
>   * 20140207.8 (2.5.0-dev)  Added ap_shutdown_conn().
>   * 20140611.0 (2.5.0-dev)  Add ap_mpm_socket_callback_fn_t.
> Changes 3rd argument's type of
> -   ap_mpm_register_socket_callback and 
> +   ap_mpm_register_socket_callback and
> ap_mpm_register_socket_callback_timeout.
>   * 20140611.1 (2.5.0-dev)  Add ap_proxy_connect_uds().
>   * 20140627.0 (2.5.0-dev)  Revert 20140611.0 change.
> @@ -473,7 +473,7 @@
>   * 20140627.8 (2.5.0-dev)  Add ap_set_listencbratio(), 
> ap_close_listeners_ex(),
>   * ap_duplicate_listeners(), ap_num_listen_buckets 
> and
>   * ap_have_so_reuseport to ap_listen.h.
> - * 20140627.9 (2.5.0-dev)  Add cgi_pass_auth and AP_CGI_PASS_AUTH_* to 
> + * 20140627.9 (2.5.0-dev)  Add cgi_pass_auth and AP_CGI_PASS_AUTH_* to
>   * core_dir_config
>   * 20140627.10 (2.5.0-dev) Add ap_proxy_de_socketfy to mod_proxy.h
>   * 20150121.0 (2.5.0-dev)  Revert field addition from core_dir_config; 
> r1653666
> @@ -500,7 +500,7 @@
>   * 20150222.9 (2.5.0-dev)  Add expr_handler to core_dir_config.
>   * 20150222.10 (2.5.0-dev) Add new ap_update_child_status...() methods,
>   * add protocol to worker_score in scoreboard.h,
> - * Add pre_close connection hook and 
> + * Add pre_close connection hook and
>   * ap_prep_lingering_close().
>   * 20150222.11 (2.5.0-dev) Split useragent_host from the conn_rec into
>   * the request_rec, with ap_get_useragent_host()
> @@ -539,7 +539,7 @@
>   * 20160608.7 (2.5.0-dev)  Add ap_check_pipeline().
>   * 20160608.8 (2.5.0-dev)  Rename ap_proxy_check_backend() to
>   * ap_proxy_check_connection().
> - * 20160608.9 (2.5.0-dev)  Renamed AP_HTTP_CONFORMANCE_LIBERAL to 
> + * 20160608.9 (2.5.0-dev)  Renamed AP_HTTP_CONFORMANCE_LIBERAL to
>   * AP_HTTP_CONFORMANCE_UNSAFE, and
>   * eliminated AP_HTTP_CONFORMANCE_LOGONLY
>   * 20160617.1 (2.5.0-dev)  Added http_whitespace and http_methods to
> @@ -559,9 +559,9 @@
>   * ap_get_module_flags()
>   * 20171014.1 (2.5.0-dev)  Add NOT_IN_DIR_CONTEXT replacing 
> NOT_IN_DIR_LOC_FILE
>   * semantics
> - * 20171014.2 (2.5.1-dev)  Add "use_specific_errors" to listen_rec and 
> + * 20171014.2 (2.5.1-dev)  Add "use_specific_errors" to listen_rec and
>   * ap_accept_error_is_nonfatal()
> - * 20171014.3 (2.5.1-dev)  AP_DECLARE ap_parse_vhost_addrs() as public 
> + * 20171014.3 (2.5.1-dev)  AP_DECLARE ap_parse_vhost_addrs() as public
>   * 20171014.4 (2.5.1-dev)  Add failontimeout_set, growth_set and lbmethod_set
>   * to proxy_balancer struct
>   * 20171014.5 (2.5.1-dev)  Add hostname_ex to proxy_worker_shared
> @@ -570,7 +570,7 @@
>   * ap_regcomp_default_cflag_by_name
>   * 20171014.7 (2.5.1-dev)  Add AP_CORE_DEFAULT macro
>   * 20171014.8 (2.5.1-dev)  Add CONN_STATE_NUM to enum conn_state_e
> - * 20171014.9 (2.5.1-dev)  Add response_field_size to proxy_worker_shared 
> + * 20171014.9 (2.5.1-dev)  Add response_field_size to proxy_worker_shared
>   * 20180417.1 (2.5.1-dev)  Toggle ap_filter_input|output_pending API to 
> _NONSTD
>   * 20180417.2 (2.5.1-dev)  Add AP_GETLINE_NOSPC_EOL flag to http_protocol.h
>   * 20180417.3 (2.5.1-dev)  Add ap_fgetline() and AP_GETLINE_NONBLOCK flag
> @@ -610,7 +610,7 @@
>   * 20180906.2 (2.5.1-dev)  Add ap_state_dir_relative()

Re: svn commit: r1904518 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy.h modules/proxy/mod_proxy_hcheck.c

2022-11-29 Thread Ruediger Pluem



On 11/29/22 2:27 PM, Eric Covener wrote:
> does the mod_proxy.h require a minor mmn bump?

I think so.

Regards

Rüdiger



Re: FW: Support for OpenSSL 3.0 in HTTPD

2022-11-22 Thread Ruediger Pluem



On 11/22/22 4:24 PM, Sandeep 1. Maurya (EXT-NSB) wrote:
> Hi,
> 
> Thanks for the update...
> If OpenSSL3.0 is not planned does it mean then HTTPD 2.4.x will extend 
> support for OpenSSL 1.1.1. on EL8 beyond below timelines.

This has nothing to do with EL8. We don't deliver OpenSSL. httpd has certain 
3rd party
requirements to deliver certain functionality. In order to provide mod_ssl 
OpenSSL 1.1.1 and OpenSSL 3.0 will do.
There are currently no plans to change the code of 2.4.x such that it requires 
OpenSSL 3.0.
Until this happens you can still compile 2.4.x against OpenSSL 1.1.1.

Regards

Rüdiger



Re: FW: Support for OpenSSL 3.0 in HTTPD

2022-11-22 Thread Ruediger Pluem



On 11/22/22 1:02 PM, Eric Covener wrote:
> On Tue, Nov 22, 2022 at 3:46 AM Sandeep 1. Maurya (EXT-NSB)
>  wrote:
>> As per OpenSSL release strategy "Version 1.1.1 will be supported until 
>> 2023-09-11 (LTS)". Will HTTPD extend the support for OpenSSL 1.1.1 on EL8 
>> beyond this timeline or there any plan to update to OpenSSL 3.0
> 
> Sounds like a question for Red Hat.
> 

I guess it is a question for us. Currently there are no plans to *require* 
Openssl 3.0 for httpd 2.4.x.

Regards

Rüdiger


Re: svn commit: r1905170 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c

2022-11-18 Thread Ruediger Pluem



On 11/17/22 6:41 PM, Emmanuel Dreyfus wrote:
> On Wed, Nov 16, 2022 at 08:05:43AM +0100, Ruediger Pluem wrote:
>> If you want to backport a patch to the 2.4.x branch just add your proposal 
>> to the STATUS file
> 
> This way?

Almost :-) Only a small nitpick. We don't backport the HTML changes to the 
documentation only the XML changes.
After the backport has happened you can either build the documentation again on 
the 2.4.x branch and commit there
(this is CTR) or you just do nothing as rebuilding the docs is part of the 
release procedure for 2.4.x.

Some people also put a svn command in the 2.4.x patch line like:

svn merge -c  1904638,1904662,1905170,1905206,1905230,1905327 
^/httpd/httpd/trunk .

Or they use a little bit different formatting. But these are all matters of 
taste and each of us has
a little different style there. You will notice this over time and find your 
own one.


> Index: STATUS
> ===
> --- STATUS  (revision 1905352)
> +++ STATUS  (working copy)
> @@ -282,7 +282,25 @@
>   make it nonblocking (by default)?
>  jim: Non-blocking seems the best way to handle...
>  
> +  *) mod_dav: Open the lock database read-only when possible
>  
> + trunk patch: http://svn.apache.org/r1905229
> + 2.4.x patch: trunk works
> + +1: manu
> +
> +  *) mod_dav: DAVlockDiscovery option to disable WebDAV lock discovery
> +
> + trunk patch: http://svn.apache.org/r1904638
> + trunk patch: http://svn.apache.org/r1904662 
> + trunk patch: http://svn.apache.org/r1905170
> + trunk patch: http://svn.apache.org/r1905206
> + trunk patch: http://svn.apache.org/r1905230
> + trunk patch: http://svn.apache.org/r1905327
> + 2.4.x patch: trunk works, except for 
> +  docs/manual/mod/mod_dav_fs.html.en.utf8 on trunk that is
> +  docs/manual/mod/mod_dav_fs.html.en on 2.4.x branch
> + +1: manu
> +
>  PATCHES/ISSUES THAT ARE STALLED
>  
>*) core: avoid duplicate headers when using ap_send_error_response.
> 

Regards

Rüdiger



Re: svn commit: r1905170 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c

2022-11-15 Thread Ruediger Pluem



On 11/16/22 2:46 AM, Emmanuel Dreyfus wrote:
> On Fri, Nov 11, 2022 at 02:12:09AM +, Emmanuel Dreyfus wrote:
>> I will let someone review xml changes in r1905230 before committing 
>> the html files.
> 
> I committed the html files.
> 
> What is the procedure for pushing changes to the 2.4 branch? 

Trunk where you committed is under CTR (Commit then Review). Hence you can just 
commit
and people will review the commit and give feedback / comments if they see a 
need.
In the worst case they will veto the change with a -1, but this only happens 
rarely.
A -1 always requires a technical justification. Otherwise it is not valid.
Typically the -1 is just used as a starting point for an intense technical 
discussion
about the patch. This discussion either leads to a revert of the patch or a 
modification
of the patch that addresses the technical concerns that lead to the -1.

Even though trunk is CTR it is wise to discuss larger changes before committing 
them.
You can either post them to the dev list or even better, open a PR on Github 
and sent a
mail about it to the dev list.
The advantage of the PR is that it will do the CI on Github and thus eases the 
review
as your peers already know then that it compiles and passes the existing tests.
Keep in mind that our Github repo is a read only mirror of our SVN repository 
and hence
PR's cannot be merged directly. But we have some handy tools at
https://svn.apache.org/repos/asf/httpd/dev-tools/github to cope with this.

The 2.4.x branch is under RTC (Review then commit) with very few exceptions 
(see the section
"Current exceptions for RTC for this branch:" in the STATUS file at 
https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x)
If you want to backport a patch to the 2.4.x branch just add your proposal to 
the STATUS file
as last proposal in the "PATCHES PROPOSED TO BACKPORT FROM TRUNK:" section.
Three committers (that can include you) need to give a +1 vote to the patch and 
no -1 must be voted.
Then the patch can be backported.
Keep in mind that backports need to follow the API and ABI rules we have for 
stable branches.
In this case this means that new stuff can be added to the API but you cannot 
remove or change
existing API's. For ABI compliance it is important to add new members of 
structs always to the end of the struct.
If a patch cannot be merged unmodified from trunk please create a patch that 
can be merged and either point to
it or create a PR on Github against the 2.4.x branch. Again the scripts in 
https://svn.apache.org/repos/asf/httpd/dev-tools/github
are handy for this task.

Regards

Rüdiger


> 
> I have the following changes for DAVLockDiscovery:
> r1905327
> r1905230
> r1905229
> r1905206
> r1905170
> r1904662
> r1904638
> 


Re: svn commit: r1905170 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c

2022-11-09 Thread Ruediger Pluem



On 11/9/22 10:59 AM, Emmanuel Dreyfus wrote:
> On Wed, Nov 09, 2022 at 08:19:47AM +0100, Ruediger Pluem wrote:
>> Better do not set it here, but leave it to 0 aka DAV_ENABLED_UNSET.
>> This makes it possible to use DAV_INHERIT_VALUE in dav_merge_dir_config
>> The corresponding code for dav_merge_dir_config is missing in this this 
>> patch.
> 
> It was committed before:
> newconf->allow_lockdiscovery = DAV_INHERIT_VALUE(parent, child,
>allow_lockdiscovery);

Sorry I missed this. Good catch.

> 
> The chnage below this seems to be enough to do the job. 
> 
> allow_lockdiscovery is only checked against DAV_ENABLED_OFF, hence 
> DAV_ENABLED_UNSET and DAV_ENABLED_ON have the same effect, which is 
> what is desired for backward compatibility sake.
> 
> Index: modules/dav/main/mod_dav.c
> ===
> --- modules/dav/main/mod_dav.c  (revision 1905191)
> +++ modules/dav/main/mod_dav.c  (working copy)
> @@ -160,7 +160,7 @@
>  
>  conf = (dav_dir_conf *)apr_pcalloc(p, sizeof(*conf));
>  
> -conf->allow_lockdiscovery = DAV_ENABLED_ON;
> +conf->allow_lockdiscovery = DAV_ENABLED_UNSET;
>  
>  /* clean up the directory to remove any trailing slash */
>  if (dir != NULL) {
> 
> 
> 

The above fixes this. It is just a change in style because currently we init 
conf to all zero's via apr_pcalloc which means all
fields are in an 'UNSET' state automatically. Hence just removing

conf->allow_lockdiscovery = DAV_ENABLED_ON;

would be more in line with the existing code.

Regards

Rüdiger


Re: Apache HTTP Server dependency on OpenSSL

2022-11-08 Thread Ruediger Pluem



On 11/9/22 8:29 AM, Turritopsis Dohrnii Teo En Ming wrote:
> Subject: Apache HTTP Server dependency on OpenSSL
> 
> Good day from Singapore,
> 
> I read that Apache HTTP Server depends on/requires OpenSSL 1.1.1 to operate a 
> TLS 1.3 web server.
> 
> Can we use OpenSSL 3.0.7 instead of OpenSSL 1.1.1? Is it supported?

2.4.54 supports OpenSSL 3.0

Regards

Rüdiger



Re: svn commit: r1905170 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c

2022-11-08 Thread Ruediger Pluem



On 11/9/22 2:12 AM, m...@apache.org wrote:
> Author: manu
> Date: Wed Nov  9 01:12:26 2022
> New Revision: 1905170
> 
> URL: http://svn.apache.org/viewvc?rev=1905170=rev
> Log:
> Turn DavLockDiscovery into a flag
> 
> As requested on dev@httpd.apache.org, turn DavLockDiscovery into a Flag.
> Expressions can still be used by enclosing the directive by 
> ...
> 
> 
> Modified:
> httpd/httpd/trunk/modules/dav/main/mod_dav.c
> 
> Modified: httpd/httpd/trunk/modules/dav/main/mod_dav.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/main/mod_dav.c?rev=1905170=1905169=1905170=diff
> ==
> --- httpd/httpd/trunk/modules/dav/main/mod_dav.c (original)
> +++ httpd/httpd/trunk/modules/dav/main/mod_dav.c Wed Nov  9 01:12:26 2022
> @@ -83,7 +83,7 @@ typedef struct {
>  const char *dir;
>  int locktimeout;
>  int allow_depthinfinity;
> -ap_expr_info_t *allow_lockdiscovery;
> +int allow_lockdiscovery;
>  
>  } dav_dir_conf;
>  
> @@ -160,6 +160,8 @@ static void *dav_create_dir_config(apr_p
>  
>  conf = (dav_dir_conf *)apr_pcalloc(p, sizeof(*conf));
>  
> +conf->allow_lockdiscovery = DAV_ENABLED_ON;
> +

Better do not set it here, but leave it to 0 aka DAV_ENABLED_UNSET.
This makes it possible to use DAV_INHERIT_VALUE in dav_merge_dir_config
The corresponding code for dav_merge_dir_config is missing in this this patch.

>  /* clean up the directory to remove any trailing slash */
>  if (dir != NULL) {
>  char *d;

Otherwise good first commit. Thanks for taking care.
Would you provide some documentation for the new directive in 
docs/manual/mod/mod_dav.xml to make it perfect?
Have a look here how to update the html after you adjusted the xml file:

https://httpd.apache.org/docs-project/docsformat.html

Typically the change to the xml file and the updated html files are committed 
separately e.g. look at

http://svn.apache.org/viewvc?rev=1904805=rev
http://svn.apache.org/viewvc?rev=1904806=rev


Regards

Rüdiger


Re: [libapreq2] nits to pick about the patches to util.c over the past few years

2022-10-31 Thread Ruediger Pluem



On 10/31/22 5:21 PM, Ruediger Pluem wrote:
> 
> 
> On 10/30/22 4:28 PM, Joe Schaefer wrote:
>> And to be frank- framing my input as me slagging on Yann is grotesque.  You 
>> ship GA releases as a team, and so when you ship a dud
>> like 2.17 you should take your lumps as a team.
> 
> I admit that the libapreq2 codebase doesn't get that much review attention as 
> other parts of httpd and does not draw that much
> developer interest. Hence we were very grateful that Yann took some time to 
> do the needful, fixed the security issue and took it
> to a release to get that issue fixed for the users. Thank you Yann for this. 
> The fact that at least the existing tests still
> passed after the changes was at least for me a good indicator that the 
> changes don't break stuff and are fine.
> I also understand if you feel upset if the codebase you wrote and regard as 
> better was changed and if you feel that the code
> deserves more love and care.
> The way to fix this is to participate here in a constructive way to get it in 
> the direction you want it to be.
> If you feel that this isn't the correct community for this codebase we can 
> also talk about this as Eric indicated.
> I am with Joe Orton and Greg that you are around for long enough to know that 
> the way you started this brought attention to your
> desires but was counterproductive in many ways (tone of the emails, number of 
> emails, top postings, too broad statements) to get
> things were you want them to be.

Having said this, lets move forward and get into the details which cases are 
broken and what parts of the code cause this and
should be different.

Regards

Rüdiger


Re: [libapreq2] nits to pick about the patches to util.c over the past few years

2022-10-31 Thread Ruediger Pluem



On 10/30/22 4:28 PM, Joe Schaefer wrote:
> And to be frank- framing my input as me slagging on Yann is grotesque.  You 
> ship GA releases as a team, and so when you ship a dud
> like 2.17 you should take your lumps as a team.

I admit that the libapreq2 codebase doesn't get that much review attention as 
other parts of httpd and does not draw that much
developer interest. Hence we were very grateful that Yann took some time to do 
the needful, fixed the security issue and took it
to a release to get that issue fixed for the users. Thank you Yann for this. 
The fact that at least the existing tests still
passed after the changes was at least for me a good indicator that the changes 
don't break stuff and are fine.
I also understand if you feel upset if the codebase you wrote and regard as 
better was changed and if you feel that the code
deserves more love and care.
The way to fix this is to participate here in a constructive way to get it in 
the direction you want it to be.
If you feel that this isn't the correct community for this codebase we can also 
talk about this as Eric indicated.
I am with Joe Orton and Greg that you are around for long enough to know that 
the way you started this brought attention to your
desires but was counterproductive in many ways (tone of the emails, number of 
emails, top postings, too broad statements) to get
things were you want them to be.


Regards

Rüdiger



Re: svn commit: r1904638 - in /httpd/httpd/trunk: changes-entries/DAVLockDiscovery.txt modules/dav/main/mod_dav.c modules/dav/main/mod_dav.h modules/dav/main/props.c

2022-10-27 Thread Ruediger Pluem



On 10/26/22 8:51 PM, Emmanuel Dreyfus wrote:
> On Mon, Oct 17, 2022 at 12:04:55PM +0200, Ruediger Pluem wrote:
>> Why do we need to use an Apache expression here? Wouldn't it be sufficient 
>> to have
>> DAVLockDiscovery as a flag (On/Off)
> 
> I posted a patch for that change, along with documentation, on
> https://bz.apache.org/bugzilla/show_bug.cgi?id=66313
> 
> Is it fine for you?
> 


Looks good. Thanks.

Regards

Rüdiger


Re: --with-pcre Path Error configuring Apache 2.4.53-.54

2022-10-24 Thread Ruediger Pluem



On 10/22/22 11:52 PM, William A Rowe Jr wrote:
> There is an alternate solution for 2.4.x that is simpler for me to offer.
> 
> it could be argued that an abandoned project, deprecated in 2015, and
> entirely abandoned effective June 2021, is only supported with an explicit
> path to the pcre-config file, and that only pcre2-config would ever be
> searched for in future 2.4.x releases, whether we observe --with-pcre
> or establish a new --with-pcre2 config flag. It might not be a bad idea

I think this is fine. It probably breaks some configure calls on some LTS Linux 
distributions
where PCRE is still delivered and people did not install the PCRE2 devel 
package yet.
But these can be fixed by either pointing to pcre-config explicitly or by 
installing the PCRE2 devel
package.

Regards

Rüdiger



Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/

2022-10-19 Thread Ruediger Pluem



On 10/19/22 11:28 AM, Stefan Eissing via dev wrote:
> 
> 
>> Am 18.10.2022 um 21:20 schrieb Roy T. Fielding :
>>
>>> On Oct 6, 2022, at 2:17 AM, Stefan Eissing via dev  
>>> wrote:
>>>
 Am 05.10.2022 um 19:34 schrieb Stefan Eissing via dev 
 :



> Am 05.10.2022 um 18:48 schrieb Eric Covener :
>
> On Wed, Oct 5, 2022 at 12:44 PM Roy T. Fielding  wrote:
>>
>>> On Sep 26, 2022, at 5:29 AM, ic...@apache.org wrote:
>>>
>>> Author: icing
>>> Date: Mon Sep 26 12:29:47 2022
>>> New Revision: 1904269
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1904269=rev
>>> Log:
>>> *) mod_http2: new directive "H2HeaderStrictness" to control the 
>>> compliance
>>>  level of header checks as defined in the HTTP/2 RFCs. Default is 7540.
>>>  9113 activates the checks for forbidden leading/trailing whitespace in
>>>  field values (available from nghttp2 v1.50.0 on).
>>
>> I am not seeing why that should be optional. It adds overhead, but it 
>> reduces
>> variability (for HPACK) and might prevent some downstream 
>> vulnerabilities, IIRC.
>> Maybe an internal switch for testing with default on?
>>>
>>> To add some more detail:
>>>
>>> - rfc9113 ch 8.2.1 states: "A field value MUST NOT start or end with an 
>>> ASCII whitespace character"
>>> - nghttp2 v1.49.0 implemented that check, non-optional. Things broke.
>>> - nghttp2 v1.50.0 added to its API so that the application can control the 
>>> behaviour on request of Daniel Stenberg
>>> - I added "H2HeaderStrictness" (in trunk only for now) to steer that.
>>>
>>> This is not a normalization issue, it's a hard "MUST NOT" e.g. rejection of 
>>> the request/response carrying such a field. While I agree that there are 
>>> many advantages in having fields more strict, the way to get there is not 
>>> clear.
>>>
>>> I am totally in favour of avoiding "H2HeaderStrictness", but just enforcing 
>>> rfc9113 here will not do. What would our response be for people whose 
>>> legacy clients/fronted applications will no longer work?
>>
>> Well, normally, I don't have a problem with just breaking them.
>> They are broken. Someone can fix them.
>>
>> It isn't a normalization issue. Whitespace that magically appeared when h2
>> changed the framing of header field values immediately became a security
>> vulnerability for all downstream recipients of h2/h3 messages that use
>> generic HTTP (semantic) field values expecting that stuff to have
>> been excluded during parsing.
>>
>> IOW, it's a security hole and our code either fixes it or gets a CVE.
>> We MUST NOT forward the malformed data in fields. That is not an option.
>>
>> OTOH, how we deal with a request containing malformed data in fields
>> (after making it well-formed) is a SHOULD send 400, not a MUST.
>> If we want to be super nice to the folks shipping bad code (or running pen
>> testers) and have an option to strip the naughty bits out while forwarding
>> the message, that's fine with me as an optional directive. But that won't
>> help them interop with the rest of the Internet.
> 
> Speaking about our implementation, I just added tests to trunk. Configuring 
> 'Header add name "$value"' and making http/1.1 requests, curl sees the 
> returned headers as:
> 
> configured, returned
> ['123 ', '123 '],  # trailing space stays
> ['123\t', '123\t'],# trailing tab stays
> [' 123', '123'],   # leading space is stripped
> ['  123', '123'],  # leading spaces are stripped
> ['\t123', '123'],  # leading tab is stripped
> 
> Test case 'test_h1_007_02'. 
> 
> Linking nghttp2 v1.49.0 and newer, without further config, will RST_STREAM 
> cases 1 and 2 when using HTTP/2. While succeeding with HTTP/1.1. I would find 
> that highly confusing.

Thanks. Initially I thought it was about request headers, but the tests are 
about response headers. Does the "no leading/trailing
whitespace" rule apply to both?

> 
> I understand that in the definition of Core HTTP, leading/trailing whitespace 
> MUST NOT carry any semantics and SHOULD be avoided. If Apache httpd, in its 
> version-independent field handling, should decide to universally strip such 
> ws, that would be totally fine with me.
> 
> The question in implementing mod_http2 is when cases 1+2 should FAIL and when 
> we should serve them. If we have any other config I can derive that from, I'd 
> happily remove "H2HeaderStrictness" again.

As far as I understand Roy, we should deny by default but have a directive that 
allows us to strip leading/trailing whitespaces
from the header values and accept them then (provided they are ok otherwise).
But I would be also fine if we just silently strip the leading/trailing ws 
always.

Regards

Rüdiger



Re: svn commit: r1904638 - in /httpd/httpd/trunk: changes-entries/DAVLockDiscovery.txt modules/dav/main/mod_dav.c modules/dav/main/mod_dav.h modules/dav/main/props.c

2022-10-17 Thread Ruediger Pluem



On 10/17/22 11:48 AM, yla...@apache.org wrote:
> Author: ylavic
> Date: Mon Oct 17 09:48:11 2022
> New Revision: 1904638
> 
> URL: http://svn.apache.org/viewvc?rev=1904638=rev
> Log:
> mod_dav: Allow to disable lock discovery via an DAVLockDiscovery expression.
> 
> mod_dav-fs scales badly when a few clients run PROPFIND requests to discover
> directory content. Each PROPFIND involves lockdiscovery, which in turn waits
> for a locked access to the file containing the lock database. Performances
> quickly drop because of lock contention on this file.
> 
> Add a DAVLockDiscovery configuration directive that allows lockdiscovery to be
> disabled. Its argument is an Apache expression so that flexible configuration
> are possible (per-request).
> 
> When lock discovery is disabled, an empty lockdiscovery property is returned 
> on
> POPRFIND methods, just like if no lock was set on the object. That should 
> cause
> no regression, since a client cannot rely on lockdiscovery to decide when a
> file should be accessed, the LOCK methood must be used.
> 
> If DAVLockDiscovery is not specified, the behavior is unchanged.

Why do we need to use an Apache expression here? Wouldn't it be sufficient to 
have
DAVLockDiscovery as a flag (On/Off) with default to On that can be placed in an
 block if expressions are needed?

Regards

Rüdiger



Re: friends of mod_proxy

2022-10-10 Thread Ruediger Pluem



On 10/10/22 6:35 PM, Eric Covener wrote:
> On Mon, Oct 10, 2022 at 11:55 AM Yann Ylavic  wrote:
>>
>> On Fri, Oct 7, 2022 at 9:14 PM Ruediger Pluem  wrote:
>>>
>>> On 10/7/22 7:11 PM, Stefan Eissing via dev wrote:
>>>>
>>>>
>>>>> Am 07.10.2022 um 18:45 schrieb Yann Ylavic :
>>>>>
>>>>
>>>> Thanks, Yann, for the detailed explanation on how this works and that the 
>>>> default does the right thing.
>>>
>>> +1. I missed the default of not reusing the connection in these cases but 
>>> having the possibility to override it like the user did.
>>> But shouldn't we default to not reusing the connection in case of a $ 
>>> inside the port as well?
>>
>> We default to disabling connection reuse for any $ substitution
>> already, the compat issue is for an explicit enablereuse=on which used
>> to "work" because it was ignored..
>>
>> How about a patch like the attached one now, which disables connection
>> reuse definitively (regardless of enablereuse) if there is a $
>> substitution in the hostname or port part of the URL?
>> The patch uses the existing "is_address_reusable" flag (set to false
>> initially in this case), and since it's not configurable by the user
>> we are safe from a connection reuse point of vue in any case.
> 
> +1 in concept
> 

Looks sensible. +1.

Regards

Rüdiger


Re: friends of mod_proxy

2022-10-07 Thread Ruediger Pluem



On 10/7/22 7:11 PM, Stefan Eissing via dev wrote:
> 
> 
>> Am 07.10.2022 um 18:45 schrieb Yann Ylavic :
>>

> 
> Thanks, Yann, for the detailed explanation on how this works and that the 
> default does the right thing.

+1. I missed the default of not reusing the connection in these cases but 
having the possibility to override it like the user did.
But shouldn't we default to not reusing the connection in case of a $ inside 
the port as well?

> 
> My guess is that such configurations are pretty rare, as their security is 
> not good in general. The only thing that comes to mind is logging a warning 
> for such cases. 

+1

Regards

Rüdiger



Re: friends of mod_proxy

2022-10-06 Thread Ruediger Pluem



On 10/6/22 4:20 PM, Stefan Eissing via dev wrote:
> Friends of mod_proxy, I have a question:
> 
> In  someone reported wrong 
> connection reuse for a config like:
> 
> ProxyPassMatch ^/(prod|dev)/([-a-z0-9]+)/(.*)$ h2://$2.internal/$1/$2/$3 
> enablereuse=on keepalive=on
> 
> Leaving aside the issue that such a pattern is insecure due to the client 
> influencing the hostname, the fact remains that mod_proxy_http2 will use a 
> previous connection, even if the matched hostname is different. I replicated 
> that, using "just" ports in a test case:
> 
> ProxyPassMatch ^/h2proxy/([0-9]+)/(.*)$ h2c://127.0.0.1:$1/$2 enablereuse=on 
> keepalive=on
> 
> Then
> 1. /h2proxy/5002/hello.py
> 2. /h2proxy/5004/hello.py
> 
> results in 2) re-using the connection of 1). The log file says for 2):
> 
> [proxy:debug] proxy_util.c(2538): AH00942: H2C: has acquired connection for 
> (127.0.0.1:80)
> [proxy:debug] proxy_util.c(2596): [remote 127.0.0.1:60121] AH00944: 
> connecting h2c://127.0.0.1:5004/hello.py to 127.0.0.1:5004
> [proxy:debug] proxy_util.c(2819): [remote 127.0.0.1:60121] AH00947: connected 
> /hello.py to 127.0.0.1:5002
> [proxy_http2:trace1] mod_proxy_http2.c(374): [remote 127.0.0.1:60121] H2: 
> determined connect to 127.0.0.1:5002
> [proxy:trace2] proxy_util.c(3101): H2C: reusing backend connection 
> 127.0.0.1:60120<>127.0.0.1:5002
> 
> and that looks wrong.
> 
> Question: do we have a bug or do we consider such ProxyPassMatch as broken 
> and require at least enablereuse=off?

Depends on your point of view :-). As you state such setups can be considered 
unsafe in general, but maybe we should set
enablereuse=off implicitly if we detect a $ in the host or port component or we 
should reject them completely.
But I guess enablereuse=off is more backwards compatible.
If you still need to reuse connections for such cases you can still use 
RewriteRules and appropriate 

Re: svn commit: r1904297 - in /httpd/httpd/trunk/modules/http2: h2_c2.c h2_mplx.c h2_mplx.h h2_proxy_session.c h2_proxy_util.c h2_push.c h2_session.c h2_session.h h2_stream.c h2_util.c h2_util.h h2_wo

2022-09-27 Thread Ruediger Pluem



On 9/27/22 12:53 PM, ic...@apache.org wrote:
> Author: icing
> Date: Tue Sep 27 10:53:51 2022
> New Revision: 1904297
> 
> URL: http://svn.apache.org/viewvc?rev=1904297=rev
> Log:
>   *) mod_http2: type adjustments and castings for 
> int/apr_uint32_t/apr_size_t/apr_off_t.
> 
> 
> Modified:
> httpd/httpd/trunk/modules/http2/h2_c2.c
> httpd/httpd/trunk/modules/http2/h2_mplx.c
> httpd/httpd/trunk/modules/http2/h2_mplx.h
> httpd/httpd/trunk/modules/http2/h2_proxy_session.c
> httpd/httpd/trunk/modules/http2/h2_proxy_util.c
> httpd/httpd/trunk/modules/http2/h2_push.c
> httpd/httpd/trunk/modules/http2/h2_session.c
> httpd/httpd/trunk/modules/http2/h2_session.h
> httpd/httpd/trunk/modules/http2/h2_stream.c
> httpd/httpd/trunk/modules/http2/h2_util.c
> httpd/httpd/trunk/modules/http2/h2_util.h
> httpd/httpd/trunk/modules/http2/h2_workers.c
> httpd/httpd/trunk/modules/http2/h2_workers.h
> httpd/httpd/trunk/modules/http2/mod_http2.c
> 

> Modified: httpd/httpd/trunk/modules/http2/h2_stream.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_stream.c?rev=1904297=1904296=1904297=diff
> ==
> --- httpd/httpd/trunk/modules/http2/h2_stream.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_stream.c Tue Sep 27 10:53:51 2022
> @@ -147,7 +147,7 @@ static int on_frame(h2_stream_state_t st
>  {
>  ap_assert(frame_type >= 0);
>  ap_assert(state >= 0);
> -if (frame_type >= maxlen) {
> +if (frame_type < 0 || (apr_size_t)frame_type >= maxlen) {

Do we need to check for frame_type < 0 with ap_assert(frame_type >= 0); two 
lines above?

>  return state; /* NOP, ignore unknown frame types */
>  }
>  return on_map(state, frame_map[frame_type]);

> Modified: httpd/httpd/trunk/modules/http2/h2_util.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_util.c?rev=1904297=1904296=1904297=diff
> ==
> --- httpd/httpd/trunk/modules/http2/h2_util.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_util.c Tue Sep 27 10:53:51 2022

> @@ -1169,56 +1169,24 @@ apr_size_t h2_util_table_bytes(apr_table
>   * h2_util for bucket brigades
>   
> **/
>  
> -static apr_status_t last_not_included(apr_bucket_brigade *bb,
> -  apr_off_t maxlen,
> -  apr_bucket **pend)
> +static void fit_bucket_into(apr_bucket *b, apr_off_t *plen)
>  {
> -apr_bucket *b;
> -apr_status_t status = APR_SUCCESS;
> -
> -if (maxlen >= 0) {
> -/* Find the bucket, up to which we reach maxlen/mem bytes */
> -for (b = APR_BRIGADE_FIRST(bb);
> - (b != APR_BRIGADE_SENTINEL(bb));
> - b = APR_BUCKET_NEXT(b)) {
> -
> -if (APR_BUCKET_IS_METADATA(b)) {
> -/* included */
> -}
> -else {
> -if (b->length == ((apr_size_t)-1)) {
> -const char *ign;
> -apr_size_t ilen;
> -status = apr_bucket_read(b, , , APR_BLOCK_READ);
> -if (status != APR_SUCCESS) {
> -return status;
> -}
> -}
> -
> -if (maxlen == 0 && b->length > 0) {
> -*pend = b;
> -return status;
> -}
> -
> -if (APR_BUCKET_IS_FILE(b)
> -#if APR_HAS_MMAP
> - || APR_BUCKET_IS_MMAP(b)
> -#endif
> - ) {
> -/* we like to move it, always */
> -}
> -else if (maxlen < (apr_off_t)b->length) {
> -apr_bucket_split(b, (apr_size_t)maxlen);
> -maxlen = 0;
> -}
> -else {
> -maxlen -= b->length;
> -}
> -}
> -}
> +/* signed apr_off_t is at least as large as unsigned apr_size_t.
> + * Propblems may arise when they are both the same size. Then

Problems instead of Propblems :-)

> + * the bucket length *may* be larger than a value we can hold
> + * in apr_off_t. Before casting b->length to apr_off_t we must
> + * check the limitations.
> + * After we resized the bucket, it is safe to cast and substract.
> + */
> +if ((sizeof(apr_off_t) == sizeof(apr_int64_t)
> + && b->length > APR_INT64_MAX)
> +   || (sizeof(apr_off_t) == sizeof(apr_int32_t)
> +   && b->length > APR_INT32_MAX)
> +   || *plen < (apr_off_t)b->length) {
> +/* bucket is longer the *plen */
> +apr_bucket_split(b, *plen);
>  }
> -*pend = APR_BRIGADE_SENTINEL(bb);
> -return status;
> +*plen -= (apr_off_t)b->length;
>  }
>  



Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/

2022-09-27 Thread Ruediger Pluem



On 9/26/22 2:29 PM, ic...@apache.org wrote:
> Author: icing
> Date: Mon Sep 26 12:29:47 2022
> New Revision: 1904269
> 
> URL: http://svn.apache.org/viewvc?rev=1904269=rev
> Log:
>   *) mod_http2: new directive "H2HeaderStrictness" to control the compliance
>  level of header checks as defined in the HTTP/2 RFCs. Default is 7540.
>  9113 activates the checks for forbidden leading/trailing whitespace in
>  field values (available from nghttp2 v1.50.0 on).
> 
>- source sync with github version
>- fix for keepalive idle wait in mpm_worker setup
>- ensuring EOS when secondary connection has been handled
>- fixed race in late input EOS arrival when stream was
>  already scheduled for execution.
> 
> 
> Added:
> httpd/httpd/trunk/changes-entries/h2_header_strictness.txt
> Modified:
> httpd/httpd/trunk/docs/manual/mod/mod_http2.xml
> httpd/httpd/trunk/modules/http2/config2.m4
> httpd/httpd/trunk/modules/http2/h2.h
> httpd/httpd/trunk/modules/http2/h2_bucket_beam.c
> httpd/httpd/trunk/modules/http2/h2_bucket_beam.h
> httpd/httpd/trunk/modules/http2/h2_c1.c
> httpd/httpd/trunk/modules/http2/h2_c1_io.c
> httpd/httpd/trunk/modules/http2/h2_c2.c
> httpd/httpd/trunk/modules/http2/h2_config.c
> httpd/httpd/trunk/modules/http2/h2_config.h
> httpd/httpd/trunk/modules/http2/h2_mplx.c
> httpd/httpd/trunk/modules/http2/h2_mplx.h
> httpd/httpd/trunk/modules/http2/h2_request.c
> httpd/httpd/trunk/modules/http2/h2_session.c
> httpd/httpd/trunk/modules/http2/h2_stream.c
> httpd/httpd/trunk/modules/http2/h2_util.c
> httpd/httpd/trunk/modules/http2/h2_version.h
> httpd/httpd/trunk/modules/http2/h2_workers.c
> httpd/httpd/trunk/modules/http2/h2_workers.h
> httpd/httpd/trunk/test/modules/http2/test_105_timeout.py
> 

> Modified: httpd/httpd/trunk/modules/http2/h2_workers.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_workers.c?rev=1904269=1904268=1904269=diff
> ==
> --- httpd/httpd/trunk/modules/http2/h2_workers.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_workers.c Mon Sep 26 12:29:47 2022

> @@ -451,7 +452,7 @@ h2_workers *h2_workers_create(server_rec
>  workers->pool = pool;
>  workers->min_active = min_active;
>  workers->max_slots = max_slots;
> -workers->idle_limit = (idle_limit > 0)? idle_limit : 
> apr_time_from_sec(10);
> +workers->idle_limit = (int)((idle_limit > 0)? idle_limit : 
> apr_time_from_sec(10));

Is it possible to change idle_limit in the workers struct to an apr_time_t? 
Otherwise the cast could truncate here in an
unexpected way as users are not limited in what they can configure for 
H2MaxWorkerIdleSeconds.
I am also not sure if a cast of a large value could result in a negative value.

>  workers->dynamic = (workers->min_active < workers->max_slots);
>  
>  ap_log_error(APLOG_MARK, APLOG_INFO, 0, s,

Regards

Rüdiger


Re: apreq documentation

2022-08-29 Thread Ruediger Pluem



On 8/28/22 4:10 PM, Eric Covener wrote:
>> If we reach an agreement on the new look, it can be uploaded on a.o.org
>> to refresh the look and be more consistent with the rest of the website.
> 
> +1 for the updated version, thanks!

+1 from me as well.

Regards

Rüdiger



Re: svn commit: r1854963 - in /httpd/httpd/trunk: ./ modules/http2/

2022-08-17 Thread Ruediger Pluem



On 8/17/22 9:57 AM, Stefan Eissing via dev wrote:
> 
> 
>> Am 17.08.2022 um 09:26 schrieb Ruediger Pluem :
>>
>>
>>
>> On 3/7/19 10:41 AM, ic...@apache.org wrote:
>>> Author: icing
>>> Date: Thu Mar  7 09:41:15 2019
>>> New Revision: 1854963
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1854963=rev
>>> Log:
>>>  *) mod_http2: new configuration directive: ```H2Padding numbits``` to 
>>> control 
>>> padding of HTTP/2 payload frames. 'numbits' is a number from 0-8,
>>> controlling the range of padding bytes added to a frame. The actual 
>>> number
>>> added is chosen randomly per frame. This applies to HEADERS, DATA and 
>>> PUSH_PROMISE
>>> frames equally. The default continues to be 0, e.g. no padding. [Stefan 
>>> Eissing] 
>>>
>>>  *) mod_http2: ripping out all the h2_req_engine internal features now that 
>>> mod_proxy_http2
>>> has no more need for it. Optional functions are still declared but no 
>>> longer implemented.
>>> While previous mod_proxy_http2 will work with this, it is recommeneded 
>>> to run the matching
>>> versions of both modules. [Stefan Eissing]
>>>
>>>  *) mod_proxy_http2: changed mod_proxy_http2 implementation and fixed 
>>> several bugs which
>>> resolve PR63170. The proxy module does now a single h2 request on the 
>>> (reused)
>>> connection and returns. [Stefan Eissing]
>>>
>>>
>>> Removed:
>>>httpd/httpd/trunk/modules/http2/h2_ngn_shed.c
>>>httpd/httpd/trunk/modules/http2/h2_ngn_shed.h
>>> Modified:
>>>httpd/httpd/trunk/CHANGES
>>>httpd/httpd/trunk/modules/http2/config2.m4
>>>httpd/httpd/trunk/modules/http2/h2.h
>>>httpd/httpd/trunk/modules/http2/h2_config.c
>>>httpd/httpd/trunk/modules/http2/h2_config.h
>>>httpd/httpd/trunk/modules/http2/h2_conn_io.c
>>>httpd/httpd/trunk/modules/http2/h2_mplx.c
>>>httpd/httpd/trunk/modules/http2/h2_mplx.h
>>>httpd/httpd/trunk/modules/http2/h2_proxy_session.c
>>>httpd/httpd/trunk/modules/http2/h2_proxy_session.h
>>>httpd/httpd/trunk/modules/http2/h2_request.c
>>>httpd/httpd/trunk/modules/http2/h2_session.c
>>>httpd/httpd/trunk/modules/http2/h2_session.h
>>>httpd/httpd/trunk/modules/http2/h2_stream.c
>>>httpd/httpd/trunk/modules/http2/h2_task.c
>>>httpd/httpd/trunk/modules/http2/h2_task.h
>>>httpd/httpd/trunk/modules/http2/h2_version.h
>>>httpd/httpd/trunk/modules/http2/mod_http2.c
>>>httpd/httpd/trunk/modules/http2/mod_http2.h
>>>httpd/httpd/trunk/modules/http2/mod_proxy_http2.c
>>>
>>
>>> Modified: httpd/httpd/trunk/modules/http2/mod_http2.h
>>> URL: 
>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/mod_http2.h?rev=1854963=1854962=1854963=diff
>>> ==
>>> --- httpd/httpd/trunk/modules/http2/mod_http2.h (original)
>>> +++ httpd/httpd/trunk/modules/http2/mod_http2.h Thu Mar  7 09:41:15 2019
>>> @@ -30,22 +30,20 @@ APR_DECLARE_OPTIONAL_FN(int,
>>>
>>>
>>> /***
>>> - * HTTP/2 request engines
>>> + * START HTTP/2 request engines (DEPRECATED)
>>>  
>>> **/
>>> +
>>> +/* The following functions were introduced for the experimental 
>>> mod_proxy_http2
>>> + * support, but have been abandoned since.
>>> + * They are still declared here for backward compatibiliy, in case someone
>>> + * tries to build an old mod_proxy_http2 against it, but will disappear
>>> + * completely sometime in the future.
>>> + */ 
>>>
>>> struct apr_thread_cond_t;
>>> -
>>> typedef struct h2_req_engine h2_req_engine;
>>> -
>>> typedef void http2_output_consumed(void *ctx, conn_rec *c, apr_off_t 
>>> consumed);
>>>
>>> -/**
>>> - * Initialize a h2_req_engine. The structure will be passed in but
>>> - * only the name and master are set. The function should initialize
>>> - * all fields.
>>> - * @param engine the allocated, partially filled structure
>>> - * @param r  the first request to process, or NULL
>>> - */
>>> typedef apr_status

Re: svn commit: r1854963 - in /httpd/httpd/trunk: ./ modules/http2/

2022-08-17 Thread Ruediger Pluem



On 3/7/19 10:41 AM, ic...@apache.org wrote:
> Author: icing
> Date: Thu Mar  7 09:41:15 2019
> New Revision: 1854963
> 
> URL: http://svn.apache.org/viewvc?rev=1854963=rev
> Log:
>   *) mod_http2: new configuration directive: ```H2Padding numbits``` to 
> control 
>  padding of HTTP/2 payload frames. 'numbits' is a number from 0-8,
>  controlling the range of padding bytes added to a frame. The actual 
> number
>  added is chosen randomly per frame. This applies to HEADERS, DATA and 
> PUSH_PROMISE
>  frames equally. The default continues to be 0, e.g. no padding. [Stefan 
> Eissing] 
>   
>   *) mod_http2: ripping out all the h2_req_engine internal features now that 
> mod_proxy_http2
>  has no more need for it. Optional functions are still declared but no 
> longer implemented.
>  While previous mod_proxy_http2 will work with this, it is recommeneded 
> to run the matching
>  versions of both modules. [Stefan Eissing]
>   
>   *) mod_proxy_http2: changed mod_proxy_http2 implementation and fixed 
> several bugs which
>  resolve PR63170. The proxy module does now a single h2 request on the 
> (reused)
>  connection and returns. [Stefan Eissing]
> 
> 
> Removed:
> httpd/httpd/trunk/modules/http2/h2_ngn_shed.c
> httpd/httpd/trunk/modules/http2/h2_ngn_shed.h
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/http2/config2.m4
> httpd/httpd/trunk/modules/http2/h2.h
> httpd/httpd/trunk/modules/http2/h2_config.c
> httpd/httpd/trunk/modules/http2/h2_config.h
> httpd/httpd/trunk/modules/http2/h2_conn_io.c
> httpd/httpd/trunk/modules/http2/h2_mplx.c
> httpd/httpd/trunk/modules/http2/h2_mplx.h
> httpd/httpd/trunk/modules/http2/h2_proxy_session.c
> httpd/httpd/trunk/modules/http2/h2_proxy_session.h
> httpd/httpd/trunk/modules/http2/h2_request.c
> httpd/httpd/trunk/modules/http2/h2_session.c
> httpd/httpd/trunk/modules/http2/h2_session.h
> httpd/httpd/trunk/modules/http2/h2_stream.c
> httpd/httpd/trunk/modules/http2/h2_task.c
> httpd/httpd/trunk/modules/http2/h2_task.h
> httpd/httpd/trunk/modules/http2/h2_version.h
> httpd/httpd/trunk/modules/http2/mod_http2.c
> httpd/httpd/trunk/modules/http2/mod_http2.h
> httpd/httpd/trunk/modules/http2/mod_proxy_http2.c
> 

> Modified: httpd/httpd/trunk/modules/http2/mod_http2.h
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/mod_http2.h?rev=1854963=1854962=1854963=diff
> ==
> --- httpd/httpd/trunk/modules/http2/mod_http2.h (original)
> +++ httpd/httpd/trunk/modules/http2/mod_http2.h Thu Mar  7 09:41:15 2019
> @@ -30,22 +30,20 @@ APR_DECLARE_OPTIONAL_FN(int,
>  
>  
>  
> /***
> - * HTTP/2 request engines
> + * START HTTP/2 request engines (DEPRECATED)
>   
> **/
> +
> +/* The following functions were introduced for the experimental 
> mod_proxy_http2
> + * support, but have been abandoned since.
> + * They are still declared here for backward compatibiliy, in case someone
> + * tries to build an old mod_proxy_http2 against it, but will disappear
> + * completely sometime in the future.
> + */ 
>   
>  struct apr_thread_cond_t;
> -
>  typedef struct h2_req_engine h2_req_engine;
> -
>  typedef void http2_output_consumed(void *ctx, conn_rec *c, apr_off_t 
> consumed);
>  
> -/**
> - * Initialize a h2_req_engine. The structure will be passed in but
> - * only the name and master are set. The function should initialize
> - * all fields.
> - * @param engine the allocated, partially filled structure
> - * @param r  the first request to process, or NULL
> - */
>  typedef apr_status_t http2_req_engine_init(h2_req_engine *engine, 
> const char *id, 
> const char *type,
> @@ -55,35 +53,11 @@ typedef apr_status_t http2_req_engine_in
> http2_output_consumed **pconsumed,
> void **pbaton);
>  
> -/**
> - * Push a request to an engine with the specified name for further 
> processing.
> - * If no such engine is available, einit is not NULL, einit is called 
> - * with a new engine record and the caller is responsible for running the
> - * new engine instance.
> - * @param engine_type the type of the engine to add the request to
> - * @param r   the request to push to an engine for processing
> - * @param einit   an optional initialization callback for a new engine 
> - *of the requested type, should no instance be available.
> - *By passing a non-NULL callback, the caller is willing
> - *to init and run a new engine itself.
> - * @return APR_SUCCESS iff slave 

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

2022-08-02 Thread Ruediger Pluem



On 8/2/22 3:19 PM, Yann Ylavic wrote:
> On Tue, Aug 2, 2022 at 2:59 PM Ruediger Pluem  wrote:
>>
>> On 8/2/22 2:31 PM, Yann Ylavic wrote:
>>> On Mon, Aug 1, 2022 at 8:15 PM Ruediger Pluem  wrote:
>>>>
>>>> On 8/1/22 5:05 PM, Ivan Zhakov wrote:
>>>>>
>>>>> My overall concern is that with the current solution as of r1902858, 
>>>>> there are valid and
>>>>> reasonable allocation patterns that will cause an unbounded memory usage.
>>>>>
>>>>> For example, nothing prevents current or future PCRE versions from doing 
>>>>> this:
>>>>>
>>>>> for (int i = 0; i < N; i++)
>>>>> {
>>>>> void *p = private_malloc();
>>>>> private_free(p);
>>>>> }
>>>>>
>>>>> which is going to cause an unbounded memory usage because private_free() 
>>>>> is
>>>>> a no-op and retains everything that was allocated.
>>>>
>>>> This is true, but these kind of allocation patterns would also cause a lot 
>>>> of problems with a malloc/free backend and thus I think
>>>> they are unlikely and would need to be addressed within PCRE. But even if 
>>>> they are happening and cannot be fixed for our users
>>>> by adjusting PCRE e.g. because they are stuck to distribution versions we 
>>>> still could tackle this then with an apr_bucket_alloc /
>>>> apr_bucket_free approach or worst case even with a malloc / free approach 
>>>> for particular "bad" PCRE versions. But for now the
>>>> current code seems to work well with the current PCRE allocation pattern.
>>>
>>> We could switch to an apr_allocator+apr_memnode_t pattern just now,
>>> and trade the 8K used by the current (sub)pool for an overhead of
>>> sizeof(apr_memnode_t)+8 only, and private_free() just calls
>>> apr_allocator_free().
>>>
>>> Something like the attached patch?
>>
>> But this would allocate at least 8k for each private_malloc call, correct?
> 
> Yes, 8K with apr-1.x (4K with apr-trunk IIRC).
> 
> But anyway pcre2_match() is going to ask at least 40K for its first
> allocation (or 20K with next versions), because it doubles the
> number/size of the heap frames when needed and starts with 20K (either
> on the stack for the current version, or on the heap for the next
> versions [0]).

Thanks for the details as I was not aware of these, but this is what it 
currently does and in the end the discussion from my point
of view was that it could do something entirely different. My point is more: 
The current implementation with the sub pool is fine
with the current allocation pattern of PCRE. If the pattern changes we would 
have ways to adopt the code on our side to react on
these changes.

Regards

Rüdiger



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

2022-08-02 Thread Ruediger Pluem



On 8/2/22 2:31 PM, Yann Ylavic wrote:
> On Mon, Aug 1, 2022 at 8:15 PM Ruediger Pluem  wrote:
>>
>> On 8/1/22 5:05 PM, Ivan Zhakov wrote:
>>>
>>> My overall concern is that with the current solution as of r1902858, there 
>>> are valid and
>>> reasonable allocation patterns that will cause an unbounded memory usage.
>>>
>>> For example, nothing prevents current or future PCRE versions from doing 
>>> this:
>>>
>>> for (int i = 0; i < N; i++)
>>> {
>>> void *p = private_malloc();
>>> private_free(p);
>>> }
>>>
>>> which is going to cause an unbounded memory usage because private_free() is
>>> a no-op and retains everything that was allocated.
>>
>> This is true, but these kind of allocation patterns would also cause a lot 
>> of problems with a malloc/free backend and thus I think
>> they are unlikely and would need to be addressed within PCRE. But even if 
>> they are happening and cannot be fixed for our users
>> by adjusting PCRE e.g. because they are stuck to distribution versions we 
>> still could tackle this then with an apr_bucket_alloc /
>> apr_bucket_free approach or worst case even with a malloc / free approach 
>> for particular "bad" PCRE versions. But for now the
>> current code seems to work well with the current PCRE allocation pattern.
> 
> We could switch to an apr_allocator+apr_memnode_t pattern just now,
> and trade the 8K used by the current (sub)pool for an overhead of
> sizeof(apr_memnode_t)+8 only, and private_free() just calls
> apr_allocator_free().
> 
> Something like the attached patch?

But this would allocate at least 8k for each private_malloc call, correct?

Regards

Rüdiger



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

2022-08-01 Thread Ruediger Pluem



On 8/1/22 5:05 PM, Ivan Zhakov wrote:
> On 2022/07/29 06:19:03 Ruediger Pluem wrote:
>>
>>
>> On 7/16/22 10:28 AM, Ivan Zhakov wrote:
>>> On Wed, 13 Jul 2022 at 18:06, Yann Ylavic >> <mailto:ylavic@gmail.com>> wrote:
>>>
>>> On Wed, Jul 13, 2022 at 1:38 PM Ivan Zhakov >> <mailto:i...@apache.org>> wrote:
>>> >
>>> > On Sun, 10 Jul 2022 at 19:52, Yann Ylavic >> <mailto:ylavic@gmail.com>> wrote:
>>> > >
>>> > > Let me explain (maybe), I thought that PCRE2's match_data were not
>>> > > only containing the captures vector but also all the frames needed 
>>> for
>>> > > matching not-too-complex regexes (at least the initial ones since 
>>> they
>>> > > will be reallocated on need). So the (supposed) 2KB looked necessary
>>> > > to me for a few stack frames.
>>> > > But the actual 256 bytes only made me look at the PCRE2 code again
>>> > > (better this time) for how that can possibly work, and guess what: 
>>> the
>>> > > initial frames are allocated on the stack ([1]) still in PCRE2, and
>>> > > that's 20KiB ([2]) of stack space for each call to pcre2_match().
>>> > >
>>> > > On Alpine Linux for instance (quite used in docker envs) there is 
>>> only
>>> > > 128KiB stack per thread, so I've always configured
>>> > > --no-recurse-on-stack with PCRE1 and used per-thread (and its pool)
>>> > > vector cache, with custom pcre_malloc/free() to avoid performances
>>> > > overhead and be able to run more complex regexes without exploding,
>>> > > and I can't do that now with PCRE2 and without requiring 20KB of 
>>> stack
>>> > > :/
>>> > > So I created a pull request ([3]).
>>> > >
>>> > The 20KB stack usage in PCRE is interesting, but somewhat unrelated. 
>>> But
>>> > I would suggest a different solution: it would be nice if PCRE 
>>> provided
>>> > a compile time directive to specify the maximum stack allocated 
>>> buffer.
>>> > So then distributions like Alpine Linux would specify more appropriate
>>> > defaults.
>>>
>>> This is what pcre2 folks proposed in [3] as a fast(ly acceptable)
>>> solution indeed.
>>> Say START_FRAMES_SIZE is set to some minimum, a single struct
>>> heapframe with 24 captures, that's (on a 64bit system):
>>>   offsetof(heapframe, ovector) + 24 * 2 * sizeof(size_t) = 125 + 384
>>> ~= 512 bytes
>>> which would be much appreciated, but would also put much more pressure
>>> on util_regex's private_malloc().
>>> Will ap_regexec() start to use malloc() quite often with the current
>>> implementation then?
>>> Up to the old/default START_FRAMES_SIZE if it was a good default 
>>> afterall?
>>>
>>> But we are not here, because I don't think distros will change the
>>> default pcre START_FRAMES_SIZE for all possible code they run, so most
>>> httpd users (which run on distros I suppose) will continue to consume
>>> 20K of stack memory for every ap_regexec() call.
>>>
>>> The flexible way for pcre2 users (and httpd maintainers) would be to
>>> let them opt-out (at runtime) of stack allocation from pcre2_match(),
>>> which is the patch I proposed.
>>>
>>> I see several problems in this approach:
>>> 1. Every apr_pool_t consumes at least one page, i.e., 4kb or 8kb depending 
>>> of the platform.
>>> 2. The implementation may introduce unbounded memory usage, if PCRE2 
>>> performs multiple malloc/free during pattern matching. It
>>> seems that currently it doesn't, but we cannot rely on the implementation 
>>> details. Stackbuf code doesn't have this problem,
>>> because it's limited to a fixed-size buffer.
>>> 3. As I said before, the proposed approach effectively increases per thread 
>>> memory usage: 4 or 8 kb for match data stored in
>>> thread state + stack size. Which is something I don't really understand, 
>>> because if we are fine with that, then we can just
>>> increase the thread stack size.
>>>
>>> With the above in mind, I'm -1 for this approach and r1902731.
>>>
>>
>> The latest implementation gi

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

2022-07-29 Thread Ruediger Pluem



On 7/16/22 10:28 AM, Ivan Zhakov wrote:
> On Wed, 13 Jul 2022 at 18:06, Yann Ylavic  > wrote:
> 
> On Wed, Jul 13, 2022 at 1:38 PM Ivan Zhakov  > wrote:
> >
> > On Sun, 10 Jul 2022 at 19:52, Yann Ylavic  > wrote:
> > >
> > > Let me explain (maybe), I thought that PCRE2's match_data were not
> > > only containing the captures vector but also all the frames needed for
> > > matching not-too-complex regexes (at least the initial ones since they
> > > will be reallocated on need). So the (supposed) 2KB looked necessary
> > > to me for a few stack frames.
> > > But the actual 256 bytes only made me look at the PCRE2 code again
> > > (better this time) for how that can possibly work, and guess what: the
> > > initial frames are allocated on the stack ([1]) still in PCRE2, and
> > > that's 20KiB ([2]) of stack space for each call to pcre2_match().
> > >
> > > On Alpine Linux for instance (quite used in docker envs) there is only
> > > 128KiB stack per thread, so I've always configured
> > > --no-recurse-on-stack with PCRE1 and used per-thread (and its pool)
> > > vector cache, with custom pcre_malloc/free() to avoid performances
> > > overhead and be able to run more complex regexes without exploding,
> > > and I can't do that now with PCRE2 and without requiring 20KB of stack
> > > :/
> > > So I created a pull request ([3]).
> > >
> > The 20KB stack usage in PCRE is interesting, but somewhat unrelated. But
> > I would suggest a different solution: it would be nice if PCRE provided
> > a compile time directive to specify the maximum stack allocated buffer.
> > So then distributions like Alpine Linux would specify more appropriate
> > defaults.
> 
> This is what pcre2 folks proposed in [3] as a fast(ly acceptable)
> solution indeed.
> Say START_FRAMES_SIZE is set to some minimum, a single struct
> heapframe with 24 captures, that's (on a 64bit system):
>   offsetof(heapframe, ovector) + 24 * 2 * sizeof(size_t) = 125 + 384
> ~= 512 bytes
> which would be much appreciated, but would also put much more pressure
> on util_regex's private_malloc().
> Will ap_regexec() start to use malloc() quite often with the current
> implementation then?
> Up to the old/default START_FRAMES_SIZE if it was a good default afterall?
> 
> But we are not here, because I don't think distros will change the
> default pcre START_FRAMES_SIZE for all possible code they run, so most
> httpd users (which run on distros I suppose) will continue to consume
> 20K of stack memory for every ap_regexec() call.
> 
> The flexible way for pcre2 users (and httpd maintainers) would be to
> let them opt-out (at runtime) of stack allocation from pcre2_match(),
> which is the patch I proposed.
> 
> I see several problems in this approach:
> 1. Every apr_pool_t consumes at least one page, i.e., 4kb or 8kb depending of 
> the platform.
> 2. The implementation may introduce unbounded memory usage, if PCRE2 performs 
> multiple malloc/free during pattern matching. It
> seems that currently it doesn't, but we cannot rely on the implementation 
> details. Stackbuf code doesn't have this problem,
> because it's limited to a fixed-size buffer.
> 3. As I said before, the proposed approach effectively increases per thread 
> memory usage: 4 or 8 kb for match data stored in
> thread state + stack size. Which is something I don't really understand, 
> because if we are fine with that, then we can just
> increase the thread stack size.
> 
> With the above in mind, I'm -1 for this approach and r1902731.
> 

The latest implementation gives you both, a small allocation on the stack that 
does not drive stack usage
up too much and that should be sufficient for many regex cases. If this does 
not work we use tls to tuck away a pointer to a per
thread pool and we only create the pool in case the stack memory isn't 
sufficient.
If we really increase the memory usage if need to use the pool isn't clear to 
me as we use a subpool of the thread pool and thus
its allocator. Maybe that allocator has free nodes still around and we reuse 
these.
With regards to unbound memory usage I don't think that this is an actual 
problem, but if PCRE would really massively malloc/free
beyond the space we use on the stack just using malloc/free to satisfy these 
needs would be problematic as well and I guess this
would probably need fixing in PCRE then.
Furthermore if this happens we could have a look at using apr_bucket_alloc / 
apr_bucket_free or as this might not fit exactly
take ideas from there.
Could you please reconsider your -1?

Regards

Rüdiger


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

2022-07-28 Thread 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

2022-07-28 Thread 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

2022-07-28 Thread 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

2022-07-28 Thread 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

2022-07-28 Thread 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



Re: Question about slotmem-shm- log file, etc. location

2022-06-23 Thread Ruediger Pluem



On 6/23/22 8:45 PM, jonmcalexan...@wellsfargo.com wrote:
> You know, that makes sense, except, when you may have multiple instances on a 
> server and want to keep those separated. This is still Relative to the 
> ServerRoot, which points to the binaries. What we need is some way it can be 
> relative to the location of the parent of the conf directory where httpd.conf 
> lives. Kind of like Tomcat's concept of CATALINA_HOME and CATALINA_BASE. :-)

No you can give an absolute path that is different for each instance.

Regards

Rüdiger

> 
> 
> Just my .02 worth.
> 
> Dream * Excel * Explore * Inspire
> Jon McAlexander
> Senior Infrastructure Engineer
> Asst. Vice President
> He/His
> 
> Middleware Product Engineering
> Enterprise CIO | EAS | Middleware | Infrastructure Solutions
> 
> 8080 Cobblestone Rd | Urbandale, IA 50322
> MAC: F4469-010
> Tel 515-988-2508 | Cell 515-988-2508
> 
> jonmcalexan...@wellsfargo.com
> This message may contain confidential and/or privileged information. If you 
> are not the addressee or authorized to receive this for the addressee, you 
> must not use, copy, disclose, or take any action based on this message or any 
> information herein. If you have received this message in error, please advise 
> the sender immediately by reply e-mail and delete this message. Thank you for 
> your cooperation.
> 
> 
>> -Original Message-
>> From: Ruediger Pluem 
>> Sent: Thursday, June 23, 2022 1:28 PM
>> To: dev@httpd.apache.org
>> Subject: Re: Question about slotmem-shm- log file, etc. location
>>
>> Have you tried using DefaultRuntimeDir
>> (https://urldefense.com/v3/__https://httpd.apache.org/docs/2.4/mod/cor
>> e.html*defaultruntimedir__;Iw!!F9svGWnIaVPGSwU!s1MtryTq7iWIm62jxvC
>> WYGG9fmIt79giC5WhKWWWTB0ttC2MRtagTdKqEs4FfQ7gLBKA50H2wdJQ9iw
>> BmOQNEac$ )
>>
>> Regards
>>
>> Rüdiger
>>
>> On 6/23/22 7:40 PM, jonmcalexan...@wellsfargo.com wrote:
>>> Hello all.
>>>
>>> I have a question and haven't been able to find an answer yet on the
>>> interwebs. I ran into an issue while setting up mod-proxy-http with the
>> balancer option and when I restarted Apache, it failed with this message:
>>>
>>>
>>>
>>> "AH02611: create: apr_shm_create(/apps/apache/2.4.53/logs/slotmem-
>> shm-p18b8fc23_3.shm) failed"
>>>
>>>
>>>
>>> Turns out I didn't have a logs folder under 2.4.53. Looks like it
>>> looks at the ServerRoot as to where the logs directory is located. We
>>> run our apache servers with separate Instance directories and
>>> centralized binaries to make upgrades, etc. easier. Is there any way to
>> specify something like a slotmem-file-target-location directive in the
>> httpd.conf of the instance, or anything like that? I did see some rumblings
>> around this from back in spring of 2012, but nothing since.
>>>
>>>
>>>
>>> Any assistance would be great.
>>>
>>> *Dream * Excel * Explore * Inspire*
>>>
>>> Jon McAlexander
>>>
>>> Senior Infrastructure Engineer
>>>
>>> Asst. Vice President
>>>
>>> He/His
>>>
>>>
>>>
>>> Middleware Product Engineering
>>>
>>> Enterprise CIO | EAS | Middleware | Infrastructure Solutions
>>>
>>>
>>>
>>> 8080 Cobblestone Rd | Urbandale, IA 50322
>>> MAC: F4469-010
>>>
>>> Tel 515-988-2508 | Cell 515-988-2508
>>>
>>>
>>>
>>> jonmcalexan...@wellsfargo.com
>> <mailto:jonmcalexan...@wellsfargo.com>
>>>
>>> This message may contain confidential and/or privileged information.
>>> If you are not the addressee or authorized to receive this for the
>>> addressee, you must not use, copy, disclose, or take any action based
>>> on this message or any information herein. If you have received this
>> message in error, please advise the sender immediately by reply e-mail and
>> delete this message. Thank you for your cooperation.
>>>
>>>
>>>
> 


Re: Question about slotmem-shm- log file, etc. location

2022-06-23 Thread Ruediger Pluem
Have you tried using DefaultRuntimeDir 
(https://httpd.apache.org/docs/2.4/mod/core.html#defaultruntimedir)

Regards

Rüdiger

On 6/23/22 7:40 PM, jonmcalexan...@wellsfargo.com wrote:
> Hello all.
> 
> I have a question and haven’t been able to find an answer yet on the 
> interwebs. I ran into an issue while setting up
> mod-proxy-http with the balancer option and when I restarted Apache, it 
> failed with this message:
> 
>  
> 
> “AH02611: create: 
> apr_shm_create(/apps/apache/2.4.53/logs/slotmem-shm-p18b8fc23_3.shm) failed"
> 
>  
> 
> Turns out I didn’t have a logs folder under 2.4.53. Looks like it looks at 
> the ServerRoot as to where the logs directory is
> located. We run our apache servers with separate Instance directories and 
> centralized binaries to make upgrades, etc. easier. Is
> there any way to specify something like a slotmem-file-target-location 
> directive in the httpd.conf of the instance, or anything
> like that? I did see some rumblings around this from back in spring of 2012, 
> but nothing since.
> 
>  
> 
> Any assistance would be great.
> 
> *Dream * Excel * Explore * Inspire*
> 
> Jon McAlexander
> 
> Senior Infrastructure Engineer
> 
> Asst. Vice President
> 
> He/His
> 
>  
> 
> Middleware Product Engineering
> 
> Enterprise CIO | EAS | Middleware | Infrastructure Solutions
> 
>  
> 
> 8080 Cobblestone Rd | Urbandale, IA 50322
> MAC: F4469-010
> 
> Tel 515-988-2508 | Cell 515-988-2508
> 
>  
> 
> jonmcalexan...@wellsfargo.com 
> 
> This message may contain confidential and/or privileged information. If you 
> are not the addressee or authorized to receive this
> for the addressee, you must not use, copy, disclose, or take any action based 
> on this message or any information herein. If you
> have received this message in error, please advise the sender immediately by 
> reply e-mail and delete this message. Thank you for
> your cooperation.
> 
>  
> 


Re: svn commit: r1902117 - /httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c

2022-06-20 Thread Ruediger Pluem



On 6/20/22 10:54 PM, rj...@apache.org wrote:
> Author: rjung
> Date: Mon Jun 20 20:54:14 2022
> New Revision: 1902117
> 
> URL: http://svn.apache.org/viewvc?rev=1902117=rev
> Log:
> *) mod_heartmonitor: Allow "HeartbeatMaxServers 0"
>to use file based storage instead of slotmem.
>Needed after setting HeartbeatMaxServers default
>to the documented value 10 in 2.4.54.
>[Jérôme Billira
> 
> Modified:
> httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c
> 
> Modified: httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c?rev=1902117=1902116=1902117=diff
> ==
> --- httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c (original)
> +++ httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c Mon Jun 20 20:54:14 
> 2022
> @@ -892,8 +892,9 @@ static const char *cmd_hm_maxworkers(cmd
>  }
>  
>  maxworkers = atoi(data);
> -if (maxworkers <= 10)
> -return "HeartbeatMaxServers: Should be bigger than 10";
> +if (maxworkers != 0 && maxworkers <= 10)

Shouldn't this be < 10, such that you can set the default value via 
HeartbeatMaxServers.
It seems strange that this can't be done. Furthermore <= 10 does not match the 
error message below.

> +return "HeartbeatMaxServers: Should be 0 for file storage, "
> +   "or greater or equal than 10 for slotmem";

Shouldn't this be documented in the documentation as well that '0' has a 
special meaning and that 1-9 are not allowed?

Regards

Rüdiger



Re: svn commit: r1902005 - in /httpd/httpd/trunk/modules/http2: h2_c1.c h2_config.c h2_config.h h2_mplx.c h2_mplx.h h2_workers.c h2_workers.h mod_http2.c

2022-06-20 Thread Ruediger Pluem



On 6/17/22 11:24 AM, ic...@apache.org wrote:
> Author: icing
> Date: Fri Jun 17 09:24:57 2022
> New Revision: 1902005
> 
> URL: http://svn.apache.org/viewvc?rev=1902005=rev
> Log:
>   *) mod_http2: new implementation of h2 worker pool.
>  - O(1) cost at registration of connection processing producers
>  - no limit on registered producers
>  - join of ongoing work on unregister
>  - callbacks to unlink dependencies into other h2 code
>  - memory cleanup on workers deactivation (on idle timeouts)
>  - idle_limit as apr_time_t instead of seconds
> 
> 
> Modified:
> httpd/httpd/trunk/modules/http2/h2_c1.c
> httpd/httpd/trunk/modules/http2/h2_config.c
> httpd/httpd/trunk/modules/http2/h2_config.h
> httpd/httpd/trunk/modules/http2/h2_mplx.c
> httpd/httpd/trunk/modules/http2/h2_mplx.h
> httpd/httpd/trunk/modules/http2/h2_workers.c
> httpd/httpd/trunk/modules/http2/h2_workers.h
> httpd/httpd/trunk/modules/http2/mod_http2.c

> Modified: httpd/httpd/trunk/modules/http2/h2_workers.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_workers.c?rev=1902005=1902004=1902005=diff
> ==
> --- httpd/httpd/trunk/modules/http2/h2_workers.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_workers.c Fri Jun 17 09:24:57 2022
>  
> @@ -385,13 +415,13 @@ static apr_status_t workers_pool_cleanup
>  }
>  
>  h2_workers *h2_workers_create(server_rec *s, apr_pool_t *pchild,
> -  int min_workers, int max_workers,
> -  int idle_secs)
> +  int max_slots, int min_active, apr_time_t 
> idle_limit)
>  {
>  apr_status_t rv;
>  h2_workers *workers;
>  apr_pool_t *pool;
> -int i, n;
> +apr_allocator_t *allocator;
> +int i, locked = 0;
>  
>  ap_assert(s);
>  ap_assert(pchild);
> @@ -401,7 +431,16 @@ h2_workers *h2_workers_create(server_rec
>   * guarded by our lock. Without this pool, all subpool creations would
>   * happen on the pool handed to us, which we do not guard.
>   */
> -apr_pool_create(, pchild);
> +rv = apr_allocator_create();
> +if (rv != APR_SUCCESS) {
> +goto cleanup;
> +}
> +rv = apr_pool_create_ex(, pchild, NULL, allocator);
> +if (rv != APR_SUCCESS) {
> +apr_allocator_destroy(allocator);
> +goto cleanup;
> +}
> +apr_allocator_owner_set(allocator, pool);
>  apr_pool_tag(pool, "h2_workers");
>  workers = apr_pcalloc(pool, sizeof(h2_workers));
>  if (!workers) {
> @@ -410,19 +449,23 @@ h2_workers *h2_workers_create(server_rec
>  
>  workers->s = s;
>  workers->pool = pool;
> -workers->min_workers = min_workers;
> -workers->max_workers = max_workers;
> -workers->max_idle_secs = (idle_secs > 0)? idle_secs : 10;
> +workers->min_active = min_active;
> +workers->max_slots = max_slots;
> +workers->idle_limit = (idle_limit > 0)? idle_limit : 
> apr_time_from_sec(10);
> +workers->dynamic = (workers->min_active < workers->max_slots);
> +
> +ap_log_error(APLOG_MARK, APLOG_INFO, 0, workers->s,
> + "h2_workers: created with min=%d max=%d idle_ms=%d",
> + workers->min_active, workers->max_slots,
> + (int)apr_time_as_msec(idle_limit));
> +
> +APR_RING_INIT(>idle, h2_slot, link);
> +APR_RING_INIT(>busy, h2_slot, link);
> +APR_RING_INIT(>free, h2_slot, link);
> +APR_RING_INIT(>zombie, h2_slot, link);
>  
> -ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, workers->s,
> - "h2_workers: created with min=%d max=%d idle_timeout=%d 
> sec",
> - workers->min_workers, workers->max_workers,
> - (int)workers->max_idle_secs);
> -/* FIXME: the fifo set we use here has limited capacity. Once the
> - * set is full, connections with new requests do a wait.
> - */
> -rv = h2_fifo_set_create(>mplxs, pool, 16 * 1024);
> -if (rv != APR_SUCCESS) goto cleanup;
> +APR_RING_INIT(>prod_active, ap_conn_producer_t, link);
> +APR_RING_INIT(>prod_idle, ap_conn_producer_t, link);
>  
>  rv = apr_threadattr_create(>thread_attr, workers->pool);
>  if (rv != APR_SUCCESS) goto cleanup;
> @@ -441,32 +484,35 @@ h2_workers *h2_workers_create(server_rec
>  if (rv != APR_SUCCESS) goto cleanup;
>  rv = apr_thread_cond_create(>all_done, workers->pool);
>  if (rv != APR_SUCCESS) goto cleanup;
> +rv = apr_thread_cond_create(>prod_done, workers->pool);
> +if (rv != APR_SUCCESS) goto cleanup;
>  
> -n = workers->nslots = workers->max_workers;
> -workers->slots = apr_pcalloc(workers->pool, n * sizeof(h2_slot));
> -if (workers->slots == NULL) {
> -n = workers->nslots = 0;
> -rv = APR_ENOMEM;
> -goto cleanup;
> -}
> -for (i = 0; i < n; ++i) {
> +apr_thread_mutex_lock(workers->lock);
> +  

Re: svn commit: r1902005 - in /httpd/httpd/trunk/modules/http2: h2_c1.c h2_config.c h2_config.h h2_mplx.c h2_mplx.h h2_workers.c h2_workers.h mod_http2.c

2022-06-20 Thread Ruediger Pluem



On 6/17/22 11:24 AM, ic...@apache.org wrote:
> Author: icing
> Date: Fri Jun 17 09:24:57 2022
> New Revision: 1902005
> 
> URL: http://svn.apache.org/viewvc?rev=1902005=rev
> Log:
>   *) mod_http2: new implementation of h2 worker pool.
>  - O(1) cost at registration of connection processing producers
>  - no limit on registered producers
>  - join of ongoing work on unregister
>  - callbacks to unlink dependencies into other h2 code
>  - memory cleanup on workers deactivation (on idle timeouts)
>  - idle_limit as apr_time_t instead of seconds
> 
> 
> Modified:
> httpd/httpd/trunk/modules/http2/h2_c1.c
> httpd/httpd/trunk/modules/http2/h2_config.c
> httpd/httpd/trunk/modules/http2/h2_config.h
> httpd/httpd/trunk/modules/http2/h2_mplx.c
> httpd/httpd/trunk/modules/http2/h2_mplx.h
> httpd/httpd/trunk/modules/http2/h2_workers.c
> httpd/httpd/trunk/modules/http2/h2_workers.h
> httpd/httpd/trunk/modules/http2/mod_http2.c

> Modified: httpd/httpd/trunk/modules/http2/h2_workers.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_workers.c?rev=1902005=1902004=1902005=diff
> ==
> --- httpd/httpd/trunk/modules/http2/h2_workers.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_workers.c Fri Jun 17 09:24:57 2022

>  
> @@ -347,37 +367,47 @@ static apr_status_t workers_pool_cleanup
>  int n, wait_sec = 5;

Should n be initialized to 0 to avoid

‘n’ may be used uninitialized in this function

?

>  
>  ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, workers->s,
> - "h2_workers: cleanup %d workers idling",
> - (int)apr_atomic_read32(>worker_count));
> -workers_abort_idle(workers);
> + "h2_workers: cleanup %d workers (%d idle)",
> + workers->active_slots, workers->idle_slots);
> +apr_thread_mutex_lock(workers->lock);
> +workers->shutdown = 1;
> +workers->aborted = 1;
> +wake_all_idles(workers);
> +apr_thread_mutex_unlock(workers->lock);
>  
>  /* wait for all the workers to become zombies and join them.
>   * this gets called after the mpm shuts down and all connections
>   * have either been handled (graceful) or we are forced exiting
>   * (ungrateful). Either way, we show limited patience. */
> -apr_thread_mutex_lock(workers->lock);
>  end = apr_time_now() + apr_time_from_sec(wait_sec);
> -while ((n = apr_atomic_read32(>worker_count)) > 0
> -   && apr_time_now() < end) {
> +while (apr_time_now() < end) {
> +apr_thread_mutex_lock(workers->lock);
> +if (!(n = workers->active_slots)) {
> +apr_thread_mutex_unlock(workers->lock);
> +break;
> +}
> +wake_all_idles(workers);
>  rv = apr_thread_cond_timedwait(workers->all_done, workers->lock, 
> timeout);
> +apr_thread_mutex_unlock(workers->lock);
> +
>  if (APR_TIMEUP == rv) {
>  ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, workers->s,
> - APLOGNO(10290) "h2_workers: waiting for idle 
> workers to close, "
> - "still seeing %d workers living",
> - apr_atomic_read32(>worker_count));
> -continue;
> + APLOGNO(10290) "h2_workers: waiting for workers to 
> close, "
> + "still seeing %d workers (%d idle) living",
> + workers->active_slots, workers->idle_slots);
>  }
>  }
>  if (n) {
>  ap_log_error(APLOG_MARK, APLOG_WARNING, 0, workers->s,
> - APLOGNO(10291) "h2_workers: cleanup, %d idle workers "
> + APLOGNO(10291) "h2_workers: cleanup, %d workers (%d 
> idle) "
>   "did not exit after %d seconds.",
> - n, wait_sec);
> + n, workers->idle_slots, wait_sec);
>  }
> -apr_thread_mutex_unlock(workers->lock);
>  ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, workers->s,
>   "h2_workers: cleanup all workers terminated");
> +apr_thread_mutex_lock(workers->lock);
>  join_zombies(workers);
> +apr_thread_mutex_unlock(workers->lock);
>  ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, workers->s,
>   "h2_workers: cleanup zombie workers joined");
>  


Regards

Rüdiger


Re: svn commit: r1902005 - in /httpd/httpd/trunk/modules/http2: h2_c1.c h2_config.c h2_config.h h2_mplx.c h2_mplx.h h2_workers.c h2_workers.h mod_http2.c

2022-06-20 Thread Ruediger Pluem



On 6/20/22 8:53 AM, Ruediger Pluem wrote:
> 
> 
> On 6/17/22 11:24 AM, ic...@apache.org wrote:
>> Author: icing
>> Date: Fri Jun 17 09:24:57 2022
>> New Revision: 1902005
>>
>> URL: http://svn.apache.org/viewvc?rev=1902005=rev
>> Log:
>>   *) mod_http2: new implementation of h2 worker pool.
>>  - O(1) cost at registration of connection processing producers
>>  - no limit on registered producers
>>  - join of ongoing work on unregister
>>  - callbacks to unlink dependencies into other h2 code
>>  - memory cleanup on workers deactivation (on idle timeouts)
>>  - idle_limit as apr_time_t instead of seconds
>>
>>
>> Modified:
>> httpd/httpd/trunk/modules/http2/h2_c1.c
>> httpd/httpd/trunk/modules/http2/h2_config.c
>> httpd/httpd/trunk/modules/http2/h2_config.h
>> httpd/httpd/trunk/modules/http2/h2_mplx.c
>> httpd/httpd/trunk/modules/http2/h2_mplx.h
>> httpd/httpd/trunk/modules/http2/h2_workers.c
>> httpd/httpd/trunk/modules/http2/h2_workers.h
>> httpd/httpd/trunk/modules/http2/mod_http2.c
>>
>> Modified: httpd/httpd/trunk/modules/http2/h2_c1.c
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_c1.c?rev=1902005=1902004=1902005=diff
>> ==
>> --- httpd/httpd/trunk/modules/http2/h2_c1.c (original)
>> +++ httpd/httpd/trunk/modules/http2/h2_c1.c Fri Jun 17 09:24:57 2022
>> @@ -56,11 +56,8 @@ apr_status_t h2_c1_child_init(apr_pool_t
>>  {
>>  apr_status_t status = APR_SUCCESS;
>>  int minw, maxw;
>> -int max_threads_per_child = 0;
>> -int idle_secs = 0;
>> +apr_time_t idle_limit;
>>  
>> -ap_mpm_query(AP_MPMQ_MAX_THREADS, _threads_per_child);
>> -
>>  status = ap_mpm_query(AP_MPMQ_IS_ASYNC, _mpm);
>>  if (status != APR_SUCCESS) {
>>  /* some MPMs do not implemnent this */
>> @@ -70,12 +67,8 @@ apr_status_t h2_c1_child_init(apr_pool_t
>>  
>>  h2_config_init(pool);
>>  
>> -h2_get_num_workers(s, , );
>> -idle_secs = h2_config_sgeti(s, H2_CONF_MAX_WORKER_IDLE_SECS);
>> -ap_log_error(APLOG_MARK, APLOG_TRACE3, 0, s,
>> - "h2_workers: min=%d max=%d, mthrpchild=%d, idle_secs=%d", 
>> - minw, maxw, max_threads_per_child, idle_secs);
>> -workers = h2_workers_create(s, pool, minw, maxw, idle_secs);
>> +h2_get_workers_config(s, , , _limit);
>> +workers = h2_workers_create(s, pool, maxw, minw, idle_limit);
> 
> Shouldn't that be
> 
> workers = h2_workers_create(s, pool, minw, maxw, idle_limit);
> 
> instead?

Scratch that. I just noticed that the order of the parameters in 
h2_workers_create was changed as well.

Regards

Rüdiger


Re: svn commit: r1902005 - in /httpd/httpd/trunk/modules/http2: h2_c1.c h2_config.c h2_config.h h2_mplx.c h2_mplx.h h2_workers.c h2_workers.h mod_http2.c

2022-06-20 Thread Ruediger Pluem



On 6/17/22 11:24 AM, ic...@apache.org wrote:
> Author: icing
> Date: Fri Jun 17 09:24:57 2022
> New Revision: 1902005
> 
> URL: http://svn.apache.org/viewvc?rev=1902005=rev
> Log:
>   *) mod_http2: new implementation of h2 worker pool.
>  - O(1) cost at registration of connection processing producers
>  - no limit on registered producers
>  - join of ongoing work on unregister
>  - callbacks to unlink dependencies into other h2 code
>  - memory cleanup on workers deactivation (on idle timeouts)
>  - idle_limit as apr_time_t instead of seconds
> 
> 
> Modified:
> httpd/httpd/trunk/modules/http2/h2_c1.c
> httpd/httpd/trunk/modules/http2/h2_config.c
> httpd/httpd/trunk/modules/http2/h2_config.h
> httpd/httpd/trunk/modules/http2/h2_mplx.c
> httpd/httpd/trunk/modules/http2/h2_mplx.h
> httpd/httpd/trunk/modules/http2/h2_workers.c
> httpd/httpd/trunk/modules/http2/h2_workers.h
> httpd/httpd/trunk/modules/http2/mod_http2.c
> 
> Modified: httpd/httpd/trunk/modules/http2/h2_c1.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_c1.c?rev=1902005=1902004=1902005=diff
> ==
> --- httpd/httpd/trunk/modules/http2/h2_c1.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_c1.c Fri Jun 17 09:24:57 2022
> @@ -56,11 +56,8 @@ apr_status_t h2_c1_child_init(apr_pool_t
>  {
>  apr_status_t status = APR_SUCCESS;
>  int minw, maxw;
> -int max_threads_per_child = 0;
> -int idle_secs = 0;
> +apr_time_t idle_limit;
>  
> -ap_mpm_query(AP_MPMQ_MAX_THREADS, _threads_per_child);
> -
>  status = ap_mpm_query(AP_MPMQ_IS_ASYNC, _mpm);
>  if (status != APR_SUCCESS) {
>  /* some MPMs do not implemnent this */
> @@ -70,12 +67,8 @@ apr_status_t h2_c1_child_init(apr_pool_t
>  
>  h2_config_init(pool);
>  
> -h2_get_num_workers(s, , );
> -idle_secs = h2_config_sgeti(s, H2_CONF_MAX_WORKER_IDLE_SECS);
> -ap_log_error(APLOG_MARK, APLOG_TRACE3, 0, s,
> - "h2_workers: min=%d max=%d, mthrpchild=%d, idle_secs=%d", 
> - minw, maxw, max_threads_per_child, idle_secs);
> -workers = h2_workers_create(s, pool, minw, maxw, idle_secs);
> +h2_get_workers_config(s, , , _limit);
> +workers = h2_workers_create(s, pool, maxw, minw, idle_limit);

Shouldn't that be

workers = h2_workers_create(s, pool, minw, maxw, idle_limit);

instead?

>   
>  h2_c_logio_add_bytes_in = 
> APR_RETRIEVE_OPTIONAL_FN(ap_logio_add_bytes_in);
>  h2_c_logio_add_bytes_out = 
> APR_RETRIEVE_OPTIONAL_FN(ap_logio_add_bytes_out);
> 

Regards

Rüdiger



Re: [httpd-site] branch main updated: add inline details

2022-06-10 Thread Ruediger Pluem



On 6/10/22 3:51 PM, cove...@apache.org wrote:
> This is an automated email from the ASF dual-hosted git repository.
> 
> covener pushed a commit to branch main
> in repository https://gitbox.apache.org/repos/asf/httpd-site.git
> 
> 
> The following commit(s) were added to refs/heads/main by this push:
>  new de34c89  add inline details
> de34c89 is described below
> 
> commit de34c893c06b0a65a23bc4684a5eaf2f85c29881
> Author: Eric Covener 
> AuthorDate: Fri Jun 10 09:51:42 2022 -0400
> 
> add inline details
> ---
>  content/security/json/CVE-2022-28614.json | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/content/security/json/CVE-2022-28614.json 
> b/content/security/json/CVE-2022-28614.json
> index 911b151..c47c580 100644
> --- a/content/security/json/CVE-2022-28614.json
> +++ b/content/security/json/CVE-2022-28614.json

>  ]
> @@ -108,15 +108,15 @@
>  "CVE_list": [],
>  "internal_comments": "",
>  "todo": [],
> -"emailed": "",
> -"userslist": "",
> +"emailed": "yes",
> +"userslist": "dev@httpd.apache.org",
>  "email": ""
>},
>"timeline": [
>  {
> -  "lang": "eng",
>"time": "2022-06-08",
> -  "value": "2.4.54 released"
> +  "lang": "eng",
> +  "value": "released in 2.4.54"

This killed it. Version needs to be at the beginning of the string.
See 
https://github.com/apache/httpd-site/blob/f3bc5a7395d1de48daf32a15de7922b91a4a0a05/content/security/cvejsontohtml.py#L36

Regards

Rüdiger

>  }
>]
>  }
> 
> 


Re: [httpd-site] branch main updated: add inline details

2022-06-10 Thread Ruediger Pluem



On 6/10/22 3:51 PM, cove...@apache.org wrote:
> This is an automated email from the ASF dual-hosted git repository.
> 
> covener pushed a commit to branch main
> in repository https://gitbox.apache.org/repos/asf/httpd-site.git
> 
> 
> The following commit(s) were added to refs/heads/main by this push:
>  new de34c89  add inline details
> de34c89 is described below
> 
> commit de34c893c06b0a65a23bc4684a5eaf2f85c29881
> Author: Eric Covener 
> AuthorDate: Fri Jun 10 09:51:42 2022 -0400
> 
> add inline details

Hm, now CVE-2022-28614.json disappeared from 
https://httpd.apache.org/security/vulnerabilities_24.html.

Regards

Rüdiger



Re: svn commit: r1901500 - in /httpd/httpd/trunk: include/http_protocol.h server/protocol.c

2022-06-09 Thread Ruediger Pluem



On 6/8/22 5:43 PM, Ivan Zhakov wrote:
> Yes, I see now. But it will be an incorrect value in case of a string
> larger than INT_MAX. Not a big issue, but IMHO strings larger than
> INT_MAX also are not big issue.

You are correct that the value will be incorrect in case of a string larger 
than INT_MAX, but as ap_rputs can only return an int
and as we cannot change this without breaking the API/ABI it looked like the 
best option to return just the value of the last call
to ap_rwrite.

One thing that comes to my mind now: As ap_rputs is static APR_INLINE in 
include/http_protocol.h I guess that third party modules
that use it need to recompile to pick this up, correct?

Regards

Rüdiger



Re: svn commit: r1901554 - /httpd/test/framework/trunk/t/modules/sed.t

2022-06-02 Thread Ruediger Pluem



On 6/2/22 10:15 PM, Ruediger Pluem wrote:
> 
> 
> On 6/2/22 6:54 PM, Yann Ylavic wrote:
>> On Thu, Jun 2, 2022 at 1:04 PM  wrote:
>>>
>>> Author: rpluem
>>> Date: Thu Jun  2 11:04:13 2022
>>> New Revision: 1901554
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1901554=rev
>>> Log:
>>> * Test returns 200, but content is !!!ERROR!!!
>>
>> On my machine it returns 400 (hitting the mem limit) with html content..
> 
> This is quite strange. On mine it does not and this is also what reading the 
> code of echo_post_handler
> in c-modules/echo_post/mod_echo_post.c tells me as ap_get_client_block fails.

Error messages:

[Thu Jun 02 22:00:57.445702 2022] [sed:error] [pid 253311:tid 139929220736768] 
(12)Cannot allocate memory: [client
127.0.0.1:47366] AH10395: error evaluating sed on input
[Thu Jun 02 22:00:57.445714 2022] [:debug] [pid 253311:tid 139929220736768] 
mod_echo_post.c(85): [client 127.0.0.1:47366]
[mod_echo_post] ap_get_client_block got error
[Thu Jun 02 22:00:57.445715 2022] [:debug] [pid 253311:tid 139929220736768] 
mod_echo_post.c(96): [client 127.0.0.1:47366]
[mod_echo_post] done reading 0 bytes, 12582912 bytes remain


Regards

Rüdiger


Re: svn commit: r1901554 - /httpd/test/framework/trunk/t/modules/sed.t

2022-06-02 Thread Ruediger Pluem



On 6/2/22 6:54 PM, Yann Ylavic wrote:
> On Thu, Jun 2, 2022 at 1:04 PM  wrote:
>>
>> Author: rpluem
>> Date: Thu Jun  2 11:04:13 2022
>> New Revision: 1901554
>>
>> URL: http://svn.apache.org/viewvc?rev=1901554=rev
>> Log:
>> * Test returns 200, but content is !!!ERROR!!!
> 
> On my machine it returns 400 (hitting the mem limit) with html content..

This is quite strange. On mine it does not and this is also what reading the 
code of echo_post_handler
in c-modules/echo_post/mod_echo_post.c tells me as ap_get_client_block fails.

> 
> Also I can't the "plan" line to work, the 'need
> "LWP::Protocol::AnyEvent::http"' raises the same error as the
> (currently failing) ci:
>   Test::plan(): skipping unrecognized directive '1' at
> /home/yle/src/apache/asf/httpd/trunk.head/framework/Apache-Test/lib/Apache/Test.pm
> line 298.
> I tried with and without the "need" keyword or with "need_lwp,
> qw(LWP::Protocol::AnyEvent::http)" like it's done elsewhere, to no
> avail.
> Sorry my Apache::Test foo is quite limited..

Hopefully r1901566 fixed this.

Regards

Rüdiger



Re: svn commit: r1901485 - /httpd/httpd/trunk/modules/proxy/proxy_util.c

2022-06-01 Thread Ruediger Pluem



On 6/1/22 12:44 PM, Yann Ylavic wrote:
> On Wed, Jun 1, 2022 at 12:39 PM Ruediger Pluem  wrote:
>>
>> On 6/1/22 11:56 AM, yla...@apache.org wrote:
>>>
>>> +/* Compute Host header */
>>>  if (dconf->preserve_host == 0) {
>>>  if (ap_strchr_c(uri->hostname, ':')) { /* if literal IPv6 address 
>>> */
>>>  if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
>>> @@ -3994,10 +3992,11 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
>>>  host = uri->hostname;
>>>  }
>>>  }
>>> +apr_table_setn(r->headers_in, "Host", host);
>>>  }
>>>  else {
>>> -/* don't want to use r->hostname, as the incoming header might 
>>> have a
>>> - * port attached
>>> +/* don't want to use r->hostname as the incoming header might have 
>>> a
>>> + * port attached, let's use the original header.
>>>   */
>>>  host = saved_host;
>>>  if (!host) {
>>> @@ -4007,10 +4006,9 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
>>>"on incoming request and preserve host set "
>>>"forcing hostname to be %s for uri %s",
>>>host, r->uri);
>>> +apr_table_setn(r->headers_in, "Host", host);
>>>  }
>>>  }
>>
>> Nitpick: Can't we do the apr_table_setn(r->headers_in, "Host", host); here 
>> instead of in each if/else branch?
> 
> This is a small optimization where if we reuse the existing Host
> header (saved_host) we don't need to set it again.
> But if it harms readability I can certainly change it as you say.

No worries. Leave it as is then. I vote for the performance benefit over the 
readability benefit in this case :-)

Regards

Rüdiger



Re: svn commit: r1901485 - /httpd/httpd/trunk/modules/proxy/proxy_util.c

2022-06-01 Thread Ruediger Pluem



On 6/1/22 11:56 AM, yla...@apache.org wrote:
> Author: ylavic
> Date: Wed Jun  1 09:56:43 2022
> New Revision: 1901485
> 
> URL: http://svn.apache.org/viewvc?rev=1901485=rev
> Log:
> mod_proxy: Let fixup hooks know about the Host header (and eventually 
> overwrite it).
> 
> If proxy_run_fixups() sets a Host header there will be two ones sent to the
> origin server.
> 
> Instead, let the hooks know about the Host by setting it in the r->headers_in
> passed to proxy_run_fixups(), and use the actual value afterwards.
> Note: if proxy_run_fixups() unsets the Host we'll keep ours.
> 
> Suggested by: rpluem
> 
> 
> Modified:
> httpd/httpd/trunk/modules/proxy/proxy_util.c
> 
> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1901485=1901484=1901485=diff
> ==
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Wed Jun  1 09:56:43 2022

> @@ -3975,9 +3975,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
>  if (force10)
>  apr_table_unset(r->headers_in, "Trailer");
>  
> -/* We used to send `Host: ` always first, so let's keep it that
> - * way. No telling which legacy backend is relying no this.
> - */
> +/* Compute Host header */
>  if (dconf->preserve_host == 0) {
>  if (ap_strchr_c(uri->hostname, ':')) { /* if literal IPv6 address */
>  if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
> @@ -3994,10 +3992,11 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
>  host = uri->hostname;
>  }
>  }
> +apr_table_setn(r->headers_in, "Host", host);
>  }
>  else {
> -/* don't want to use r->hostname, as the incoming header might have a
> - * port attached
> +/* don't want to use r->hostname as the incoming header might have a
> + * port attached, let's use the original header.
>   */
>  host = saved_host;
>  if (!host) {
> @@ -4007,10 +4006,9 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
>"on incoming request and preserve host set "
>"forcing hostname to be %s for uri %s",
>host, r->uri);
> +apr_table_setn(r->headers_in, "Host", host);
>  }
>  }

Nitpick: Can't we do the apr_table_setn(r->headers_in, "Host", host); here 
instead of in each if/else branch?

> -ap_h1_append_header(header_brigade, r->pool, "Host", host);
> -apr_table_unset(r->headers_in, "Host");
>  
>  /* handle Via */
>  if (conf->viaopt == via_block) {


Regards

Rüdiger



Re: svn commit: r1901460 - /httpd/httpd/trunk/modules/proxy/proxy_util.c

2022-06-01 Thread Ruediger Pluem



On 6/1/22 11:10 AM, Yann Ylavic wrote:
> On Wed, Jun 1, 2022 at 8:29 AM Ruediger Pluem  wrote:
>>
>> On 5/31/22 5:06 PM, yla...@apache.org wrote:

>>>  }
>>> +ap_h1_append_header(header_brigade, r->pool, "Host", host);
>>> +apr_table_unset(r->headers_in, "Host");
>>
>> This is nothing introduced by this change but something I noticed. If the 
>> fixup hook adds back a Host header,
>> we would sent two Host headers to the backend.
> 
> I thought about it, and also that proxy_run_fixups doesn't know about
> the forwarded Host header.
> I hesitated between simply ignoring any Host set by proxy_run_fixups
> (i.e. move the apr_table_unset("Host") after that) or take it into
> account.
> The latter would be something like this:

+1. This is more safe.

Regards

Rüdiger



Re: svn commit: r1901460 - /httpd/httpd/trunk/modules/proxy/proxy_util.c

2022-06-01 Thread Ruediger Pluem



On 5/31/22 5:06 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Tue May 31 15:06:13 2022
> New Revision: 1901460
> 
> URL: http://svn.apache.org/viewvc?rev=1901460=rev
> Log:
> mod_proxy: Align ap_proxy_create_hdrbrgd() with 2.4.x's.
> 
> In 2.4.x, the copy of r->headers_in is left in r->headers_in for the whole
> function, while the original r->headers_in are restored at the end. This
> is simpler and avoids the r->headers_in <=> saved_headers_in danse when
> calling a function that modifies r->headers_in in place.
> 
> Align with 2.4.x, no functional change.
> 
> 
> Modified:
> httpd/httpd/trunk/modules/proxy/proxy_util.c
> 
> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1901460=1901459=1901460=diff
> ==
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Tue May 31 15:06:13 2022

> @@ -3939,52 +3942,49 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
>   * icing: if p indeed lives longer than r->pool, we should allocate
>   * all new header values from r->pool as well and avoid leakage.
>   */
> -request_headers = apr_table_copy(r->pool, r->headers_in);
> +r->headers_in = apr_table_copy(r->pool, saved_headers_in);
>  
>  /* We used to send `Host: ` always first, so let's keep it that
>   * way. No telling which legacy backend is relying no this.
>   */
>  if (dconf->preserve_host == 0) {
> -const char *nhost;
>  if (ap_strchr_c(uri->hostname, ':')) { /* if literal IPv6 address */
>  if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
> -nhost = apr_pstrcat(r->pool, "[", uri->hostname, "]:",
> -uri->port_str, NULL);
> +host = apr_pstrcat(r->pool, "[", uri->hostname, "]:",
> +   uri->port_str, NULL);
>  } else {
> -nhost = apr_pstrcat(r->pool, "[", uri->hostname, "]", NULL);
> +host = apr_pstrcat(r->pool, "[", uri->hostname, "]", NULL);
>  }
>  } else {
>  if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
> -nhost = apr_pstrcat(r->pool, uri->hostname, ":",
> -uri->port_str, NULL);
> +host = apr_pstrcat(r->pool, uri->hostname, ":",
> +   uri->port_str, NULL);
>  } else {
> -nhost = uri->hostname;
> +host = uri->hostname;
>  }
>  }
> -ap_h1_append_header(header_brigade, r->pool, "Host", nhost);
> -apr_table_unset(request_headers, "Host");
>  }
>  else {
>  /* don't want to use r->hostname, as the incoming header might have a
>   * port attached
>   */
> -const char* hostname = apr_table_get(request_headers, "Host");
> -if (!hostname) {
> -hostname =  r->server->server_hostname;
> +host = saved_host;
> +if (!host) {
> +host =  r->server->server_hostname;
>  ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01092)
>"no HTTP 0.9 request (with no host line) "
>"on incoming request and preserve host set "
>"forcing hostname to be %s for uri %s",
> -  hostname, r->uri);
> +  host, r->uri);
>  }
> -ap_h1_append_header(header_brigade, r->pool, "Host", hostname);
> -apr_table_unset(request_headers, "Host");
>  }
> +ap_h1_append_header(header_brigade, r->pool, "Host", host);
> +apr_table_unset(r->headers_in, "Host");

This is nothing introduced by this change but something I noticed. If the fixup 
hook adds back a Host header,
we would sent two Host headers to the backend.

Regards

Rüdiger



Re: tcp send buffering and keepalive races

2022-05-30 Thread Ruediger Pluem



On 5/30/22 2:35 PM, Frank Gingras wrote:
> Can someone remove Nam Ho from the ML please? The spamming has been going on 
> for weeks now.
> 

Removed

Regards

Rüdiger


Re: tcp send buffering and keepalive races

2022-05-30 Thread Ruediger Pluem



On 5/27/22 7:33 PM, Eric Covener wrote:
> People might recall an event bug where keepalive connections might be
> closed up to 200ms early (r1874350).
> 
> I was recently looking at something with $bigco hat on where (IIUC) a
> slow TTFB for a proxied request causes TCP congestion to kick in and
> makes even a relatively short response sit in the write buffer.
> 
>>From the behavior, it appears the browser is:
> 
> 1) willing to use nearly every millisecond of the advertised KeepAlive
> time for reusing a connection from its pool
> 2) starts counting from when the response is complete
> 3) can't be asked to use Expect: 100-continue on an XHR POST
> 4) leaves error handling up to the caller and doesn't give it a ton of 
> feedback
> 
> This results in Apache starting the keepalive countdown "tens" of
> milliseconds early while the last bytes of the response are in the
> queue. If we get unlucky, a POST and a FIN cross in the night on a
> subsequent request.
> 
> These types of investigations can be really painful.  Is there any
> harm in allowing the server to act like "KeepAliveTimeout 5" is e.g.
> "KeepAliveTimeout 5200ms".
> 
> If this fudge buffer existed as an addl directive (rather than a trick
> documented in KeepAliveTimeout) , would it be reasonable as a non-zero
> default to discourage this race?
> 

In the end you want to get to a KeepAlive we announce to the client and
a KeepAlive which is longer than that that we execute.
My understanding of keepalive is that the client cannot take for granted
that a connection is really kept alive for as long as it was announced by
the server (it SHOULD be but there seems no MUST) and in fact we close keepalive
connections if get too busy and keeping these would prevent us from accepting
new connections.
Hence I think the issue will not be fixed in all situations.
I am willing to have this possibility, I guess best by adding an additional
amount of grace to the KeepAliveTimeout configurable by a directive, but I think
it should be zero by default to avoid confusion unless the behavior you report 
above
is widespread.

Regards

Rüdiger


Re: release anyone?

2022-05-25 Thread Ruediger Pluem



On 5/25/22 2:15 PM, Stefan Eissing wrote:
> Anyone feeling release vibes in the air? 
> 
> it's been a good 2.5 months and some things have accumulated. 
> Maybe the start of June would be a good target?

+1

Regards

Rüdiger



Re: strcasecmp raises its...

2022-05-20 Thread Ruediger Pluem



On 5/19/22 7:54 PM, Stefan Eissing wrote:
> 
> 
>> Am 19.05.2022 um 17:20 schrieb Ruediger Pluem :
>>
>>

>>>
>>> +1 from me for replacing our protocol+config handling code with the 
>>> ap_cstr_casecmp().
>>
>> +1. Just to mention: Christophe already did quite some work in this area.
> 
> Thanks, Christophe.
> 
> For my understanding: the code in APR for tables uses strcasecmp() and I am 
> probably just too stupid to see where this is redefined?
> 

>From my point of view apr_tables use strcasecmp and this is not redefined. I 
>guess one of the reasons is that the apr_tables stuff
has been there forever in APR and the apr_cstr stuff only came into APR 2016 
and the code of apr_tables was not adjusted after
this. The other reason at least for 1.x might be concerns that switching to 
apr_cstr might change the behavior of apr_tables in a
way that is incompatible with the versioning rules of APR.
But this is a discussion for dev@apr.

Regards

Rüdiger


Re: strcasecmp raises its...

2022-05-19 Thread Ruediger Pluem



On 5/19/22 5:15 PM, Stefan Eissing wrote:
> 
> 
>> Am 19.05.2022 um 16:44 schrieb Joe Orton :
>>
>> On Wed, May 18, 2022 at 05:34:22PM +0200, Ruediger Pluem wrote:
>>> On 5/18/22 4:55 PM, Joe Orton wrote:
>>>> I think for httpd it is only safe and sane to run httpd with LANG=C, we 
>>>> do this in the default service scripts in Fedora/RHEL for a very long 
>>>> time. Other than the protocol parsing issues you can get in non-C 
>>>> locales, you can also get "surprises" when sort order can change with 
>>>> the system locale, impacting e.g. config file load ordering and more.
>>>
>>> Don't you need a locale sensitive case insensitive string comparison in 
>>> case of case blind file systems which support extended
>>> latin characters? I know these Germans with their Umlaute :-).
>>
>> Heh. Well, I got away with it so far :)
>>
>>>> So IMHO it is probably sufficient & simpler to adjust apachectl to set 
>>>> LANG=C rather than trying to eliminate strcasecmp, and add another 
>>>> strcasecmp() reimplementation in APR, in this case.
>>>
>>> We already have this implementation in APR and we use the
>>> httpd one which is just a forward port from APR to httpd until we require a 
>>> sufficient recent APR version in several places.
>>> The question is just if we should use them everywhere and thus do the 
>>> correct thing no matter what locale is set.
>>
>> Ah, I missed that, thanks.
>>
>> +1 from me on doing replacement of strcasecmp() with the 
>> locale-insensitive versions then. At least with config options, protocol 
>> data, it is definitely right.
>>
> 
> +1 from me for replacing our protocol+config handling code with the 
> ap_cstr_casecmp().

+1. Just to mention: Christophe already did quite some work in this area.

Regards

Rüdiger



Re: strcasecmp raises its...

2022-05-19 Thread Ruediger Pluem



On 5/18/22 7:17 PM, Nick Kew wrote:
> 
>> On 18 May 2022, at 16:34, Ruediger Pluem  wrote:
>>
>> Rüdiger
> 
> What locale are YOU in there?  Any attempt at locale is going to have to draw 
> lines:

de_DE.UTF-8

> what are the rules for when Ruediger == Rüdiger?

There can be none. Because I can transcribe ü to ue , but not every ue is an ü 
the other way around.

Regards

Rüdiger



Re: strcasecmp raises its...

2022-05-18 Thread Ruediger Pluem



On 5/18/22 4:55 PM, Joe Orton wrote:
> On Wed, May 18, 2022 at 12:53:57PM +0200, Ruediger Pluem wrote:
>>
>>
>> On 5/18/22 12:19 PM, Stefan Eissing wrote:
>>> 2022 and we discuss strcasecmp() again?
>>>
>>> Background: OpenSSL 3.0.3 added OPENSSL_strcasecmp() and friends and there 
>>> are several issue around their implementation. Up to this version, they 
>>> relied on the POSIX strcasecmp(). Whatever their reasons for their change...
>>>
>>> Checking our sources, we have ap_cstr_casecmp() that does the right thing. 
>>> But 
>>> - we do not use it everywhere
>>> - it is not part of APR which relies on the POSIX strcasecmp(), esp. 
>>> apr_table does.
>>
>> It is, but it may not be used where it possibly should:
>>
>> https://apr.apache.org/docs/apr/1.7/group__apr__cstr.html
>>
>>>
>>> I want to handshake with you regarding this:
>>> 1. should we scan our sources for strcasecmp and replace it with 
>>> ap_cstr_casecmp()?
>>
>> If I remember correctly ap_cstr_casecmp was only designed to be used for 
>> comparisons of HTTP protocol strings as it is locale
>> agnostic. Hence I am not sure if it is correct to use it everywhere. From 
>> the documentation:
>>
>> **
>>  * Perform a case-insensitive comparison of two strings @a str1 and @a str2,
>>  * treating upper and lower case values of the 26 standard C/POSIX alphabetic
>>  * characters as equivalent. Extended latin characters outside of this set
>>  * are treated as unique octets, irrespective of the current locale.
>>
>> Hence it might be wrong to use it in cases where you need to respect the 
>> locale.
> 
> Are there really any cases like that in httpd?
> 
> I think for httpd it is only safe and sane to run httpd with LANG=C, we 
> do this in the default service scripts in Fedora/RHEL for a very long 
> time. Other than the protocol parsing issues you can get in non-C 
> locales, you can also get "surprises" when sort order can change with 
> the system locale, impacting e.g. config file load ordering and more.

Don't you need a locale sensitive case insensitive string comparison in case of 
case blind file systems which support extended
latin characters? I know these Germans with their Umlaute :-).

> 
> So IMHO it is probably sufficient & simpler to adjust apachectl to set 
> LANG=C rather than trying to eliminate strcasecmp, and add another 
> strcasecmp() reimplementation in APR, in this case.

We already have this implementation in APR and we use the
httpd one which is just a forward port from APR to httpd until we require a 
sufficient recent APR version in several places.
The question is just if we should use them everywhere and thus do the correct 
thing no matter what locale is set.

Regards

Rüdiger


Re: CVE-2022-1388

2022-05-18 Thread Ruediger Pluem



On 5/18/22 2:31 PM, Eric Covener wrote:
>> Given the above, I believe the interpretation of X-F5-Auth-Token should
>> be that it is an end-to-end header, and should therefore NOT be removed
>> from the proxied request.
>>
>> The text does say "All other headers *defined by HTTP/1.1* are
>> end-to-end headers" (emphasis mine, of course), and the X-F5-Auth-Token
>> header isn't defined by HTTP/1.1 (it's a custom one), but I think the
>> definition of specific hop-by-hop headers implies that *all* other
>> headers should be considered end-to-end.
> 
> I don't think that interpretation can be squared with e.g.
> 
> https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.1
> https://datatracker.ietf.org/doc/html/rfc7230#section-6.1
> 

I get to the same conclusion. The current proxy code does it correctly.

Regards

Rüdiger


Re: strcasecmp raises its...

2022-05-18 Thread Ruediger Pluem



On 5/18/22 12:19 PM, Stefan Eissing wrote:
> 2022 and we discuss strcasecmp() again?
> 
> Background: OpenSSL 3.0.3 added OPENSSL_strcasecmp() and friends and there 
> are several issue around their implementation. Up to this version, they 
> relied on the POSIX strcasecmp(). Whatever their reasons for their change...
> 
> Checking our sources, we have ap_cstr_casecmp() that does the right thing. 
> But 
> - we do not use it everywhere
> - it is not part of APR which relies on the POSIX strcasecmp(), esp. 
> apr_table does.

It is, but it may not be used where it possibly should:

https://apr.apache.org/docs/apr/1.7/group__apr__cstr.html

> 
> I want to handshake with you regarding this:
> 1. should we scan our sources for strcasecmp and replace it with 
> ap_cstr_casecmp()?

If I remember correctly ap_cstr_casecmp was only designed to be used for 
comparisons of HTTP protocol strings as it is locale
agnostic. Hence I am not sure if it is correct to use it everywhere. From the 
documentation:

**
 * Perform a case-insensitive comparison of two strings @a str1 and @a str2,
 * treating upper and lower case values of the 26 standard C/POSIX alphabetic
 * characters as equivalent. Extended latin characters outside of this set
 * are treated as unique octets, irrespective of the current locale.

Hence it might be wrong to use it in cases where you need to respect the locale.


Regards

Rüdiger



Re: svn commit: r1900805 - /httpd/httpd/branches/2.4.x/test/travis_run_linux.sh

2022-05-13 Thread Ruediger Pluem



On 5/11/22 10:20 AM, ic...@apache.org wrote:
> Author: icing
> Date: Wed May 11 08:20:43 2022
> New Revision: 1900805
> 
> URL: http://svn.apache.org/viewvc?rev=1900805=rev
> Log:
>   *) test: log rustc and cbindget versions used to build rustls-ffi
>  to speed up analysis of any problems in the future.
> 
> 

Doesn't this need to get forward ported to trunk as well?

Regards

Rüdiger


Re: Error while configuring/compiling Apache 2.4.53

2022-04-25 Thread Ruediger Pluem
See https://bz.apache.org/bugzilla/show_bug.cgi?id=66000

Regards

Rüdiger

On 4/25/22 11:16 AM, Ganeshprasad MS wrote:
> CONFIDENTIAL & RESTRICTED
> 
> 
> Hi Team,
> 
>  
> 
> (I am trying to contact Apache HTTPD community from last few days but no 
> response, kindly respond to my below query ☹)
> 
>  
> 
> We have downloaded the Apache HTTPD source httpd-2.4.53.tar.gz 
>  from Apache
> software foundation and trying to compile/build the Apache binaries, but we 
> are facing the below error while configuring the
> Apache HTTPD.
> 
>  
> 
> FYI: I am not facing the issue while configuring *2.4.52 and below* versions.
> 
>  
> 
> Kindly help me in identifying/resolving the issue asap.
> 
>  
> 
>  
> 
> *Error in ./configure step:*
> 
>  
> 
> 
> 
> Configuring HTTPD build
> 
> 
> 
> Options: --prefix 
> /remote/users/gms/2.4.53/apache-pack/build/PKL-AWS-2.4.53.0/Release 
> --enable-proxy --enable-proxy-ajp
> --enable-proxy-balancer --enable-proxy-http --enable-rewrite --enable-info 
> --enable-headers --enable-expires --enable-deflate
> --enable-ssl 
> --with-ssl=/remote/users/gms/2.4.53/apache-pack/build/PKL-AWS-2.4.53.0/Release
> --with-pcre=/remote/users/gms/2.4.53/apache-pack/build/pcre 
> --with-apr=/remote/users/gms/2.4.53/apache-pack/build/apr
> --with-apr-util=/remote/users/gms/2.4.53/apache-pack/build/apr-util 
> --with-crypto
> 
> CPPFLAGS:
> 
> checking for chosen layout... Apache
> 
> checking for working mkdir -p... yes
> 
> checking for grep that handles long lines and -e... /usr/bin/grep
> 
> checking for egrep... /usr/bin/grep -E
> 
> checking build system type... x86_64-pc-linux-gnu
> 
> checking host system type... x86_64-pc-linux-gnu
> 
> checking target system type... x86_64-pc-linux-gnu
> 
> configure:
> 
> configure: Configuring Apache Portable Runtime library...
> 
> configure:
> 
> checking for APR... yes
> 
>   setting CC to "gcc"
> 
>   setting CPP to "gcc -E"
> 
>   setting CFLAGS to " -g -O2 -pthread"
> 
>   setting CPPFLAGS to " -DLINUX -D_REENTRANT -D_GNU_SOURCE"
> 
>   setting LDFLAGS to " "
> 
> configure:
> 
> configure: Configuring Apache Portable Runtime Utility library...
> 
> configure:
> 
> checking for APR-util... yes
> 
> checking for gcc... gcc
> 
> checking whether the C compiler works... yes
> 
> checking for C compiler default output file name... a.out
> 
> checking for suffix of executables...
> 
> checking whether we are cross compiling... no
> 
> checking for suffix of object files... o
> 
> checking whether we are using the GNU C compiler... yes
> 
> checking whether gcc accepts -g... yes
> 
> checking for gcc option to accept ISO C89... none needed
> 
> checking how to run the C preprocessor... gcc -E
> 
> checking for gcc option to accept ISO C99... -std=gnu99
> 
> checking for -pcre2-config... no
> 
> checking for -pcre-config... no
> 
> checking for pcre2-config... no
> 
> checking for pcre-config... pcre-config
> 
> configure: error: Did not find working script at pcre-config
> 
> 
> 
> Exited on error
> 
>  
> 
> Regards,
> 
> Ganeshprasad
> 
>  
> 
>  
> 


Re: svn commit: r1899609 - in /httpd/httpd/trunk: changes-entries/core_ap_escape_quotes.txt server/util.c test/unit/util.c

2022-04-15 Thread Ruediger Pluem



On 4/15/22 4:21 PM, Stefan Eissing wrote:
> 
> 
>> Am 15.04.2022 um 15:24 schrieb Yann Ylavic :
>>
>> On Wed, Apr 6, 2022 at 11:17 AM  wrote:
>>>
>>> Modified: httpd/httpd/trunk/server/util.c
>>> URL: 
>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1899609=1899608=1899609=diff
>>> ==
>>> --- httpd/httpd/trunk/server/util.c (original)
>>> +++ httpd/httpd/trunk/server/util.c Wed Apr  6 09:17:42 2022
>>> @@ -2615,7 +2615,7 @@ AP_DECLARE(void) ap_content_type_tolower
>>>  */
>>> AP_DECLARE(char *) ap_escape_quotes(apr_pool_t *p, const char *instring)
>>> {
>>> -int newlen = 0;
>>> +apr_ssize_t extra = 0;
>>
>> Shouldn't it be an apr_size_t?
> 
> Similar comment raised on the PR https://github.com/apache/httpd/pull/298
> 
> Not totally sure. The thing is that C in general has a problem with
> strings where ptrdiff_t (apr_ssize_t) is not sufficient. Allocating something
> larger than ptridff_t leads to undefined behaviour.
> 
> So, maybe we should check that "(inchr - instring) + extra + 1" does not
> wrap around?


I did some tests with gcc 8.5 and it seems to do the "expected" thing if two 
pointers are farther away than PTRDIFF_MAX:
It provides the correct difference. This does not work if the difference would 
be negative (just swapped the pointers for the test).
Having said this the above does not mean that other compilers or other versions 
of gcc behave the same as the specs for C do not
define a behavior in this case. I guess the best thing we can do is:

1. Make extra  apr_size_t
2. Check that inchr - instring remains >= 0 .
3. Store inchr - instring in an apr_size_t.
4. Check that inchr - instring <= SIZE_MAX - extra - 1
5. If 2. and 4. are true calculate (inchr - instring) + extra + 1 otherwise 
abort() as at least 4 means we would be OOM anyway and
   2. does not offer a sane way to us to solve this.

Regards

Rüdiger




Re: svn commit: r1899858 - /httpd/httpd/trunk/server/mpm/event/event.c

2022-04-14 Thread Ruediger Pluem



On 4/14/22 4:38 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Thu Apr 14 14:38:03 2022
> New Revision: 1899858
> 
> URL: http://svn.apache.org/viewvc?rev=1899858=rev
> Log:
> mpm_event: Handle children killed pathologically.
> 
> If children processes get killed (SIGSEGV/SIGABRT/..) early after starting or
> frequently enough then we never enter perform_idle_server_maintenance() to
> try something.
> 
> Below three successive children killed restart them immediately, above three
> let's sleep the usual 1s (to avoid fork()s flood) and do the idle maintenance.
> 
> 
> Modified:
> httpd/httpd/trunk/server/mpm/event/event.c
> 
> Modified: httpd/httpd/trunk/server/mpm/event/event.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1899858=1899857=1899858=diff
> ==
> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> +++ httpd/httpd/trunk/server/mpm/event/event.c Thu Apr 14 14:38:03 2022
> @@ -3382,6 +3382,7 @@ static void server_main_loop(int remaini
>  {
>  int num_buckets = retained->mpm->num_buckets;
>  int max_daemon_used = 0;
> +int successive_signals = 0;
>  int child_slot;
>  apr_exit_why_e exitwhy;
>  int status, processed_status;
> @@ -3460,11 +3461,31 @@ static void server_main_loop(int remaini
>  /* Don't perform idle maintenance when a child dies,
>   * only do it when there's a timeout.  Remember only a
>   * finite number of children can die, and it's pretty
> - * pathological for a lot to die suddenly.
> + * pathological for a lot to die suddenly.  If that happens
> + * anyway, protect against fork()+kill() flood by not restarting
> + * more than 3 children if no timeout happened in between,
> + * otherwise we keep going with idle maintenance.
>   */
> -continue;
> +if (child_slot < 0 || !APR_PROC_CHECK_SIGNALED(exitwhy)) {
> +continue;
> +}
> +if (++successive_signals >= 3) {
> +if (successive_signals % 10 == 3) {
> +ap_log_error(APLOG_MARK, APLOG_WARNING, 0,
> + ap_server_conf, APLOGNO(10392)
> + "children are killed successively!");
> +}
> +apr_sleep(apr_time_from_sec(1));

Why do we need the sleep here? Why can't we just do continue?

> +}
> +else {
> +++remaining_children_to_start;
> +}
> +}
> +else {
> +successive_signals = 0;
>  }
> -else if (remaining_children_to_start) {
> +
> +if (remaining_children_to_start) {
>  /* we hit a 1 second timeout in which none of the previous
>   * generation of children needed to be reaped... so assume
>   * they're all done, and pick up the slack if any is left.
> 
> 
> 

Regards

Rüdiger



Re: Faster start children after fatal signals?

2022-04-14 Thread Ruediger Pluem



On 4/14/22 3:43 PM, Yann Ylavic wrote:
> On Thu, Apr 14, 2022 at 1:44 PM Ruediger Pluem  wrote:
>>
>> On 4/14/22 1:16 PM, Yann Ylavic wrote:
>>> On Thu, Apr 14, 2022 at 12:09 PM Ruediger Pluem  wrote:
>>>>
>>>> On 4/14/22 11:52 AM, Yann Ylavic wrote:
>>>>>
>>>>> Any signal that killed the process would mean something unexpected
>>>>> happened since we clean_child_exit() otherwise?
>>>>
>>>> Hm. You mean that the case for
>>>>
>>>> case SIGTERM:
>>>> case SIGHUP:
>>>> case AP_SIG_GRACEFUL:
>>>>
>>>> in ap_process_child_status
>>>> never triggers as the child catches these and does a clean_child_exit()?
>>>
>>> Yes
>>>
>>>>
>>>> If yes, the above seems to be a simple solution, but it would also capture 
>>>> SIGKILL which might
>>>> have been issued by our parent. Does this matter?
>>>
>>> I think our SIGKILLs (for ungraceful restart) are "captured" by
>>> ap_reclaim_child_processes(), that is outside server_main_loop() so it
>>> should be OK..
>>
>> Good point. Then I am fine with your proposed patch.
> 
> Thinking more about it, I came to something like this:
> 
> Index: server/mpm/event/event.c
> ===
> --- server/mpm/event/event.c(revision 1899818)
> +++ server/mpm/event/event.c(working copy)
> @@ -3382,6 +3382,7 @@ static void server_main_loop(int remaining_childre
>  {
>  int num_buckets = retained->mpm->num_buckets;
>  int max_daemon_used = 0;
> +int successive_children_signaled = 0;
>  int child_slot;
>  apr_exit_why_e exitwhy;
>  int status, processed_status;
> @@ -3460,11 +3461,26 @@ static void server_main_loop(int remaining_childre
>  /* Don't perform idle maintenance when a child dies,
>   * only do it when there's a timeout.  Remember only a
>   * finite number of children can die, and it's pretty
> - * pathological for a lot to die suddenly.
> + * pathological for a lot to die suddenly.  If that happens
> + * anyway, protect against pathological segfault flood by not
> + * restarting more than 3 children if no timeout happened in
> + * between, otherwise we let the usual maintenance go.
>   */
> -continue;
> +if (child_slot < 0 || !APR_PROC_CHECK_SIGNALED(exitwhy)) {
> +continue;
> +}
> +if (++successive_children_signaled >= 3) {
> +apr_sleep(apr_time_from_sec(1));

Why can't we just continue here? We either have something to reap in the next 
iteration which is fine or not where we would sleep
1 second anyway..

> +}
> +else {
> +remaining_children_to_start++;
> +}
>  }
> -else if (remaining_children_to_start) {
> +else {
> +successive_children_signaled = 0;
> +}
> +
> +if (remaining_children_to_start) {
>  /* we hit a 1 second timeout in which none of the previous
>   * generation of children needed to be reaped... so assume
>   * they're all done, and pick up the slack if any is left.
> --
> 
> I have not tested it yet but possibly without this if many children
> segfault successively we might enter a busy fork() loop.
> The above would restart killed children immediately (using the
> remaining_children_to_start mechanism) only if there are less than 3
> successively, otherwise sleep 1s and
> perform_idle_server_maintenance().
> 
> WDYT?

Having a stopper for the fork loop sounds sensible. We may should log something 
if we hit it to ease analysis for situations where
the childs quickly segfault.

Regards

Rüdiger



Re: Faster start children after fatal signals?

2022-04-14 Thread Ruediger Pluem



On 4/14/22 1:16 PM, Yann Ylavic wrote:
> On Thu, Apr 14, 2022 at 12:09 PM Ruediger Pluem  wrote:
>>
>> On 4/14/22 11:52 AM, Yann Ylavic wrote:
>>>
>>> Any signal that killed the process would mean something unexpected
>>> happened since we clean_child_exit() otherwise?
>>
>> Hm. You mean that the case for
>>
>> case SIGTERM:
>> case SIGHUP:
>> case AP_SIG_GRACEFUL:
>>
>> in ap_process_child_status
>> never triggers as the child catches these and does a clean_child_exit()?
> 
> Yes
> 
>>
>> If yes, the above seems to be a simple solution, but it would also capture 
>> SIGKILL which might
>> have been issued by our parent. Does this matter?
> 
> I think our SIGKILLs (for ungraceful restart) are "captured" by
> ap_reclaim_child_processes(), that is outside server_main_loop() so it
> should be OK..

Good point. Then I am fine with your proposed patch.
> 
>>
>> Do we need something for processes that die due to MaxConnectionsPerChild or 
>> do we expect them to die at a slow path where the
>> current approach is fine?
> 
> Hm, good question. Restarting them if not necessary from a maintenance
> metrics perspective is no better/worse than waiting for a maintenance
> cycle, so I'd say let them be maintained as we do now..
> Ideally (maybe) we'd pass a timeout to ap_wait_or_timeout() to make
> sure that maintenance cycles really happen every 1s, regardless of
> exited processes in the meantime (i.e. handle a "remaining" timeout
> instead of the hardcoded 1s which currently may result in
> 0.9s+waitpid()+0.9s+waitpid()+timeout thus here ~2s but possibly more
> between two maintenance cycles).

Hm, but ap_wait_or_timeout only waits if there is nothing to reap, which would 
mean we would wait at max 1 s (+ processing time
for multiple ap_wait_or_timeout calls) until we perform the idle server 
maintenance, or do I miss your point?


Regards

Rüdiger




Re: Faster start children after fatal signals?

2022-04-14 Thread Ruediger Pluem



On 4/14/22 11:52 AM, Yann Ylavic wrote:
> On Thu, Apr 14, 2022 at 11:21 AM Ruediger Pluem  wrote:
>>
>> On 4/13/22 5:41 PM, Yann Ylavic wrote:
>>> On Wed, Apr 13, 2022 at 4:30 PM Ruediger Pluem  wrote:
>>>>
>>>> While looking at PR65769 I stumbled across the below in event.c (same in 
>>>> worker.c)
>>>>
>>>> 3460/* Don't perform idle maintenance when a child dies,
>>>> 3461 * only do it when there's a timeout.  Remember only a
>>>> 3462 * finite number of children can die, and it's pretty
>>>> 3463 * pathological for a lot to die suddenly.
>>>> 3464 */
>>>> 3465continue;
>>>>
>>>> In case several processes or even all die by a segfault we would wait 
>>>> until we have processed all that processes plus one second
>>>> until we start new processes. Shouldn't we perform an 
>>>> perform_idle_server_maintenance in case the process died because of a fatal
>>>> signal?
>>>
>>> +1
>>>
>>
>> In order to solve this I would like to add another APEXIT_ constant (e.g. 
>> APEXIT_FATAL_SIGNAL).
> 
> Maybe something like:
> 
> Index: server/mpm/event/event.c
> ===
> --- server/mpm/event/event.c(revision 1899818)
> +++ server/mpm/event/event.c(working copy)
> @@ -3462,7 +3462,9 @@ static void server_main_loop(int remaining_childre
>   * finite number of children can die, and it's pretty
>   * pathological for a lot to die suddenly.
>   */
> -continue;
> +if (!APR_PROC_CHECK_SIGNALED(exitwhy)) {
> +continue;
> +}
>  }
>  else if (remaining_children_to_start) {
>  /* we hit a 1 second timeout in which none of the previous
> --
> 
> could do?
> Any signal that killed the process would mean something unexpected
> happened since we clean_child_exit() otherwise?

Hm. You mean that the case for

case SIGTERM:
case SIGHUP:
case AP_SIG_GRACEFUL:

in ap_process_child_status
never triggers as the child catches these and does a clean_child_exit()?

If yes, the above seems to be a simple solution, but it would also capture 
SIGKILL which might
have been issued by our parent. Does this matter?

Do we need something for processes that die due to MaxConnectionsPerChild or do 
we expect them to die at a slow path where the
current approach is fine?

Regards

Rüdiger


Re: Faster start children after fatal signals?

2022-04-14 Thread Ruediger Pluem



On 4/13/22 5:41 PM, Yann Ylavic wrote:
> On Wed, Apr 13, 2022 at 4:30 PM Ruediger Pluem  wrote:
>>
>> While looking at PR65769 I stumbled across the below in event.c (same in 
>> worker.c)
>>
>> 3460/* Don't perform idle maintenance when a child dies,
>> 3461 * only do it when there's a timeout.  Remember only a
>> 3462 * finite number of children can die, and it's pretty
>> 3463 * pathological for a lot to die suddenly.
>> 3464 */
>> 3465continue;
>>
>> In case several processes or even all die by a segfault we would wait until 
>> we have processed all that processes plus one second
>> until we start new processes. Shouldn't we perform an 
>> perform_idle_server_maintenance in case the process died because of a fatal
>> signal?
> 
> +1
> 

In order to solve this I would like to add another APEXIT_ constant (e.g. 
APEXIT_FATAL_SIGNAL). Looking at httpd.h
the values of the existing constants seem a little bit strange:

/** a normal exit */
#define APEXIT_OK   0x0
/** A fatal error arising during the server's init sequence */
#define APEXIT_INIT 0x2
/**  The child died during its init sequence */
#define APEXIT_CHILDINIT0x3
/**
 *   The child exited due to a resource shortage.
 *   The parent should limit the rate of forking until
 *   the situation is resolved.
 */
#define APEXIT_CHILDSICK0x7
/**
 * A fatal error, resulting in the whole server aborting.
 * If a child exits with this error, the parent process
 * considers this a server-wide fatal error and aborts.
 */
#define APEXIT_CHILDFATAL   0xf


I haven't found any hint during my investigation why they are as they are on 
not just 1, 2, 3, 
Any ideas?


Regards

Rüdiger


Re: svn commit: r1899648 - in /httpd/httpd/trunk: changes-entries/core_response_buckets.txt include/mod_core.h modules/http/http_core.c modules/http/http_filters.c modules/http/http_protocol.c server/

2022-04-14 Thread Ruediger Pluem



On 4/14/22 10:18 AM, Stefan Eissing wrote:
> 
> 
>> Am 13.04.2022 um 17:16 schrieb Ruediger Pluem :
>>
>>
>>
>> On 4/7/22 12:41 PM, ic...@apache.org wrote:
>>> Author: icing
>>> Date: Thu Apr  7 10:41:46 2022
>>> New Revision: 1899648
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1899648=rev
>>> Log:
>>>  *) core/mod_http: use RESPONSE meta buckets and a new HTTP/1.x specific
>>> filter to send responses through the output filter chain.
>>> Specifically: the HTTP_HEADER output filter and 
>>> ap_send_interim_response()
>>> create a RESPONSE bucket and no longer are concerned with HTTP/1.x
>>> serialization.
>>> A new HTTP1_RESPONSE_OUT transcode filter writes the proper HTTP/1.x
>>> bytes when dealing with a RESPONSE bucket. That filter installs itself
>>> on the pre_read_request hook when the connection has protocol 
>>> 'http/1.1'.
>>>
>>>
>>> Added:
>>>httpd/httpd/trunk/changes-entries/core_response_buckets.txt
>>> Modified:
>>>httpd/httpd/trunk/include/mod_core.h
>>>httpd/httpd/trunk/modules/http/http_core.c
>>>httpd/httpd/trunk/modules/http/http_filters.c
>>>httpd/httpd/trunk/modules/http/http_protocol.c
>>>httpd/httpd/trunk/server/protocol.c
>>>
>>
>>> Modified: httpd/httpd/trunk/modules/http/http_filters.c
>>> URL: 
>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1899648=1899647=1899648=diff
>>> ==
>>> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
>>> +++ httpd/httpd/trunk/modules/http/http_filters.c Thu Apr  7 10:41:46 2022
>>
>>> @@ -1364,62 +1178,124 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>> return rv;
>>> }
>>> }
>>> -
>>> -for (e = APR_BRIGADE_FIRST(b);
>>> - e != APR_BRIGADE_SENTINEL(b);
>>> - e = APR_BUCKET_NEXT(e))
>>> -{
>>> -if (AP_BUCKET_IS_ERROR(e) && !eb) {
>>> -eb = e->data;
>>> -continue;
>>> -}
>>> -if (APR_BUCKET_IS_EOS(e)) {
>>> -if (!eos) eos = e;
>>> -continue;
>>> -}
>>> -/*
>>> - * If we see an EOC bucket it is a signal that we should get out
>>> - * of the way doing nothing.
>>> +else {
>>> +/* Determine if it is time to insert the response bucket for
>>> + * the request. Request handlers just write content or an EOS
>>> + * and that needs to take the current state of request_rec to
>>> + * send on status and headers as a response bucket.
>>> + *
>>> + * But we also send interim responses (as response buckets)
>>> + * through this filter and that must not trigger generating
>>> + * an additional response bucket.
>>> + *
>>> + * Waiting on a DATA/ERROR/EOS bucket alone is not enough,
>>> + * unfortunately, as some handlers trigger response generation
>>> + * by just writing a FLUSH (see mod_lua's websocket for example).
>>>  */
>>> -if (AP_BUCKET_IS_EOC(e)) {
>>> -ap_remove_output_filter(f);
>>> -return ap_pass_brigade(f->next, b);
>>> +for (e = APR_BRIGADE_FIRST(b);
>>> + e != APR_BRIGADE_SENTINEL(b) && !trigger;
>>> + e = APR_BUCKET_NEXT(e))
>>> +{
>>> +if (AP_BUCKET_IS_RESPONSE(e)) {
>>> +/* remember the last one if there are many. */
>>> +respb = e;
>>> +}
>>> +else if (APR_BUCKET_IS_FLUSH(e)) {
>>> +/* flush without response bucket triggers */
>>> +if (!respb) trigger = e;
>>> +}
>>> +else if (APR_BUCKET_IS_EOS(e)) {
>>> +trigger = e;
>>> +}
>>> +else if (AP_BUCKET_IS_ERROR(e)) {
>>> +/* Need to handle this below via ap_die() */
>>> +break;
>>> +}
>>> +else {
>>> +/* First content bucket, always triggering the response.*/
>>> +

Re: svn commit: r1899648 - in /httpd/httpd/trunk: changes-entries/core_response_buckets.txt include/mod_core.h modules/http/http_core.c modules/http/http_filters.c modules/http/http_protocol.c server/

2022-04-13 Thread Ruediger Pluem



On 4/7/22 12:41 PM, ic...@apache.org wrote:
> Author: icing
> Date: Thu Apr  7 10:41:46 2022
> New Revision: 1899648
> 
> URL: http://svn.apache.org/viewvc?rev=1899648=rev
> Log:
>   *) core/mod_http: use RESPONSE meta buckets and a new HTTP/1.x specific
>  filter to send responses through the output filter chain.
>  Specifically: the HTTP_HEADER output filter and 
> ap_send_interim_response()
>  create a RESPONSE bucket and no longer are concerned with HTTP/1.x
>  serialization.
>  A new HTTP1_RESPONSE_OUT transcode filter writes the proper HTTP/1.x
>  bytes when dealing with a RESPONSE bucket. That filter installs itself
>  on the pre_read_request hook when the connection has protocol 'http/1.1'.
> 
> 
> Added:
> httpd/httpd/trunk/changes-entries/core_response_buckets.txt
> Modified:
> httpd/httpd/trunk/include/mod_core.h
> httpd/httpd/trunk/modules/http/http_core.c
> httpd/httpd/trunk/modules/http/http_filters.c
> httpd/httpd/trunk/modules/http/http_protocol.c
> httpd/httpd/trunk/server/protocol.c
> 

> Modified: httpd/httpd/trunk/modules/http/http_filters.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1899648=1899647=1899648=diff
> ==
> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
> +++ httpd/httpd/trunk/modules/http/http_filters.c Thu Apr  7 10:41:46 2022

> @@ -1364,62 +1178,124 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>  return rv;
>  }
>  }
> -
> -for (e = APR_BRIGADE_FIRST(b);
> - e != APR_BRIGADE_SENTINEL(b);
> - e = APR_BUCKET_NEXT(e))
> -{
> -if (AP_BUCKET_IS_ERROR(e) && !eb) {
> -eb = e->data;
> -continue;
> -}
> -if (APR_BUCKET_IS_EOS(e)) {
> -if (!eos) eos = e;
> -continue;
> -}
> -/*
> - * If we see an EOC bucket it is a signal that we should get out
> - * of the way doing nothing.
> +else {
> +/* Determine if it is time to insert the response bucket for
> + * the request. Request handlers just write content or an EOS
> + * and that needs to take the current state of request_rec to
> + * send on status and headers as a response bucket.
> + *
> + * But we also send interim responses (as response buckets)
> + * through this filter and that must not trigger generating
> + * an additional response bucket.
> + *
> + * Waiting on a DATA/ERROR/EOS bucket alone is not enough,
> + * unfortunately, as some handlers trigger response generation
> + * by just writing a FLUSH (see mod_lua's websocket for example).
>   */
> -if (AP_BUCKET_IS_EOC(e)) {
> -ap_remove_output_filter(f);
> -return ap_pass_brigade(f->next, b);
> +for (e = APR_BRIGADE_FIRST(b);
> + e != APR_BRIGADE_SENTINEL(b) && !trigger;
> + e = APR_BUCKET_NEXT(e))
> +{
> +if (AP_BUCKET_IS_RESPONSE(e)) {
> +/* remember the last one if there are many. */
> +respb = e;
> +}
> +else if (APR_BUCKET_IS_FLUSH(e)) {
> +/* flush without response bucket triggers */
> +if (!respb) trigger = e;
> +}
> +else if (APR_BUCKET_IS_EOS(e)) {
> +trigger = e;
> +}
> +else if (AP_BUCKET_IS_ERROR(e)) {
> +/* Need to handle this below via ap_die() */
> +break;
> +}
> +else {
> +/* First content bucket, always triggering the response.*/
> +trigger = e;
> +}

Why don't we remove ourselves any longer if we hit an EOC bucket and pass stuff 
on immediately?
If we want to handle this HTTP protocol agnostic here we need to ensure that 
the EOC bucket
triggers the immediate removal of the ap_h1_response_out_filter once the 
ap_h1_response_out_filter
sees it without putting any headers on the wire.
I am not sure what the appropriate behaviour would be in the HTTP > 1.1 case.

>  }
> -}
>  
> -if (!ctx->headers_sent && !check_headers(r)) {
> -/* We may come back here from ap_die() below,
> - * so clear anything from this response.
> - */
> -apr_table_clear(r->headers_out);
> -apr_table_clear(r->err_headers_out);
> -apr_brigade_cleanup(b);
> +if (respb) {
> +ap_bucket_response *resp = respb->data;
> +if (resp->status >= 200 || resp->status == 101) {

Why using the literal 101 instead of HTTP_SWITCHING_PROTOCOLS?

> +/* Someone is passing the final response, remember it
> + * so we no longer generate one. */
> +ctx->final_status = resp->status;
> +   

Faster start children after fatal signals?

2022-04-13 Thread Ruediger Pluem
While looking at PR65769 I stumbled across the below in event.c (same in 
worker.c)

3460/* Don't perform idle maintenance when a child dies,
3461 * only do it when there's a timeout.  Remember only a
3462 * finite number of children can die, and it's pretty
3463 * pathological for a lot to die suddenly.
3464 */
3465continue;

In case several processes or even all die by a segfault we would wait until we 
have processed all that processes plus one second
until we start new processes. Shouldn't we perform an 
perform_idle_server_maintenance in case the process died because of a fatal
signal?

Regards

Rüdiger


Re: svn commit: r1899777 - /httpd/httpd/trunk/server/mpm/event/event.c

2022-04-13 Thread Ruediger Pluem



On 4/12/22 2:08 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Tue Apr 12 12:08:02 2022
> New Revision: 1899777
> 
> URL: http://svn.apache.org/viewvc?rev=1899777=rev
> Log:
> mpm_event: Fix accounting of active/total processes on ungraceful restart.
> 
> Children processes terminated by ap_{reclaim,relieve}_child_processes() were
> were not un-accounted for total_daemons and active_daemons, which was done in
> server_main_loop() only. This led to perform_idle_server_maintenance() 
> thinking
> it was over the limit of children processes and never create new ones.
> 
> Have this accounting right in event_note_child_{started,stopped}() which is
> called both at runtime and reload time.
> 
> * server/mpm/event/event.c(struct event_retained_data):
>   Rename field max_daemons_limit to max_daemon_used to better describe what
>   it's about and to align with AP_MPMQ_MAX_DAEMON_USED.
> 
> * server/mpm/event/event.c(event_note_child_stopped):
>   Renamed from event_note_child_killed() to clarify that it's not only called
>   when a child is killed (i.e. on restart) but whenever a child has stopped.
> 
> * server/mpm/event/event.c(event_note_child_stopped):
>   Move decrementing {active,total}_daemons and marking child's threads as
>   SERVER_DEAD from server_main_loop() so that it's done both at runtime and
>   reload time. Log the current number/state of daemons at APLOG_DEBUG level
>   for each child stopped.
> 
> * server/mpm/event/event.c(event_note_child_started):
>   Move incrementing {active,total}_daemons from make_child() for symmetry,
>   given that make_child() calls event_note_child_started(). Log the current
>   number/state of daemons at APLOG_DEBUG level for each child started.
> 
> * server/mpm/event/event.c(perform_idle_server_maintenance):
>   Fix possible miscounting of retained->max_daemon_used accross the multiple
>   calls to perform_idle_server_maintenance() if ListenCoresBucketsRatio > 0.
>   Pass an int *max_daemon_used which starts at zero and is bumped consistently
>   for all the buckets, while retained->max_daemon_used is updated only after
>   all the buckets have been maintained.
> 
> * server/mpm/event/event.c(perform_idle_server_maintenance):
>   Use event_note_child_stopped() to handle exited children processes.
> 
> 
> Fixes: BZ 66004
> 
> 
> Modified:
> httpd/httpd/trunk/server/mpm/event/event.c
> 
> Modified: httpd/httpd/trunk/server/mpm/event/event.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1899777=1899776=1899777=diff
> ==
> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> +++ httpd/httpd/trunk/server/mpm/event/event.c Tue Apr 12 12:08:02 2022

> @@ -3447,9 +3480,11 @@ static void server_main_loop(int remaini
>  continue;
>  }
>  
> +max_daemon_used = 0;
>  for (i = 0; i < num_buckets; i++) {
> -perform_idle_server_maintenance(i);
> +perform_idle_server_maintenance(i, _daemon_used);
>  }
> +retained->max_daemon_used = max_daemon_used;

Shouldn't we do the above only when max_daemon_used > retained->max_daemon_used 
? Otherwise we might shrink
retained->max_daemon_used if make_child increased it?

Regards

Rüdiger



Re: svn commit: r1899552 - in /httpd/httpd/trunk: modules/http/ modules/proxy/ test/modules/http1/ test/modules/http1/htdocs/ test/modules/http1/htdocs/cgi/ test/modules/http1/htdocs/cgi/files/ test/m

2022-04-05 Thread Ruediger Pluem



On 4/5/22 9:53 AM, Stefan Eissing wrote:
> 
> 
>> Am 05.04.2022 um 09:34 schrieb Ruediger Pluem :
>>
>>
>>
>> On 4/5/22 9:13 AM, Stefan Eissing wrote:
>>>
>>>
>>>> Am 04.04.2022 um 16:07 schrieb Ruediger Pluem :
>>>>
>>>>
>>>>
>>>> On 4/4/22 1:08 PM, ic...@apache.org wrote:
>>>>> Author: icing
>>>>> Date: Mon Apr 4 11:08:58 2022
>>>>> New Revision: 1899552
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1899552=rev
>>>>> Log:
>>>>> *) mod_http: genereate HEADERS buckets for trailers
>>>>> mod_proxy: forward trailers on chunked request encoding
>>>>> test: add http/1.x test cases in pytest
>>>>>
>>>>>
>>>>> Added:
>>>>> httpd/httpd/trunk/test/modules/http1/
>>>>> httpd/httpd/trunk/test/modules/http1/__init__.py
>>>>> httpd/httpd/trunk/test/modules/http1/conftest.py
>>>>> httpd/httpd/trunk/test/modules/http1/env.py
>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/
>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/
>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/
>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/empty.txt
>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/hello.py (with props)
>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/upload.py (with props)
>>>>> httpd/httpd/trunk/test/modules/http1/mod_h1test/
>>>>> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.c
>>>>> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.slo
>>>>> httpd/httpd/trunk/test/modules/http1/test_001_alive.py
>>>>> httpd/httpd/trunk/test/modules/http1/test_003_get.py
>>>>> httpd/httpd/trunk/test/modules/http1/test_004_post.py
>>>>> httpd/httpd/trunk/test/modules/http1/test_005_trailers.py
>>>>> httpd/httpd/trunk/test/modules/http1/test_006_unsafe.py
>>>>> httpd/httpd/trunk/test/modules/http1/test_007_strict.py
>>>>> Modified:
>>>>> httpd/httpd/trunk/modules/http/http_filters.c
>>>>> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
>>>>> httpd/httpd/trunk/test/modules/http2/env.py
>>>>> httpd/httpd/trunk/test/pyhttpd/conf.py
>>>>> httpd/httpd/trunk/test/pyhttpd/env.py
>>>>>
>>>>> Modified: httpd/httpd/trunk/modules/http/http_filters.c
>>>>> URL: 
>>>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1899552=1899551=1899552=diff
>>>>> ==
>>>>> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
>>>>> +++ httpd/httpd/trunk/modules/http/http_filters.c Mon Apr 4 11:08:58 2022
>>>>> @@ -1312,6 +1312,15 @@ AP_DECLARE_NONSTD(int) ap_send_http_trac
>>>>> return DONE;
>>>>> }
>>>>>
>>>>> +static apr_bucket *create_trailers_bucket(request_rec *r, 
>>>>> apr_bucket_alloc_t *bucket_alloc)
>>>>> +{
>>>>> + if (r->trailers_out && !apr_is_empty_table(r->trailers_out)) {
>>>>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "sending trailers");
>>>>> + return ap_bucket_headers_create(r->trailers_out, r->pool, bucket_alloc);
>>>>> + }
>>>>> + return NULL;
>>>>> +}
>>>>> +
>>>>> typedef struct header_filter_ctx {
>>>>> int headers_sent;
>>>>> } header_filter_ctx;
>>>>> @@ -1323,7 +1332,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>>>> conn_rec *c = r->connection;
>>>>> int header_only = (r->header_only || AP_STATUS_IS_HEADER_ONLY(r->status));
>>>>> const char *protocol = NULL;
>>>>> - apr_bucket *e;
>>>>> + apr_bucket *e, *eos = NULL;
>>>>> apr_bucket_brigade *b2;
>>>>> header_struct h;
>>>>> header_filter_ctx *ctx = f->ctx;
>>>>> @@ -1364,6 +1373,10 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>>>> eb = e->data;
>>>>> continue;
>>>>> }
>>>>> + if (APR_BUCKET_IS_EOS(e)) {
>>>>> + if (!eos) eos = e;
>>>>> + continue;
>>>>> + }
>>>>> /*
>>>>

Re: svn commit: r1899552 - in /httpd/httpd/trunk: modules/http/ modules/proxy/ test/modules/http1/ test/modules/http1/htdocs/ test/modules/http1/htdocs/cgi/ test/modules/http1/htdocs/cgi/files/ test/m

2022-04-05 Thread Ruediger Pluem



On 4/5/22 9:13 AM, Stefan Eissing wrote:
> 
> 
>> Am 04.04.2022 um 16:07 schrieb Ruediger Pluem :
>>
>>
>>
>> On 4/4/22 1:08 PM, ic...@apache.org wrote:
>>> Author: icing
>>> Date: Mon Apr  4 11:08:58 2022
>>> New Revision: 1899552
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1899552=rev
>>> Log:
>>>  *) mod_http: genereate HEADERS buckets for trailers
>>> mod_proxy: forward trailers on chunked request encoding
>>> test: add http/1.x test cases in pytest
>>>
>>>
>>> Added:
>>>httpd/httpd/trunk/test/modules/http1/
>>>httpd/httpd/trunk/test/modules/http1/__init__.py
>>>httpd/httpd/trunk/test/modules/http1/conftest.py
>>>httpd/httpd/trunk/test/modules/http1/env.py
>>>httpd/httpd/trunk/test/modules/http1/htdocs/
>>>httpd/httpd/trunk/test/modules/http1/htdocs/cgi/
>>>httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/
>>>httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/empty.txt
>>>httpd/httpd/trunk/test/modules/http1/htdocs/cgi/hello.py   (with props)
>>>httpd/httpd/trunk/test/modules/http1/htdocs/cgi/upload.py   (with props)
>>>httpd/httpd/trunk/test/modules/http1/mod_h1test/
>>>httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.c
>>>httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.slo
>>>httpd/httpd/trunk/test/modules/http1/test_001_alive.py
>>>httpd/httpd/trunk/test/modules/http1/test_003_get.py
>>>httpd/httpd/trunk/test/modules/http1/test_004_post.py
>>>httpd/httpd/trunk/test/modules/http1/test_005_trailers.py
>>>httpd/httpd/trunk/test/modules/http1/test_006_unsafe.py
>>>httpd/httpd/trunk/test/modules/http1/test_007_strict.py
>>> Modified:
>>>httpd/httpd/trunk/modules/http/http_filters.c
>>>httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
>>>httpd/httpd/trunk/test/modules/http2/env.py
>>>httpd/httpd/trunk/test/pyhttpd/conf.py
>>>httpd/httpd/trunk/test/pyhttpd/env.py
>>>
>>> Modified: httpd/httpd/trunk/modules/http/http_filters.c
>>> URL: 
>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1899552=1899551=1899552=diff
>>> ==
>>> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
>>> +++ httpd/httpd/trunk/modules/http/http_filters.c Mon Apr  4 11:08:58 2022
>>> @@ -1312,6 +1312,15 @@ AP_DECLARE_NONSTD(int) ap_send_http_trac
>>> return DONE;
>>> }
>>>
>>> +static apr_bucket *create_trailers_bucket(request_rec *r, 
>>> apr_bucket_alloc_t *bucket_alloc)
>>> +{
>>> +if (r->trailers_out && !apr_is_empty_table(r->trailers_out)) {
>>> +ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "sending trailers");
>>> +return ap_bucket_headers_create(r->trailers_out, r->pool, 
>>> bucket_alloc);
>>> +}
>>> +return NULL;
>>> +}
>>> +
>>> typedef struct header_filter_ctx {
>>> int headers_sent;
>>> } header_filter_ctx;
>>> @@ -1323,7 +1332,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>> conn_rec *c = r->connection;
>>> int header_only = (r->header_only || 
>>> AP_STATUS_IS_HEADER_ONLY(r->status));
>>> const char *protocol = NULL;
>>> -apr_bucket *e;
>>> +apr_bucket *e, *eos = NULL;
>>> apr_bucket_brigade *b2;
>>> header_struct h;
>>> header_filter_ctx *ctx = f->ctx;
>>> @@ -1364,6 +1373,10 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>> eb = e->data;
>>> continue;
>>> }
>>> +if (APR_BUCKET_IS_EOS(e)) {
>>> +if (!eos) eos = e;
>>> +continue;
>>> +}
>>> /*
>>>  * If we see an EOC bucket it is a signal that we should get out
>>>  * of the way doing nothing.
>>> @@ -1416,6 +1429,22 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>> goto out;
>>> }
>>>
>>> +if (eos) {
>>> +/* on having seen EOS and added possible trailers, we
>>> + * can remove this filter.
>>> + */
>>> +e = create_trailers_bucket(r, b->bucket_alloc);
>>> +if (e) {
>>> +APR_BUCKET_INSERT_BEFORE(eos, e);
>>> +}
>>> +ap_remove_output_filter(f);
>>> +}
>>> +
>>> +if (ctx->headers_sent) {
>>> +/* we did already the stuff below, just pass on */
>>> +return ap_pass_brigade(f->next, b);
>>> +}
>>> +
>>
>> I guess this does not work for header_only requests with trailing headers.
> 
> The thought came up if we want/need/may support that. Can/should a 304 have 
> trailers if the unconditional request had?

What about HEAD requests where the corresponding GET has trailers?

Regards

Rüdiger


Re: svn commit: r1899552 - in /httpd/httpd/trunk: modules/http/ modules/proxy/ test/modules/http1/ test/modules/http1/htdocs/ test/modules/http1/htdocs/cgi/ test/modules/http1/htdocs/cgi/files/ test/m

2022-04-04 Thread Ruediger Pluem



On 4/4/22 1:08 PM, ic...@apache.org wrote:
> Author: icing
> Date: Mon Apr  4 11:08:58 2022
> New Revision: 1899552
> 
> URL: http://svn.apache.org/viewvc?rev=1899552=rev
> Log:
>   *) mod_http: genereate HEADERS buckets for trailers
>  mod_proxy: forward trailers on chunked request encoding
>  test: add http/1.x test cases in pytest
> 
> 
> Added:
> httpd/httpd/trunk/test/modules/http1/
> httpd/httpd/trunk/test/modules/http1/__init__.py
> httpd/httpd/trunk/test/modules/http1/conftest.py
> httpd/httpd/trunk/test/modules/http1/env.py
> httpd/httpd/trunk/test/modules/http1/htdocs/
> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/
> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/
> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/empty.txt
> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/hello.py   (with props)
> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/upload.py   (with props)
> httpd/httpd/trunk/test/modules/http1/mod_h1test/
> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.c
> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.slo
> httpd/httpd/trunk/test/modules/http1/test_001_alive.py
> httpd/httpd/trunk/test/modules/http1/test_003_get.py
> httpd/httpd/trunk/test/modules/http1/test_004_post.py
> httpd/httpd/trunk/test/modules/http1/test_005_trailers.py
> httpd/httpd/trunk/test/modules/http1/test_006_unsafe.py
> httpd/httpd/trunk/test/modules/http1/test_007_strict.py
> Modified:
> httpd/httpd/trunk/modules/http/http_filters.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
> httpd/httpd/trunk/test/modules/http2/env.py
> httpd/httpd/trunk/test/pyhttpd/conf.py
> httpd/httpd/trunk/test/pyhttpd/env.py
> 
> Modified: httpd/httpd/trunk/modules/http/http_filters.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1899552=1899551=1899552=diff
> ==
> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
> +++ httpd/httpd/trunk/modules/http/http_filters.c Mon Apr  4 11:08:58 2022
> @@ -1312,6 +1312,15 @@ AP_DECLARE_NONSTD(int) ap_send_http_trac
>  return DONE;
>  }
>  
> +static apr_bucket *create_trailers_bucket(request_rec *r, apr_bucket_alloc_t 
> *bucket_alloc)
> +{
> +if (r->trailers_out && !apr_is_empty_table(r->trailers_out)) {
> +ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "sending trailers");
> +return ap_bucket_headers_create(r->trailers_out, r->pool, 
> bucket_alloc);
> +}
> +return NULL;
> +}
> +
>  typedef struct header_filter_ctx {
>  int headers_sent;
>  } header_filter_ctx;
> @@ -1323,7 +1332,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>  conn_rec *c = r->connection;
>  int header_only = (r->header_only || 
> AP_STATUS_IS_HEADER_ONLY(r->status));
>  const char *protocol = NULL;
> -apr_bucket *e;
> +apr_bucket *e, *eos = NULL;
>  apr_bucket_brigade *b2;
>  header_struct h;
>  header_filter_ctx *ctx = f->ctx;
> @@ -1364,6 +1373,10 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>  eb = e->data;
>  continue;
>  }
> +if (APR_BUCKET_IS_EOS(e)) {
> +if (!eos) eos = e;
> +continue;
> +}
>  /*
>   * If we see an EOC bucket it is a signal that we should get out
>   * of the way doing nothing.
> @@ -1416,6 +1429,22 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>  goto out;
>  }
>  
> +if (eos) {
> +/* on having seen EOS and added possible trailers, we
> + * can remove this filter.
> + */
> +e = create_trailers_bucket(r, b->bucket_alloc);
> +if (e) {
> +APR_BUCKET_INSERT_BEFORE(eos, e);
> +}
> +ap_remove_output_filter(f);
> +}
> +
> +if (ctx->headers_sent) {
> +/* we did already the stuff below, just pass on */
> +return ap_pass_brigade(f->next, b);
> +}
> +

I guess this does not work for header_only requests with trailing headers.

>  /*
>   * Now that we are ready to send a response, we need to combine the two
>   * header field tables into a single table.  If we don't do this, our

Regards

Rüdiger



Re: svn commit: r1899550 - in /httpd/httpd/trunk: include/ap_mmn.h include/http_protocol.h modules/http/http_protocol.c modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c

2022-04-04 Thread Ruediger Pluem



On 4/4/22 11:41 AM, ic...@apache.org wrote:
> Author: icing
> Date: Mon Apr  4 09:41:25 2022
> New Revision: 1899550
> 
> URL: http://svn.apache.org/viewvc?rev=1899550=rev
> Log:
>   *) core: add ap_h1_append_header() for single header values.
>   *) mod_proxy: use of new ap_h1_header(s) functions for
>  formatting HTTP/1.1 requests.
> 
> 
> Modified:
> httpd/httpd/trunk/include/ap_mmn.h
> httpd/httpd/trunk/include/http_protocol.h
> httpd/httpd/trunk/modules/http/http_protocol.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
> httpd/httpd/trunk/modules/proxy/proxy_util.c
> 

> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1899550=1899549=1899550=diff
> ==
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Mon Apr  4 09:41:25 2022

> @@ -3913,28 +3910,51 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
>  ap_xlate_proto_to_ascii(buf, strlen(buf));
>  e = apr_bucket_pool_create(buf, strlen(buf), p, c->bucket_alloc);
>  APR_BRIGADE_INSERT_TAIL(header_brigade, e);
> +
> +/*
> + * Make a copy on r->headers_in for the request we make to the backend.
> + * This we modify according to our configuration and connection handling.
> + * Leave the original headers we received from the client untouched.
> + *
> + * Note: We need to take r->pool for apr_table_copy as the key / value
> + * pairs in r->headers_in have been created out of r->pool and
> + * p might be (and actually is) a longer living pool.

Hm. I found two calls to ap_proxy_create_hdrbr in mod_proxy_http.c and 
mod_proxy_wstunnel.c and both seem to pass r->pool as pool p.
There is currently no documentation on how p relates to r->pool.
But I agree with your further comments that we should allocate further headers 
from r->pool to be consistent in the table and
to use r->pool for the table copy.


> + * This would trigger the bad pool ancestry abort in apr_table_copy if
> + * apr is compiled with APR_POOL_DEBUG.
> + *
> + * icing: if p indeed lives longer than r->pool, we should allocate
> + * all new header values from r->pool as well and avoid leakage.
> + */
> +request_headers = apr_table_copy(r->pool, r->headers_in);
> +
> +/* We used to send `Host: ` always first, so let's keep it that
> + * way. No telling which legacy backend is relying no this.
> + */
>  if (dconf->preserve_host == 0) {
> +const char *nhost;
>  if (ap_strchr_c(uri->hostname, ':')) { /* if literal IPv6 address */
>  if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
> -buf = apr_pstrcat(p, "Host: [", uri->hostname, "]:",
> -  uri->port_str, CRLF, NULL);
> +nhost = apr_pstrcat(r->pool, "[", uri->hostname, "]:",
> +uri->port_str, NULL);
>  } else {
> -buf = apr_pstrcat(p, "Host: [", uri->hostname, "]", CRLF, 
> NULL);
> +nhost = apr_pstrcat(r->pool, "[", uri->hostname, "]", NULL);
>  }
>  } else {
>  if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
> -buf = apr_pstrcat(p, "Host: ", uri->hostname, ":",
> -  uri->port_str, CRLF, NULL);
> +nhost = apr_pstrcat(r->pool, uri->hostname, ":",
> +uri->port_str, NULL);
>  } else {
> -buf = apr_pstrcat(p, "Host: ", uri->hostname, CRLF, NULL);
> +nhost = uri->hostname;
>  }
>  }
> +ap_h1_append_header(header_brigade, r->pool, "Host", nhost);
> +apr_table_unset(request_headers, "Host");
>  }
>  else {
>  /* don't want to use r->hostname, as the incoming header might have a
>   * port attached
>   */
> -const char* hostname = apr_table_get(r->headers_in,"Host");
> +const char* hostname = apr_table_get(request_headers, "Host");
>  if (!hostname) {
>  hostname =  r->server->server_hostname;
>  ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01092)

Regards

Rüdiger



Re: svn commit: r1899390 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h

2022-04-01 Thread Ruediger Pluem



On 4/1/22 10:23 AM, jean-frederic clere wrote:
> On 01/04/2022 10:03, Ruediger Pluem wrote:
>>

> 
> Sure I have a PR to revert, waiting on travis...
> 
>> Feel free to add a detection for such empty proxy blocks. I think a warning
>> should be sufficient. How do you want to detect this? By inspecting 
>> new_dir_conf after ap_walk_config was executed?
> 
> putting it in the proxysection() is not the best, correct?

I haven't made detailed thoughts on how to implement this detection. Hence I 
cannot answer :-)

Regards

Rüdiger



Re: svn commit: r1899390 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h

2022-04-01 Thread Ruediger Pluem



On 4/1/22 8:47 AM, jean-frederic clere wrote:
> On 31/03/2022 12:59, Ruediger Pluem wrote:
>>
>>
>> On 3/31/22 12:34 PM, Stefan Eissing wrote:
>>>
>>>
>>>> Am 31.03.2022 um 11:55 schrieb Ruediger Pluem :
>>>>
>>>>
>>>>
>>>> On 3/31/22 11:11 AM, Ruediger Pluem wrote:
>>>>>
>>>>>
>>>>> On 3/30/22 4:42 PM, jfcl...@apache.org wrote:
>>>>>> Author: jfclere
>>>>>> Date: Wed Mar 30 14:42:14 2022
>>>>>> New Revision: 1899390
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=1899390=rev
>>>>>> Log:
>>>>>> Add WorkerBalancerGrowth. To allow creation of workers
>>>>>> to dynamically added balancers.
>>>>>>
>>>>>> Modified:
>>>>>> httpd/httpd/trunk/CHANGES
>>>>>> httpd/httpd/trunk/modules/proxy/mod_proxy.c
>>>>>> httpd/httpd/trunk/modules/proxy/mod_proxy.h
>>>>>>
>>>>>> Modified: httpd/httpd/trunk/CHANGES
>>>>>> URL: 
>>>>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1899390=1899389=1899390=diff
>>>>>> ==
>>>>>> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
>>>>>> +++ httpd/httpd/trunk/CHANGES [utf-8] Wed Mar 30 14:42:14 2022
>>>>>> @@ -1,6 +1,10 @@
>>>>>> -*- coding: utf-8 -*-
>>>>>> Changes with Apache 2.5.1
>>>>>>
>>>>>> + *) mod_proxy: Add WorkerBalancerGrowth to allow adding workers to
>>>>>> + balancer created dynamically or via "empty" 
>>>>>> + [Jean-Frederic Clere]
>>>>>
>>>>> I am not sure why this is needed. You can already do this via
>>>>>
>>>>> 
>>>>> 
>>>>
>>>> Or
>>>>
>>>> 
>>>> ProxySet growth=10
>>>> 
>>>
>>> FYI: Travis trunk also fails almost completely. Does not seem to accept a 
>>> proxy configuration.
>>
>> This is because the if
>>
>> +    if (!ap_strchr_c(conf->p, ':'))
>> +    return apr_pstrcat(cmd->pool, thiscmd->name,
>> +   "> arguments are not supported for non url.",
>> +   NULL);
>>
>> should not return with an error, but just encapsulate the remainder of the 
>> block. And I think the further
>> return apr_pstrcat are also wrong.
>>
>> But as said I am not sure about the purpose at all as you can already do, 
>> what the patch should provide if I understand the patch
>> correctly.
> 
> The purpose was to be able to add a balancer in the balancer-manager handle 
> but that needs to pre-create the mutex and the slots
> for the workers.
> 
> While looking to that I noted that:
> 
> 
> 
> was doing nothing, the balancer is ignored, I should I revert the patch and 
> add an error message if there is an empty entry like
> this one?

Do




or


ProxySet growth=10


work and do what you want? If yes, please revert. Feel free to add a detection 
for such empty proxy blocks. I think a warning
should be sufficient. How do you want to detect this? By inspecting 
new_dir_conf after ap_walk_config was executed?

Regards

Rüdiger


Re: svn commit: r1899390 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h

2022-03-31 Thread Ruediger Pluem



On 3/31/22 12:34 PM, Stefan Eissing wrote:
> 
> 
>> Am 31.03.2022 um 11:55 schrieb Ruediger Pluem :
>>
>>
>>
>> On 3/31/22 11:11 AM, Ruediger Pluem wrote:
>>>
>>>
>>> On 3/30/22 4:42 PM, jfcl...@apache.org wrote:
>>>> Author: jfclere
>>>> Date: Wed Mar 30 14:42:14 2022
>>>> New Revision: 1899390
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1899390=rev
>>>> Log:
>>>> Add WorkerBalancerGrowth. To allow creation of workers
>>>> to dynamically added balancers.
>>>>
>>>> Modified:
>>>> httpd/httpd/trunk/CHANGES
>>>> httpd/httpd/trunk/modules/proxy/mod_proxy.c
>>>> httpd/httpd/trunk/modules/proxy/mod_proxy.h
>>>>
>>>> Modified: httpd/httpd/trunk/CHANGES
>>>> URL: 
>>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1899390=1899389=1899390=diff
>>>> ==
>>>> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
>>>> +++ httpd/httpd/trunk/CHANGES [utf-8] Wed Mar 30 14:42:14 2022
>>>> @@ -1,6 +1,10 @@
>>>> -*- coding: utf-8 -*-
>>>> Changes with Apache 2.5.1
>>>>
>>>> + *) mod_proxy: Add WorkerBalancerGrowth to allow adding workers to
>>>> + balancer created dynamically or via "empty" 
>>>> + [Jean-Frederic Clere]
>>>
>>> I am not sure why this is needed. You can already do this via
>>>
>>> 
>>> 
>>
>> Or
>>
>> 
>> ProxySet growth=10
>> 
> 
> FYI: Travis trunk also fails almost completely. Does not seem to accept a 
> proxy configuration.

This is because the if

+if (!ap_strchr_c(conf->p, ':'))
+return apr_pstrcat(cmd->pool, thiscmd->name,
+   "> arguments are not supported for non url.",
+   NULL);

should not return with an error, but just encapsulate the remainder of the 
block. And I think the further
return apr_pstrcat are also wrong.

But as said I am not sure about the purpose at all as you can already do, what 
the patch should provide if I understand the patch
correctly.

Regards

Rüdiger


Re: svn commit: r1899390 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h

2022-03-31 Thread Ruediger Pluem



On 3/31/22 11:11 AM, Ruediger Pluem wrote:
> 
> 
> On 3/30/22 4:42 PM, jfcl...@apache.org wrote:
>> Author: jfclere
>> Date: Wed Mar 30 14:42:14 2022
>> New Revision: 1899390
>>
>> URL: http://svn.apache.org/viewvc?rev=1899390=rev
>> Log:
>> Add WorkerBalancerGrowth. To allow creation of workers
>> to dynamically added balancers.
>>
>> Modified:
>> httpd/httpd/trunk/CHANGES
>> httpd/httpd/trunk/modules/proxy/mod_proxy.c
>> httpd/httpd/trunk/modules/proxy/mod_proxy.h
>>
>> Modified: httpd/httpd/trunk/CHANGES
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1899390=1899389=1899390=diff
>> ==
>> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
>> +++ httpd/httpd/trunk/CHANGES [utf-8] Wed Mar 30 14:42:14 2022
>> @@ -1,6 +1,10 @@
>>   -*- coding: utf-8 
>> -*-
>>  Changes with Apache 2.5.1
>>  
>> +  *) mod_proxy: Add WorkerBalancerGrowth to allow adding workers to
>> + balancer created dynamically or via "empty" 
>> + [Jean-Frederic Clere]
> 
> I am not sure why this is needed. You can already do this via
> 
> 
> 

Or


ProxySet growth=10


Regards

Rüdiger


Re: svn commit: r1899390 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h

2022-03-31 Thread Ruediger Pluem



On 3/30/22 4:42 PM, jfcl...@apache.org wrote:
> Author: jfclere
> Date: Wed Mar 30 14:42:14 2022
> New Revision: 1899390
> 
> URL: http://svn.apache.org/viewvc?rev=1899390=rev
> Log:
> Add WorkerBalancerGrowth. To allow creation of workers
> to dynamically added balancers.
> 
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/proxy/mod_proxy.c
> httpd/httpd/trunk/modules/proxy/mod_proxy.h
> 
> Modified: httpd/httpd/trunk/CHANGES
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1899390=1899389=1899390=diff
> ==
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Wed Mar 30 14:42:14 2022
> @@ -1,6 +1,10 @@
>   -*- coding: utf-8 
> -*-
>  Changes with Apache 2.5.1
>  
> +  *) mod_proxy: Add WorkerBalancerGrowth to allow adding workers to
> + balancer created dynamically or via "empty" 
> + [Jean-Frederic Clere]

I am not sure why this is needed. You can already do this via




Furthermore WorkerBalancerGrowth is inconsistent as the growth parameter I 
mention above only allows 100 as maximum and
WorkerBalancerGrowth allows 1000.

Regards

Rüdiger




Re: svn commit: r1899032 - in /httpd/httpd/trunk: ./ build/ changes-entries/ include/ modules/http2/ server/ test/modules/http2/

2022-03-29 Thread Ruediger Pluem



On 3/18/22 10:52 AM, ic...@apache.org wrote:
> Author: icing
> Date: Fri Mar 18 09:52:52 2022
> New Revision: 1899032
> 
> URL: http://svn.apache.org/viewvc?rev=1899032=rev
> Log:
>   *) core: adding a new hook and method to the API:
>  create_secondary_connection and ap_create_secondary_connection()
>  to setup connections related to a "master" one, as used in
>  the HTTP/2 protocol implementation.
> 
>   *) mod_http2: using the new API calls to get rid of knowledge
>  about how the core handles conn_rec specifics.
>  Improvements in pollset stream handling to use less sets.
>  Using atomic read/writes instead of volatiles now.
>  Keeping a reserve of "transit" pools and bucket_allocs for
>  use on secondary connections to avoid repeated setup/teardowns.
> 
> 
> Added:
> httpd/httpd/trunk/changes-entries/core_secondary_conn.txt
> Modified:
> httpd/httpd/trunk/   (props changed)
> httpd/httpd/trunk/build/   (props changed)
> httpd/httpd/trunk/include/ap_mmn.h
> httpd/httpd/trunk/include/http_connection.h
> httpd/httpd/trunk/modules/http2/h2_c2.c
> httpd/httpd/trunk/modules/http2/h2_c2.h
> httpd/httpd/trunk/modules/http2/h2_conn_ctx.c
> httpd/httpd/trunk/modules/http2/h2_conn_ctx.h
> httpd/httpd/trunk/modules/http2/h2_mplx.c
> httpd/httpd/trunk/modules/http2/h2_mplx.h
> httpd/httpd/trunk/modules/http2/h2_stream.c
> httpd/httpd/trunk/modules/http2/h2_workers.c
> httpd/httpd/trunk/modules/http2/h2_workers.h
> httpd/httpd/trunk/server/connection.c
> httpd/httpd/trunk/server/core.c
> httpd/httpd/trunk/test/modules/http2/test_711_load_post_cgi.py
> 


> Modified: httpd/httpd/trunk/server/core.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core.c?rev=1899032=1899031=1899032=diff
> ==
> --- httpd/httpd/trunk/server/core.c (original)
> +++ httpd/httpd/trunk/server/core.c Fri Mar 18 09:52:52 2022

> @@ -5517,6 +5519,54 @@ static conn_rec *core_create_conn(apr_po
>  return c;
>  }
>  
> +static conn_rec *core_create_secondary_conn(apr_pool_t *ptrans,
> +conn_rec *master,
> +apr_bucket_alloc_t *alloc)
> +{
> +apr_pool_t *pool;
> +conn_config_t *conn_config;
> +conn_rec *c = (conn_rec *) apr_pmemdup(ptrans, master, sizeof(*c));

Why don't we allocate the structure from the subpool below?

> +
> +/* Got a connection structure, so initialize what fields we can
> + * (the rest are zeroed out by pcalloc).
> + */

I don't get the comment above. We copy the existing master conn_rec. We
don't apr_pcalloc here.


> +apr_pool_create(, ptrans);

Why do we need a subpool here? In c2_transit_create we already create
a subpool with a dedicated allocator.
Is there a usecase for a subpool without a dedicated allocator?
If yes does it make sense to allow passing a dedicated allocator to the hook
to create a subpool using it (just like in c2_transit_create) and if NULL is
supplied only create a subpool like now?


> +apr_pool_tag(pool, "secondary_conn");
> +
> +/* we copied everything, now replace what is different */
> +c->master = master;
> +c->pool = pool;
> +c->bucket_alloc = alloc;
> +c->conn_config = ap_create_conn_config(pool);
> +c->notes = apr_table_make(pool, 5);
> +c->slaves = apr_array_make(pool, 20, sizeof(conn_slave_rec *));
> +c->requests = apr_array_make(pool, 20, sizeof(request_rec *));
> +c->input_filters = NULL;
> +c->output_filters = NULL;
> +c->filter_conn_ctx = NULL;
> +
> +/* prevent mpm_event from making wrong assumptions about this connection,
> + * like e.g. using its socket for an async read check. */
> +c->clogging_input_filters = 1;
> +
> +c->log = NULL;
> +c->aborted = 0;
> +c->keepalives = 0;
> +
> +/* FIXME: mpms (and maybe other) parts think that there is always
> + * a socket for a connection. We cannot use the master socket for
> + * secondary connections, as this gets modified (closed?) when
> + * the secondary connection terminates.
> + * There seem to be some checks for c->master necessary in other
> + * places.
> + */
> +conn_config = apr_pcalloc(pool, sizeof(*conn_config));
> +conn_config->socket = dummy_socket;
> +ap_set_core_module_config(c->conn_config, conn_config);
> +
> +return c;
> +}
> +
>  static int core_pre_connection(conn_rec *c, void *csd)
>  {
>  conn_config_t *conn_config;


Regards

Rüdiger


Re: [VOTE] Release httpd-2.4.53-rc2 as httpd-2.4.53

2022-03-11 Thread Ruediger Pluem



On 3/11/22 6:00 PM, Stefan Eissing wrote:
> Thank you all! With 7 +1 votes and no other, I announce rc2 has passed.
> 
> I will do the release of 2.4.53 on Monday.
> 
> Kind Regards and a nice weekend to you all,

You too and thanks for RM'ing.

Regards

Rüdiger



Re: [VOTE] Release httpd-2.4.53-rc2 as httpd-2.4.53

2022-03-11 Thread Ruediger Pluem



On 3/11/22 1:14 PM, Stefan Eissing wrote:
> While the 72 hours have not all passed yet, the votes so far are positive.
> 
> Given that no -1 vote will come, I call for a vote on the release time:
> 
> [ ] +1: release it later today, it needs to get out!
> [ ] +0: do not really care
> [ ] -1: better do it Monday, Admins have a life, too, you know?

I would be in favor of doing it on Monday, but if others see a need for today I 
would be fine with this as well.

Regards

Rüdiger



Re: About our autoconf problems

2022-03-10 Thread Ruediger Pluem



On 3/10/22 9:09 AM, Rainer Jung wrote:
> First thanks to Stefan for downgrading his release system. It is a good 
> workaround for the observed problems.
> 
> Since downgrades are not a long term solution, here's the status info to 
> avoid confusion.
> 
> We observed two problems that had to do with using a recent autoconf version 
> (2.70 an above) during releasing:
> 
> - bundled APR configure broken (ongoing)
> 
> This is already fixed by adjusting one of our autoconf macros in APR 1.7.x 
> and trunk (I think not in 1.6.x). But we didn't have an
> APR release since a long time, so the deps tarball for httpd still contains 
> the latest but unfixed APR version. Once we will have
> a new APR release und bundle that, this autoconf problem will be gone and the 
> latest autoconf should again be fine.
> 
> - thread local detection broken in httpd.h (fixed)
> 
> This is not a configure time problem, but a compile time problem. But it is 
> still related to configure, because since autoconf
> 2.70 our call to AC_PROG_CC also checks whether the compiler support C11. For 
> some gcc versions, eg. 4.8.x, this leads in
> "-std=gnu11" being added to the compiler flags. This in combination with 
> httpd.h in 2.4.53-rc1 lead to a compile time failure.

But isn't this a bug in autoconf to add this as 4.8.x does not implement C11? 
Or should we blame it on gcc 4.8.x to accept
-std=gnu11 if it doesn't implement this?
Irrespective of the answers to the the questions above it is good that we have 
the fix in httpd.h.

Regards

Rüdiger


Re: [VOTE] Release httpd-2.4.53-rc1 as httpd-2.4.53

2022-03-09 Thread Ruediger Pluem



On 3/9/22 2:46 PM, Stefan Eissing wrote:
> Are we ready for an rc2 for 2.4.53?

With r1898786 I would say yes.

Regards

Rüdiger



Re: backports

2022-03-09 Thread Ruediger Pluem



On 3/8/22 4:15 PM, Stefan Eissing wrote:
> 
> 
>> Am 08.03.2022 um 14:34 schrieb Jim Jagielski :
>>
>>> On Mar 8, 2022, at 7:58 AM, Graham Leggett  wrote:
>>>
>>>
>>> I would far rather the empty APLOGNO check was part of the build.
>>>
>>> Vastly simpler.
>>>
>>
>> I agree w/ that...
> 
> I have the feeling that the work that has went into making our
> tests run on travis is not sufficiently honoured in this discussion.
> 
> Looking back on the last 6 years I participated here, the situation
> now is *vastly* improved to what we had before. For me, the Travis CI 
> status is now *the* objective status of healthiness of our branches.
> Not because I am a Travis fanboy, but because we had nothing in the
> project before that was available to every team member.
> 
> Speaking as RM, I will *never* make a release on a branch that is
> not "green". That would be unprofessional. As it is not the task
> of an RM to make it green, it already needs to be that way.
> 
> As a developer, I very much prefer to commit to a "green" branch and
> see that it stays green with my changes. On a red branch, this becomes
> harder to analyse. The recent example of the async handshake showed
> that it become more and more difficult to isolate the breaking 
> changes and progress for everyone was hindered by this.
> 
> Now, that does not mean Travis CI is the end of all efforts. But
> it is a good start and it is worth expanding on, IMO.
> 
> Also we need to realise that aligning our way of working more to
> what is now common practise in the field actually *lowers* the 
> barriers for participation.
> 

+1 to this. While I agree that the ultimate decision on merging or not should 
be with us and that
there should be no blocking from Github in the end, I can only imagine very 
rare conditions where
merging a PR that fails the checks makes sense. Mainly in case of time pressure 
due to a security release
and the failure is known to be unrelated.
Otherwise I would strongly encourage to either fix the test that fails or the 
code that causes this test to fail.
Since we use the CI on every commit much effort was done to clean our test 
suite such that we no longer
have these "yeah I know it fails for ages for unknown reasons, but I don't 
care" tests any longer.

I understand and appreciate the concerns that we don't want to force people to 
create a Github account if they don't like.
>From my point of view this is exactly what gitbox is for when it comes to 
>committing. With regards to workflows though we could
still use the good old STATUS file for backport decisions, but I am not sure if 
most people would like to continue using it if
we would have a voting workflow on PR's (which we don't have yet, but which 
looks like to be doable).
I guess we should have an eye on whether we can somehow export all the data in 
addition to the code itself (issues, PR's, etc.)
from Github such that there would be a chance to retain that data in case 
Github pulls the plug.

With regards to the dependency on services like Github and Travis:
Well as far as I can see there is already a trend in infra to move to SaaS 
services, Ponymail, Jira and Confluence come to my
mind, that are already moved there or where there are plans for doing so. I 
guess we need to say a little bit goodbye to an all
"onprem" infrastructure purely created from open source where noone can change 
conditions such that we cannot use them any longer
or where we can quickly move to something else.
Hence if Github and / or Travis would switch to whatever unacceptable 
conditions short term this would have a huge impact on the
ASF and also already on this project. But we would still have access to the 
code via gitbox.

Regards

Rüdiger


Re: backports

2022-03-09 Thread Ruediger Pluem



On 3/9/22 1:11 PM, Yann Ylavic wrote:
> On Tue, Mar 8, 2022 at 2:34 PM Jim Jagielski  wrote:
>>
>>> On Mar 8, 2022, at 7:58 AM, Graham Leggett  wrote:
>>>
>>> I would far rather the empty APLOGNO check was part of the build.
>>>
>>> Vastly simpler.
>>
>>
>> I agree w/ that...
> 
> I for one prefer post processing of APLOGNO()s, if `make` started to
> fail because of that while coding (i.e. while adding/removing
> APLOGNO()s when "playing" with the code) it would be a pain IMO.
> So I prefer our ci failing occasionally because one[1] forgot a final
> `make update-log-tags` (the fix is easy and no one is really blocked
> by this in the meantime) rather than more work with APLOGNO()s while
> coding.

Big +1 to this.

Regards

Rüdiger



Re: [VOTE] Release httpd-2.4.53-rc1 as httpd-2.4.53

2022-03-09 Thread Ruediger Pluem



On 3/9/22 11:34 AM, Rainer Jung wrote:
> Am 09.03.2022 um 08:37 schrieb Ruediger Pluem:
>>
>>
>> On 3/8/22 10:09 PM, Rainer Jung wrote:
>>>
>>
>>>
>>> You gcc 4.8 workaround for _Thread_local still looks good.
>>>
>>> Solaris builds and all unit tests not yet done but compiles fine for all my 
>>> Linuxes.
>>>
>>> Thanks!
>>
>> Thanks for testing. I committed to trunk as r1898771 to throw it into our 
>> CI. If we need to tweak it further we can do later.
>>
>> BTW: Once we have a final patch the same needs to be done for APR trunk and 
>> APR 1.8.x.
> 
> Interesting: I did also compile APR trunk in my httpd release preparations 
> but didn't observe the failure. This probably was due
> to the fact, that our APR trunk configure does not decide to add -std=gnu11, 
> so the test in the header file doesn't result in
> _Thread_local being used.
> 
> Of course it still makes sense to apply the same patch (and also to other APR 
> branches containing the thread local code). Just
> wondering, whether the different handling of the"std" compiler flag for httpd 
> and apr trunk is intentional.
> 

Weird. I don't see -std=gnu11 added at all to my httpd build on RedHat 8.

gcc --version
gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-4)
autoconf --version
autoconf (GNU Autoconf) 2.69

But I have tested the patch and get AP_THREAD_LOCAL set to _Thread_local as it 
should.

Regards

Rüdiger


Re: [VOTE] Release httpd-2.4.53-rc1 as httpd-2.4.53

2022-03-08 Thread Ruediger Pluem



On 3/8/22 10:09 PM, Rainer Jung wrote:
> 

>>
>> Two additional things I already saw, but which are non.critical:
>>
>> - configure flag --with-pcre no longer accepts an external pcre 8 
>> installation directory. It seems in that case the flag must
>> now point to the path of the pcre-config script instead.

Hmm. This could break existing configure calls. Probably not good for a patch 
release. Thanks for pointing out.

Regards

Rüdiger



Re: [VOTE] Release httpd-2.4.53-rc1 as httpd-2.4.53

2022-03-08 Thread Ruediger Pluem



On 3/8/22 10:09 PM, Rainer Jung wrote:
> 

> 
> You gcc 4.8 workaround for _Thread_local still looks good.
> 
> Solaris builds and all unit tests not yet done but compiles fine for all my 
> Linuxes.
> 
> Thanks!

Thanks for testing. I committed to trunk as r1898771 to throw it into our CI. 
If we need to tweak it further we can do later.

BTW: Once we have a final patch the same needs to be done for APR trunk and APR 
1.8.x.

Regards

Rüdiger



Re: [VOTE] Release httpd-2.4.53-rc1 as httpd-2.4.53

2022-03-08 Thread Ruediger Pluem



On 3/8/22 4:38 PM, Rainer Jung wrote:
> Am 08.03.2022 um 16:33 schrieb Rainer Jung:
>>
>> Am 07.03.2022 um 16:55 schrieb Stefan Eissing:
>>> Hi all,
>>>
>>> Please find below the proposed release tarball and signatures:
>>>
>>> https://dist.apache.org/repos/dist/dev/httpd/
>>>
>>> I would like to call a VOTE over the next few days to release
>>> this candidate tarball httpd-2.4.53-rc1 as 2.4.53:
>>> [ ] +1: It's not just good, it's good enough!
>>> [ ] +0: Let's have a talk.
>>> [ ] -1: There's trouble in paradise. Here's what's wrong.
>>>
>>> The computed digests of the tarball up for vote are:
>>> sha256: 7559255a37adc5cb6f568ba224e435420f0a138cd9564162c9528b8ac08737b3 
>>> *httpd-2.4.53-rc1.tar.gz
>>> sha512:
>>> a7734e2fa35389678be74bbc18136eef482ff9daecc2c1ed9c2c5eb410182735a94ff6ad248d0d4b6266e90161f2f8052d4871f381723c996ace0412b0219961
>>>  *httpd-2.4.53-rc1.tar.gz
>>>
>>>
>>> The SVN candidate source is found at tags/2.4.53-rc1-candidate.
>>>
>>> Kind Regards,
>>> Stefan
>>
>> Not a vote yet, just an observation: on SLES12 (gcc 4.8.3, glibc 2.19) and 
>> RHEL7 (gcc 4.8.2, glibc 2.19) with platform
>> compilers, configure detects it wants to set -std=gnu11. During make I get a 
>> compilation error:
>>
>> server/util.c:3183:1: error: unknown type name ‘_Thread_local’
>>
>> The type "_Thread_local" comes from include/httpd.h:
>>
>>    2438  #if defined(__cplusplus) && __cplusplus >= 201103L
>>    2439  #define AP_THREAD_LOCAL thread_local
>>    2440  #elif defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112
>>    2441  #define AP_THREAD_LOCAL _Thread_local
>> 
>>    2442  #elif defined(__GNUC__) /* works for clang too */
>>    2443  #define AP_THREAD_LOCAL __thread
>>    2444  #elif defined(WIN32) && defined(_MSC_VER)
>>    2445  #define AP_THREAD_LOCAL __declspec(thread)
>>    2446  #endif
>>    2447  #endif /* ndef AP_NO_THREAD_LOCAL */
>>
>> It looks like the detection is broken?
> 
> See also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=203066

Thanks for the pointer. Does the below fix it?

Index: include/httpd.h
===
--- include/httpd.h (revision 1898730)
+++ include/httpd.h (working copy)
@@ -2587,7 +2587,9 @@
  */
 #if defined(__cplusplus) && __cplusplus >= 201103L
 #define AP_THREAD_LOCAL thread_local
-#elif defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112
+#elif defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112 && \
+  (!defined(__GNUC__) || \
+  __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 9))
 #define AP_THREAD_LOCAL _Thread_local
 #elif defined(__GNUC__) /* works for clang too */
 #define AP_THREAD_LOCAL __thread


Regards

Rüdiger


Re: [VOTE] Release httpd-2.4.53-rc1 as httpd-2.4.53

2022-03-08 Thread Ruediger Pluem



On 3/8/22 12:57 PM, Joe Orton wrote:
> On Mon, Mar 07, 2022 at 04:55:54PM +0100, Stefan Eissing wrote:
>> Hi all,
>>
>> Please find below the proposed release tarball and signatures:
>>
>> https://dist.apache.org/repos/dist/dev/httpd/
>>
>> I would like to call a VOTE over the next few days to release
>> this candidate tarball httpd-2.4.53-rc1 as 2.4.53:
>> [X] +1: It's not just good, it's good enough!
>> [ ] +0: Let's have a talk.
>> [ ] -1: There's trouble in paradise. Here's what's wrong.
>>
>> The computed digests of the tarball up for vote are:
>> sha256: 7559255a37adc5cb6f568ba224e435420f0a138cd9564162c9528b8ac08737b3 
>> *httpd-2.4.53-rc1.tar.gz
>> sha512: 
>> a7734e2fa35389678be74bbc18136eef482ff9daecc2c1ed9c2c5eb410182735a94ff6ad248d0d4b6266e90161f2f8052d4871f381723c996ace0412b0219961
>>  *httpd-2.4.53-rc1.tar.gz
>>
>> The SVN candidate source is found at tags/2.4.53-rc1-candidate.
> 
> +1 for release and thanks for RMing.  Passes test suite on Fedora 35, 
> RHEL 8, and 9 Beta(ish).
> 
> I got a new "may be uninitialized" warning with with the GCC 12 shapshot 
> used in Fedora 36 (which is still under development and can be 
> unreliable). I think it's unreachable, if we enter here:
> 
> https://github.com/apache/httpd/blob/trunk/modules/lua/lua_request.c#L244
> 
> r->remaining must be > 0 and hence length > rpos is guaranteed and the 
> loop will iterate at least once.

I agree, but I am also fine to init len_read to -1 to avoid this. But I think 
this is not a blocker.
Thanks for bringing it to attention. r1898731.


Regards

Rüdiger



Re: backports

2022-03-08 Thread Ruediger Pluem



On 3/7/22 12:23 PM, Joe Orton wrote:

> 
> AIUI you can configure github to allow merges even if tests fail, though 
> I'm not familiar with that, has anybody played with the settings there?
> 

Haven't played with them, but the below looks like a good starting point:

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests

Regards

Rüdiger


Re: svn commit: r1898586 - in /httpd/httpd/branches/2.4.x: ./ changes-entries/pr65881.txt modules/http2/h2_request.c test/modules/http2/env.py test/modules/http2/htdocs/cgi/hello.py test/modules/http2

2022-03-04 Thread Ruediger Pluem



On 3/4/22 10:50 AM, Stefan Eissing wrote:
> 
> 
>> Am 04.03.2022 um 10:22 schrieb Ruediger Pluem :
>>
>>
>>
>> On 3/4/22 9:51 AM, ic...@apache.org wrote:
>>> Author: icing
>>> Date: Fri Mar  4 08:51:47 2022
>>> New Revision: 1898586
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1898586=rev
>>> Log:
>>> merge of 1898146,1898173 from trunk:
>>>
>>>  *) mod_http2: preserve the port number given in a HTTP/1.1
>>> request that was Upgraded to HTTP/2. Fixes PR65881.
>>>
>>>
>>> Added:
>>>httpd/httpd/branches/2.4.x/changes-entries/pr65881.txt
>>>  - copied unchanged from r1898146, 
>>> httpd/httpd/trunk/changes-entries/pr65881.txt
>>>httpd/httpd/branches/2.4.x/test/modules/http2/test_502_proxy_port.py
>>>  - copied unchanged from r1898146, 
>>> httpd/httpd/trunk/test/modules/http2/test_502_proxy_port.py
>>> Modified:
>>>httpd/httpd/branches/2.4.x/   (props changed)
>>>httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
>>>httpd/httpd/branches/2.4.x/test/modules/http2/env.py
>>>httpd/httpd/branches/2.4.x/test/modules/http2/htdocs/cgi/hello.py
>>>
>>> Propchange: httpd/httpd/branches/2.4.x/
>>> --
>>>  Merged /httpd/httpd/trunk:r1898146,1898173
>>>
>>> Modified: httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
>>> URL: 
>>> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/http2/h2_request.c?rev=1898586=1898585=1898586=diff
>>> ==
>>> --- httpd/httpd/branches/2.4.x/modules/http2/h2_request.c (original)
>>> +++ httpd/httpd/branches/2.4.x/modules/http2/h2_request.c Fri Mar  4 
>>> 08:51:47 2022
>>> @@ -69,12 +69,25 @@ apr_status_t h2_request_rcreate(h2_reque
>>> return APR_EINVAL;
>>> }
>>>
>>> -if (!ap_strchr_c(authority, ':') && r->server && r->server->port) {
>>> -apr_port_t defport = apr_uri_port_of_scheme(scheme);
>>> -if (defport != r->server->port) {
>>> -/* port info missing and port is not default for scheme: 
>>> append */
>>> -authority = apr_psprintf(pool, "%s:%d", authority,
>>> - (int)r->server->port);
>>> +/* The authority we carry in h2_request is the 'authority' part of
>>> + * the URL for the request. r->hostname has stripped any port info that
>>> + * might have been present. Do we need to add it?
>>> + */
>>> +if (!ap_strchr_c(authority, ':')) {
>>> +if (r->parsed_uri.port_str) {
>>> +/* Yes, it was there, add it again. */
>>> +authority = apr_pstrcat(pool, authority, ":", 
>>> r->parsed_uri.port_str, NULL);
>>> +}
>>> +else if (!r->parsed_uri.hostname && r->server && r->server->port) {
>>> +/* If there was no hostname in the parsed URL, the URL was 
>>> relative.
>>> + * In that case, we restore port from our server->port, if it
>>> + * is known and not the default port for the scheme. */
>>> +apr_port_t defport = apr_uri_port_of_scheme(scheme);
>>> +if (defport != r->server->port) {
>>> +/* port info missing and port is not default for scheme: 
>>> append */
>>> +authority = apr_psprintf(pool, "%s:%d", authority,
>>> + (int)r->server->port);
>>> +}
>>> }
>>> }
>>
>> Sorry for my late comment, but I think the above ignores the setting of 
>> UseCanonicalPhysicalPort and UseCanonicalName.
>>
>> I think we should add what is returned by ap_get_server_port in case this 
>> differs from apr_uri_port_of_scheme(scheme)
>>
>> I think of something like the below:
>>
>> Index: modules/http2/h2_request.c
>> ===
>> --- modules/http2/h2_request.c   (revision 1898509)
>> +++ modules/http2/h2_request.c   (working copy)
>> @@ -95,21 +95,13 @@
>>  * might have been present. Do we need to add it?
>>  */
>> if (!ap_strchr_c(authori

Re: svn commit: r1898586 - in /httpd/httpd/branches/2.4.x: ./ changes-entries/pr65881.txt modules/http2/h2_request.c test/modules/http2/env.py test/modules/http2/htdocs/cgi/hello.py test/modules/http2

2022-03-04 Thread Ruediger Pluem



On 3/4/22 9:51 AM, ic...@apache.org wrote:
> Author: icing
> Date: Fri Mar  4 08:51:47 2022
> New Revision: 1898586
> 
> URL: http://svn.apache.org/viewvc?rev=1898586=rev
> Log:
> merge of 1898146,1898173 from trunk:
> 
>   *) mod_http2: preserve the port number given in a HTTP/1.1
>  request that was Upgraded to HTTP/2. Fixes PR65881.
> 
> 
> Added:
> httpd/httpd/branches/2.4.x/changes-entries/pr65881.txt
>   - copied unchanged from r1898146, 
> httpd/httpd/trunk/changes-entries/pr65881.txt
> httpd/httpd/branches/2.4.x/test/modules/http2/test_502_proxy_port.py
>   - copied unchanged from r1898146, 
> httpd/httpd/trunk/test/modules/http2/test_502_proxy_port.py
> Modified:
> httpd/httpd/branches/2.4.x/   (props changed)
> httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
> httpd/httpd/branches/2.4.x/test/modules/http2/env.py
> httpd/httpd/branches/2.4.x/test/modules/http2/htdocs/cgi/hello.py
> 
> Propchange: httpd/httpd/branches/2.4.x/
> --
>   Merged /httpd/httpd/trunk:r1898146,1898173
> 
> Modified: httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/http2/h2_request.c?rev=1898586=1898585=1898586=diff
> ==
> --- httpd/httpd/branches/2.4.x/modules/http2/h2_request.c (original)
> +++ httpd/httpd/branches/2.4.x/modules/http2/h2_request.c Fri Mar  4 08:51:47 
> 2022
> @@ -69,12 +69,25 @@ apr_status_t h2_request_rcreate(h2_reque
>  return APR_EINVAL;
>  }
>  
> -if (!ap_strchr_c(authority, ':') && r->server && r->server->port) {
> -apr_port_t defport = apr_uri_port_of_scheme(scheme);
> -if (defport != r->server->port) {
> -/* port info missing and port is not default for scheme: append 
> */
> -authority = apr_psprintf(pool, "%s:%d", authority,
> - (int)r->server->port);
> +/* The authority we carry in h2_request is the 'authority' part of
> + * the URL for the request. r->hostname has stripped any port info that
> + * might have been present. Do we need to add it?
> + */
> +if (!ap_strchr_c(authority, ':')) {
> +if (r->parsed_uri.port_str) {
> +/* Yes, it was there, add it again. */
> +authority = apr_pstrcat(pool, authority, ":", 
> r->parsed_uri.port_str, NULL);
> +}
> +else if (!r->parsed_uri.hostname && r->server && r->server->port) {
> +/* If there was no hostname in the parsed URL, the URL was 
> relative.
> + * In that case, we restore port from our server->port, if it
> + * is known and not the default port for the scheme. */
> +apr_port_t defport = apr_uri_port_of_scheme(scheme);
> +if (defport != r->server->port) {
> +/* port info missing and port is not default for scheme: 
> append */
> +authority = apr_psprintf(pool, "%s:%d", authority,
> + (int)r->server->port);
> +}
>  }
>  }

Sorry for my late comment, but I think the above ignores the setting of 
UseCanonicalPhysicalPort and UseCanonicalName.

I think we should add what is returned by ap_get_server_port in case this 
differs from apr_uri_port_of_scheme(scheme)

I think of something like the below:

Index: modules/http2/h2_request.c
===
--- modules/http2/h2_request.c  (revision 1898509)
+++ modules/http2/h2_request.c  (working copy)
@@ -95,21 +95,13 @@
  * might have been present. Do we need to add it?
  */
 if (!ap_strchr_c(authority, ':')) {
-if (r->parsed_uri.port_str) {
-/* Yes, it was there, add it again. */
-authority = apr_pstrcat(pool, authority, ":", 
r->parsed_uri.port_str, NULL);
+apr_port_t defport = apr_uri_port_of_scheme(scheme);
+apr_port_t port = ap_get_server_port(r);
+
+if (defport != port) {
+/* port info missing and port is not default for scheme: append */
+authority = apr_psprintf(pool, "%s:%d", authority, (int)port);
 }
-else if (!r->parsed_uri.hostname && r->server && r->server->port) {
-/* If there was no hostname in the parsed URL, the URL was 
relative.
- * In that case, we restore port from our server->port, if it
- * is known and not the default port for the scheme. */
-apr_port_t defport = apr_uri_port_of_scheme(scheme);
-if (defport != r->server->port) {
-/* port info missing and port is not default for scheme: 
append */
-authority = apr_psprintf(pool, "%s:%d", authority,
- (int)r->server->port);
-}
-}
 }

 

Re: svn commit: r1898566 - in /httpd/httpd/branches/2.4.x: ./ modules/aaa/ modules/cache/ modules/dav/fs/ modules/dav/lock/ modules/mappers/ modules/proxy/

2022-03-04 Thread Ruediger Pluem



On 3/4/22 9:24 AM, Stefan Eissing wrote:
> 
> 
>> Am 04.03.2022 um 08:32 schrieb Ruediger Pluem :
>>
>>
>>
>> On 3/3/22 5:40 PM, Joe Orton wrote:
>>> On Thu, Mar 03, 2022 at 05:11:52PM +0100, Ruediger Pluem wrote:
>>>> On 3/3/22 4:49 PM, Joe Orton wrote:
>>>>> Folks (in no way pointing a finger at Jim who just did merging duty), it 
>>>>> is not hard to test your backport proposals, either in an SVN branch or 
>>>>> a github PR if you want better testing coverage before you submit for 
>>>>> review.
>>>>
>>>> A quick question on this. If I branch 2.4.x
>>>>
>>>> 1. Travis will run at all (because their is a .travis.yml in that branch)?
>>>
>>> Yup, Travis will definitely run for all branches, e.g. it works for the 
>>> candidate-2.4.x branches:
>>>
>>> https://app.travis-ci.com/github/apache/httpd/branches
>>>
>>>> 2. But the conditions in .travis.yml will likely not cause travis to run 
>>>> the same tests as for 2.4.x, but likely the trunk ones,
>>>>   correct? Hence we need adjusted conditions in .travis.yml and we need to 
>>>> define some kind of naming rules for branches from
>>>>   trunk and 2.4.x to ensure that the correct tests and builds are running?
>>>
>>> Oh, good question.  I'm not sure how the "branch" variable appears in an 
>>> arbitrary branch but it's possible we'd need to tweak the conditions 
>>> again, yes.  If we used a naming rule of "branches/2.4.x-*" for 2.4.x 
>>> backports would that be reasonable?  This is most common from examples
>>
>> Sounds reasonable, but given that for candidates we use candidate-2.4.x we 
>> should change this to 2.4.x-candidate if we set a
>> naming convention of branches/2.4.x-*.
> 
> I can change that easily, but the pattern be better: branches/2.4.* since the 
> candidate carries the to be released version, not 2.4.x (the name 
> branches/2.4.x-candidate-2.4.54 is silly and I refuse to go there -.-)

Yeah, let's start a fierce naming discussion :-). No seriously: branches/2.4.* 
is absolutely fine for me. We just need to align
changes to the release scripts and .travis.yml.

Regards

Rüdiger



Re: svn commit: r1898566 - in /httpd/httpd/branches/2.4.x: ./ modules/aaa/ modules/cache/ modules/dav/fs/ modules/dav/lock/ modules/mappers/ modules/proxy/

2022-03-03 Thread Ruediger Pluem



On 3/3/22 5:40 PM, Joe Orton wrote:
> On Thu, Mar 03, 2022 at 05:11:52PM +0100, Ruediger Pluem wrote:
>> On 3/3/22 4:49 PM, Joe Orton wrote:
>>> Folks (in no way pointing a finger at Jim who just did merging duty), it 
>>> is not hard to test your backport proposals, either in an SVN branch or 
>>> a github PR if you want better testing coverage before you submit for 
>>> review.
>>
>> A quick question on this. If I branch 2.4.x
>>
>> 1. Travis will run at all (because their is a .travis.yml in that branch)?
> 
> Yup, Travis will definitely run for all branches, e.g. it works for the 
> candidate-2.4.x branches:
> 
> https://app.travis-ci.com/github/apache/httpd/branches
> 
>> 2. But the conditions in .travis.yml will likely not cause travis to run the 
>> same tests as for 2.4.x, but likely the trunk ones,
>>correct? Hence we need adjusted conditions in .travis.yml and we need to 
>> define some kind of naming rules for branches from
>>trunk and 2.4.x to ensure that the correct tests and builds are running?
> 
> Oh, good question.  I'm not sure how the "branch" variable appears in an 
> arbitrary branch but it's possible we'd need to tweak the conditions 
> again, yes.  If we used a naming rule of "branches/2.4.x-*" for 2.4.x 
> backports would that be reasonable?  This is most common from examples

Sounds reasonable, but given that for candidates we use candidate-2.4.x we 
should change this to 2.4.x-candidate if we set a
naming convention of branches/2.4.x-*.

Regards

Rüdiger


Re: svn commit: r1898566 - in /httpd/httpd/branches/2.4.x: ./ modules/aaa/ modules/cache/ modules/dav/fs/ modules/dav/lock/ modules/mappers/ modules/proxy/

2022-03-03 Thread Ruediger Pluem



On 3/3/22 6:47 PM, Jim Jagielski wrote:
> Do we have a users guide for this specific implementation and setup? TIA!

Have a look here:

http://svn.apache.org/viewvc/httpd/httpd/trunk/test/README.travis?view=markup
http://svn.apache.org/viewvc/httpd/dev-tools/github/README?view=markup

Regards

Rüdiger




Re: svn commit: r1898566 - in /httpd/httpd/branches/2.4.x: ./ modules/aaa/ modules/cache/ modules/dav/fs/ modules/dav/lock/ modules/mappers/ modules/proxy/

2022-03-03 Thread Ruediger Pluem



On 3/3/22 4:49 PM, Joe Orton wrote:
> On Thu, Mar 03, 2022 at 01:30:47PM -, j...@apache.org wrote:
>> Author: jim
>> Date: Thu Mar  3 13:30:46 2022
>> New Revision: 1898566
>>
>> URL: http://svn.apache.org/viewvc?rev=1898566=rev
>> Log:
>> dbm backport approved and merged
> 
> This has broken the CI with several new warnings and empty APLOGNO()
> 
> https://app.travis-ci.com/github/apache/httpd/builds/247346699
> 
> Folks (in no way pointing a finger at Jim who just did merging duty), it 
> is not hard to test your backport proposals, either in an SVN branch or 
> a github PR if you want better testing coverage before you submit for 
> review.

A quick question on this. If I branch 2.4.x

1. Travis will run at all (because their is a .travis.yml in that branch)?
2. But the conditions in .travis.yml will likely not cause travis to run the 
same tests as for 2.4.x, but likely the trunk ones,
   correct? Hence we need adjusted conditions in .travis.yml and we need to 
define some kind of naming rules for branches from
   trunk and 2.4.x to ensure that the correct tests and builds are running?

Regards

Rüdiger



Re: module threads and mpm lifecycle

2022-02-23 Thread Ruediger Pluem



On 2/23/22 11:39 AM, Stefan Eissing wrote:
> One thing that is currently missing is a way to shutdown/reap/join module 
> threads when the mpm exits.
> 
> Example: 
> 
> mod_watchdog creates several threads post_config which are only joined on 
> pool destruction.
> 
> Problem:
> 
> pool destruction is not ordered between modules and dependencies on order are 
> not
> fully known. This leads to crashes in OpenSSL, for example, when mod_ssl is 
> destructed
> before all watchdogs using OpenSSL are joined (OpenSSL 3.x seems to do better 
> on this, 
> but the point remains valid).
> 
> There has been attempts by Yann to make a patch that make the OpenSSL 
> termination
> way later (adding it to a 'higher' pool destruction). But that would only 
> solve
> this particular problem and not any other 3rd party dependency.
> 
> Proposal:
> 
> Add a hook 'child_exiting(int graceful)' where modules can register
> and do their own thread join/reap.

How does that differ from the child_stopping hook? Or is child_stopping only 
used when we shutdown the whole httpd not just one child?

Regards

Rüdiger


Re: svn commit: r1898286 - /httpd/httpd/trunk/server/util_expr_eval.c

2022-02-21 Thread Ruediger Pluem



On 2/21/22 10:07 PM, jaillet...@apache.org wrote:
> Author: jailletc36
> Date: Mon Feb 21 21:07:35 2022
> New Revision: 1898286
> 
> URL: http://svn.apache.org/viewvc?rev=1898286=rev
> Log:
> There is no point in calling ap_varbuf_grow() here, it is already
> called from within ap_varbuf_strmemcat().
> 
> Moreover, 2nd parameter should be the minimum total new length, not
> the amount of the growth. So this call is likely to be a no-op.

I agree that the current usage of ap_varbuf_grow is wrong as it should be the 
complete length and not the growth.
OTOH the idea seems to be to avoid too much reallocations and memcpys here.
We could precalculate the length, but I am not sure if this creates more 
overhead than needed here. If we precalculate the length
the question is also if the ap_varbuf_ is still the best api for this or we 
should switch to apr_pstrcatv instead to avoid
having to strlen all elements of the list twice. Of course this costs a little 
more memory for the struct iovec array.

Regards

Rüdiger




  1   2   3   4   5   6   7   8   9   10   >