Re: svn commit: r1918003 - in /httpd/httpd/trunk/modules/http2: h2_c1.c h2_c2.c h2_mplx.c h2_mplx.h h2_session.c h2_session.h h2_version.h

[Yann Ylavic]

On Tue, May 28, 2024 at 4:06 PM Stefan Eissing via dev
 wrote:
>
>
>
> > Am 28.05.2024 um 15:45 schrieb Yann Ylavic :
> >
> > On Tue, May 28, 2024 at 12:47 PM Stefan Eissing via dev
> >  wrote:
> >>
> >>> Am 27.05.2024 um 14:08 schrieb Yann Ylavic :
> >>>
> >>> Per our discussion the other day, if you want to avoid c1 connections
> >>> to be killed by mpm_event on high load in this case, I think you can
> >>> do this here:
> >>>
> >>> Index: modules/http2/h2_c1.c
> >>> ===
> >>> --- modules/http2/h2_c1.c(revision 1918003)
> >>> +++ modules/http2/h2_c1.c(working copy)
> >>> @@ -147,6 +147,7 @@ apr_status_t h2_c1_run(conn_rec *c)
> >>> && mpm_state != AP_MPMQ_STOPPING);
> >>>
> >>>if (c->cs) {
> >>> +c->clogging_input_filters = 0;
> >>>switch (conn_ctx->session->state) {
> >>>case H2_SESSION_ST_INIT:
> >>>case H2_SESSION_ST_IDLE:
> >>> @@ -159,6 +160,7 @@ apr_status_t h2_c1_run(conn_rec *c)
> >>> * See PR 63534.
> >>> */
> >>>c->cs->sense = CONN_SENSE_WANT_READ;
> >>> +c->clogging_input_filters = 1;
> >>>03465}
> >>>break;
> >>>case H2_SESSION_ST_CLEANUP:
> >>> --
> >>>
> >>> c->clogging_input_filters = 1 will tell the MPM to always call
> >>> process_connection() hooks after the WRITE_COMPLETION state did the
> >>> poll(), rather than entering the (killable) keepalive state.
> >>>
> >>> Looks like the correct workaround with current mpm_event..
> >>
> >> Just so I get this right. It will return to processing after the
> >> write is done or after a POLLIN happened? In the first case, it
> >> will not have really a positive effect on worker allocations,
> >> seems to me.
> >
> > Yes, it will return to processing after POLLIN happens thanks to
> > CONN_SENSE_WANT_READ, even if there are pending output data.
> > But it's a really convoluted handling of POLLIN/POLLOUT in mpm_event,
> > with the need of that obscure c->clogging_input_filters..
> > So I just created https://github.com/apache/httpd/pull/448 to do that
> > better (and it includes the changes to h2_c1_run() too).
> > The plan could be to merge that to trunk and include it in your
> > backport proposal (if it works for you)?
> >
>
> Great!

Backport PR in https://github.com/apache/httpd/pull/449 ;)


Regards;
Yann.

Re: svn commit: r1918003 - in /httpd/httpd/trunk/modules/http2: h2_c1.c h2_c2.c h2_mplx.c h2_mplx.h h2_session.c h2_session.h h2_version.h

[Yann Ylavic]

On Tue, May 28, 2024 at 12:47 PM Stefan Eissing via dev
 wrote:
>
> > Am 27.05.2024 um 14:08 schrieb Yann Ylavic :
> >
> > Per our discussion the other day, if you want to avoid c1 connections
> > to be killed by mpm_event on high load in this case, I think you can
> > do this here:
> >
> > Index: modules/http2/h2_c1.c
> > ===
> > --- modules/http2/h2_c1.c(revision 1918003)
> > +++ modules/http2/h2_c1.c(working copy)
> > @@ -147,6 +147,7 @@ apr_status_t h2_c1_run(conn_rec *c)
> >  && mpm_state != AP_MPMQ_STOPPING);
> >
> > if (c->cs) {
> > +c->clogging_input_filters = 0;
> > switch (conn_ctx->session->state) {
> > case H2_SESSION_ST_INIT:
> > case H2_SESSION_ST_IDLE:
> > @@ -159,6 +160,7 @@ apr_status_t h2_c1_run(conn_rec *c)
> >  * See PR 63534.
> >  */
> > c->cs->sense = CONN_SENSE_WANT_READ;
> > +c->clogging_input_filters = 1;
> > 03465}
> > break;
> > case H2_SESSION_ST_CLEANUP:
> > --
> >
> > c->clogging_input_filters = 1 will tell the MPM to always call
> > process_connection() hooks after the WRITE_COMPLETION state did the
> > poll(), rather than entering the (killable) keepalive state.
> >
> > Looks like the correct workaround with current mpm_event..
>
> Just so I get this right. It will return to processing after the
> write is done or after a POLLIN happened? In the first case, it
> will not have really a positive effect on worker allocations,
> seems to me.

Yes, it will return to processing after POLLIN happens thanks to
CONN_SENSE_WANT_READ, even if there are pending output data.
But it's a really convoluted handling of POLLIN/POLLOUT in mpm_event,
with the need of that obscure c->clogging_input_filters..
So I just created https://github.com/apache/httpd/pull/448 to do that
better (and it includes the changes to h2_c1_run() too).
The plan could be to merge that to trunk and include it in your
backport proposal (if it works for you)?


Regards;
Yann.

Re: svn commit: r1918003 - in /httpd/httpd/trunk/modules/http2: h2_c1.c h2_c2.c h2_mplx.c h2_mplx.h h2_session.c h2_session.h h2_version.h

[Yann Ylavic]

On Mon, May 27, 2024 at 1:04 PM  wrote:
>
> Author: icing
> Date: Mon May 27 11:04:52 2024
> New Revision: 1918003
>
> URL: http://svn.apache.org/viewvc?rev=1918003=rev
> Log:
>  *) mod_http2: sync with module's github.
> - on newer HTTPD versions, return connection monitoring
>   to the event MPM when block on client updates.
>   2.4.x versions still treat connections in the event
>   MPM as KeepAlive and purge them on load in the middle
>   of response processing.
> - spelling fixes
> - support for yield calls in c2 "network" filter
[]
>
> --- httpd/httpd/trunk/modules/http2/h2_c1.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_c1.c Mon May 27 11:04:52 2024
> @@ -116,7 +116,7 @@ cleanup:
>  apr_status_t h2_c1_run(conn_rec *c)
>  {
>  apr_status_t status;
> -int mpm_state = 0;
> +int mpm_state = 0, keepalive = 0;
>  h2_conn_ctx_t *conn_ctx = h2_conn_ctx_get(c);
>
>  ap_assert(conn_ctx);
> @@ -127,7 +127,7 @@ apr_status_t h2_c1_run(conn_rec *c)
>  c->cs->state = CONN_STATE_HANDLER;
>  }
>
> -status = h2_session_process(conn_ctx->session, async_mpm);
> +status = h2_session_process(conn_ctx->session, async_mpm, 
> );
>
>  if (APR_STATUS_IS_EOF(status)) {
>  ap_log_cerror(APLOG_MARK, APLOG_DEBUG, status, c,
> @@ -153,7 +153,7 @@ apr_status_t h2_c1_run(conn_rec *c)
>  case H2_SESSION_ST_BUSY:
>  case H2_SESSION_ST_WAIT:
>  c->cs->state = CONN_STATE_WRITE_COMPLETION;
> -if (c->cs && !conn_ctx->session->remote.emitted_count) {
> +if (!keepalive) {
>  /* let the MPM know that we are not done and want
>   * the Timeout behaviour instead of a KeepAliveTimeout
>   * See PR 63534.

Per our discussion the other day, if you want to avoid c1 connections
to be killed by mpm_event on high load in this case, I think you can
do this here:

Index: modules/http2/h2_c1.c
===
--- modules/http2/h2_c1.c(revision 1918003)
+++ modules/http2/h2_c1.c(working copy)
@@ -147,6 +147,7 @@ apr_status_t h2_c1_run(conn_rec *c)
  && mpm_state != AP_MPMQ_STOPPING);

 if (c->cs) {
+c->clogging_input_filters = 0;
 switch (conn_ctx->session->state) {
 case H2_SESSION_ST_INIT:
 case H2_SESSION_ST_IDLE:
@@ -159,6 +160,7 @@ apr_status_t h2_c1_run(conn_rec *c)
  * See PR 63534.
  */
 c->cs->sense = CONN_SENSE_WANT_READ;
+c->clogging_input_filters = 1;
 }
 break;
 case H2_SESSION_ST_CLEANUP:
--

c->clogging_input_filters = 1 will tell the MPM to always call
process_connection() hooks after the WRITE_COMPLETION state did the
poll(), rather than entering the (killable) keepalive state.

Looks like the correct workaround with current mpm_event..


Regards;
Yann.

Re: more async handling in mod_h2

[Yann Ylavic]

On Tue, May 14, 2024 at 12:21 PM Stefan Eissing via dev
 wrote:
>
> Tried your PR and it works nicely without any changes to current mod_h2.

Great, thanks for testing!

With the current mod_h2 I think the connections returned to mpm_event
use the KeepAliveTimeout rather than Timeout. Enough for the
WINDOW_UPDATE to arrive in your tests maybe if keepalive connections
don't get killed early by the MPM like in httpd-trunk?

>
> Made a test with `StartServers 1` opening 300 HTTP/2 connections and a
> 1KB connection window size. This means writing responses very frequently
> stalls on flow control and returns to the MPM.
>
> With plain trunk, this fails with connections being closed early. With
> PR 294 all are successful, even when throttling `MaxRequestWorkers 25`.

Yeah, the new queuing mechanism (fully nonblocking) and
connections_above_limit() heuristics in PR 294 allow for much better
scheduling with less threads from my testing (not potentially blocking
scenarios obviously).
There is even an "MaxRequestWorkers N auto" which automa[tg]ically
sets ThreadsPerChild to the number of CPUs and the rest accordingly
(Min/MaxSpareThreads, ...) for the N expected concurrent connections.
>From my testing still it seems to behave better than trunk (even with
the usual/more ThreadsPerChild there). Supposedly these heuristics and
auto settings can be improved/fixed from others' thoughts, I'm all
ears ;)

>
> Adding your proposed changes to h2_c1.c also works. Published as 
> https://github.com/icing/mod_h2/pull/281

I think this is what you want (i.e. use Timeout), while with a
min_connection_timeout() hook you could fine-grain that timeout per
h2_c1 state too if it makes sense (see reqtimeout_min_timeout() and
how CONN_STATE_PROCESS uses ap_get_connection_timeout() in mpm_event
to apply any module's min timeout of the moment eventually).

Sorry it sounds a bit like I'm trying to sell my whole PR again :)
Let me see how I can stage to trunk what's useful and meaningful
there, mod_h2 looks like a good candidate to test it after all :p

Also (or as a further note), if for anything early/full connection
level handling (like mod_ssl handshake or mod_h2 c1) the
CONN_STATE_PROCESS/AGAIN mechanism seems usable, I can't find how for
a request handler for instance we could return AGAIN, we'd need to
handle that at too many levels (everything in the call chain would
need to be adapted to recover from the process_connection hook, if
ever possible..).
So like in mod_proxy_wstunnel (or the fallback to mod_proxy_http) and
what happens already in trunk for an upgraded websocket connection,
an/my (medium/long term) plan is for mod_proxy_http's normal HTTP
traffic which would block (either on the client or backend side) to
return to the MPM by maintaining a mod_proxy internal state and using
the ap_mpm_register_poll_callback_timeout()/SUSPENDED mechanism.
Not sure how it'd work/behave for mod_h2's c2_process() though, it
would get ap_process_request() == SUSPENDED meaning that either/both
of the client/beam/pipe or the backend connection are being polled by
the MPM already, and when some event is available on either/both side
it's mod_proxy_http's callback which will be called again to continue
its read/write/forwarding work.
I think the h2 workers are not needed anymore in this scenario, but
how would mod_h2 know that h1 processing is going to be fully async?
Looks like it can't for all httpd configurations (and third party
modules), so the h2 workers would still be there but be prepared to be
SUSPENDED too (i.e. when the MPM calls back mod_proxy_http after
poll()ing it will be from a MPM worker thread, not an h2 worker)?

Well, enough (whishful?) thoughts for tonight (-_-)


Cheers;
Yann.

Re: more async handling in mod_h2

[Yann Ylavic]

On Mon, May 13, 2024 at 9:02 PM Yann Ylavic  wrote:
>
> On Mon, May 13, 2024 at 6:31 PM Stefan Eissing  wrote:
> >
> > > Am 13.05.2024 um 17:41 schrieb Yann Ylavic :
> > >
> > > On Mon, May 13, 2024 at 1:32 PM Stefan Eissing
> > >>
> > >> With the change in PR 280, we return on being flow blocked. The 
> > >> response(s) are *not* finished. If MPM now closes such a connection 
> > >> under load, the client will most likely try again. This seems counter 
> > >> productive.
> > >
> > > AFAICT the CONN_STATE_WRITE_COMPLETION state is different depending on
> > > CONN_SENSE_WANT_READ and CONN_SENSE_WANT_WRITE/DEFAULT.
> > > With CONN_SENSE_WANT_WRITE/DEFAULT, mpm_event will indeed assume flush
> > > (nonblocking) before close,
>
> It's actually a flush before *keepalive* (unless !AP_CONN_KEEPALIVE),
> though it's not what you want anyway.
>
> > > but with CONN_SENSE_WANT_READ it will poll
> > > for read on the connection and come back to the process_connection
> > > hooks when something is in.
> > >
> > > When mod_h2 waits for some WINDOW_UPDATE it wants the MPM to POLLIN
> > > (right?), h2_c1_hook_process_connection() should recover/continue with
> > > the new data from the client. And since it's a c1 connection I don't
> > > think there needs to be some new pollset/queues sizing adjustment to
> > > do either, so we should be good?
> >
> > Yes, mod_h2 does CONN_STATE_WRITE_COMPLETION and CONN_SENSE_WANT_READ and 
> > it works nicely. The only thing is that, when I open many connections, 
> > unfinished ones get dropped very fast. Like after a few milliseconds.
>
> What do you mean by unfinished ones, which state are they in (i.e. how
> do they end up in the MPM with a keepalive state which is the only one
> where connections are "early" killed under high load)?
> I think this can only happen when mpm_event gets a connection with
> c->cs->state == CONN_STATE_WRITE_COMPLETION but c->cs->sense !=
> CONN_SENSE_WANT_READ, is that a mod_h2 possible thing?

Hm, I think I figured this out by myself.
As I said above CONN_STATE_WRITE_COMPLETION leads to
CONN_STATE_CHECK_REQUEST_LINE_READABLE (i.e keepalive state) once the
completion is done (or the POLLIN with c->cs->sense ==
CONN_SENSE_WANT_READ) and if there is no input data already pending.
This is really not what mod_h2 wants but rather
ap_run_process_connection() to always be called after POLLIN.
We could handle that specifically for the CONN_SENSE_WANT_READ case
but it's yet more abuse of CONN_STATE_WRITE_COMPLETION imho.
Let's see how CONN_STATE_PROCESS (from PR 294 below) works to take the
relevant bits in trunk eventually.

>
> > >>
> > >> Therefore I am hesitant if this change is beneficial for us. It frees up 
> > >> a worker thread - that is good - but. Do we need a new "connection 
> > >> state" here?
> > >>
> > >> WDYT?
> > >
> > > I think the semantics of CONN_STATE_WRITE_COMPLETION with
> > > CONN_SENSE_WANT_* are quite unintuitive (for the least), so we
> > > probably should have different states for CONN_STATE_WRITE_COMPLETION
> > > (flush before close) and CONN_STATE_POLL_READ/POLL_WRITE/POLL_RW which
> > > would be how a process_connection hook would return to the MPM just
> > > for poll()ing.
> >
> > I think that would be excellent, yes. Trigger polling with the connection 
> > Timeout value.
>
> There is https://github.com/apache/httpd/pull/294 with quite some
> changes to mpm_event. Sorry this is still WIP and needs to be split
> more, probably in multiple PRs too, but it works for the tests I've
> done so far (I just rebased it on latest trunk).
> This change starts with Graham's AGAIN return code (from
> process_connection hooks) for letting the MPM do the polling for SSL
> handshakes that'd block for instance. It then expanded to a
> CONN_STATE_PROCESS state (renamed from CONN_STATE_READ_REQUEST_LINE)
> where mpm_event receiving AGAIN from ap_run_process_connection() will
> simply poll according to the c->cs->sense value
> (WANT_READ/WANT_WRITE).
> This PR also removes all keepalive kills, though I don't think mod_h2
> should rely on this state?
>
> Could you try running your mod_h2 changes on top of it and with
> h2_c1_run() doing something like:
> c->cs->state = CONN_STATE_PROCESS;
> c->cs->state = CONN_SENSE_WANT_READ; /* or _WRITE */

This should be c->cs->sense = CONN_SENSE_WANT_READ of course..

> return AGAIN;
> where you want mpm_event to simply poll() for read (or write)?
>
>
> Regards;
> Yann.

Re: more async handling in mod_h2

[Yann Ylavic]

On Mon, May 13, 2024 at 6:31 PM Stefan Eissing  wrote:
>
> > Am 13.05.2024 um 17:41 schrieb Yann Ylavic :
> >
> > On Mon, May 13, 2024 at 1:32 PM Stefan Eissing
> >>
> >> With the change in PR 280, we return on being flow blocked. The 
> >> response(s) are *not* finished. If MPM now closes such a connection under 
> >> load, the client will most likely try again. This seems counter productive.
> >
> > AFAICT the CONN_STATE_WRITE_COMPLETION state is different depending on
> > CONN_SENSE_WANT_READ and CONN_SENSE_WANT_WRITE/DEFAULT.
> > With CONN_SENSE_WANT_WRITE/DEFAULT, mpm_event will indeed assume flush
> > (nonblocking) before close,

It's actually a flush before *keepalive* (unless !AP_CONN_KEEPALIVE),
though it's not what you want anyway.

> > but with CONN_SENSE_WANT_READ it will poll
> > for read on the connection and come back to the process_connection
> > hooks when something is in.
> >
> > When mod_h2 waits for some WINDOW_UPDATE it wants the MPM to POLLIN
> > (right?), h2_c1_hook_process_connection() should recover/continue with
> > the new data from the client. And since it's a c1 connection I don't
> > think there needs to be some new pollset/queues sizing adjustment to
> > do either, so we should be good?
>
> Yes, mod_h2 does CONN_STATE_WRITE_COMPLETION and CONN_SENSE_WANT_READ and it 
> works nicely. The only thing is that, when I open many connections, 
> unfinished ones get dropped very fast. Like after a few milliseconds.

What do you mean by unfinished ones, which state are they in (i.e. how
do they end up in the MPM with a keepalive state which is the only one
where connections are "early" killed under high load)?
I think this can only happen when mpm_event gets a connection with
c->cs->state == CONN_STATE_WRITE_COMPLETION but c->cs->sense !=
CONN_SENSE_WANT_READ, is that a mod_h2 possible thing?

> >>
> >> Therefore I am hesitant if this change is beneficial for us. It frees up a 
> >> worker thread - that is good - but. Do we need a new "connection state" 
> >> here?
> >>
> >> WDYT?
> >
> > I think the semantics of CONN_STATE_WRITE_COMPLETION with
> > CONN_SENSE_WANT_* are quite unintuitive (for the least), so we
> > probably should have different states for CONN_STATE_WRITE_COMPLETION
> > (flush before close) and CONN_STATE_POLL_READ/POLL_WRITE/POLL_RW which
> > would be how a process_connection hook would return to the MPM just
> > for poll()ing.
>
> I think that would be excellent, yes. Trigger polling with the connection 
> Timeout value.

There is https://github.com/apache/httpd/pull/294 with quite some
changes to mpm_event. Sorry this is still WIP and needs to be split
more, probably in multiple PRs too, but it works for the tests I've
done so far (I just rebased it on latest trunk).
This change starts with Graham's AGAIN return code (from
process_connection hooks) for letting the MPM do the polling for SSL
handshakes that'd block for instance. It then expanded to a
CONN_STATE_PROCESS state (renamed from CONN_STATE_READ_REQUEST_LINE)
where mpm_event receiving AGAIN from ap_run_process_connection() will
simply poll according to the c->cs->sense value
(WANT_READ/WANT_WRITE).
This PR also removes all keepalive kills, though I don't think mod_h2
should rely on this state?

Could you try running your mod_h2 changes on top of it and with
h2_c1_run() doing something like:
c->cs->state = CONN_STATE_PROCESS;
c->cs->state = CONN_SENSE_WANT_READ; /* or _WRITE */
return AGAIN;
where you want mpm_event to simply poll() for read (or write)?


Regards;
Yann.

Re: more async handling in mod_h2

[Yann Ylavic]

On Mon, May 13, 2024 at 1:32 PM Stefan Eissing via dev
 wrote:
>
> I have merged https://github.com/icing/mod_h2/pull/280 to the mod-h2 on 
> github. With mpm_event, this will return HTTP/2 connections more often to the 
> mpm, thus freeing a worker.
>
> While this sounds good, I am not sure this is beneficial for a server under 
> load. The current connection state handling is designed for HTTP/1.x where a 
> connection is "given back" to the MPM at the end of a request.
>
> AFAICT, the MPM assumes that, once any pending output is written, it may 
> close the connection under load. Because in HTTP/1.x it means the connection 
> has served the last response completely. The client should be grateful and 
> cope well with the connection being closed subsequently due to other clients 
> demands.
>
> In HTTP/2 so far, we did return to the MPM only when all requests had been 
> served. The connection is therefore really in a similar state to HTTP/1.x. 
> The client has got its responses, we are free to close.
>
> With the change in PR 280, we return on being flow blocked. The response(s) 
> are *not* finished. If MPM now closes such a connection under load, the 
> client will most likely try again. This seems counter productive.

AFAICT the CONN_STATE_WRITE_COMPLETION state is different depending on
CONN_SENSE_WANT_READ and CONN_SENSE_WANT_WRITE/DEFAULT.
With CONN_SENSE_WANT_WRITE/DEFAULT, mpm_event will indeed assume flush
(nonblocking) before close, but with CONN_SENSE_WANT_READ it will poll
for read on the connection and come back to the process_connection
hooks when something is in.

When mod_h2 waits for some WINDOW_UPDATE it wants the MPM to POLLIN
(right?), h2_c1_hook_process_connection() should recover/continue with
the new data from the client. And since it's a c1 connection I don't
think there needs to be some new pollset/queues sizing adjustment to
do either, so we should be good?

>
> Therefore I am hesitant if this change is beneficial for us. It frees up a 
> worker thread - that is good - but. Do we need a new "connection state" here?
>
> WDYT?

I think the semantics of CONN_STATE_WRITE_COMPLETION with
CONN_SENSE_WANT_* are quite unintuitive (for the least), so we
probably should have different states for CONN_STATE_WRITE_COMPLETION
(flush before close) and CONN_STATE_POLL_READ/POLL_WRITE/POLL_RW which
would be how a process_connection hook would return to the MPM just
for poll()ing.


Regards;
Yann.

Re: [WIP] mod_authnz_ldap / mod_ldap support for APR v2 LDAP API

[Yann Ylavic]

On Thu, Apr 18, 2024 at 3:21 PM Joe Orton  wrote:
>
> On Thu, Apr 18, 2024 at 09:40:21AM +0100, Graham Leggett via dev wrote:
> > Hi all,
> >
> > The attached patch is a current work in progress patch for httpd-trunk to 
> > use the new apr_ldap API that just landing in APR.
> >
> > The highlights:
> >
> > - Complete replacement of the previous API.
> > - Will work against apr-2 (just landed) and apr-util-1.7 (after backport).
> > - Linking to the underlying LDAP API has been removed and is no longer 
> > needed.
>
> This design decision seems surprising to me, what does this add? Adding
> another abstraction layer to allow runtime selection of the LDAP library
> seems like a step backwards (a lot of complexity with no benefit).
>
> Unlike with e.g. a database backend selection users don't care about
> picking/configuring among many LDAP libraries.

FWIW I tried some time ago to avoid the very same kind of complexity
with apr_crypto vs TLS/SSL library but it was -1'd.
I would prefer direct linking of those libraries too, databases are a
special case which we shouldn't generalize for every library used by
APR (IMHO).


Regards;
Yann.

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

[Yann Ylavic]

On Fri, Apr 12, 2024 at 3:02 PM Ruediger Pluem  wrote:
>
> On 4/12/24 12:35 PM, yla...@apache.org wrote:
> > Author: ylavic
> > Date: Fri Apr 12 10:35:10 2024
> > New Revision: 1916925
> >
> > URL: http://svn.apache.org/viewvc?rev=1916925=rev
> > Log:
> > mpm_event: Simplify pollset "good methods" vs APR_POLLSET_WAKEABLE.
> >
> > * server/mpm/event/event.c(setup_threads_runtime):
> >   Simplify pollset creation code.
> >
> > All pollset "good methods" implement APR_POLLSET_WAKEABLE and wake-ability
> > is quite important for MPM event's correctness anyway so simplify code 
> > around
> > pollset creation so as not to suggest that APR_POLLSET_NODEFAULT if favored
> > against APR_POLLSET_WAKEABLE.
> >
> > While at it account for the wakeup pipe in the pollset_size since not all
> > pollset methods seem to do it internally in APR.
> >
> >
> > 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=1916925=1916924=1916925=diff
> > ==
> > --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> > +++ httpd/httpd/trunk/server/mpm/event/event.c Fri Apr 12 10:35:10 2024
> > @@ -2630,9 +2630,9 @@ static void setup_threads_runtime(void)
> >
> >  /* Create the main pollset */
> >  pollset_flags = APR_POLLSET_THREADSAFE | APR_POLLSET_NOCOPY |
> > -APR_POLLSET_NODEFAULT | APR_POLLSET_WAKEABLE;
> > +APR_POLLSET_WAKEABLE | APR_POLLSET_NODEFAULT;
> >  for (i = 0; i < sizeof(good_methods) / sizeof(good_methods[0]); i++) {
> > -rv = apr_pollset_create_ex(_pollset, pollset_size, pruntime,
> > +rv = apr_pollset_create_ex(_pollset, pollset_size + 1, 
> > pruntime,
>
> You explained this in the commit message above (thanks), but I think it would 
> be good to add a comment
> here in the code why +1 is added to the pollset_size to have the rational at 
> hand when reading the code.

Good point, done in r1916929 for both event and worker.


Regards;
Yann.

Re: pytest results for 2.4.59

[Yann Ylavic]

On Sat, Apr 6, 2024 at 10:46 AM jean-frederic clere  wrote:
>
> It seems pthread_kill(t, 0) returns 0 even the thread t has exited...
> older version of fedora will return 3 (I have tried fc28)
>
> The small example (taken from the internet) seems to behave like httpd
> reporting running threads that have exited...
>
> I have created https://bugzilla.redhat.com/show_bug.cgi?id=2273757 in
> case it is a fedora/rhel bug.

FWIW I find the new pthread_kill() behaviour completely useless. ESRCH
was for threads that exited already but were not joined (i.e.
"inactive" per POSIX), but for threads that are joined/detached (i.e.
"dead" or "end of lifetime") as documented by POSIX it's completely
useless because any pthread_ function usage is already undefined
behaviour on those (IOW, no one should do this).
So glibc conforming to POSIX here by removing ESRCH is just pointless,
they just killed pthread_kill(, 0) for no/bad reasons..

Re: pytest results for 2.4.59

[Yann Ylavic]

On Sun, Apr 7, 2024 at 2:16 PM Rainer Jung  wrote:
>
> Am 07.04.24 um 09:42 schrieb jean-frederic clere:
> > On 4/6/24 20:02, Rainer Jung wrote:
> >
> >> When you say Yann's patch helps, it means especially there are not
> >> more SIGTERM messages in the logs resp. no more teardown checks failing?
> >
> > Yes with Yann's patch, the "AH00045: child process n still did not
> > exit, sending a SIGTERM" messages are gone and teardown checks are passing.
>
> I could now also do some tests and for me the SIGTERM messages for
> worker on RHEL 9 during pytest are also gone with Yann's woker MPM
> patch. Plus I didn't get any new failures. Perl test framework is fine
> and pytest only some of the few occasional failures and errors I had
> also seen before.
>
> So it would be nice if we would apply Yann's patch for the worker mpm.

Thanks for testing, now in r1916926.
Finally I did not use the "good methods" (APR_POLLSET_NODEFAULT) to
create the pollset (like in mpm_event) but left the pollset
implementation to be selected as before. It's now using
APR_POLLSET_WAKEABLE though to help wakeup_listener().


Regards;
Yann.

Re: pytest results for 2.4.59

[Yann Ylavic]

On Sat, Apr 6, 2024 at 10:46 AM jean-frederic clere  wrote:
>
> On 4/5/24 07:55, Ruediger Pluem wrote:
> >
> > Are you able to provide a stacktrace of the hanging process (thread apply 
> > all bt full)?
>
> It seems pthread_kill(t, 0) returns 0 even the thread t has exited...
> older version of fedora will return 3 (I have tried fc28)

If pthread_kill() does not work we probably should use the global
"dying" variable like in mpm_event.
But it's not clear from your earlier "bt full" whether there are other
threads, could you try "thread apply all bt full" instead to show all
the threads?
It's clear from the main thread's backtrace that it's waiting for the
listener in the "iter" loop, but nothing tells if the listener already
exited or not. The listener for instance could be waiting indefinitely
apr_pollset_poll() at this point, and since there is no pollset wakeup
in mpm_worker I don't think that wakeup_listener() can help here.
So maybe we need to add an apr_pollset_wakeup() in wakeup_listener()
too, like in mpm_event too.

Overall something like the attached patch?


Regards;
Yann.
Index: server/mpm/worker/worker.c
===
--- server/mpm/worker/worker.c	(revision 1916768)
+++ server/mpm/worker/worker.c	(working copy)
@@ -125,10 +125,11 @@ static int max_workers = 0;
 static int server_limit = 0;
 static int thread_limit = 0;
 static int had_healthy_child = 0;
-static int dying = 0;
+static volatile int dying = 0;
 static int workers_may_exit = 0;
 static int start_thread_may_exit = 0;
 static int listener_may_exit = 0;
+static int listener_is_wakeable = 0; /* Pollset supports APR_POLLSET_WAKEABLE */
 static int requests_this_child;
 static int num_listensocks = 0;
 static int resource_shortage = 0;
@@ -272,6 +273,15 @@ static void close_worker_sockets(void)
 static void wakeup_listener(void)
 {
 listener_may_exit = 1;
+
+/* Unblock the listener if it's poll()ing */
+if (worker_pollset && listener_is_wakeable) {
+apr_pollset_wakeup(worker_pollset);
+}
+
+/* unblock the listener if it's waiting for a worker */
+ap_queue_info_term(worker_queue_info);
+
 if (!listener_os_thread) {
 /* XXX there is an obscure path that this doesn't handle perfectly:
  * right after listener thread is created but before
@@ -280,10 +290,6 @@ static void wakeup_listener(void)
  */
 return;
 }
-
-/* unblock the listener if it's waiting for a worker */
-ap_queue_info_term(worker_queue_info);
-
 /*
  * we should just be able to "kill(ap_my_pid, LISTENER_SIGNAL)" on all
  * platforms and wake up the listener thread since it is the only thread
@@ -716,6 +722,7 @@ static void * APR_THREAD_FUNC listener_thread(apr_
 ap_close_listeners_ex(my_bucket->listeners);
 ap_queue_info_free_idle_pools(worker_queue_info);
 ap_queue_term(worker_queue);
+
 dying = 1;
 ap_scoreboard_image->parent[process_slot].quiescing = 1;
 
@@ -861,6 +868,10 @@ static void create_listener_thread(thread_starter
 static void setup_threads_runtime(void)
 {
 ap_listen_rec *lr;
+int pollset_flags, i;
+const int good_methods[] = { APR_POLLSET_KQUEUE,
+ APR_POLLSET_PORT,
+ APR_POLLSET_EPOLL };
 apr_status_t rv;
 
 /* All threads (listener, workers) and synchronization objects (queues,
@@ -894,9 +905,31 @@ static void setup_threads_runtime(void)
 }
 
 /* Create the main pollset */
-rv = apr_pollset_create(_pollset, num_listensocks, pruntime,
-APR_POLLSET_NOCOPY);
+pollset_flags = APR_POLLSET_NOCOPY | APR_POLLSET_NODEFAULT | APR_POLLSET_WAKEABLE;
+for (i = 0; i < sizeof(good_methods) / sizeof(good_methods[0]); i++) {
+rv = apr_pollset_create_ex(_pollset, num_listensocks, pruntime,
+   pollset_flags, good_methods[i]);
+if (rv == APR_SUCCESS) {
+listener_is_wakeable = 1;
+break;
+}
+}
 if (rv != APR_SUCCESS) {
+pollset_flags &= ~APR_POLLSET_WAKEABLE;
+for (i = 0; i < sizeof(good_methods) / sizeof(good_methods[0]); i++) {
+rv = apr_pollset_create_ex(_pollset, num_listensocks, pruntime,
+   pollset_flags, good_methods[i]);
+if (rv == APR_SUCCESS) {
+break;
+}
+}
+}
+if (rv != APR_SUCCESS) {
+pollset_flags &= ~APR_POLLSET_NODEFAULT;
+rv = apr_pollset_create(_pollset, num_listensocks, pruntime,
+pollset_flags);
+}
+if (rv != APR_SUCCESS) {
 ap_log_error(APLOG_MARK, APLOG_EMERG, rv, ap_server_conf, APLOGNO(03285)
  "Couldn't create pollset in thread;"
  " check system or user limits");
@@ -1031,19 +1064,17 @@ static void join_workers(apr_thread_t *listener, a
  

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

[Yann Ylavic]

On Thu, Apr 4, 2024 at 12:52 PM Steffen Land  wrote:
>
> -1
> Get an error:
>
> Error   C2065   'DAV_WALKTYPE_TOLERANT': undeclared identifier  mod_dav_fs
>   C:\VS17\Win32\httpd-2.4\modules\dav\fs\repos.c  1599

Are you compiling with an old "mod_dav.h" somewhere in the -I[nclude] path?
Because DAV_WALKTYPE_TOLERANT is well defined in the new "mod_dav.h"
(of 2.4.59) which is also correctly #include'd in
"modules\dav\fs\repos.c"..

Regards;
Yann.

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

[Yann Ylavic]

On Wed, Apr 3, 2024 at 2:26 PM Eric Covener  wrote:
>
> I would like to call a SHORTENED VOTE to release
> this candidate tarball httpd-2.4.59-rc1 as 2.4.59:

[X] +1: It's not just good, it's good enough!

All good from my testing on debian(s).

Thanks Eric!

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

[Yann Ylavic]

On Tue, Mar 12, 2024 at 4:08 PM Eric Covener  wrote:
>
> below + POD wakeup
>
> Did not force the path yet where the listener is started (or fold in
> the scoreboard change )

+1, maybe ap_queue_term() rather than ap_queue_interrupt_all().

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

[Yann Ylavic]

On Tue, Mar 12, 2024 at 3:30 PM Eric Covener  wrote:
>
> On Tue, Mar 12, 2024 at 10:19 AM Yann Ylavic  wrote:
> >
> > On Tue, Mar 12, 2024 at 3:03 PM Eric Covener  wrote:
> > >
> > > On Tue, Mar 12, 2024 at 8:48 AM Yann Ylavic  wrote:
> > > >
> > > > Maybe it could be:
> > > > if (threads_created) {
> > >
> > > not listener_started?
> > >
> > > threads_started>0  could just mean we had no scoreboard issues but
> > > pthread_create failed on anything but the first thread.
> >
> > Don't we want the workers to gracefully stop whenever at least one was
> > created, or we may deadlock in clean_child_exit()?
>
> Yes, I see what you mean now.  I will try to force some pthread_create
> errors w/ the patch soon.

Possibly we want this bit too:
 rv = ap_thread_create([i], thread_attr,
   worker_thread, my_info, pruntime);
 if (rv != APR_SUCCESS) {
+ap_update_child_status_from_indexes(my_child_num, i,
+SERVER_DEAD, NULL);

to restore SERVER_DEAD for the thread we could not create..

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

[Yann Ylavic]

On Tue, Mar 12, 2024 at 3:03 PM Eric Covener  wrote:
>
> On Tue, Mar 12, 2024 at 8:48 AM Yann Ylavic  wrote:
> >
> > Maybe it could be:
> > if (threads_created) {
>
> not listener_started?
>
> threads_started>0  could just mean we had no scoreboard issues but
> pthread_create failed on anything but the first thread.

Don't we want the workers to gracefully stop whenever at least one was
created, or we may deadlock in clean_child_exit()?

>
> > resource_shortage = 1;
> > signal_threads(ST_GRACEFUL);
> > break;
>
> I think this would have us still outer looping to keep trying to create 
> threads?

Yes, exiting start_threads() is better I think, we should not create
more threads anyway.

>
> > }
> > clean_child_exit(APEXIT_CHILDSICK);
>
> >> We should probably prevent the listener from starting too, like:
>
> Could be confusing, maybe we can instead dodge creating the listener
> if resource_shortage=1?

So something like the attached patch?


Regards;
Yann.
Index: server/mpm/event/event.c
===
--- server/mpm/event/event.c	(revision 1916254)
+++ server/mpm/event/event.c	(working copy)
@@ -2748,8 +2748,18 @@ static void *APR_THREAD_FUNC start_threads(apr_thr
 ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf,
  APLOGNO(03104)
  "ap_thread_create: unable to create worker thread");
-/* let the parent decide how bad this really is */
-signal_threads(listener_started ? ST_GRACEFUL : ST_UNGRACEFUL);
+/* Let the parent decide how bad this really is by returning
+ * APEXIT_CHILDSICK. If threads were created already let them
+ * stop cleanly first to avoid deadlocks in clean_child_exit(),
+ * just stop creating new ones here (but set resource_shortage
+ * to return APEXIT_CHILDSICK still when the child exists).
+ */
+if (threads_created) {
+resource_shortage = 1;
+signal_threads(ST_GRACEFUL);
+apr_thread_exit(thd, APR_SUCCESS);
+return NULL;
+}
 clean_child_exit(APEXIT_CHILDSICK);
 }
 threads_created++;

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

[Yann Ylavic]

On Tue, Mar 12, 2024 at 1:46 PM Yann Ylavic  wrote:
>
> Maybe it could be:

We should probably prevent the listener from starting too, like:

> if (threads_created) {
> resource_shortage = 1;
> signal_threads(ST_GRACEFUL);
  listener_started = 1; /* don't start it if not already */
> break;
> }
> clean_child_exit(APEXIT_CHILDSICK);
> ?
>
>
> Regards;
> Yann.

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

[Yann Ylavic]

On Tue, Mar 12, 2024 at 1:06 PM Eric Covener  wrote:
>
> On Mon, Mar 11, 2024 at 8:28 PM  wrote:
> >
> > Author: covener
> > Date: Tue Mar 12 00:28:34 2024
> > New Revision: 1916243
> >
> > URL: http://svn.apache.org/viewvc?rev=1916243=rev
> > Log:
> > use graceful exit if lister started
> >
> > 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=1916243=1916242=1916243=diff
> > ==
> > --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> > +++ httpd/httpd/trunk/server/mpm/event/event.c Tue Mar 12 00:28:34 2024
> > @@ -2749,7 +2749,7 @@ static void *APR_THREAD_FUNC start_threa
> >   APLOGNO(03104)
> >   "ap_thread_create: unable to create worker 
> > thread");
> >  /* let the parent decide how bad this really is */
> > -signal_threads(ST_UNGRACEFUL);
> > +signal_threads(listener_started ? ST_GRACEFUL : 
> > ST_UNGRACEFUL);
> >  clean_child_exit(APEXIT_CHILDSICK);
> >  }
>
> Maybe this option is silly, if we are going to nearly immediately
> clear pchild and call exit().

Maybe it could be:
if (threads_created) {
resource_shortage = 1;
signal_threads(ST_GRACEFUL);
break;
}
clean_child_exit(APEXIT_CHILDSICK);
?


Regards;
Yann.

Re: svn commit: r1916068 - in /httpd/httpd/trunk: .github/workflows/linux.yml test/travis_before_linux.sh

[Yann Ylavic]

On Fri, Mar 1, 2024 at 2:12 PM Joe Orton  wrote:
>
> On Fri, Mar 01, 2024 at 01:52:15PM +0100, Yann Ylavic wrote:
> > On Fri, Mar 1, 2024 at 1:42 PM Yann Ylavic  wrote:
> > >
> > > On Fri, Mar 1, 2024 at 1:24 PM Joe Orton  wrote:
> > > >
> > > > Do you still want that
> > > > TestSSLCA.pm change merged?
> > >
> > > I think it can be useful for those who test httpd with openssl1 still
> > > (not maintained anymore, but we have to keep compatibility in 2.4 at
> > > least).
> >
> > But the issue with this patch is that it doesn't check which openssl
> > version httpd is actually using, so it always generates pkcs#1 keys
> > even if not needed.
> > If we had a way to check the system's openssl AND httpd's openssl are
> > < 3 it would be better, but I don't see how to do this.
>
> I suppose we could export the detected version from configure via apxs
> -q and pick it up in Apache::Test, but I think it would be likely to
> make the whole house of cards even more fragile. So I'm not sure it's
> worth investing effort in that tbh. Better to assume/require that the
> bin/openssl version matches the version mod_ssl uses.

Yes agreed, let's drop this patch. There is still the
$APACHE_TEST_OPENSSL_CMD workaround to force the openssl version used
by the framework to align with httpd's (for those who want to test
with openssl < 3).

Thanks!
Yann.

>
> Regards, Joe
>

Re: svn commit: r1916068 - in /httpd/httpd/trunk: .github/workflows/linux.yml test/travis_before_linux.sh

[Yann Ylavic]

On Fri, Mar 1, 2024 at 1:42 PM Yann Ylavic  wrote:
>
> On Fri, Mar 1, 2024 at 1:24 PM Joe Orton  wrote:
> >
> > Also - I guess the note about *not* accepting PKCS#8 format keys in
> > https://httpd.apache.org/docs/2.4/mod/mod_ssl.html#sslproxymachinecertificatefile
> > is now wrong then?
>
> OpenSSL >= 3 can surely load keys in pkcs#8 format since it's the
> default for genrsa now, hopefully it can still load the pkcs#1 ones
> still (I didn't try that) or it would be a mess for mod_proxy (and the
> docs)..
> Let me try that first and if it's ok I think we can simply say that
> the note applies to openssl < 3 only.

The perl framework seems to pass all the tests here if I use openssl
>= 3 for both the system and httpd and then force pkcs#1 keys (i.e.
genrsa -traditional), so it seems fine to say that
SSLProxyMachineCertificateFile works with pkcs#1 and pkcs#8 keys when
httpd's openssl >= 3 but only pkcs#1 ones when openssl < 3.

Re: svn commit: r1916068 - in /httpd/httpd/trunk: .github/workflows/linux.yml test/travis_before_linux.sh

[Yann Ylavic]

On Fri, Mar 1, 2024 at 1:42 PM Yann Ylavic  wrote:
>
> On Fri, Mar 1, 2024 at 1:24 PM Joe Orton  wrote:
> >
> > Do you still want that
> > TestSSLCA.pm change merged?
>
> I think it can be useful for those who test httpd with openssl1 still
> (not maintained anymore, but we have to keep compatibility in 2.4 at
> least).

But the issue with this patch is that it doesn't check which openssl
version httpd is actually using, so it always generates pkcs#1 keys
even if not needed.
If we had a way to check the system's openssl AND httpd's openssl are
< 3 it would be better, but I don't see how to do this.

Re: svn commit: r1916068 - in /httpd/httpd/trunk: .github/workflows/linux.yml test/travis_before_linux.sh

[Yann Ylavic]

On Fri, Mar 1, 2024 at 1:24 PM Joe Orton  wrote:
>
> On Fri, Mar 01, 2024 at 12:59:10PM +0100, Yann Ylavic wrote:
> > On Fri, Mar 1, 2024 at 11:15 AM  wrote:
> > >
> > > Author: jorton
> > > Date: Fri Mar  1 10:15:13 2024
> > > New Revision: 1916068
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1916068=rev
> > > Log:
> > > CI: add OpenSSL 3.2, test OpenSSL 3.x using Apache::Test
> > > trunk to pick up r1916067.
> >
> > I had to modify Apache-Test too when running the perl test framework
> > with openssl >= 3.0 and proposed a patch here [1] (not enough karma to
> > commit on perl.a.o).
> > It was an issue with mod_proxy's client certs IIRC, which r1916067 is
> > possibly fixing too, but just in case you are still fighting with this
> > ;)
>
> Ah, interesting, thanks. I should read dev@perl more often!
>
> I haven't seen that particularly failure, and trunk seems to now be
> working (touch wood) with 3.1 and 3.2. The Ubuntu runners are all on
> OpenSSL 3.0 anyway, and r1916058 ensures that TestSSLCA.pm is using the
> bin/openssl from the installed version of OpenSSL rather than a
> possibly-mismatched system /usr/bin/openssl. Do you still want that
> TestSSLCA.pm change merged?

I think it can be useful for those who test httpd with openssl1 still
(not maintained anymore, but we have to keep compatibility in 2.4 at
least).

>
> Also - I guess the note about *not* accepting PKCS#8 format keys in
> https://httpd.apache.org/docs/2.4/mod/mod_ssl.html#sslproxymachinecertificatefile
> is now wrong then?

OpenSSL >= 3 can surely load keys in pkcs#8 format since it's the
default for genrsa now, hopefully it can still load the pkcs#1 ones
still (I didn't try that) or it would be a mess for mod_proxy (and the
docs)..
Let me try that first and if it's ok I think we can simply say that
the note applies to openssl < 3 only.

Re: svn commit: r1916068 - in /httpd/httpd/trunk: .github/workflows/linux.yml test/travis_before_linux.sh

[Yann Ylavic]

On Fri, Mar 1, 2024 at 12:59 PM Yann Ylavic  wrote:
>
> On Fri, Mar 1, 2024 at 11:15 AM  wrote:
> >
> > Author: jorton
> > Date: Fri Mar  1 10:15:13 2024
> > New Revision: 1916068
> >
> > URL: http://svn.apache.org/viewvc?rev=1916068=rev
> > Log:
> > CI: add OpenSSL 3.2, test OpenSSL 3.x using Apache::Test
> > trunk to pick up r1916067.
>
> I had to modify Apache-Test too when running the perl test framework
> with openssl >= 3.0 and proposed a patch here [1] (not enough karma to
> commit on perl.a.o).
> It was an issue with mod_proxy's client certs IIRC, which r1916067 is
> possibly fixing too, but just in case you are still fighting with this
> ;)
>
> [1] https://lists.apache.org/thread/z655wrccpqsvxdwyj8znrp6qd194c410

Well, this is an issue if trying to test httpd linked with openssl < 3
on a system with openssl >= 3 (perl will the system's by default), so
it's not what our ci is doing now IIUC, but should it (or should one
try this)..

Re: svn commit: r1916068 - in /httpd/httpd/trunk: .github/workflows/linux.yml test/travis_before_linux.sh

[Yann Ylavic]

On Fri, Mar 1, 2024 at 11:15 AM  wrote:
>
> Author: jorton
> Date: Fri Mar  1 10:15:13 2024
> New Revision: 1916068
>
> URL: http://svn.apache.org/viewvc?rev=1916068=rev
> Log:
> CI: add OpenSSL 3.2, test OpenSSL 3.x using Apache::Test
> trunk to pick up r1916067.

I had to modify Apache-Test too when running the perl test framework
with openssl >= 3.0 and proposed a patch here [1] (not enough karma to
commit on perl.a.o).
It was an issue with mod_proxy's client certs IIRC, which r1916067 is
possibly fixing too, but just in case you are still fighting with this
;)

Regards;
Yann.

[1] https://lists.apache.org/thread/z655wrccpqsvxdwyj8znrp6qd194c410

Re: svn commit: r1913815 - in /httpd/httpd/trunk: changes-entries/pr68080.txt modules/ssl/mod_ssl.c modules/ssl/ssl_engine_config.c modules/ssl/ssl_private.h

[Yann Ylavic]

On Mon, Feb 19, 2024 at 5:36 PM jean-frederic clere  wrote:
>
> On 11/15/23 23:09, yla...@apache.org wrote:
> > Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_config.c
> > URL:http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_config.c?rev=1913815=1913814=1913815=diff
> > ==
> > --- httpd/httpd/trunk/modules/ssl/ssl_engine_config.c (original)
> > +++ httpd/httpd/trunk/modules/ssl/ssl_engine_config.c Wed Nov 15 22:09:05 
> > 2023
> > @@ -669,7 +669,6 @@ const char *ssl_cmd_SSLPassPhraseDialog(
> >   return NULL;
> >   }
> >
> > -#if defined(HAVE_OPENSSL_ENGINE_H) && defined(HAVE_ENGINE_INIT)
> >   const char *ssl_cmd_SSLCryptoDevice(cmd_parms *cmd,
> >   void *dcfg,
> >   const char *arg)
> > @@ -714,7 +713,6 @@ const char *ssl_cmd_SSLCryptoDevice(cmd_
> >
> >   return NULL;
> >   }
> > -#endif
>
> I think that is causing compilation problems with:
> mc->szCryptoDevice = NULL;
> in ssl_cmd_SSLCryptoDevice().

Thanks, I suppose this happens with the latest OpenSSL versions where
they removed the ENGINE API completely?
Fixed in r1915889 hopefully (should probably be backported to 2.4.x
since r1913815 made it there already).


Regards;
Yann.

Re: libapreq subproject roll call

[Yann Ylavic]

On Fri, Feb 16, 2024 at 2:11 PM Eric Covener  wrote:
>
> I count myself as a release vote of last resort only, but i don't
> think we should be committing to future fixes/releases if nearly
> everyone is in this category.

+1, since I know the code I can eventually look at or propose patches
after the last release (if needed, possibly in a patches/ directory or
so), but I'm not willing to engage more than that by myself.


Regards;
Yann.

Re: release apreq 2.18 and mothball the project

[Yann Ylavic]

On Thu, Feb 15, 2024 at 5:12 AM Joe Schaefer wrote:
>
> I gave you beta males an entire year

His Alpha Most Serene Highness is too kind to us..

> to find an excuse for completely boning the modperl user community for no 
> good reason,

_You_ did that (boning the modperl user community), by spitting your
venom on everyone here (and in all the forums) and nothing else.
You, as a member of the httpd team, could have fixed it and
started/encouraged a new release with the one-liner fix, yet your
first/primary point was to insult others for having touched your
(alphally broken) code and introduced a (betally edge) regression,
which His Alpha Most Serene Highness' test suite didn't caught btw.

> So far, the only thing to come of putting apreq inside httpd is that third 
> party quality control teams put some elbow grease into fuzzing its various 
> parsers and found a sore spot that you bland engineers felt put upon to fix.  
> And so you botched that patch too, largely out of spite for the makework.

Actually it did find multiple sore spots, otherwise you can imagine
that we plebs would never have dared to touch as much of His Alpha
Most Serene Highness' code.

Re: PR #363

[Yann Ylavic]

+1

On Thu, Jan 25, 2024 at 3:45 PM Eric Covener  wrote:
>
> I wouldn't mind move/rename to README.md
>
> On Thu, Jan 25, 2024, 10:40 AM Joe Orton  wrote:
>>
>> On Thu, Jan 25, 2024 at 08:12:24AM +0100, Ruediger Pluem wrote:
>> > Tried it in r1915391 and it seems to work. Not sure if there are
>> > general downsides / objections with regards to symlinks in our
>> > repository. But trunk is CTR :-).
>>
>> Oh, that looks really nice. +1
>>
>> Thanks to you, Rich, and Mayank Patil.
>>
>> Regards, Joe
>>

Re: PR #363

[Yann Ylavic]

On Wed, Jan 24, 2024 at 5:07 PM  wrote:
>
> On Wed, 2024-01-24 at 17:01 +0100, Yann Ylavic wrote:
> > On Wed, Jan 24, 2024 at 4:56 PM Mads Toftum  wrote:
> > >
> > > On Wed, Jan 24, 2024 at 10:06:43AM -0500, rbo...@rcbowen.com wrote:
> > > > I've been poking at some of our open PRs looking for docs-only
> > > > changes
> > > > that we can possibly clean up.
> > > >
> > > > PR #363 - https://github.com/apache/httpd/pull/363 - converts
> > > > README
> > > > from plaintext to markdown, and makes no other changes. This
> > > > makes it
> > > > prettier on GitHub.
> > > >
> > > > I almost just CTR'ed it, but it occurred to me that someone might
> > > > find
> > > > this objectionable, so I thought I'd ask first.
> > >
> > > I don't have strong opinions on this, but we'll end up with a
> > > READNE
> > > that's less readable for those not viewing it through github.
> >
> > Maybe we could keep the line breaks as before for the text to keep it
> > as readable as before w/o a markdown viewer?
>
> Yeah, I've done that in my local copy of the patch.
>
> So this is basically changing:
>
> Whossname
> -
>
> to
>
> ## Whossname
>
> Which really doesn't change much in plain text mode.

Agreed, the main readability issue (in plain text mode) is long lines
IMHO, headings/links do not read so bad.

Re: PR #363

[Yann Ylavic]

On Wed, Jan 24, 2024 at 4:56 PM Mads Toftum  wrote:
>
> On Wed, Jan 24, 2024 at 10:06:43AM -0500, rbo...@rcbowen.com wrote:
> > I've been poking at some of our open PRs looking for docs-only changes
> > that we can possibly clean up.
> >
> > PR #363 - https://github.com/apache/httpd/pull/363 - converts README
> > from plaintext to markdown, and makes no other changes. This makes it
> > prettier on GitHub.
> >
> > I almost just CTR'ed it, but it occurred to me that someone might find
> > this objectionable, so I thought I'd ask first.
>
> I don't have strong opinions on this, but we'll end up with a READNE
> that's less readable for those not viewing it through github.

Maybe we could keep the line breaks as before for the text to keep it
as readable as before w/o a markdown viewer?


Regards;
Yann.

Re: process_regexp bug, infinite recursion

[Yann Ylavic]

On Mon, Jan 8, 2024 at 5:54 PM Ruediger Pluem  wrote:
>
> On 1/8/24 1:37 PM, Yann Ylavic wrote:
> >
> > As noted in v2 we have an issue here by "losing" the beginning of the
> > value on recursion:
> > /* XXX: recursing by using AP_REG_NOTBOL (because we are not at 
> > ^
> >  * anymore) and then "losing" the beginning of the string is not
> >  * always correct. Say we match "(?<=a)ba" against "ababa", on
> >  * recursion ap_regexec_len() will not know that the second "b" 
> > is
> >  * preceded by "a" thus not match. We'd need a new 
> > ap_regexec_ex()
> >  * that can take match_end as an offset to fix this..
> >  */
> >
> > Not sure how far we should go with this patch..
>
> I think things do not get worse in this respect because of this patch but 
> only improve
> in the sense that an infinite recursion is avoided.
> Hence +1 on the patch.

I finally went with the full thing in r1915267, r1915268 and r1915271
(with new tests in r1915269 for what didn't work well).


Regards;
Yann.

Re: process_regexp bug, infinite recursion

[Yann Ylavic]

On Mon, Jan 8, 2024 at 10:49 AM Ruediger Pluem  wrote:
>
> On 1/5/24 3:08 PM, Yann Ylavic wrote:
> >
> > process_regexp.diff
> >
> > Index: modules/metadata/mod_headers.c
> > ===
> > --- modules/metadata/mod_headers.c(revision 1914804)
> > +++ modules/metadata/mod_headers.c(working copy)
> > @@ -622,41 +622,70 @@ static char* process_tags(header_entry *hdr, reque
> >  }
> >  return str ? str : "";
> >  }
> > -static const char *process_regexp(header_entry *hdr, const char *value,
> > -  request_rec *r)
> > +
> > +static const char *process_regexp_rec(header_entry *hdr, const char *value,
> > +  request_rec *r, ap_regmatch_t 
> > pmatch[],
> > +  int flags)
> >  {
> > -ap_regmatch_t pmatch[AP_MAX_REG_MATCH];
> > -const char *subs;
> > -const char *remainder;
> >  char *ret;
> > -int diffsz;
> > -if (ap_regexec(hdr->regex, value, AP_MAX_REG_MATCH, pmatch, 0)) {
> > +const char *subs, *remainder;
> > +apr_size_t val_len, subs_len, rem_len;
> > +apr_size_t match_start, match_end;
> > +
> > +val_len = strlen(value);
> > +if (ap_regexec_len(hdr->regex, value, val_len,
> > +   AP_MAX_REG_MATCH, pmatch,
> > +   flags)) {
> >  /* no match, nothing to do */
> >  return value;
> >  }
> > +
> >  /* Process tags in the input string rather than the resulting
> > * substitution to avoid surprises
> > */
> > -subs = ap_pregsub(r->pool, process_tags(hdr, r), value, 
> > AP_MAX_REG_MATCH, pmatch);
> > +subs = ap_pregsub(r->pool, process_tags(hdr, r), value,
> > +  AP_MAX_REG_MATCH, pmatch);
> >  if (subs == NULL)
> >  return NULL;
> > -diffsz = strlen(subs) - (pmatch[0].rm_eo - pmatch[0].rm_so);
> > +subs_len = strlen(subs);
> > +
> > +ap_assert(pmatch[0].rm_so >= 0 && pmatch[0].rm_so <= pmatch[0].rm_eo);
> > +match_start = pmatch[0].rm_so;
> > +match_end = pmatch[0].rm_eo;
> >  if (hdr->action == hdr_edit) {
> > -remainder = value + pmatch[0].rm_eo;
> > +remainder = value + match_end;
> >  }
> >  else { /* recurse to edit multiple matches if applicable */
> > -remainder = process_regexp(hdr, value + pmatch[0].rm_eo, r);
> > -if (remainder == NULL)
> > -return NULL;
> > -diffsz += strlen(remainder) - strlen(value + pmatch[0].rm_eo);
> > +if (match_end == 0 && val_len) {
> > +/* advance on empty match to avoid infinite recursion */
> > +match_start = match_end = 1;
>
> Not debating that
>
> Header edit* headername ^ prefix
>
> should be be
>
> Header edit headername ^ prefix
>
> but wouldn't that do the wrong thing and add the prefix *after* the first 
> character of the header value?

Yes correct, this patch won't set the skipped char at the right place finally.
New v2 attached.

>
> > +}
> > +if (match_end < val_len) {
> > +remainder = process_regexp_rec(hdr, value + match_end,
>
> Shouldn't we increase match_end just here by 1 if match_end == 0 && val_len ?

See v2, match_end is not used below anyway.


>
> > +   r, pmatch, AP_REG_NOTBOL);

As noted in v2 we have an issue here by "losing" the beginning of the
value on recursion:
/* XXX: recursing by using AP_REG_NOTBOL (because we are not at ^
 * anymore) and then "losing" the beginning of the string is not
 * always correct. Say we match "(?<=a)ba" against "ababa", on
 * recursion ap_regexec_len() will not know that the second "b" is
 * preceded by "a" thus not match. We'd need a new ap_regexec_ex()
 * that can take match_end as an offset to fix this..
 */

Not sure how far we should go with this patch..


Regards;
Yann.
Index: modules/metadata/mod_headers.c
===
--- modules/metadata/mod_headers.c	(revision 1914804)
+++ modules/metadata/mod_headers.c	(working copy)
@@ -622,41 +622,90 @@ static char* process_tags(header_entry *hdr, reque
 }
 return str ? str : "";
 }
-static const char *process_regexp(header_entry 

Re: process_regexp bug, infinite recursion

[Yann Ylavic]

On Fri, Jan 5, 2024 at 3:11 AM Eric Covener  wrote:
>
> On Thu, Jan 4, 2024 at 9:04 PM Jason Pyeron  wrote:
> >
> > I am having some issue searching Bugzilla for any issue involving 
> > process_regexp in mod_headers.c .
> >
> > It finds nothing, so I am assuming I did something wrong in my search. Will 
> > file bug if not already filed.
> >
> > We are investigating an infinite loop (stack overflow) issue, caused by 
> > "securing" a system.
> >
> > ZZZ-STIG-SV-214288r881493_rule.conf:Header always edit* Set-Cookie ^(.*)$ 
> > $1;HttpOnly;secure
>
> edit* just makes no sense at all here when you are capturing/replacing
> the entire string and will loop by definition.

Maybe we should avoid infinite recursion in any case, like in the
attached patch?
How does it work for you Jason?

Regards;
Yann.
Index: modules/metadata/mod_headers.c
===
--- modules/metadata/mod_headers.c	(revision 1914804)
+++ modules/metadata/mod_headers.c	(working copy)
@@ -622,41 +622,70 @@ static char* process_tags(header_entry *hdr, reque
 }
 return str ? str : "";
 }
-static const char *process_regexp(header_entry *hdr, const char *value,
-  request_rec *r)
+
+static const char *process_regexp_rec(header_entry *hdr, const char *value,
+  request_rec *r, ap_regmatch_t pmatch[],
+  int flags)
 {
-ap_regmatch_t pmatch[AP_MAX_REG_MATCH];
-const char *subs;
-const char *remainder;
 char *ret;
-int diffsz;
-if (ap_regexec(hdr->regex, value, AP_MAX_REG_MATCH, pmatch, 0)) {
+const char *subs, *remainder;
+apr_size_t val_len, subs_len, rem_len;
+apr_size_t match_start, match_end;
+
+val_len = strlen(value);
+if (ap_regexec_len(hdr->regex, value, val_len,
+   AP_MAX_REG_MATCH, pmatch,
+   flags)) {
 /* no match, nothing to do */
 return value;
 }
+
 /* Process tags in the input string rather than the resulting
* substitution to avoid surprises
*/
-subs = ap_pregsub(r->pool, process_tags(hdr, r), value, AP_MAX_REG_MATCH, pmatch);
+subs = ap_pregsub(r->pool, process_tags(hdr, r), value,
+  AP_MAX_REG_MATCH, pmatch);
 if (subs == NULL)
 return NULL;
-diffsz = strlen(subs) - (pmatch[0].rm_eo - pmatch[0].rm_so);
+subs_len = strlen(subs);
+
+ap_assert(pmatch[0].rm_so >= 0 && pmatch[0].rm_so <= pmatch[0].rm_eo);
+match_start = pmatch[0].rm_so;
+match_end = pmatch[0].rm_eo;
 if (hdr->action == hdr_edit) {
-remainder = value + pmatch[0].rm_eo;
+remainder = value + match_end;
 }
 else { /* recurse to edit multiple matches if applicable */
-remainder = process_regexp(hdr, value + pmatch[0].rm_eo, r);
-if (remainder == NULL)
-return NULL;
-diffsz += strlen(remainder) - strlen(value + pmatch[0].rm_eo);
+if (match_end == 0 && val_len) {
+/* advance on empty match to avoid infinite recursion */
+match_start = match_end = 1;
+}
+if (match_end < val_len) {
+remainder = process_regexp_rec(hdr, value + match_end,
+   r, pmatch, AP_REG_NOTBOL);
+if (remainder == NULL)
+return NULL;
+}
+else {
+remainder = "";
+}
 }
-ret = apr_palloc(r->pool, strlen(value) + 1 + diffsz);
-memcpy(ret, value, pmatch[0].rm_so);
-strcpy(ret + pmatch[0].rm_so, subs);
-strcat(ret, remainder);
+rem_len = strlen(remainder);
+
+ret = apr_palloc(r->pool, match_start + subs_len + rem_len + 1);
+memcpy(ret, value, match_start);
+memcpy(ret + match_start, subs, subs_len);
+memcpy(ret + match_start + subs_len, remainder, rem_len + 1);
 return ret;
 }
 
+static const char *process_regexp(header_entry *hdr, const char *value,
+  request_rec *r)
+{
+ap_regmatch_t pmatch[AP_MAX_REG_MATCH];
+return process_regexp_rec(hdr, value, r, pmatch, 0);
+}
+
 static int echo_header(void *v, const char *key, const char *val)
 {
 echo_do *ed = (echo_do *)v;

Re: svn commit: r1914804 - in /httpd/httpd/trunk: changes-entries/flushing-chunks.txt modules/http/chunk_filter.c

[Yann Ylavic]

On Wed, Dec 20, 2023 at 5:50 PM Joe Orton  wrote:
>
> On Wed, 20 Dec 2023, 16:30 Yann Ylavic,  wrote:
>>
>> For the last-chunk, I think we need
>
> I think that's not necessary because in the special case eos=flush, in the 
> normal case flush is NULL. Actually the reordering of the tests above doesn't 
> make any difference either, I could have skipped that. In the first iteration 
> of the patch I renamed eos to "terminator" to make it more obvious.

You are right, I missed that "eos = e" and not "eos =
APR_BUCKET_NEXT(e)" in this case.
My noise, all good!

Regards;
Yann.

Re: svn commit: r1914804 - in /httpd/httpd/trunk: changes-entries/flushing-chunks.txt modules/http/chunk_filter.c

[Yann Ylavic]

On Wed, Dec 20, 2023 at 5:20 PM Yann Ylavic  wrote:
>
> On Wed, Dec 20, 2023 at 4:56 PM  wrote:
> >
> > Author: jorton
> > Date: Wed Dec 20 15:56:15 2023
> > New Revision: 1914804
> >
> > URL: http://svn.apache.org/viewvc?rev=1914804=rev
> > Log:
> > * modules/http/chunk_filter.c (ap_http_chunk_filter): For a brigade
> >   containing [FLUSH EOS], insert the last-chunk terminator before the
> >   FLUSH rather than between the FLUSH and the EOS.
> >
> > Github: closes #400
>
> It seems that we should set the last-chunk before the FLUSH only if
> the latter precedes an EOS?
>
> So below:
>
> > --- httpd/httpd/trunk/modules/http/chunk_filter.c (original)
> > +++ httpd/httpd/trunk/modules/http/chunk_filter.c Wed Dec 20 15:56:15 2023
> > @@ -64,7 +64,7 @@ apr_status_t ap_http_chunk_filter(ap_fil
> >
> >  for (more = tmp = NULL; b; b = more, more = NULL) {
> >  apr_off_t bytes = 0;
> > -apr_bucket *eos = NULL;
> > +apr_bucket *eos = NULL; /* EOS bucket, or FLUSH preceding EOS */
> >  apr_bucket *flush = NULL;
> >  /* XXX: chunk_hdr must remain at this scope since it is used in a
> >   *  transient bucket.
> > @@ -99,8 +99,20 @@ apr_status_t ap_http_chunk_filter(ap_fil
> >  }
> >  if (APR_BUCKET_IS_FLUSH(e)) {
> >  flush = e;
>
> Don't set "flush" above.
>
> > +
> > +/* Special case to catch common brigade ending of
> > + * [FLUSH] [EOS] - insert the last_chunk before
> > + * the FLUSH rather than between the FLUSH and the
> > + * EOS. */
> >  if (e != APR_BRIGADE_LAST(b)) {
> > -more = apr_brigade_split_ex(b, APR_BUCKET_NEXT(e), 
> > tmp);
> > +if (APR_BUCKET_IS_EOS(APR_BUCKET_NEXT(e))) {
>
> But here only?
>
> > +eos = e;
> > +/* anything after EOS is dropped, no need
> > + * to split. */
> > +}
> > +else {
> > +more = apr_brigade_split_ex(b, 
> > APR_BUCKET_NEXT(e), tmp);
> > +}
> >  }
> >  break;
> >  }
> > @@ -173,12 +185,12 @@ apr_status_t ap_http_chunk_filter(ap_fil
> >   * FLUSH bucket, or appended to the brigade
> >   */
> >  e = apr_bucket_immortal_create(CRLF_ASCII, 2, c->bucket_alloc);
> > -if (eos != NULL) {
> > -APR_BUCKET_INSERT_BEFORE(eos, e);
> > -}
> > -else if (flush != NULL) {
> > +if (flush != NULL) {
> >  APR_BUCKET_INSERT_BEFORE(flush, e);
> >  }
> > +else if (eos != NULL) {
> > +APR_BUCKET_INSERT_BEFORE(eos, e);
> > +}
> >  else {
> >  APR_BRIGADE_INSERT_TAIL(b, e);
> >  }

Ah no, this is for every chunk so we are good here.
For the last-chunk, I think we need:

Index: modules/http/chunk_filter.c
===
--- modules/http/chunk_filter.c(revision 1914804)
+++ modules/http/chunk_filter.c(working copy)
@@ -217,7 +217,7 @@ apr_status_t ap_http_chunk_filter(ap_filter_t *f,
  * now.
  */
 if (eos && !ctx->bad_gateway_seen) {
-ap_h1_add_end_chunk(b, eos, f->r, ctx->trailers);
+ap_h1_add_end_chunk(b, flush ? flush : eos, f->r, ctx->trailers);
 }

 /* pass the brigade to the next filter. */
--
?

Re: svn commit: r1914804 - in /httpd/httpd/trunk: changes-entries/flushing-chunks.txt modules/http/chunk_filter.c

[Yann Ylavic]

On Wed, Dec 20, 2023 at 4:56 PM  wrote:
>
> Author: jorton
> Date: Wed Dec 20 15:56:15 2023
> New Revision: 1914804
>
> URL: http://svn.apache.org/viewvc?rev=1914804=rev
> Log:
> * modules/http/chunk_filter.c (ap_http_chunk_filter): For a brigade
>   containing [FLUSH EOS], insert the last-chunk terminator before the
>   FLUSH rather than between the FLUSH and the EOS.
>
> Github: closes #400

It seems that we should set the last-chunk before the FLUSH only if
the latter precedes an EOS?

So below:

> --- httpd/httpd/trunk/modules/http/chunk_filter.c (original)
> +++ httpd/httpd/trunk/modules/http/chunk_filter.c Wed Dec 20 15:56:15 2023
> @@ -64,7 +64,7 @@ apr_status_t ap_http_chunk_filter(ap_fil
>
>  for (more = tmp = NULL; b; b = more, more = NULL) {
>  apr_off_t bytes = 0;
> -apr_bucket *eos = NULL;
> +apr_bucket *eos = NULL; /* EOS bucket, or FLUSH preceding EOS */
>  apr_bucket *flush = NULL;
>  /* XXX: chunk_hdr must remain at this scope since it is used in a
>   *  transient bucket.
> @@ -99,8 +99,20 @@ apr_status_t ap_http_chunk_filter(ap_fil
>  }
>  if (APR_BUCKET_IS_FLUSH(e)) {
>  flush = e;

Don't set "flush" above.

> +
> +/* Special case to catch common brigade ending of
> + * [FLUSH] [EOS] - insert the last_chunk before
> + * the FLUSH rather than between the FLUSH and the
> + * EOS. */
>  if (e != APR_BRIGADE_LAST(b)) {
> -more = apr_brigade_split_ex(b, APR_BUCKET_NEXT(e), 
> tmp);
> +if (APR_BUCKET_IS_EOS(APR_BUCKET_NEXT(e))) {

But here only?

> +eos = e;
> +/* anything after EOS is dropped, no need
> + * to split. */
> +}
> +else {
> +more = apr_brigade_split_ex(b, 
> APR_BUCKET_NEXT(e), tmp);
> +}
>  }
>  break;
>  }
> @@ -173,12 +185,12 @@ apr_status_t ap_http_chunk_filter(ap_fil
>   * FLUSH bucket, or appended to the brigade
>   */
>  e = apr_bucket_immortal_create(CRLF_ASCII, 2, c->bucket_alloc);
> -if (eos != NULL) {
> -APR_BUCKET_INSERT_BEFORE(eos, e);
> -}
> -else if (flush != NULL) {
> +if (flush != NULL) {
>  APR_BUCKET_INSERT_BEFORE(flush, e);
>  }
> +else if (eos != NULL) {
> +APR_BUCKET_INSERT_BEFORE(eos, e);
> +}
>  else {
>  APR_BRIGADE_INSERT_TAIL(b, e);
>  }


Regards;
Yann.

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Yann Ylavic]

On Wed, Dec 20, 2023 at 4:57 PM Joe Orton  wrote:
>
> On Wed, Dec 20, 2023 at 04:24:32PM +0100, Ruediger Pluem wrote:
> > On 12/20/23 4:08 PM, Yann Ylavic wrote:
> > > On Wed, Dec 20, 2023 at 2:40 PM Joe Orton  wrote:
> > >> https://github.com/apache/httpd/pull/400
> > >
> > > Thanks, looks good to me.
> >
> > +1
>
> Thanks a lot for the quick reviews. Merged in r1914804 and happy Xmas :)

Happy Holidays! ;)

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Yann Ylavic]

On Wed, Dec 20, 2023 at 4:45 PM Yann Ylavic  wrote:
>
> On Wed, Dec 20, 2023 at 4:18 PM Eric Norris  wrote:
> >
> > On Wed, Dec 20, 2023 at 10:09 AM Yann Ylavic  wrote:
> > >
> > > So I think what the POC or mod_php should be doing is [FLUSH EOS] or
> > > something might not work in the chain sooner or later?
> >
> > I believe that is what the POC was doing here
> > https://gist.github.com/ericnorris/37c7dac401b50d270867e82a47bdd294#file-mod_example_hooks-c-L58-L64
> > - unfortunately though, the chunk filter behavior requires that we
> > send another FLUSH bucket
> > (https://gist.github.com/ericnorris/37c7dac401b50d270867e82a47bdd294#file-mod_example_hooks-c-L67-L68)
> > in order to get the last chunk marker to go immediately to the client.
>
> Ah ok I see now (didn't look at the PR before where the POC is
> linked), besides the second FLUSH it seems to be doing things
> correctly.
>
> > Without this, the last chunk marker sits in the bucket brigade until
> > the POC finishes and something in the Apache internals sends it out. I
> > agree though that sending a flush after an EOS is strange, and I noted
> > in my response to Joe that maybe the chunk filter change alone is
> > enough to solve our problem.
>
> I think it's fixed by Joe's PR/patch, the last-chunk is now inserted
> before the FLUSH (previously it was before EOS even if a FLUSH was
> there too).
>
> But Joe's patch won't make it before the next release at best, until
> then you could either:
> 1. [FLUSH][EOS] (two passes on r->output_filters)
> 2. [EOS][FLUSH] (first EOS passed on r->output_filters, second FLUSH
> passed on r->connection->output_filters).
> Not pretty but should work..

You might want to consider "FlushMaxPipelined 0" in the VirtualHost if
you want every response to be flushed too..

>
> Regards;
> Yann.

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Yann Ylavic]

On Wed, Dec 20, 2023 at 4:18 PM Eric Norris  wrote:
>
> On Wed, Dec 20, 2023 at 10:09 AM Yann Ylavic  wrote:
> >
> > So I think what the POC or mod_php should be doing is [FLUSH EOS] or
> > something might not work in the chain sooner or later?
>
> I believe that is what the POC was doing here
> https://gist.github.com/ericnorris/37c7dac401b50d270867e82a47bdd294#file-mod_example_hooks-c-L58-L64
> - unfortunately though, the chunk filter behavior requires that we
> send another FLUSH bucket
> (https://gist.github.com/ericnorris/37c7dac401b50d270867e82a47bdd294#file-mod_example_hooks-c-L67-L68)
> in order to get the last chunk marker to go immediately to the client.

Ah ok I see now (didn't look at the PR before where the POC is
linked), besides the second FLUSH it seems to be doing things
correctly.

> Without this, the last chunk marker sits in the bucket brigade until
> the POC finishes and something in the Apache internals sends it out. I
> agree though that sending a flush after an EOS is strange, and I noted
> in my response to Joe that maybe the chunk filter change alone is
> enough to solve our problem.

I think it's fixed by Joe's PR/patch, the last-chunk is now inserted
before the FLUSH (previously it was before EOS even if a FLUSH was
there too).

But Joe's patch won't make it before the next release at best, until
then you could either:
1. [FLUSH][EOS] (two passes on r->output_filters)
2. [EOS][FLUSH] (first EOS passed on r->output_filters, second FLUSH
passed on r->connection->output_filters).
Not pretty but should work..

Regards;
Yann.

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Yann Ylavic]

On Wed, Dec 20, 2023 at 2:40 PM Joe Orton  wrote:
>
> I was surprised this made a difference to the behaviour on the wire. It
> seems like the chunk filter has suboptimal behaviour here. If you take
> an output brigade like either:
>
> a) [HEAP FLUSH EOS]
> b) [HEAP FLUSH EOS FLUSH]
>
> in both cases the chunk filter would output two brigades:
>
> [chunk-hdr HEAP crlf FLUSH] [last-chunk EOS]
>
> Significantly there is no FLUSH in the second brigade even for case (b),
> because the chunk filter also drops everything after EOS. It would be
> clearly better/correct if the chunk filter produces a single brigade for
> this very common output pattern:
>
> [chunk-hdr HEAP crlf last-chunk FLUSH EOS]
>
> correctly preserving the semantics of the FLUSH. I've tried this here:
>
> https://github.com/apache/httpd/pull/400

Thanks, looks good to me.

> >
> > On Mon, Oct 9, 2023 at 2:50 PM Eric Norris  wrote:
> > >
> > > At Etsy, we use mod_php and were investigating what we thought was
> > > surprising behavior - that code executed during PHP's shutdown phase
> > > prevented the request from terminating, even if we didn't expect to send
> > > any additional output to the client.
> > >
> > > We determined that this was not surprising given mod_php's
> > > implementation, but after we developed a proof-of-concept patch that
> > > sent an EOS bucket and a flush bucket via a "userland" PHP function, we
> > > were surprised that it didn't work when compression was enabled for the
> > > request.

I'm wondering if these cases are valid/supported though:
c) [HEAP EOS FLUSH]
d) [HEAP EOS] [FLUSH] (with separate FLUSH but on r->output_filters still)
which seems to be what mod_php and the "userland" POC do?

I thought nothing should be sent on r->output_filters after EOS (only
c->output_filters might forward metadata in between requests), and at
least in ap_http_chunk_filter() this won't work since, as Joe said,
everything after EOS being dropped breaks c) and the filter not
removing itself after EOS breaks d).

So I think what the POC or mod_php should be doing is [FLUSH EOS] or
something might not work in the chain sooner or later?


Regards;
Yann.

Re: svn commit: r1914365 - in /httpd/httpd/trunk: changes-entries/ssl-providers.txt docs/log-message-tags/next-number docs/manual/mod/mod_ssl.xml modules/ssl/ssl_engine_init.c modules/ssl/ssl_engine_p

[Yann Ylavic]

On Wed, Dec 6, 2023 at 11:05 AM Yann Ylavic  wrote:
>
> On Tue, Dec 5, 2023 at 4:26 PM  wrote:
> >
> > Author: jorton
> > Date: Tue Dec  5 15:26:22 2023
> > New Revision: 1914365
> >
> > URL: http://svn.apache.org/viewvc?rev=1914365=rev
> > Log:
> > mod_ssl: Add support for loading keys from OpenSSL 3.x providers via
> > the STORE API. Separates compile-time support for the STORE API
> > (supported in 3.x) from support for the ENGINE API (deprecated in
> > 3.x).
> >
> > * modules/ssl/ssl_private.h: Define MODSSL_HAVE_OPENSSL_STORE for
> >   OpenSSL 3.0+.
> >
> > * modules/ssl/ssl_engine_pphrase.c (modssl_load_store_uri,
> >   modssl_load_keypair_store): New functions.
> >   (modssl_load_keypair_engine): Renamed from modssl_load_keypair_engine.
> >   (modssl_load_engine_keypair): Reimplement to use new STORE-based
> >   functions if SSLCryptoDevice was not configured, or else old
> >   ENGINE implementation.
> >
> > * modules/ssl/ssl_util.c (modssl_is_engine_id): Match pkcs11: URIs
> >   also for the OpenSSL 3.x STORE API.
> >
> > * modules/ssl/ssl_engine_init.c (ssl_init_server_certs): Tweak log
> >   message on error paths for the provider/STORE case.
> >
> > Signed-off-by: Ingo Franzki 
> > Submitted by: Ingo Franzki 
> > Github: closes #397, closes #398
> >
> []
> >
> > Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c
> > URL: 
> > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c?rev=1914365=1914364=1914365=diff
> > ==
> > --- httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c (original)
> > +++ httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c Tue Dec  5 15:26:22 
> > 2023
> []
> > +
> > +apr_status_t modssl_load_engine_keypair(server_rec *s, apr_pool_t *p,
> > +const char *vhostid,
> > +const char *certid, const char 
> > *keyid,
> > +X509 **pubkey, EVP_PKEY **privkey)
> > +{
> > +#if MODSSL_HAVE_OPENSSL_STORE
> > +SSLModConfigRec *mc = myModConfig(s);
> > +
> > +if (!mc->szCryptoDevice)
> > +return modssl_load_keypair_store(s, p, vhostid, certid, keyid,
> > + pubkey, privkey);
> > +#endif
> > +#if MODSSL_HAVE_ENGINE_API
> > +return modssl_load_keypair_engine(s, p, vhostid, certid, keyid,
> > +  pubkey, privkey);
> >  #else
> >  return APR_ENOTIMPL;
> >  #endif
>
> Hm, it seems that with openssl-3+ we can handle/support pkcs#11 URIs
> only via the store API now.
> modssl_load_keypair_store() will fail/die if it can't find the
> cert/key in the STORE, but couldn't modssl_load_keypair_engine() find
> them if the OpenSSL configuration (and underlying lib, e.g. libp11)
> still uses the legacy engine API? The engine API is still available in
> openssl-3 and might still be used IIUC.
>
> So don't we need something like this:
>
> apr_status_t rv = APR_ENOTIMPL;
> #if MODSSL_HAVE_OPENSSL_STORE
> SSLModConfigRec *mc = myModConfig(s);
> if (!mc->szCryptoDevice)
> rv = modssl_load_keypair_store(s, p, vhostid, certid, keyid,
>pubkey, privkey);
> #endif
> #if MODSSL_HAVE_ENGINE_API
> if (rv == APR_ENOTIMPL)
> rv = modssl_load_keypair_engine(s, p, vhostid, certid, keyid,
> pubkey, privkey);
> #endif
> return rv;
>
> and somehow make modssl_load_keypair_store() return APR_ENOTIMPL when
> there is no store to get the cert/key from?

Oh, scratch that. Actually the engine API requires a "SSLCryptoDevice
pkcs11" too, so we wouldn't take the !mc->szCryptoDevice path.
Sorry for the noise.

Regards;
Yann.

Re: svn commit: r1914365 - in /httpd/httpd/trunk: changes-entries/ssl-providers.txt docs/log-message-tags/next-number docs/manual/mod/mod_ssl.xml modules/ssl/ssl_engine_init.c modules/ssl/ssl_engine_p

[Yann Ylavic]

On Tue, Dec 5, 2023 at 4:26 PM  wrote:
>
> Author: jorton
> Date: Tue Dec  5 15:26:22 2023
> New Revision: 1914365
>
> URL: http://svn.apache.org/viewvc?rev=1914365=rev
> Log:
> mod_ssl: Add support for loading keys from OpenSSL 3.x providers via
> the STORE API. Separates compile-time support for the STORE API
> (supported in 3.x) from support for the ENGINE API (deprecated in
> 3.x).
>
> * modules/ssl/ssl_private.h: Define MODSSL_HAVE_OPENSSL_STORE for
>   OpenSSL 3.0+.
>
> * modules/ssl/ssl_engine_pphrase.c (modssl_load_store_uri,
>   modssl_load_keypair_store): New functions.
>   (modssl_load_keypair_engine): Renamed from modssl_load_keypair_engine.
>   (modssl_load_engine_keypair): Reimplement to use new STORE-based
>   functions if SSLCryptoDevice was not configured, or else old
>   ENGINE implementation.
>
> * modules/ssl/ssl_util.c (modssl_is_engine_id): Match pkcs11: URIs
>   also for the OpenSSL 3.x STORE API.
>
> * modules/ssl/ssl_engine_init.c (ssl_init_server_certs): Tweak log
>   message on error paths for the provider/STORE case.
>
> Signed-off-by: Ingo Franzki 
> Submitted by: Ingo Franzki 
> Github: closes #397, closes #398
>
[]
>
> Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c?rev=1914365=1914364=1914365=diff
> ==
> --- httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c Tue Dec  5 15:26:22 
> 2023
[]
> +
> +apr_status_t modssl_load_engine_keypair(server_rec *s, apr_pool_t *p,
> +const char *vhostid,
> +const char *certid, const char 
> *keyid,
> +X509 **pubkey, EVP_PKEY **privkey)
> +{
> +#if MODSSL_HAVE_OPENSSL_STORE
> +SSLModConfigRec *mc = myModConfig(s);
> +
> +if (!mc->szCryptoDevice)
> +return modssl_load_keypair_store(s, p, vhostid, certid, keyid,
> + pubkey, privkey);
> +#endif
> +#if MODSSL_HAVE_ENGINE_API
> +return modssl_load_keypair_engine(s, p, vhostid, certid, keyid,
> +  pubkey, privkey);
>  #else
>  return APR_ENOTIMPL;
>  #endif

Hm, it seems that with openssl-3+ we can handle/support pkcs#11 URIs
only via the store API now.
modssl_load_keypair_store() will fail/die if it can't find the
cert/key in the STORE, but couldn't modssl_load_keypair_engine() find
them if the OpenSSL configuration (and underlying lib, e.g. libp11)
still uses the legacy engine API? The engine API is still available in
openssl-3 and might still be used IIUC.

So don't we need something like this:

apr_status_t rv = APR_ENOTIMPL;
#if MODSSL_HAVE_OPENSSL_STORE
SSLModConfigRec *mc = myModConfig(s);
if (!mc->szCryptoDevice)
rv = modssl_load_keypair_store(s, p, vhostid, certid, keyid,
   pubkey, privkey);
#endif
#if MODSSL_HAVE_ENGINE_API
if (rv == APR_ENOTIMPL)
rv = modssl_load_keypair_engine(s, p, vhostid, certid, keyid,
pubkey, privkey);
#endif
return rv;

and somehow make modssl_load_keypair_store() return APR_ENOTIMPL when
there is no store to get the cert/key from?


Regards;
Yann.

Re: mod_ssl: Add support for loading keys from OpenSSL 3.x providers via STORE

[Yann Ylavic]

Hi;

On Mon, Dec 4, 2023 at 8:53 AM Ingo Franzki  wrote:
>
> On 02.12.2023 11:20, Graham Leggett via dev wrote:
> > On 27 Nov 2023, at 15:02, Ingo Franzki  wrote:
> >
> >> The mod_ssl module has support for loading keys and certificates from 
> >> OpenSSL engines via PKCS#11 URIs at SSLCertificateFile and 
> >> SSLCertificateKeyFile, e.g. using the PKCS#11 engine part of libp11 
> >> (https://github.com/OpenSC/libp11).
> >>
> >> This works fine, but with OpenSSL 3.0 engines got deprecated, and a new 
> >> provider concept is used.
> >> OpenSSL 1.1.1 is no longer supported by the OpenSSL organization 
> >> (https://www.openssl.org/blog/blog/2023/09/11/eol-111/),
> >> and newer distributions all have OpenSSL 3.x included.
> >> Currently, engines do still work, bit since they are deprecated, they will 
> >> at some point in time no longer be working.
> >>
> >> With OpenSSL 3.x providers one can implements loading of keys and 
> >> certificates by implementing a STORE method.
> >> With this, keys and certificates can be loaded for example from PKCS#11 
> >> modules via PKCS#11 URIs, just like it was possible with an PKCS#11 engine.
> >>
> >> Please find below some code changes required to support loading the server 
> >> private key and certificates from a PKCS#11 provider using OpenSSL STORE 
> >> providers.
> >
> > Definite +1 in principle.

+1, thanks for the patch!

>
> Please see the patch file attached.
> I also fixed to minor bugs that I found during testing.
>
> You can also look at the patch here:
> https://github.com/ifranzki/httpd/commit/4bb3ea191bc2c77608b4811817ad7f63177dd931
>
> If you want, I can even submit a pull request to 
> https://github.com/apache/httpd.
> Let me know what you prefer.

Yes please do this, it's easier to comment on the code and it also
gets tested by the ci.


Regards;
Yann.

Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Yann Ylavic]

On Fri, Nov 24, 2023 at 3:28 PM Ruediger Pluem  wrote:
>
> On 11/24/23 10:59 AM, Yann Ylavic wrote:
> > On Thu, Nov 23, 2023 at 5:47 PM Ruediger Pluem  wrote:
> >>
> >> On 11/23/23 5:05 PM, Yann Ylavic wrote:
> >>> On Thu, Nov 23, 2023 at 4:46 PM Yann Ylavic  wrote:
> >>>>
> >>>> On Thu, Nov 23, 2023 at 4:17 PM Ruediger Pluem  wrote:
> >>>>>
> >>>>> On 11/23/23 3:53 PM, Yann Ylavic wrote:
> >>
> >>>>> I am asking because I guess I am hit now by races in the byrequests LB
> >>>>> with the worker->s->lbstatus.
> >>>>> I probably want to look for a way to solve this in a more generic way
> >>>>> for various shared worker fields.
> >>>>
> >>>> lbstatus is an int so the 32bit apr_atomic API could do I think.
> >>>
> >>> I mean, if lbstatus becomes inconsistent because of the race then we
> >>
> >> My current theory is that this is the case.
> >>
> >>> can do something, but if it is e.g. that a worker switches from error
> >>> state to non-error become some threads can connect while some others
> >>> can't this is something we should address elsewhere (like a failure
> >>> threshold).
> >>>
> >>>> Maybe we need some ap_atomic_{int,long,size_t,..}_*() helpers for
> >>>> system dependent widths. It preferably would be implemented in APR but
> >>>> in the meantime we could have something in httpd already.
> >>>> What we should avoid IMO is needing 64bit atomics on 32bit systems
> >>>> (because we'd have to reimplement the mutexes etc), but apart from
> >>>> that I think we can wrap anything using the existing apr atomics.
> >>>
> >>> What we need for lbstatus is ap_atomic_int_or() and
> >>> ap_atomic_int_and_not() it seems, both could be implemented with a
> >>> cas32 loop.
> >>
> >> What functionality are ap_atomic_int_or() / ap_atomic_int_and_not() are 
> >> supposed to deliver?
> >> I am having a hard time guessing it from the name :-).
> >
> > Actually I mixed ->lbstatus and ->status, so I thought we needed an
> > atomic "or" and an atomic "and not" to handle the ->status mask..
> >
> > But ->lbstatus is much like the ->busy count and updated almost at the
> > same place, though it's an "int" and we can't reuse the same functions
> > for both :/
> > It seems that we need to make sure that 0 <= lbstatus <= INT_MAX too,
>
> I think lbstatus is allowed to become negative.

Tried something in https://github.com/apache/httpd/pull/396
WDYT?

Regards;
Yann.

Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Fri, Nov 24, 2023 at 12:41 PM Graham Leggett  wrote:
>
> On 24 Nov 2023, at 10:03, Yann Ylavic  wrote:
>
> > +1 this is pretty much what Rüdiger proposed earlier and it aligns
> > with the proposed 2.4.x backport so I understand better :)
>
> Rüdiger’s patch had conflicts in the time he posted it and now, I reapplied 
> his changes in the above patch and confirmed they worked.
>
> Who wants to commit? (Am happy to)

Sure, go ahead.

Regards;
Yann.

Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Thu, Nov 23, 2023 at 3:00 PM Ruediger Pluem  wrote:
>
> On 11/23/23 2:28 PM, Yann Ylavic wrote:
> > On Thu, Nov 23, 2023 at 1:33 PM Ruediger Pluem  wrote:
> >>
> >>
> >>
> >> On 11/23/23 12:52 PM, Graham Leggett via dev wrote:
> >>> On 23 Nov 2023, at 11:25, Ruediger Pluem  wrote:
> >>>
> >>>>> -if (req->dn == NULL || !*req->dn) {
> >>>>> -ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
> >>>>> -  "auth_ldap authorize: require ldap-filter: 
> >>>>> user's DN "
> >>>>> -  "has not been defined; failing authorization");
> >>>>> -return AUTHZ_DENIED;
> >>>>> +if (req->dn == NULL || !*req->dn) {
> >>>>> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
> >>>>> +  "auth_ldap authorize: require ldap-search: 
> >>>>> user's DN "
> >>>>> +  "has not been defined; failing 
> >>>>> authorization");
> >>>>> +return AUTHZ_DENIED;
> >>>>> +}
> >>>>
> >>>> Why do we need to get the dn in case that r->user is not NULL and why is 
> >>>> it a reason to fail if we don't get a dn for this user?
> >>>
> >>> This message is misleading, the DN we care about is not the DN of the 
> >>> user, but rather the DN of the object returned in the
> >>> ldap-search, which may or may not bear a relation to the user.
> >>
> >> Unfortunately the message is correct and we do just that via 
> >> get_dn_for_nonldap_authn (line 1451). The DN you mention we look for
> >> later in line 1480 via util_ldap_cache_getuserdn.
> >
> > My guess was that if r->user is set we always want to use it for ldap
> > requests/authn? Or maybe there should be another Require ldap-!search
> > configured to achieve that (if wanted) since ldap-search is special..
>
> I think for ldap-search things depend on the configured filter expression. It 
> may involve a previously authenticated user, in
> which case it has similar approaches like ldap-filter or even require 
> valid-user or it does not include that and may remind more
> of a mod_authz_host with a dynamic IP list stored in LDAP. Or the certificate 
> example Graham gave.
>
> >
> >> BTW: I guess the following patch is missing as we don't store the dn / 
> >> vals we get from util_ldap_cache_getuserdn but just
> >> "forget" it once we leave the function.
> >>
> >>
> >> Index: modules/aaa/mod_authnz_ldap.c
> >> ===
> >> --- modules/aaa/mod_authnz_ldap.c   (revision 1914069)
> >> +++ modules/aaa/mod_authnz_ldap.c   (working copy)
> >> @@ -1482,10 +1482,12 @@
> >>
> >>  /* Make sure that the filtered search returned a single dn */
> >>  if (result == LDAP_SUCCESS && dn) {
> >> +req->dn = dn;
> >> +req->vals = vals;
> >>  ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02631)
> >>"auth_ldap authorize: require ldap-search: "
> >>"authorization successful");
> >> -set_request_vars(r, LDAP_AUTHZ, req->vals);
> >> +set_request_vars(r, LDAP_AUTHZ, vals);
> >>  return AUTHZ_GRANTED;
> >>  }
> >>  else {
> >>
> >>
> >>>
> >>> For example the ldap-search might be doing a lookup on (sucks thumb) the 
> >>> issuer of a certificate, which if it matches some LDAP
> >>> object means it is allowed. The DN will not refer to the user, but 
> >>> something else.
> >>
> >> I get that. Hence I wonder why we care of the dn fitting to r->user set 
> >> before.
> >>
> >>>
> >>> On a side note, ldap-search requires that exactly one LDAP object 
> >>> matches, we might want to support many matches, or a specific
> >>> number of matches.
> >
> > I still don't get where "Make sure that the filtered search returned a
> > single dn" is enforced, is it because result != LDAP_SUCCESS
> > otherwise, or dn == NULL if there are multiple objects/DNs?
>
> Have a look at line 2117 and following in modules/ldap/util_ldap.c. We call 
> it in line 1480 of modules/aaa/mod_authnz_ldap.c via
> util_ldap_cache_getuserdn. Result will be LDAP_NO_SUCH_OBJECT and dn will 
> stay the same as before the call.

Thanks, got it.


Regards;
Yann.

Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Thu, Nov 23, 2023 at 11:06 PM Graham Leggett via dev
 wrote:
>
> On 23 Nov 2023, at 13:28, Yann Ylavic  wrote:
>
> Unfortunately the message is correct and we do just that via 
> get_dn_for_nonldap_authn (line 1451). The DN you mention we look for
> later in line 1480 via util_ldap_cache_getuserdn.
>
>
> My guess was that if r->user is set we always want to use it for ldap
> requests/authn? Or maybe there should be another Require ldap-!search
>
> configured to achieve that (if wanted) since ldap-search is special..
>
>
> We already have that, it’s ldap-filter.
>
> In more detail, ldap-filter takes an LDAP filter, for example 
> (objectclass=mailRecipient), and then it takes an r->user, for example 
> f...@example.com, and then it combines them to create a new filter 
> (&(uid=f...@example.com)(objectclass=mailRecipient)), and if exactly one (not 
> zero, not two) objects exist, it is a match.
>
> ldap-search ignores the r->user special case and broadens this to any 
> expression you can image.
>
> On a side note, ldap-search requires that exactly one LDAP object matches, we 
> might want to support many matches, or a specific
> number of matches.
>
>
> I still don't get where "Make sure that the filtered search returned a
> single dn" is enforced, is it because result != LDAP_SUCCESS
> otherwise, or dn == NULL if there are multiple objects/DNs?
>
>
> It’s enforced here:
>
> https://github.com/apache/httpd/blob/trunk/modules/ldap/util_ldap.c#L2118
>
> For historic reasons, the function is called uldap_cache_getuserdn(), but 
> what the function actually does is run a single LDAP query, and expect one 
> and only one object in return.
>
> ldap-search runs a single LDAP query, and expects one and only one object in 
> return, it just isn’t a user.

Thanks for the explanation.


Regards;
Yann.

Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Fri, Nov 24, 2023 at 10:45 AM Graham Leggett via dev
 wrote:
>
> I completely misunderstood this - I had the idea that build_request_config() 
> was being removed, when it was being left behind, sorry about that.
> The patch that applies to trunk looks like this, and I just tested it and it 
> works:
>
> Index: modules/aaa/mod_authnz_ldap.c
> ===
> --- modules/aaa/mod_authnz_ldap.c (revision 1914067)
> +++ modules/aaa/mod_authnz_ldap.c (working copy)
> @@ -1441,24 +1441,6 @@
>  req = build_request_config(r);
>  }
>  ldc = get_connection_for_authz(r, LDAP_SEARCH);
> -if (!req->dn && r->user) {
> -authz_status rv;
> -if (!*r->user) {
> -ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(10487)
> -  "ldap authorize: Userid is blank, AuthType=%s",
> -  r->ap_auth_type);
> -}
> -rv = get_dn_for_nonldap_authn(r, ldc);
> -if (rv != AUTHZ_GRANTED) {
> -return rv;
> -}
> -if (req->dn == NULL || !*req->dn) {
> -ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
> -  "auth_ldap authorize: require ldap-search: user's 
> DN "
> -  "has not been defined; failing authorization");
> -return AUTHZ_DENIED;
> -}
> -}
>
>  require = ap_expr_str_exec(r, expr, );
>  if (err) {
> @@ -1482,6 +1464,7 @@
>
>  /* Make sure that the filtered search returned a single dn */
>  if (result == LDAP_SUCCESS && dn) {
> +req->dn = dn;
>  ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02631)
>"auth_ldap authorize: require ldap-search: "
>"authorization successful");

+1 this is pretty much what Rüdiger proposed earlier and it aligns
with the proposed 2.4.x backport so I understand better :)

Regards;
Yann.

Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Yann Ylavic]

On Thu, Nov 23, 2023 at 5:47 PM Ruediger Pluem  wrote:
>
> On 11/23/23 5:05 PM, Yann Ylavic wrote:
> > On Thu, Nov 23, 2023 at 4:46 PM Yann Ylavic  wrote:
> >>
> >> On Thu, Nov 23, 2023 at 4:17 PM Ruediger Pluem  wrote:
> >>>
> >>> On 11/23/23 3:53 PM, Yann Ylavic wrote:
>
> >>> I am asking because I guess I am hit now by races in the byrequests LB
> >>> with the worker->s->lbstatus.
> >>> I probably want to look for a way to solve this in a more generic way
> >>> for various shared worker fields.
> >>
> >> lbstatus is an int so the 32bit apr_atomic API could do I think.
> >
> > I mean, if lbstatus becomes inconsistent because of the race then we
>
> My current theory is that this is the case.
>
> > can do something, but if it is e.g. that a worker switches from error
> > state to non-error become some threads can connect while some others
> > can't this is something we should address elsewhere (like a failure
> > threshold).
> >
> >> Maybe we need some ap_atomic_{int,long,size_t,..}_*() helpers for
> >> system dependent widths. It preferably would be implemented in APR but
> >> in the meantime we could have something in httpd already.
> >> What we should avoid IMO is needing 64bit atomics on 32bit systems
> >> (because we'd have to reimplement the mutexes etc), but apart from
> >> that I think we can wrap anything using the existing apr atomics.
> >
> > What we need for lbstatus is ap_atomic_int_or() and
> > ap_atomic_int_and_not() it seems, both could be implemented with a
> > cas32 loop.
>
> What functionality are ap_atomic_int_or() / ap_atomic_int_and_not() are 
> supposed to deliver?
> I am having a hard time guessing it from the name :-).

Actually I mixed ->lbstatus and ->status, so I thought we needed an
atomic "or" and an atomic "and not" to handle the ->status mask..

But ->lbstatus is much like the ->busy count and updated almost at the
same place, though it's an "int" and we can't reuse the same functions
for both :/
It seems that we need to make sure that 0 <= lbstatus <= INT_MAX too,
and later we'll possibly want atomic add/set for ->transferred and
->read used by bytraffic which are off_t..

So the abstraction I'm seeing is:
  PROXY_DECLARE(int) ap_atomic_int_get(volatile int *val);
  PROXY_DECLARE(void) ap_atomic_int_set(volatile int *val, int to);
  PROXY_DECLARE(int) ap_atomic_int_sub_not_min(volatile int *val, int
sub, int min);
  PROXY_DECLARE(int) ap_atomic_int_add_not_max(volatile int *val, int
add, int max);
which all return the old *val (maybe ap_atomic_int_set() too).
The same for uint, long, ulong, and then point size_t/off_t to them
depending on their width. We'd implement the ones needed as we use
them only, but note that if we need off_t (or longlong or ulonglong)
at some point we possibly want to require a 64bit system for them (as
I said earlier).
Finally we'd use apr_atomic_int_* for ->lbstatus, apr_atomic_size[t]_*
for ->busy etc.

So it's an abstraction but not really a short one to write, should be
quite mechanical though with the same code structure as what we have
in this commit (possibly we can use macros for the implementation).
Once we have that, it could be useful for APR too, though there we'd
rather point the system types to the 32/64bits base ones.
Sorry I can't think of both generic and simpler, it's still better
than ap_proxy_{get,set,sub,add}_{many_proxy_worker_shared_fields}()
IMO.

Regards;
Yann.

Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Yann Ylavic]

On Thu, Nov 23, 2023 at 4:46 PM Yann Ylavic  wrote:
>
> On Thu, Nov 23, 2023 at 4:17 PM Ruediger Pluem  wrote:
> >
> > On 11/23/23 3:53 PM, Yann Ylavic wrote:
> > >
> > > As for the 64bit atomics vs APR version dance, it's because the former
> > > are not available before apr-1.7 and likely not reliable before 1.7.4
> > > (at least for some architectures where builtins are not available). In
> > > any case we need a fallback for apr < 1.7, but maybe to keep this
> > > simpler we should fall back to non-atomics (as before) in this case.
> > > It all looks over complicated for the feature, but I can't think of
> > > something simpler and still correct..
> >
> > Understood.
> >
> > I am asking because I guess I am hit now by races in the byrequests LB
> > with the worker->s->lbstatus.
> > I probably want to look for a way to solve this in a more generic way
> > for various shared worker fields.
>
> lbstatus is an int so the 32bit apr_atomic API could do I think.

I mean, if lbstatus becomes inconsistent because of the race then we
can do something, but if it is e.g. that a worker switches from error
state to non-error become some threads can connect while some others
can't this is something we should address elsewhere (like a failure
threshold).

> Maybe we need some ap_atomic_{int,long,size_t,..}_*() helpers for
> system dependent widths. It preferably would be implemented in APR but
> in the meantime we could have something in httpd already.
> What we should avoid IMO is needing 64bit atomics on 32bit systems
> (because we'd have to reimplement the mutexes etc), but apart from
> that I think we can wrap anything using the existing apr atomics.

What we need for lbstatus is ap_atomic_int_or() and
ap_atomic_int_and_not() it seems, both could be implemented with a
cas32 loop.

>
> Regards;
> Yann.

Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Yann Ylavic]

On Thu, Nov 23, 2023 at 4:17 PM Ruediger Pluem  wrote:
>
> On 11/23/23 3:53 PM, Yann Ylavic wrote:
> >
> > As for the 64bit atomics vs APR version dance, it's because the former
> > are not available before apr-1.7 and likely not reliable before 1.7.4
> > (at least for some architectures where builtins are not available). In
> > any case we need a fallback for apr < 1.7, but maybe to keep this
> > simpler we should fall back to non-atomics (as before) in this case.
> > It all looks over complicated for the feature, but I can't think of
> > something simpler and still correct..
>
> Understood.
>
> I am asking because I guess I am hit now by races in the byrequests LB
> with the worker->s->lbstatus.
> I probably want to look for a way to solve this in a more generic way
> for various shared worker fields.

lbstatus is an int so the 32bit apr_atomic API could do I think.
Maybe we need some ap_atomic_{int,long,size_t,..}_*() helpers for
system dependent widths. It preferably would be implemented in APR but
in the meantime we could have something in httpd already.
What we should avoid IMO is needing 64bit atomics on 32bit systems
(because we'd have to reimplement the mutexes etc), but apart from
that I think we can wrap anything using the existing apr atomics.

Regards;
Yann.

Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h

[Yann Ylavic]

On Thu, Nov 23, 2023 at 12:11 PM Ruediger Pluem  wrote:
>
> On 9/11/23 3:50 PM, jfcl...@apache.org wrote:
> > Author: jfclere
> > Date: Mon Sep 11 13:50:21 2023
> > New Revision: 1912245
> >
> > URL: http://svn.apache.org/viewvc?rev=1912245=rev
> > Log:
> > Arrange the bybusyness logic and prevent bad busy values
> > this closes #383
[]
> > --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> > +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Mon Sep 11 13:50:21 2023
> > @@ -21,6 +21,7 @@
> >  #include "apr_version.h"
> >  #include "apr_strings.h"
> >  #include "apr_hash.h"
> > +#include "apr_atomic.h"
> >  #include "http_core.h"
> >  #include "proxy_util.h"
> >  #include "ajp.h"
> > @@ -4984,6 +4985,124 @@ PROXY_DECLARE(apr_status_t) ap_proxy_tun
> >  return APR_SUCCESS;
> >  }
> >
> > +PROXY_DECLARE(apr_status_t) decrement_busy_count(void *worker_)
>
> Why can't we use the respective apr_atomic_sub/add/inc/dec* functions here
> instead of the logic below?

This has been discussed in https://github.com/apache/httpd/pull/383,
at least for why the "if atomic_dec() < 0 then atomic_inc()"
proposed originally was racy.
So the below implements dec_not_zero and inc_not_at_max to keep ->busy
in the bounds, like the original non-atomic functions.
I think the under/overflows could still happen without this because,
even if we inc per request and dec on request pool cleanup, there is a
balancer->lbmethod->reset() which may clear ->busy at any time (IIUC),
not to talk about a child crash (though then ->busy may not be very
reliable anyway, under/overflow or not..).

As for the 64bit atomics vs APR version dance, it's because the former
are not available before apr-1.7 and likely not reliable before 1.7.4
(at least for some architectures where builtins are not available). In
any case we need a fallback for apr < 1.7, but maybe to keep this
simpler we should fall back to non-atomics (as before) in this case.
It all looks over complicated for the feature, but I can't think of
something simpler and still correct..

>
> > +{
> > +apr_size_t val;
> > +proxy_worker *worker = worker_;
> > +
> > +#if APR_SIZEOF_VOIDP == 4
> > +AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint32_t));
> > +val = apr_atomic_read32(>s->busy);
> > +while (val > 0) {
> > +apr_size_t old = val;
> > +val = apr_atomic_cas32(>s->busy, val - 1, old);
> > +if (val == old) {
> > +break;
> > +}
> > +}
> > +#elif APR_VERSION_AT_LEAST(1,7,4) /* APR 64bit atomics not safe before 
> > 1.7.4 */
> > +AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint64_t));
> > +val = apr_atomic_read64(>s->busy);
> > +while (val > 0) {
> > +apr_size_t old = val;
> > +val = apr_atomic_cas64(>s->busy, val - 1, old);
> > +if (val == old) {
> > +break;
> > +}
> > +}
> > +#else /* Use atomics for (64bit) pointers */
> > +void *volatile *busy_p = (void *)>s->busy;
> > +AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(void*));
> > +AP_DEBUG_ASSERT((apr_uintptr_t)busy_p % sizeof(void*) == 0);
> > +val = (apr_uintptr_t)apr_atomic_casptr((void *)busy_p, NULL, NULL);
> > +while (val > 0) {
> > +apr_size_t old = val;
> > +val = (apr_uintptr_t)apr_atomic_casptr((void *)busy_p,
> > +   (void *)(apr_uintptr_t)(val 
> > - 1),
> > +   (void *)(apr_uintptr_t)old);
> > +if (val == old) {
> > +break;
> > +}
> > +}
> > +#endif
> > +return APR_SUCCESS;
> > +}
> > +
> > +PROXY_DECLARE(void) increment_busy_count(proxy_worker *worker)
> > +{
> > +apr_size_t val;
> > +#if APR_SIZEOF_VOIDP == 4
> > +AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint32_t));
> > +val = apr_atomic_read32(>s->busy);
> > +while (val < APR_INT32_MAX) {
> > +apr_size_t old = val;
> > +val = apr_atomic_cas32(>s->busy, val + 1, old);
> > +if (val == old) {
> > +break;
> > +}
> > +}
> > +#elif APR_VERSION_AT_LEAST(1,7,4) /* APR 64bit atomics not safe before 
> > 1.7.4 */
> > +AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint64_t));
> > +val = apr_atomic_read64(>s->busy);
> > +while (val < APR_INT64_MAX) {
> > +apr_size_t old = val;
> > +val = apr_atomic_cas64(>s->busy, val + 1, old);
> > +if (val == old) {
> > +break;
> > +}
> > +}
> > +#else /* Use atomics for (64bit) pointers */
> > +void *volatile *busy_p = (void *)>s->busy;
> > +AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(void*));
> > +AP_DEBUG_ASSERT((apr_uintptr_t)busy_p % sizeof(void*) == 0);
> > +val = (apr_uintptr_t)apr_atomic_casptr((void *)busy_p, NULL, NULL);
> > +while (val < APR_INT64_MAX) {
> > +apr_size_t old = val;
> > +val = (apr_uintptr_t)apr_atomic_casptr((void *)busy_p,
> > +   

Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Thu, Nov 23, 2023 at 1:33 PM Ruediger Pluem  wrote:
>
>
>
> On 11/23/23 12:52 PM, Graham Leggett via dev wrote:
> > On 23 Nov 2023, at 11:25, Ruediger Pluem  wrote:
> >
> >>> -if (req->dn == NULL || !*req->dn) {
> >>> -ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
> >>> -  "auth_ldap authorize: require ldap-filter: user's 
> >>> DN "
> >>> -  "has not been defined; failing authorization");
> >>> -return AUTHZ_DENIED;
> >>> +if (req->dn == NULL || !*req->dn) {
> >>> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
> >>> +  "auth_ldap authorize: require ldap-search: 
> >>> user's DN "
> >>> +  "has not been defined; failing authorization");
> >>> +return AUTHZ_DENIED;
> >>> +}
> >>
> >> Why do we need to get the dn in case that r->user is not NULL and why is 
> >> it a reason to fail if we don't get a dn for this user?
> >
> > This message is misleading, the DN we care about is not the DN of the user, 
> > but rather the DN of the object returned in the
> > ldap-search, which may or may not bear a relation to the user.
>
> Unfortunately the message is correct and we do just that via 
> get_dn_for_nonldap_authn (line 1451). The DN you mention we look for
> later in line 1480 via util_ldap_cache_getuserdn.

My guess was that if r->user is set we always want to use it for ldap
requests/authn? Or maybe there should be another Require ldap-!search
configured to achieve that (if wanted) since ldap-search is special..

> BTW: I guess the following patch is missing as we don't store the dn / vals 
> we get from util_ldap_cache_getuserdn but just
> "forget" it once we leave the function.
>
>
> Index: modules/aaa/mod_authnz_ldap.c
> ===
> --- modules/aaa/mod_authnz_ldap.c   (revision 1914069)
> +++ modules/aaa/mod_authnz_ldap.c   (working copy)
> @@ -1482,10 +1482,12 @@
>
>  /* Make sure that the filtered search returned a single dn */
>  if (result == LDAP_SUCCESS && dn) {
> +req->dn = dn;
> +req->vals = vals;
>  ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02631)
>"auth_ldap authorize: require ldap-search: "
>"authorization successful");
> -set_request_vars(r, LDAP_AUTHZ, req->vals);
> +set_request_vars(r, LDAP_AUTHZ, vals);
>  return AUTHZ_GRANTED;
>  }
>  else {
>
>
> >
> > For example the ldap-search might be doing a lookup on (sucks thumb) the 
> > issuer of a certificate, which if it matches some LDAP
> > object means it is allowed. The DN will not refer to the user, but 
> > something else.
>
> I get that. Hence I wonder why we care of the dn fitting to r->user set 
> before.
>
> >
> > On a side note, ldap-search requires that exactly one LDAP object matches, 
> > we might want to support many matches, or a specific
> > number of matches.

I still don't get where "Make sure that the filtered search returned a
single dn" is enforced, is it because result != LDAP_SUCCESS
otherwise, or dn == NULL if there are multiple objects/DNs?


Regards;
Yann.

Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Wed, Nov 22, 2023 at 3:40 PM Graham Leggett via dev
 wrote:
>
> On 20 Nov 2023, at 15:32, Yann Ylavic  wrote:
>>
>> OK, I drop a new v3 here just in case and let those who know how LDAP
>> authn/authz work take whatever :)
>> This is just to show that there is some room for
>> factorization/disambiguation in this code..
>
> Just tested this, and it appears to work fine.
>
> One tiny detail, these is a logging line that refers to ldap-filter inside 
> ldap-search, but other than that, +1.
>
> +  "auth_ldap authorize: require ldap-filter: user's 
> DN "

Please take it, I do not have the environment for now to compile
and/or test this.

Regards;
Yann.

Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Mon, Nov 20, 2023 at 4:21 PM Ruediger Pluem  wrote:
>
> On 11/20/23 4:05 PM, Yann Ylavic wrote:
> > On Mon, Nov 20, 2023 at 3:46 PM Yann Ylavic  wrote:
> >>
> >> On Mon, Nov 20, 2023 at 2:33 PM Yann Ylavic  wrote:
> >>>
> >>> On Mon, Nov 20, 2023 at 1:57 PM Graham Leggett via dev
> >>>  wrote:
> >>>>
> >>>> On 20 Nov 2023, at 12:26, Ruediger Pluem  wrote:
> >>>>
> >>>> Or we need to ensure that authn_ldap_build_filter is NULL safe and 
> >>>> returns in a sensible way if user == NULL.
> >>>>
> >>>>
> >>>> This is the option we need I think - it’s possible that ldapsearch could 
> >>>> be used without a user.
> >>>
> >>> In the proposed 2.4.x backport of ldapsearch_check_authorization()
> >>> there is no call to get_dn_for_nonldap_authn() nor
> >>> authn_ldap_build_filter(). The Require expression is passed directly
> >>> to util_ldap_cache_getuserdn(), so what is building a filter with
> >>> r->user about in the ldapsearch case finally?
> >>
> >> I mean, isn't what we need for something like the attached patch?
> >> This would call get_dn_for_nonldap_authn() only "if we have been
> >> authenticated by some other module than mod_auth_ldap" (per comment in
> >> the code), and do nothing about r->user otherwise.
> >> Again, I don't really know how mod_ldap is supposed to work so
> >> possibly this is all irrelevant..
> >
> > A more complete/correct patch would be this attached v2 anyway.
>
> +1 to the stuff outside ldapsearch_check_authorization. For the stuff inside 
> ldapsearch_check_authorization I guess my patch
> is closer to what ldapsearch intends to do do.

OK, I drop a new v3 here just in case and let those who know how LDAP
authn/authz work take whatever :)
This is just to show that there is some room for
factorization/disambiguation in this code..


Regards;
Yann.
Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c	(revision 1913977)
+++ modules/aaa/mod_authnz_ldap.c	(working copy)
@@ -767,32 +767,27 @@ static authz_status ldapuser_check_authorization(r
 return AUTHZ_DENIED;
 }
 
-if (!req) {
-authz_status rv = AUTHZ_DENIED;
-req = build_request_config(r);
-ldc = get_connection_for_authz(r, LDAP_COMPARE);
-if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) { 
-return rv;
-}
-}
-else { 
-ldc = get_connection_for_authz(r, LDAP_COMPARE);
-}
-
-
 /*
  * If we have been authenticated by some other module than mod_authnz_ldap,
  * the req structure needed for authorization needs to be created
  * and populated with the userid and DN of the account in LDAP
  */
-
-
-if (!*r->user) {
-ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01699)
-"ldap authorize: Userid is blank, AuthType=%s",
-r->ap_auth_type);
+if (!req) {
+req = build_request_config(r);
 }
-
+ldc = get_connection_for_authz(r, LDAP_COMPARE);
+if (!req->dn) {
+authz_status rv;
+if (!*r->user) {
+ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01699)
+  "ldap authorize: Userid is blank, AuthType=%s",
+  r->ap_auth_type);
+}
+rv = get_dn_for_nonldap_authn(r, ldc);
+if (rv != AUTHZ_GRANTED) {
+return rv;
+}
+}
 if (req->dn == NULL || !*req->dn) {
 ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01702)
   "auth_ldap authorize: require user: user's DN has not "
@@ -895,17 +890,27 @@ static authz_status ldapgroup_check_authorization(
 return AUTHZ_DENIED;
 }
 
+/*
+ * If we have been authenticated by some other module than mod_authnz_ldap,
+ * the req structure needed for authorization needs to be created
+ * and populated with the userid and DN of the account in LDAP
+ */
 if (!req) {
-authz_status rv = AUTHZ_DENIED;
 req = build_request_config(r);
-ldc = get_connection_for_authz(r, LDAP_COMPARE);
-if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
+}
+ldc = get_connection_for_authz(r, LDAP_COMPARE);
+if (!req->dn) {
+authz_status rv;
+if (!*r->user) {
+ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01699)
+  "ldap authorize: Userid is blank, AuthType=%s",
+  r->ap_auth_type);
+

Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Mon, Nov 20, 2023 at 3:46 PM Yann Ylavic  wrote:
>
> On Mon, Nov 20, 2023 at 2:33 PM Yann Ylavic  wrote:
> >
> > On Mon, Nov 20, 2023 at 1:57 PM Graham Leggett via dev
> >  wrote:
> > >
> > > On 20 Nov 2023, at 12:26, Ruediger Pluem  wrote:
> > >
> > > Or we need to ensure that authn_ldap_build_filter is NULL safe and 
> > > returns in a sensible way if user == NULL.
> > >
> > >
> > > This is the option we need I think - it’s possible that ldapsearch could 
> > > be used without a user.
> >
> > In the proposed 2.4.x backport of ldapsearch_check_authorization()
> > there is no call to get_dn_for_nonldap_authn() nor
> > authn_ldap_build_filter(). The Require expression is passed directly
> > to util_ldap_cache_getuserdn(), so what is building a filter with
> > r->user about in the ldapsearch case finally?
>
> I mean, isn't what we need for something like the attached patch?
> This would call get_dn_for_nonldap_authn() only "if we have been
> authenticated by some other module than mod_auth_ldap" (per comment in
> the code), and do nothing about r->user otherwise.
> Again, I don't really know how mod_ldap is supposed to work so
> possibly this is all irrelevant..

A more complete/correct patch would be this attached v2 anyway.

>
> Regards;
> Yann.
Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c	(revision 1913977)
+++ modules/aaa/mod_authnz_ldap.c	(working copy)
@@ -767,19 +767,6 @@ static authz_status ldapuser_check_authorization(r
 return AUTHZ_DENIED;
 }
 
-if (!req) {
-authz_status rv = AUTHZ_DENIED;
-req = build_request_config(r);
-ldc = get_connection_for_authz(r, LDAP_COMPARE);
-if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) { 
-return rv;
-}
-}
-else { 
-ldc = get_connection_for_authz(r, LDAP_COMPARE);
-}
-
-
 /*
  * If we have been authenticated by some other module than mod_authnz_ldap,
  * the req structure needed for authorization needs to be created
@@ -786,7 +773,6 @@ static authz_status ldapuser_check_authorization(r
  * and populated with the userid and DN of the account in LDAP
  */
 
-
 if (!*r->user) {
 ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01699)
 "ldap authorize: Userid is blank, AuthType=%s",
@@ -793,6 +779,17 @@ static authz_status ldapuser_check_authorization(r
 r->ap_auth_type);
 }
 
+if (!req) {
+req = build_request_config(r);
+}
+ldc = get_connection_for_authz(r, LDAP_COMPARE);
+if (!req->dn) {
+authz_status rv = get_dn_for_nonldap_authn(r, ldc);
+if (rv != AUTHZ_GRANTED) {
+return rv;
+}
+}
+
 if (req->dn == NULL || !*req->dn) {
 ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01702)
   "auth_ldap authorize: require user: user's DN has not "
@@ -895,17 +892,28 @@ static authz_status ldapgroup_check_authorization(
 return AUTHZ_DENIED;
 }
 
+/*
+ * If we have been authenticated by some other module than mod_authnz_ldap,
+ * the req structure needed for authorization needs to be created
+ * and populated with the userid and DN of the account in LDAP
+ */
+
+if (!*r->user) {
+ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01699)
+"ldap authorize: Userid is blank, AuthType=%s",
+r->ap_auth_type);
+}
+
 if (!req) {
-authz_status rv = AUTHZ_DENIED;
 req = build_request_config(r);
-ldc = get_connection_for_authz(r, LDAP_COMPARE);
-if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
+}
+ldc = get_connection_for_authz(r, LDAP_COMPARE);
+if (!req->dn) {
+authz_status rv = get_dn_for_nonldap_authn(r, ldc);
+if (rv != AUTHZ_GRANTED) {
 return rv;
 }
 }
-else { 
-ldc = get_connection_for_authz(r, LDAP_COMPARE);
-}
 
 /*
  * If there are no elements in the group attribute array, the default should be
@@ -1109,16 +1117,15 @@ static authz_status ldapdn_check_authorization(req
 }
 
 if (!req) {
-authz_status rv = AUTHZ_DENIED;
 req = build_request_config(r);
-ldc = get_connection_for_authz(r, LDAP_SEARCH); /* comparedn is a search */
-if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
+}
+ldc = get_connection_for_authz(r, LDAP_SEARCH); /* comparedn is a search */
+if (!req->dn) {
+authz_status rv = get_dn_for_nonldap_authn(r, ldc);
+if (rv != AUTHZ_GRANTED) {
 return rv;
 }

Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Mon, Nov 20, 2023 at 2:33 PM Yann Ylavic  wrote:
>
> On Mon, Nov 20, 2023 at 1:57 PM Graham Leggett via dev
>  wrote:
> >
> > On 20 Nov 2023, at 12:26, Ruediger Pluem  wrote:
> >
> > Or we need to ensure that authn_ldap_build_filter is NULL safe and returns 
> > in a sensible way if user == NULL.
> >
> >
> > This is the option we need I think - it’s possible that ldapsearch could be 
> > used without a user.
>
> In the proposed 2.4.x backport of ldapsearch_check_authorization()
> there is no call to get_dn_for_nonldap_authn() nor
> authn_ldap_build_filter(). The Require expression is passed directly
> to util_ldap_cache_getuserdn(), so what is building a filter with
> r->user about in the ldapsearch case finally?

I mean, isn't what we need for something like the attached patch?
This would call get_dn_for_nonldap_authn() only "if we have been
authenticated by some other module than mod_auth_ldap" (per comment in
the code), and do nothing about r->user otherwise.
Again, I don't really know how mod_ldap is supposed to work so
possibly this is all irrelevant..

Regards;
Yann.
Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c	(revision 1913977)
+++ modules/aaa/mod_authnz_ldap.c	(working copy)
@@ -1435,31 +1435,29 @@ static authz_status ldapsearch_check_authorization
 return AUTHZ_DENIED;
 }
 
+ldc = get_connection_for_authz(r, LDAP_SEARCH);
+
 /*
  * If we have been authenticated by some other module than mod_auth_ldap,
  * the req structure needed for authorization needs to be created
  * and populated with the userid and DN of the account in LDAP
  */
-
 if (!req) {
-authz_status rv = AUTHZ_DENIED;
 req = build_request_config(r);
-ldc = get_connection_for_authz(r, LDAP_SEARCH);
-if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
-return rv;
+if (r->user && *r->user) {
+authz_status rv = get_dn_for_nonldap_authn(r, ldc);
+if (rv != AUTHZ_GRANTED) {
+return rv;
+}
+if (req->dn == NULL || !*req->dn) {
+ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
+  "auth_ldap authorize: require ldap-filter: user's DN "
+  "has not been defined; failing authorization");
+return AUTHZ_DENIED;
+}
 }
 }
-else {
-ldc = get_connection_for_authz(r, LDAP_SEARCH);
-}
 
-if (req->dn == NULL || !*req->dn) {
-ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
-  "auth_ldap authorize: require ldap-filter: user's DN "
-  "has not been defined; failing authorization");
-return AUTHZ_DENIED;
-}
-
 require = ap_expr_str_exec(r, expr, );
 if (err) {
 ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02629)

Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Mon, Nov 20, 2023 at 1:57 PM Graham Leggett via dev
 wrote:
>
> On 20 Nov 2023, at 12:26, Ruediger Pluem  wrote:
>
> Or we need to ensure that authn_ldap_build_filter is NULL safe and returns in 
> a sensible way if user == NULL.
>
>
> This is the option we need I think - it’s possible that ldapsearch could be 
> used without a user.

In the proposed 2.4.x backport of ldapsearch_check_authorization()
there is no call to get_dn_for_nonldap_authn() nor
authn_ldap_build_filter(). The Require expression is passed directly
to util_ldap_cache_getuserdn(), so what is building a filter with
r->user about in the ldapsearch case finally?

>
> Regards,
> Graham
> —
>

Re: svn commit: r1913962 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Mon, Nov 20, 2023 at 12:05 PM Yann Ylavic  wrote:
>
> On Mon, Nov 20, 2023 at 11:54 AM Graham Leggett via dev
>  wrote:
> >
> > On 20 Nov 2023, at 10:44, Yann Ylavic  wrote:
> >
> > >> URL: http://svn.apache.org/viewvc?rev=1913962=rev
> > >> Log:
> > >> Apply earlier fix to the ldapsearch case:
> > >>
> > >> Arrange for backend LDAP connections to be returned
> > >> to the pool by a fixup hook rather than staying locked
> > >> until the end of (a potentially slow) request.
> > >
> > > It seems that this commit aligns the checks/setup of ldapsearch with
> > > the ones of ldapfilter, but nothing about LDAP connections
> > > recycling/reuse?
> >
> > That’s correct - the recycling/reuse code is being backported separately, 
> > it all depends on this code.
> >
> > The end goal is for all trunk changes to be applied to v2.4 and each is 
> > aligned.
> >
> > > In ldapfilter_check_authorization() we bail out early if r->user is
> > > NULL but not here in ldapsearch_check_authorization(), can't it
> > > happen?
> >
> > In ldapsearch we don’t care about the user, it’s purely whether the filter 
> > expression being applied in the search returns exactly one result.
>
> Fine, but if r->user is NULL here we'll segfault (NULL dereference) in
> "if (!*r->user)" here.

Probably an unfortunate copy/paste in trunk only (not in your backport
patch3), fixed in r1913977.

Regards;
Yann.

Re: svn commit: r1913962 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Mon, Nov 20, 2023 at 11:54 AM Graham Leggett via dev
 wrote:
>
> On 20 Nov 2023, at 10:44, Yann Ylavic  wrote:
>
> >> URL: http://svn.apache.org/viewvc?rev=1913962=rev
> >> Log:
> >> Apply earlier fix to the ldapsearch case:
> >>
> >> Arrange for backend LDAP connections to be returned
> >> to the pool by a fixup hook rather than staying locked
> >> until the end of (a potentially slow) request.
> >
> > It seems that this commit aligns the checks/setup of ldapsearch with
> > the ones of ldapfilter, but nothing about LDAP connections
> > recycling/reuse?
>
> That’s correct - the recycling/reuse code is being backported separately, it 
> all depends on this code.
>
> The end goal is for all trunk changes to be applied to v2.4 and each is 
> aligned.
>
> > In ldapfilter_check_authorization() we bail out early if r->user is
> > NULL but not here in ldapsearch_check_authorization(), can't it
> > happen?
>
> In ldapsearch we don’t care about the user, it’s purely whether the filter 
> expression being applied in the search returns exactly one result.

Fine, but if r->user is NULL here we'll segfault (NULL dereference) in
"if (!*r->user)" here.


Regards;
Yann.

Re: svn commit: r1913962 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Sun, Nov 19, 2023 at 11:45 AM  wrote:
>
> Author: minfrin
> Date: Sun Nov 19 10:45:05 2023
> New Revision: 1913962
>
> URL: http://svn.apache.org/viewvc?rev=1913962=rev
> Log:
> Apply earlier fix to the ldapsearch case:
>
> Arrange for backend LDAP connections to be returned
> to the pool by a fixup hook rather than staying locked
> until the end of (a potentially slow) request.

It seems that this commit aligns the checks/setup of ldapsearch with
the ones of ldapfilter, but nothing about LDAP connections
recycling/reuse?

>
> --- httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c (original)
> +++ httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c Sun Nov 19 10:45:05 2023
> @@ -1429,12 +1429,40 @@ static authz_status ldapsearch_check_aut
>  return AUTHZ_DENIED;
>  }
>
> -if (sec->host) {
> +if (!sec->host) {
> +ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01738)
> +  "auth_ldap authorize: no sec->host - weird...?");
> +return AUTHZ_DENIED;
> +}
> +
> +/*
> + * If we have been authenticated by some other module than mod_auth_ldap,
> + * the req structure needed for authorization needs to be created
> + * and populated with the userid and DN of the account in LDAP
> + */
> +
> +if (!*r->user) {
> +ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01739)
> +"ldap authorize: Userid is blank, AuthType=%s",
> +r->ap_auth_type);
> +}

In ldapfilter_check_authorization() we bail out early if r->user is
NULL but not here in ldapsearch_check_authorization(), can't it
happen?

> +
> +if (!req) {
> +authz_status rv = AUTHZ_DENIED;
> +req = build_request_config(r);
>  ldc = get_connection_for_authz(r, LDAP_SEARCH);
> +if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
> +return rv;
> +}
>  }
>  else {
> -ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(02636)
> -  "auth_ldap authorize: no sec->host - weird...?");
> +ldc = get_connection_for_authz(r, LDAP_SEARCH);
> +}
> +
> +if (req->dn == NULL || !*req->dn) {
> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01742)
> +  "auth_ldap authorize: require ldap-filter: user's DN "
> +  "has not been defined; failing authorization");
>  return AUTHZ_DENIED;
>  }

Regards;
Yann.

Re: svn commit: r1589993 - in /httpd/httpd/trunk: CHANGES docs/manual/expr.xml docs/manual/mod/mod_authnz_ldap.xml modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Wed, Apr 30, 2014 at 1:02 AM Yann Ylavic  wrote:
>
> On Tue, Apr 29, 2014 at 10:54 PM, Christophe JAILLET
>  wrote:
> > Hi,
> >
> > doc does not build because of  below:
> >
> > CJ
> >
> > Le 25/04/2014 13:14, minf...@apache.org a écrit :
> >> +
> >> +LocationMatch ^/dav/(?[^/]+)/
> >
> >   ^ There
> >
>
> Hmm, won't LocationMatch itself be broken by the inner <>s ?

Wow, fortunately I didn't hold my breath on this one :)
Someone needs to answer to this former/younger/naive me though and
since I'm on this commit again: look Yann, this match is double-quoted
now so we should be fine!

Re: svn commit: r1589993 - in /httpd/httpd/trunk: CHANGES docs/manual/expr.xml docs/manual/mod/mod_authnz_ldap.xml modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Fri, Apr 25, 2014 at 1:15 PM  wrote:
>
> Author: minfrin
> Date: Fri Apr 25 11:14:36 2014
> New Revision: 1589993
>
> URL: http://svn.apache.org/r1589993
> Log:
> Add the ldap-search option to mod_authnz_ldap, allowing authorization
> to be based on arbitrary expressions that do not include the username.
[]
>
> --- httpd/httpd/trunk/docs/manual/mod/mod_authnz_ldap.xml (original)
> +++ httpd/httpd/trunk/docs/manual/mod/mod_authnz_ldap.xml Fri Apr 25 11:14:36 
> 2014
[]
> @@ -508,6 +514,28 @@ AuthLDAPMaxSubGroupDepth 1
>
>  
>
> +Require ldap-search
> +
> +The Require ldap-search directive allows the
> +administrator to grant access based on a generic LDAP search filter 
> using an
> +expression. If there is exactly one match to 
> the search filter,
> +regardless of the distinguished name, access is granted.

I get from this that there should be one match..

>
> --- httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c (original)
> +++ httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c Fri Apr 25 11:14:36 2014
[]
>
> +static authz_status ldapsearch_check_authorization(request_rec *r,
> +   const char *require_args,
> +   const void 
> *parsed_require_args)
> +{
> +int result = 0;
> +authn_ldap_config_t *sec =
> +(authn_ldap_config_t *)ap_get_module_config(r->per_dir_config, 
> _ldap_module);
> +
> +util_ldap_connection_t *ldc = NULL;
> +
> +const char *err = NULL;
> +const ap_expr_info_t *expr = parsed_require_args;
> +const char *require;
> +const char *t;
> +const char *dn = NULL;
> +
> +if (!sec->have_ldap_url) {
> +return AUTHZ_DENIED;
> +}
> +
> +if (sec->host) {
> +ldc = get_connection_for_authz(r, LDAP_SEARCH);
> +apr_pool_cleanup_register(r->pool, ldc,
> +  authnz_ldap_cleanup_connection_close,
> +  apr_pool_cleanup_null);
> +}
> +else {
> +ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01738)
> +  "auth_ldap authorize: no sec->host - weird...?");
> +return AUTHZ_DENIED;
> +}
> +
> +require = ap_expr_str_exec(r, expr, );
> +if (err) {
> +ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
> +  "auth_ldap authorize: require ldap-search: Can't "
> +  "evaluate require expression: %s", err);
> +return AUTHZ_DENIED;
> +}
> +
> +t = require;
> +
> +if (t[0]) {
> +const char **vals;
> +
> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO()
> +  "auth_ldap authorize: checking filter %s", t);
> +
> +/* Search for the user DN */
> +result = util_ldap_cache_getuserdn(r, ldc, sec->url, sec->basedn,
> + sec->scope, sec->attributes, t, , );
> +
> +/* Make sure that the filtered search returned a single dn */

And it's restated here..

> +if (result == LDAP_SUCCESS && dn) {
> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO()
> +  "auth_ldap authorize: require ldap-search: "
> +  "authorization successful");
> +return AUTHZ_GRANTED;

I get that for "ldap-filter" (unlike for "ldap-search here) we'll do a
util_ldap_cache_comparedn() to (double) check the returned DN somehow
(sorry I don't really know how LDAP works), not here though because we
don't require a particular DN but just a single one.
But what makes sure that it's the case here?

> +}
> +else {
> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO()
> +  "auth_ldap authorize: require ldap-search: "
> +  "%s authorization failed [%s][%s]",
> +  t, ldc->reason, ldap_err2string(result));
> +}
> +}
> +
> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO()
> +  "auth_ldap authorize filter: authorization denied for "
> +  "to %s", r->uri);
> +
> +return AUTHZ_DENIED;
> +}


Regards;
Yann.

Re: svn commit: r1591012 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Sat, Nov 18, 2023 at 5:59 PM Yann Ylavic  wrote:
>
> All in all, I'd rewrite this function like in the attached patch (not
> even compile tested, just to show what I'm talking about..).

Oh it seems that the callers want the "filtbuf" to be \0 terminated
(even in case of error), so this v2 probably..

>
> Regards;
> Yann.
Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c	(revision 1913813)
+++ modules/aaa/mod_authnz_ldap.c	(working copy)
@@ -206,7 +206,7 @@ static const char* authn_ldap_xlate_password(reque
  * search filter will be (&(posixid=*)(uid=userj)).
  */
 #define FILTER_LENGTH MAX_STRING_LEN
-static apr_status_t authn_ldap_build_filter(char *filtbuf,
+static apr_status_t authn_ldap_build_filter(char filtbuf[FILTER_LENGTH],
  request_rec *r,
  const char *user,
  const char *filter,
@@ -219,6 +219,7 @@ static const char* authn_ldap_xlate_password(reque
 apr_size_t outbytes;
 char *outbuf;
 int nofilter = 0, len;
+apr_statut rv = APR_SUCCESS;
 
 if (!filter) {
 filter = sec->filter;
@@ -244,7 +245,7 @@ static const char* authn_ldap_xlate_password(reque
  * config-supplied portions.
  */
 
-if ((nofilter = (filter && !strcasecmp(filter, "none" { 
+if ((nofilter = (!filter || !*filter || !strcasecmp(filter, "none" { 
 len = apr_snprintf(filtbuf, FILTER_LENGTH, "(%s=", sec->attribute);
 }
 else { 
@@ -256,12 +257,13 @@ static const char* authn_ldap_xlate_password(reque
  * LDAP filter metachars are escaped.
  */
 filtbuf_end = filtbuf + FILTER_LENGTH - 1;
+for (p = user, q = filtbuf + len; *p; ) {
+if (strchr("*()\\", *p) != NULL) {
 #if APR_HAS_MICROSOFT_LDAPSDK
-for (p = user, q=filtbuf + len;
- *p && q < filtbuf_end; ) {
-if (strchr("*()\\", *p) != NULL) {
-if ( q + 3 >= filtbuf_end)
-  break;  /* Don't write part of escape sequence if we can't write all of it */
+if (q + 3 >= filtbuf_end) { /* accounts for final \0 */
+rv = APR_EGENERAL;
+goto out;
+}
 *q++ = '\\';
 switch ( *p++ )
 {
@@ -281,23 +283,24 @@ static const char* authn_ldap_xlate_password(reque
 *q++ = '5';
 *q++ = 'c';
 break;
-}
-}
-else
-*q++ = *p++;
-}
+}
 #else
-for (p = user, q=filtbuf + len;
- *p && q < filtbuf_end; *q++ = *p++) {
-if (strchr("*()\\", *p) != NULL) {
+if (q + 2 >= filtbuf_end) { /* accounts for final \0 */
+rv = APR_EGENERAL;
+goto out;
+}
 *q++ = '\\';
-if (q >= filtbuf_end) {
-  break;
+*q++ = *p++;
+#endif
+}
+else {
+if (q + 1 >= filtbuf_end) { /* accounts for final \0 */
+rv = APR_EGENERAL;
+goto out;
 }
+*q++ = *p++;
 }
 }
-#endif
-*q = '\0';
 
 /*
  * Append the closing parens of the filter, unless doing so would
@@ -305,23 +308,24 @@ static const char* authn_ldap_xlate_password(reque
  */
 
 if (nofilter) { 
-if (q + 1 <= filtbuf_end) {
-strcat(filtbuf, ")");
+if (q + 1 >= filtbuf_end) { /* accounts for final \0 */
+rv = APR_EGENERAL;
+goto out;
 }
-else {
-return APR_EGENERAL;
-}
+*q++ = ')';
 } 
 else { 
-if (q + 2 <= filtbuf_end) {
-strcat(filtbuf, "))");
+if (q + 2 >= filtbuf_end) { /* accounts for final \0 */
+rv = APR_EGENERAL;
+goto out;
 }
-else {
-return APR_EGENERAL;
-}
+*q++ = ')';
+*q++ = ')';
 }
 
-return APR_SUCCESS;
+out:
+*q = '\0';
+return rv;
 }
 
 static void *create_authnz_ldap_dir_config(apr_pool_t *p, char *d)

Re: svn commit: r1591012 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/aaa/mod_authnz_ldap.c

[Yann Ylavic]

On Tue, Apr 29, 2014 at 6:06 PM  wrote:
>
> Author: minfrin
> Date: Tue Apr 29 16:05:56 2014
> New Revision: 1591012
>
> URL: http://svn.apache.org/r1591012
> Log:
> mod_authnz_ldap: Fail explicitly when the filter is too long. Remove
> unnecessary apr_pstrdup() and strlen().

It seems that the escaping cases don't error out if filtbuf is not
large enough? (see below)

>
> --- httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c (original)
> +++ httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c Tue Apr 29 16:05:56 2014
[]
> @@ -205,31 +202,23 @@ static const char* authn_ldap_xlate_pass
>   * search filter will be (&(posixid=*)(uid=userj)).
>   */
>  #define FILTER_LENGTH MAX_STRING_LEN
> -static void authn_ldap_build_filter(char *filtbuf,
> +static apr_status_t authn_ldap_build_filter(char *filtbuf,
>   request_rec *r,
> - const char* sent_user,
> - const char* sent_filter,
> + const char *user,
> + const char *filter,
>   authn_ldap_config_t *sec)
>  {
[]
> @@ -264,7 +253,7 @@ static void authn_ldap_build_filter(char
>   */
>  filtbuf_end = filtbuf + FILTER_LENGTH - 1;
>  #if APR_HAS_MICROSOFT_LDAPSDK
> -for (p = user, q=filtbuf + strlen(filtbuf);
> +for (p = user, q=filtbuf + len;
>   *p && q < filtbuf_end; ) {
>  if (strchr("*()\\", *p) != NULL) {
>  if ( q + 3 >= filtbuf_end)

Here we break the loop and fall through with no error.

> @@ -294,7 +283,7 @@ static void authn_ldap_build_filter(char
>  *q++ = *p++;
>  }
>  #else
> -for (p = user, q=filtbuf + strlen(filtbuf);
> +for (p = user, q=filtbuf + len;
>   *p && q < filtbuf_end; *q++ = *p++) {
>  if (strchr("*()\\", *p) != NULL) {
>  *q++ = '\\';

Next it's:
if (q >= filtbuf_end) {
  break;
}
so same here, plus it's not consistent because for
APR_HAS_MICROSOFT_LDAPSDK in the previous hunk we break out before
'\\' while here it's after.

> @@ -312,14 +301,23 @@ static void authn_ldap_build_filter(char
>   */
>
>  if (nofilter) {
> -if (q + 1 <= filtbuf_end)
> +if (q + 1 <= filtbuf_end) {
>  strcat(filtbuf, ")");
> +}
> +else {
> +return APR_EGENERAL;
> +}
>  }
>  else {
> -if (q + 2 <= filtbuf_end)
> +if (q + 2 <= filtbuf_end) {
>  strcat(filtbuf, "))");

> +}
> +else {
> +return APR_EGENERAL;
> +}
>  }

These final checks do not catch the escaping truncations either
because we might have left some room. Also the strcat()s look
inefficient since "q" points at the end of "filtbuf" already.

>
> +return APR_SUCCESS;
>  }


All in all, I'd rewrite this function like in the attached patch (not
even compile tested, just to show what I'm talking about..).

Regards;
Yann.
Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c	(revision 1913813)
+++ modules/aaa/mod_authnz_ldap.c	(working copy)
@@ -206,7 +206,7 @@ static const char* authn_ldap_xlate_password(reque
  * search filter will be (&(posixid=*)(uid=userj)).
  */
 #define FILTER_LENGTH MAX_STRING_LEN
-static apr_status_t authn_ldap_build_filter(char *filtbuf,
+static apr_status_t authn_ldap_build_filter(char filtbuf[FILTER_LENGTH],
  request_rec *r,
  const char *user,
  const char *filter,
@@ -244,7 +244,7 @@ static const char* authn_ldap_xlate_password(reque
  * config-supplied portions.
  */
 
-if ((nofilter = (filter && !strcasecmp(filter, "none" { 
+if ((nofilter = (!filter || !*filter || !strcasecmp(filter, "none" { 
 len = apr_snprintf(filtbuf, FILTER_LENGTH, "(%s=", sec->attribute);
 }
 else { 
@@ -256,12 +256,12 @@ static const char* authn_ldap_xlate_password(reque
  * LDAP filter metachars are escaped.
  */
 filtbuf_end = filtbuf + FILTER_LENGTH - 1;
+for (p = user, q = filtbuf + len; *p; ) {
+if (strchr("*()\\", *p) != NULL) {
 #if APR_HAS_MICROSOFT_LDAPSDK
-for (p = user, q=filtbuf + len;
- *p && q < filtbuf_end; ) {
-if (strchr("*()\\", *p) != NULL) {
-if ( q + 3 >= filtbuf_end)
-  break;  /* Don't write part of escape sequence if we can't write all of it */
+if (q + 3 >= filtbuf_end) { /* accounts for final \0 */
+return APR_EGENERAL;
+}
 *q++ = '\\';
 switch ( *p++ )
 {
@@ -281,23 +281,22 @@ static const char* authn_ldap_xlate_password(reque
 *q++ = '5';
 *q++ = 'c';
 break;
-}
-}
-else
-*q++ = *p++;
-}

Re: svn commit: r1913936 - /httpd/httpd/branches/2.4.x/STATUS

[Yann Ylavic]

On Sat, Nov 18, 2023 at 3:43 PM Yann Ylavic  wrote:
>
> On Sat, Nov 18, 2023 at 3:17 PM  wrote:
> >
> > Author: minfrin
> > Date: Sat Nov 18 14:17:11 2023
> > New Revision: 1913936
> >
> > URL: http://svn.apache.org/viewvc?rev=1913936=rev
> > Log:
> > Update vote, note need for mmn bump.
> >
> > Modified:
> > httpd/httpd/branches/2.4.x/STATUS
> >
> > Modified: httpd/httpd/branches/2.4.x/STATUS
> > URL: 
> > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/STATUS?rev=1913936=1913935=1913936=diff
> > ==
> > --- httpd/httpd/branches/2.4.x/STATUS (original)
> > +++ httpd/httpd/branches/2.4.x/STATUS Sat Nov 18 14:17:11 2023
> > @@ -174,10 +174,13 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
> >*) mod_proxy: prevent bad busy values and problems with bybusyness
> >   Trunk version of patch:
> > https://svn.apache.org/r1912245
> > +   https://svn.apache.org/r1913930
> >  svn merge -c 1912245 ^/httpd/httpd/trunk .
> > - +1: jfclere, minfrin
> > +svn merge -c 1913930 ^/httpd/httpd/trunk .
> > + +1: minfrin, ylavic
> >   ylavic: +1 with r1913930, we can't provide exported functions w/o an
> >   ap_[proxy_] prefix.
> > + minfrin: r1913930 added, votes reset, mmn bump needed
>
> Do we need an MMN bump for some ap_proxy_ functions added to
> proxy_util.h (non public header), though AP_PROXY_DECLARE()d because
> they need to be accessible from several proxy modules SSOs?

I meant DSOs here..

>
>
> Regards;
> Yann.

Re: svn commit: r1913936 - /httpd/httpd/branches/2.4.x/STATUS

[Yann Ylavic]

On Sat, Nov 18, 2023 at 3:17 PM  wrote:
>
> Author: minfrin
> Date: Sat Nov 18 14:17:11 2023
> New Revision: 1913936
>
> URL: http://svn.apache.org/viewvc?rev=1913936=rev
> Log:
> Update vote, note need for mmn bump.
>
> Modified:
> httpd/httpd/branches/2.4.x/STATUS
>
> Modified: httpd/httpd/branches/2.4.x/STATUS
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/STATUS?rev=1913936=1913935=1913936=diff
> ==
> --- httpd/httpd/branches/2.4.x/STATUS (original)
> +++ httpd/httpd/branches/2.4.x/STATUS Sat Nov 18 14:17:11 2023
> @@ -174,10 +174,13 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
>*) mod_proxy: prevent bad busy values and problems with bybusyness
>   Trunk version of patch:
> https://svn.apache.org/r1912245
> +   https://svn.apache.org/r1913930
>  svn merge -c 1912245 ^/httpd/httpd/trunk .
> - +1: jfclere, minfrin
> +svn merge -c 1913930 ^/httpd/httpd/trunk .
> + +1: minfrin, ylavic
>   ylavic: +1 with r1913930, we can't provide exported functions w/o an
>   ap_[proxy_] prefix.
> + minfrin: r1913930 added, votes reset, mmn bump needed

Do we need an MMN bump for some ap_proxy_ functions added to
proxy_util.h (non public header), though AP_PROXY_DECLARE()d because
they need to be accessible from several proxy modules SSOs?


Regards;
Yann.

Re: svn commit: r1874101 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_init.c ssl_private.h

[Yann Ylavic]

On Thu, Nov 16, 2023 at 2:13 PM Yann Ylavic  wrote:
>
> Since r1908537 it's now:
>   #if LIBRESSL_VERSION_NUMBER < 0x207f
>   #define MODSSL_USE_OPENSSL_PRE_1_1_API 1
>   #else
>   #define MODSSL_USE_OPENSSL_PRE_1_1_API 0
>   #endif
> so the above hunk is probably not needed anymore (though harmless).

I went ahead and removed it in r1913838 (FWIW).

>
> Regards;
> Yann.

Re: svn commit: r1874101 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_init.c ssl_private.h

[Yann Ylavic]

On Mon, Feb 17, 2020 at 8:52 AM  wrote:
>
> Author: gbechis
> Date: Mon Feb 17 07:52:55 2020
> New Revision: 1874101
>
> URL: http://svn.apache.org/viewvc?rev=1874101=rev
> Log:
> fix build with LibreSSL 2.0.7+
> bz 64047
>
> --- httpd/httpd/trunk/modules/ssl/ssl_private.h (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_private.h Mon Feb 17 07:52:55 2020
> @@ -228,9 +228,11 @@
>  #define BN_get_rfc3526_prime_4096  get_rfc3526_prime_4096
>  #define BN_get_rfc3526_prime_6144  get_rfc3526_prime_6144
>  #define BN_get_rfc3526_prime_8192  get_rfc3526_prime_8192
> +#if !defined(LIBRESSL_VERSION_NUMBER) || LIBRESSL_VERSION_NUMBER < 
> 0x207fL
>  #define BIO_set_init(x,v)  (x->init=v)
>  #define BIO_get_data(x)(x->ptr)
>  #define BIO_set_data(x,v)  (x->ptr=v)
> +#endif
>  #define BIO_get_shutdown(x)(x->shutdown)
>  #define BIO_set_shutdown(x,v)  (x->shutdown=v)
>  #define DH_bits(x) (BN_num_bits(x->p))

This block is enclosed by an:
  #if MODSSL_USE_OPENSSL_PRE_1_1_API
so I wonder if this change is still needed after
  
https://github.com/apache/httpd/pull/381/commits/5d154a4823c3a3593629328f90a64da908c04114#diff-71e13cebcaceeab74eaf5cfd988de6099fcfe10a59001af590a922e8d4532748L145-R165
from r1908537 (i.e. after your changes).

The issue was (I think) that MODSSL_USE_OPENSSL_PRE_1_1_API was
incorrectly defined as (for LibreSSL):
  #define MODSSL_USE_OPENSSL_PRE_1_1_API (LIBRESSL_VERSION_NUMBER < 0x207f)
and the preprocessor can't evaluate this in an #if (my bad for having
spread this kind of #defines in httpd and apr, which I now need to
fix..).

Since r1908537 it's now:
  #if LIBRESSL_VERSION_NUMBER < 0x207f
  #define MODSSL_USE_OPENSSL_PRE_1_1_API 1
  #else
  #define MODSSL_USE_OPENSSL_PRE_1_1_API 0
  #endif
so the above hunk is probably not needed anymore (though harmless).


Regards;
Yann.

Re: svn commit: r1912459 - in /httpd/httpd/trunk: include/ modules/proxy/

[Yann Ylavic]

On Thu, Nov 2, 2023 at 2:30 PM Ruediger Pluem  wrote:
>
> On 11/2/23 1:47 PM, Yann Ylavic wrote:
> > On Sat, Sep 30, 2023 at 7:19 PM Ruediger Pluem  wrote:
> >>
> >> On 9/21/23 3:15 PM, yla...@apache.org wrote:
> >>
> >>> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> >>> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Thu Sep 21 13:15:35 2023
> >>
> >>> @@ -2683,56 +3116,83 @@ ap_proxy_determine_connection(apr_pool_t
> >>> +
> >>> +if (proxyname) {
> >>> +forward_info *forward;
> >>> +
> >>> +hostname = proxyname;
> >>> +hostport = proxyport;
> >>> +
> >>> +/* Reset forward info if they changed */
> >>> +if (conn->is_ssl
> >>> +&& (!(forward = conn->forward)
> >>> +|| forward->target_port != uri->port
> >>> +|| ap_cstr_casecmp(forward->target_host,
> >>> +   uri->hostname) != 0)) {
> >>> +apr_pool_t *fwd_pool = conn->pool;
> >>> +if (worker->s->is_address_reusable) {
> >>> +if (conn->fwd_pool) {
> >>> +apr_pool_clear(conn->fwd_pool);
> >>> +}
> >>> +else {
> >>> +apr_pool_create(>fwd_pool, conn->pool);
> >>> +}
> >>
> >>
> >> Don't we need to
> >>
> >> fwd_pool = conn->fwd_pool
> >>
> >> ??
> >
> > Sorry I missed your message somehow.
> > Yes you are right of course!
> >
> > And with a fresh look at this new forward_info reuse mechanism I think
> > we also need to check whether the ->proxy_auth has changed too.
> > Something like the attached (which also includes your proposed change 
> > above)?
>
> Looks good.

Thanks, r1913534.


Regards;
Yann.

Re: svn commit: r1912459 - in /httpd/httpd/trunk: include/ modules/proxy/

[Yann Ylavic]

On Sat, Sep 30, 2023 at 7:19 PM Ruediger Pluem  wrote:
>
> On 9/21/23 3:15 PM, yla...@apache.org wrote:
>
> > --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> > +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Thu Sep 21 13:15:35 2023
>
> > @@ -2683,56 +3116,83 @@ ap_proxy_determine_connection(apr_pool_t
> > +
> > +if (proxyname) {
> > +forward_info *forward;
> > +
> > +hostname = proxyname;
> > +hostport = proxyport;
> > +
> > +/* Reset forward info if they changed */
> > +if (conn->is_ssl
> > +&& (!(forward = conn->forward)
> > +|| forward->target_port != uri->port
> > +|| ap_cstr_casecmp(forward->target_host,
> > +   uri->hostname) != 0)) {
> > +apr_pool_t *fwd_pool = conn->pool;
> > +if (worker->s->is_address_reusable) {
> > +if (conn->fwd_pool) {
> > +apr_pool_clear(conn->fwd_pool);
> > +}
> > +else {
> > +apr_pool_create(>fwd_pool, conn->pool);
> > +}
>
>
> Don't we need to
>
> fwd_pool = conn->fwd_pool
>
> ??

Sorry I missed your message somehow.
Yes you are right of course!

And with a fresh look at this new forward_info reuse mechanism I think
we also need to check whether the ->proxy_auth has changed too.
Something like the attached (which also includes your proposed change above)?


Regards;
Yann.
Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c	(revision 1913529)
+++ modules/proxy/proxy_util.c	(working copy)
@@ -47,7 +47,7 @@ APLOG_USE_MODULE(proxy);
 /*
  * Opaque structure containing target server info when
  * using a forward proxy.
- * Up to now only used in combination with HTTP CONNECT.
+ * Up to now only used in combination with HTTP CONNECT to ProxyRemote
  */
 typedef struct {
 int  use_http_connect; /* Use SSL Tunneling via HTTP CONNECT */
@@ -3154,58 +3154,65 @@ ap_proxy_determine_connection(apr_pool_t *p, reque
 const char *hostname = uri->hostname;
 apr_port_t hostport = uri->port;
 
+/* Not a remote CONNECT until further notice */
+conn->forward = NULL;
+
 if (proxyname) {
-forward_info *forward;
-
 hostname = proxyname;
 hostport = proxyport;
 
-/* Reset forward info if they changed */
-if (conn->is_ssl
-&& (!(forward = conn->forward)
+/*
+ * If we have a remote proxy and the protocol is HTTPS,
+ * then we need to prepend a HTTP CONNECT request before
+ * sending our actual HTTPS requests.
+ */
+if (conn->is_ssl) {
+forward_info *forward;
+const char *proxy_auth;
+
+/* Do we want to pass Proxy-Authorization along?
+ * If we haven't used it, then YES
+ * If we have used it then MAYBE: RFC2616 says we MAY propagate it.
+ * So let's make it configurable by env.
+ * The logic here is the same used in mod_proxy_http.
+ */
+proxy_auth = apr_table_get(r->notes, "proxy-basic-creds");
+if (proxy_auth == NULL
+&& (r->user == NULL /* we haven't yet authenticated */
+|| apr_table_get(r->subprocess_env, "Proxy-Chain-Auth"))) {
+proxy_auth = apr_table_get(r->headers_in, "Proxy-Authorization");
+}
+if (proxy_auth != NULL && proxy_auth[0] == '\0') {
+proxy_auth = NULL;
+}
+
+/* Reset forward info if they changed */
+if (!(forward = conn->forward)
 || forward->target_port != uri->port
-|| ap_cstr_casecmp(forward->target_host,
-   uri->hostname) != 0)) {
-apr_pool_t *fwd_pool = conn->pool;
-if (worker->s->is_address_reusable) {
-if (conn->fwd_pool) {
-apr_pool_clear(conn->fwd_pool);
+|| ap_cstr_casecmp(forward->target_host, uri->hostname) != 0
+|| (forward->proxy_auth != NULL) != (proxy_auth != NULL)
+|| (forward->proxy_auth != NULL && proxy_auth != NULL &&
+strcmp(forward->proxy_auth, proxy_auth) != 0)) {
+apr_pool_t *fwd_pool = conn->pool;
+if (worker->s->is_address_reusable) {
+if (conn->fwd_pool) {
+apr_pool_clear(conn->fwd_pool);
+}
+else {
+apr_pool_create(>fwd_pool, 

Re: [VOTE] Release httpd-2.4.58-rc3 as httpd-2.4.58

[Yann Ylavic]

On Mon, Oct 16, 2023 at 5:10 PM Stefan Eissing via dev
 wrote:
>
> I would like to call a VOTE over the next few days to release
> this candidate tarball httpd-2.4.58-rc3 as 2.4.58:

[X] +1: It's not just good, it's good enough!

Tested on Debian 12 and 13.

Thanks for RMing Stefan!

Re: mod_ssl SSL_OP_IGNORE_UNEXPECTED_EOF: "unexpected eof while reading"

[Yann Ylavic]

On Thu, Sep 7, 2023 at 6:09 PM Yann Ylavic  wrote:
>
> On Wed, Aug 30, 2023 at 1:22 PM Rainer Jung  wrote:
> >
> > OpenSSL 3 flags some abortive shutdowns as an error different to what
> > 1.1.1 did. This results in info log output in httpd:
> >
> > [Tue Aug 29 12:33:06.787210 2023] [ssl:info] [pid 1994673:tid 1994737]
> > SSL Library Error: error:0A000126:SSL routines::unexpected eof while reading
> > [Tue Aug 29 12:33:06.787374 2023] [ssl:info] [pid 1994673:tid 1994737]
> > [client 1.2.3.4:54790] AH01998: Connection closed to child 215 with
> > abortive shutdown (server myserver:443)
>
> The info looks legit to me (someone closed the connection with no
> close_notify), possibly we want to log it at APLOG_DEBUG/TRACEx still
> if it happens too often?
> We don't do that though for SSL_ERROR_ZERO_RETURN in openssl < 3, but
> maybe we should too like in the attached patch (instead of r1912015)?

Scratch that patch, SSL_ERROR_ZERO_RETURN is actually when
close_notify was received, we'd rather need to test SSL_ERROR_SYSCALL
&& errno == 0 with openssl < 0, which is more tricky in httpd with the
EOS bucket vs APR_EOF.
Hm, not sure we want to complicate this more..

>
> Regards;
> Yann.

Re: mod_ssl SSL_OP_IGNORE_UNEXPECTED_EOF: "unexpected eof while reading"

[Yann Ylavic]

On Wed, Aug 30, 2023 at 1:22 PM Rainer Jung  wrote:
>
> OpenSSL 3 flags some abortive shutdowns as an error different to what
> 1.1.1 did. This results in info log output in httpd:
>
> [Tue Aug 29 12:33:06.787210 2023] [ssl:info] [pid 1994673:tid 1994737]
> SSL Library Error: error:0A000126:SSL routines::unexpected eof while reading
> [Tue Aug 29 12:33:06.787374 2023] [ssl:info] [pid 1994673:tid 1994737]
> [client 1.2.3.4:54790] AH01998: Connection closed to child 215 with
> abortive shutdown (server myserver:443)

The info looks legit to me (someone closed the connection with no
close_notify), possibly we want to log it at APLOG_DEBUG/TRACEx still
if it happens too often?
We don't do that though for SSL_ERROR_ZERO_RETURN in openssl < 3, but
maybe we should too like in the attached patch (instead of r1912015)?

Regards;
Yann.
Index: modules/ssl/ssl_engine_io.c
===
--- modules/ssl/ssl_engine_io.c	(revision 1911906)
+++ modules/ssl/ssl_engine_io.c	(working copy)
@@ -721,6 +721,19 @@ static apr_status_t ssl_io_input_read(bio_filter_i
 ssl_err = SSL_get_error(inctx->filter_ctx->pssl, rc);
 c = (conn_rec*)SSL_get_app_data(inctx->filter_ctx->pssl);
 
+#ifdef SSL_R_UNEXPECTED_EOF_WHILE_READING
+if (ERR_GET_LIB(ERR_peek_error()) == ERR_LIB_SSL &&
+ERR_GET_REASON(ERR_peek_error()) == SSL_R_UNEXPECTED_EOF_WHILE_READING) {
+ssl_err = SSL_ERROR_ZERO_RETURN;
+}
+#endif
+if (ssl_err == SSL_ERROR_ZERO_RETURN) {
+ap_log_cerror(APLOG_MARK, APLOG_TRACE1, inctx->rc, c,
+  "SSL input filter at EOF with no close_notify.");
+inctx->rc = APR_EOF;
+break;
+}
+
 if (ssl_err == SSL_ERROR_WANT_READ) {
 /*
  * If OpenSSL wants to read more, and we were nonblocking,
@@ -741,7 +754,8 @@ static apr_status_t ssl_io_input_read(bio_filter_i
 }
 continue;  /* Blocking and nothing yet?  Try again. */
 }
-else if (ssl_err == SSL_ERROR_SYSCALL) {
+
+if (ssl_err == SSL_ERROR_SYSCALL) {
 if (APR_STATUS_IS_EAGAIN(inctx->rc)
 || APR_STATUS_IS_EINTR(inctx->rc)) {
 /* Already read something, return APR_SUCCESS instead. */
@@ -763,10 +777,6 @@ static apr_status_t ssl_io_input_read(bio_filter_i
   "SSL input filter read failed.");
 }
 }
-else if (rc == 0 && ssl_err == SSL_ERROR_ZERO_RETURN) {
-inctx->rc = APR_EOF;
-break;
-}
 else /* if (ssl_err == SSL_ERROR_SSL) */ {
 /*
  * Log SSL errors and any unexpected conditions.
@@ -1413,17 +1423,8 @@ static apr_status_t ssl_io_filter_handshake(ssl_fi
 apr_status_t rc = inctx->rc ? inctx->rc : outctx->rc ;
 ssl_err = SSL_get_error(filter_ctx->pssl, n);
 
-if (ssl_err == SSL_ERROR_ZERO_RETURN) {
+if (ssl_err == SSL_ERROR_WANT_READ) {
 /*
- * The case where the connection was closed before any data
- * was transferred. That's not a real error and can occur
- * sporadically with some clients.
- */
-ap_log_cerror(APLOG_MARK, APLOG_INFO, rc, c, APLOGNO(02006)
- "SSL handshake stopped: connection was closed");
-}
-else if (ssl_err == SSL_ERROR_WANT_READ) {
-/*
  * This is in addition to what was present earlier. It is
  * borrowed from openssl_state_machine.c [mod_tls].
  * TBD.
@@ -1431,8 +1432,9 @@ static apr_status_t ssl_io_filter_handshake(ssl_fi
 outctx->rc = APR_EAGAIN;
 return APR_EAGAIN;
 }
-else if (ERR_GET_LIB(ERR_peek_error()) == ERR_LIB_SSL &&
- ERR_GET_REASON(ERR_peek_error()) == SSL_R_HTTP_REQUEST) {
+
+if (ERR_GET_LIB(ERR_peek_error()) == ERR_LIB_SSL &&
+ERR_GET_REASON(ERR_peek_error()) == SSL_R_HTTP_REQUEST) {
 /*
  * The case where OpenSSL has recognized a HTTP request:
  * This means the client speaks plain HTTP on our HTTPS port.
@@ -1441,6 +1443,22 @@ static apr_status_t ssl_io_filter_handshake(ssl_fi
  */
 return MODSSL_ERROR_HTTP_ON_HTTPS;
 }
+
+#ifdef SSL_R_UNEXPECTED_EOF_WHILE_READING
+if (ERR_GET_LIB(ERR_peek_error()) == ERR_LIB_SSL &&
+ERR_GET_REASON(ERR_peek_error()) == SSL_R_UNEXPECTED_EOF_WHILE_READING) {
+ssl_err = SSL_ERROR_ZERO_RETURN;
+}
+#endif
+if (ssl_err == SSL_ERROR_ZERO_RETURN) {
+/*
+ * The case where the connection was closed before any data
+ * was transferred. That's not a real error and can 

Re: balancers bybusyness, bytraffic and byrequest thread/process safe issues

[Yann Ylavic]

On Wed, Sep 6, 2023 at 6:29 PM Yann Ylavic  wrote:
>
> As for the memory orders on success/failure, they have nothing to do
> with the likeliness of success/failure

Well the memory orderings specified can certainly influence the
likeliness of success/failure since a weaker ordering implies less
synchronization thus probably more concurrency, what I meant is that
they don't influence *correctness* of the returned values!


Regards;
Yann.

Re: balancers bybusyness, bytraffic and byrequest thread/process safe issues

[Yann Ylavic]

On Wed, Sep 6, 2023 at 1:17 PM jean-frederic clere  wrote:
>
> On 8/31/23 18:20, Jim Jagielski wrote:
> > Isn't the call to find the best balancer mutex protected?
>
> Look to apr_atomic_cas32() and the APR code (1.7.x) I noted that we
> don't test the return value of __atomic_compare_exchange_n()

IIUC we don't need to since apr_atomic_cas32() does not return it, see below.

>
> +++
> PR_DECLARE(apr_uint32_t) apr_atomic_cas32(volatile apr_uint32_t *mem,
> apr_uint32_t val,
> apr_uint32_t cmp)
> {
> #if HAVE__ATOMIC_BUILTINS
>  __atomic_compare_exchange_n(mem, , val, 0, __ATOMIC_SEQ_CST,
> __ATOMIC_SEQ_CST);
>  return cmp;
> #else
>  return __sync_val_compare_and_swap(mem, cmp, val);
> #endif
> +++
>
> and:
> https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
> Says:
> Otherwise, false is returned and memory is affected according to
> failure_memorder. This memory order cannot be __ATOMIC_RELEASE nor
> __ATOMIC_ACQ_REL. It also cannot be a stronger order than that specified
> by success_memorder.
>
> So we use __ATOMIC_SEQ_CST so we can't fail or do I miss something?

These docs (also) say:
"""
bool __atomic_compare_exchange_n (type *ptr, type *expected, type desired, ...)
This compares the contents of *ptr with the contents of *expected. If
equal, the operation is a read-modify-write operation that writes
desired into *ptr. If they are not equal, the operation is a read and
the current contents of *ptr are written into *expected.
"""
So __atomic_compare_exchange_n() guarantees that *expected will always
be set to the value of *ptr before the call (be it changed or not),
that's all we care about as returned value.

Typical usage of apr_atomic_cas32() is:
   ret = apr_atomic_cas32(mem, val, cmp);
   if (ret == cmp) {
  /* *mem changed to val, old value was ret (== cmp) */
   }
   else {
  /* *mem unchanged, current value is ret (!=t cmp) */
   }
meaning that done/true (or noop/false) is given by ret == cmp (or
respectively ret != cmp), no explicit bool is returned. The CAS
guarantees that if *mem is equal to cmp then it's set to val, so it's
enough to check that ret == cmp (i.e. old value was cmp) to assume
success.

As for the memory orders on success/failure, they have nothing to do
with the likeliness of success/failure (which solely depends on the
current value of *ptr compared to the one in *expected), they are a
way to tell which memory ordering applies to the CAS operation w.r.t.
the simultaneous atomic operations on the same *ptr memory (for more
details you could find out about the acquire, release and
acquire+release semantics relating to the C++ memory model used by the
__atomic builtins). Roughly this is a way to tell how reads/writes on
different CPUs should synchronize with each other (reads waiting or
not for in-flight writes to finish and vice versa).
The APR uses __ATOMIC_SEQ_CST for all its __atomic operations which is
the stronger memory ordering (full barrier) since there is no way
(yet?) for the users to specify their own (i.e. a faster/weaker one
but safe enough for their usage, which the APR can't guess..).


Regards;
Yann.

Re: new HTTPProtocolOption for C-L+chunked?

[Yann Ylavic]

On Wed, Aug 16, 2023 at 1:43 PM Ruediger Pluem  wrote:
>
> On 8/16/23 1:32 PM, Eric Covener wrote:
> >>> So a few questions:
> >>>
> >>> - Is it reasonable as a standalone additional HTTPProtocolOption to
> >>> decide the behavior?
> >>> - Thoughts on behavior change in 2.4.x?
> >>> - 400 as a status code?
> >>>
> >>> https://httpwg.org/specs/rfc9112.html#rfc.section.6.1.p.15
> >>>
> >>> A server MAY reject a request that contains both Content-Length and
> >>> Transfer-Encoding or process such a request in accordance with the
> >>> Transfer-Encoding alone. Regardless, the server MUST close the
> >>> connection after responding to such a request to avoid the potential
> >>> attacks.
> >>
> >> We currently ignore the content-length header, proceed and close the 
> >> connection
> >> afterwards as suggested above. Do you suggest that we should reject such 
> >> requests
> >> based on a configuration setting?
> >
> > Yes, reject when both are set.
> >
>
> Sounds fine for me as long as we don't make it the default, at least not in 
> 2.4

Fine by me too, likely those requests come mainly/solely from the
usual suspects (pentesters, auditors, papers..), so I'd be fine with
blocking by default with an opt-out too (eg. HttpProtocolOptions
Allow_CL+TE). I've never seen a "legitimate" request with both set
personally, if it happens for some app it might be interesting for the
users to know it and fix their chain, we've done the same for "Strict"
and "Allow0.9" without too much pain IIRC.


Regards;
Yann.

Re: svn commit: r1910861 - /httpd/httpd/trunk/support/ab.c

[Yann Ylavic]

On Mon, Jul 10, 2023 at 10:53 AM Ruediger Pluem  wrote:
>
> On 7/10/23 10:11 AM, Joe Orton wrote:
> > On Fri, Jul 07, 2023 at 03:52:46PM -, yla...@apache.org wrote:
> >> Author: ylavic
> >> Date: Fri Jul  7 15:52:45 2023
> >> New Revision: 1910861
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1910861=rev
> >> Log:
> >> ab: Fix accounting of started connections.
> >>
> >> Revert when a kept alive connection is aborted on read.
> >> Stop the polling loop when there is nothing to poll anymore, it's simpler.
> >
> > Not related to this change but I noticed that trunk ab has stopped
> > failing if trying to connect to a closed port, strace looks like:
> >
> > epoll_wait(3, [{events=EPOLLOUT|EPOLLERR|EPOLLHUP, data={u32=33073216, 
> > u64=33073216}}], 2, 3) = 1
> > epoll_wait(3, [{events=EPOLLOUT|EPOLLERR|EPOLLHUP, data={u32=33073216, 
> > u64=33073216}}], 2, 3) = 1
> > ...
> >
> > With HUP set in the returned events it enters the first if() at
> > https://github.com/apache/httpd/blob/trunk/support/ab.c#L2523 but
> > STATE_CONNECTING is not handled there and so it loops indefinitely.
> > That's as far as I debugged it, sorry. Any ideas?
>
> I would guess that the below fixes the loop, but I am not sure if it is the 
> correct fix.
>
> Index: support/ab.c
> ===
> --- support/ab.c(revision 1910910)
> +++ support/ab.c(working copy)
> @@ -2534,6 +2534,11 @@
>  case STATE_READ:
>  read_response(c);
>  break;
> +case STATE_CONNECTING:
> +if (rtnew & APR_POLLHUP) {
> +try_reconnect(c, APR_ENOPOLL);
> +}
> +break;
>  }
>
>  continue;

I think this is not reached with rtnevents == POLLOUT|POLLHUP because
it takes the first POLLIN|POLLHUP continue-branch.
I moved the check for POLLOUT first in r1910911, which fixed the issue for me.


Regards;
Yann.

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

[Yann Ylavic]

On Fri, Jun 30, 2023 at 4:08 PM Yann Ylavic  wrote:
>
> On Fri, Jun 30, 2023 at 2:35 PM Ruediger Pluem  wrote:
> >
> > On 6/29/23 5:08 PM, Yann Ylavic wrote:
> > > On Tue, Apr 25, 2023 at 1:57 PM  wrote:
> > >>
> > >> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> > >> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Tue Apr 25 11:57:22 2023
> > >> @@ -2790,7 +2790,11 @@ ap_proxy_determine_connection(apr_pool_t
> > >>   * The single DNS lookup is used once per worker.
> > >>   * If dynamic change is needed then set the addr to 
> > >> NULL
> > >>   * inside dynamic config to force the lookup.
> > >> + *
> > >> + * Clear the dns_pool before to avoid a memory leak 
> > >> in case
> > >> + * we did the lookup again.
> > >>   */
> > >> +apr_pool_clear(worker->cp->dns_pool);
> > >>  err = apr_sockaddr_info_get(,
> > >>  conn->hostname, 
> > >> APR_UNSPEC,
> > >>  conn->port, 0,
> > >
> > > I'm not sure we can clear the dns_pool, some conn->addr allocated on
> > > it may be still in use by other threads.
> > > While reviewing your backport proposal, I noticed that
> > > worker->cp->addr was used concurrently in several places in mod_proxy
> > > with no particular care, so I started to code a follow up, but it
> > > needs that apr_pool_clear() too somewhere to avoid the leak and
> > > figured it may not be safe (like the above).
> > > I attach the patch here to show what I mean by insecure
> > > worker->cp->addr usage if we start to modify it concurrently, though
> > > more work is needed it seems..
> > >
> > > If I am correct we need to refcount the worker->cp->addr (or rather a
> > > worker->address struct). I had a patch which does that to handle a
> > > proxy address_ttl parameter (above which address is renewed), I can
> > > try to revive and mix it with the attached one, in a PR. WDYT?
> >
> > Thanks for the good catch. With regards to mod_proxy_ajp I think we should
> > use conn->addr instead of worker->cp->addr. We cannot be sure that 
> > worker->cp->addr is really set.
> > I guess it did not cause any problems so far as in the AJP case we always 
> > reuse connections by default
> > and hence worker->cp->addr is set.
> >
> > I think using a refcount could be burdensome as we need to ensure correct 
> > increase and decrease of the refcount and
> > we might have 3rd party modules that do not play well. Hence I propose a 
> > copy approach (but thinking of it, it might
> > fail in the same or other ways with 3rd party modules using 
> > worker->cp->addr directly in a similar way like mod_proxy_ajp does today)
>
> Let me look at what I can come up with and compare with what you
> propose below (which does not look trivial either from a quick look).
> That is to say, I need a bit more time to have an opinion here :)

So I went with https://github.com/apache/httpd/pull/367 (sorry, not
much detailed commits yet), and specifically the first commit for the
reuse / ttl / force-expiring of the worker address(es).

The new function is (named after ap_proxy_determine_connection):

/*
 * Resolve an address, reusing the one of the worker if any.
 * @param proxy_function calling proxy scheme (http, ajp, ...)
 * @param conn proxy connection the address is used for
 * @param hostname host to resolve (should be the worker's if reusable)
 * @param hostport port to resolve (should be the worker's if reusable)
 * @param r current request (if any)
 * @param s current server (or NULL if r != NULL and ap_proxyerror()
 *  should be called on error)
 * @return APR_SUCCESS or an error
 */
PROXY_DECLARE(apr_status_t) ap_proxy_determine_address(const char
*proxy_function,
   proxy_conn_rec *conn,
   const char *hostname,
   apr_port_t hostport,
   request_rec *r,
   server_rec *s);

and it takes a proxy_conn_rec only, which simplifies things and which
we can provide everywhere after all (see the changes in mod_proxy_ftp
and

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

[Yann Ylavic]

On Mon, Jul 3, 2023 at 11:10 AM Ruediger Pluem  wrote:
>
>
>
> On 7/3/23 10:17 AM, Stefan Eissing via dev wrote:
> >
> >
> >> Am 30.06.2023 um 17:22 schrieb Stefan Eissing via dev 
> >> :
> >>
> >>
> >>
> >>> Am 30.06.2023 um 16:45 schrieb Ruediger Pluem :
> >>>
> >>>
> >>>
> >>> On 6/30/23 4:15 PM, Stefan Eissing via dev wrote:
> >>>>
> >>>>
> >>>>> Am 30.06.2023 um 15:56 schrieb Yann Ylavic :
> >>>>>
> >>>>> On Fri, Jun 30, 2023 at 2:44 PM Ruediger Pluem  
> >>>>> wrote:
> >>>>>>
> >>>>>> On 6/30/23 11:08 AM, ic...@apache.org wrote:
> >>>>>>> Author: icing
> >>>>>>> Date: Fri Jun 30 09:08:23 2023
> >>>>>>> New Revision: 1910704
> >>>>>>>
> >>>>>>> URL: http://svn.apache.org/viewvc?rev=1910704=rev
> >>>>>>> Log:
> >>>>>>> proxy: in proxy tunnels, use the smaller timeout value of
> >>>>>>> client and origin as timeout for polling the tunnel.
> >>>>>>>
> >>>>>>>
> >>>>>>> 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=1910704=1910703=1910704=diff
> >>>>>>> ==
> >>>>>>> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> >>>>>>> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Fri Jun 30 09:08:23 
> >>>>>>> 2023
> >>>>>>> @@ -4921,9 +4921,9 @@ PROXY_DECLARE(apr_status_t) ap_proxy_tun
> >>>>>>>   apr_socket_timeout_get(tunnel->origin->pfd->desc.s, 
> >>>>>>> _timeout);
> >>>>>>>   apr_socket_opt_set(tunnel->origin->pfd->desc.s, APR_SO_NONBLOCK, 1);
> >>>>>>>
> >>>>>>> -/* Defaults to the biggest timeout of both connections */
> >>>>>>> -tunnel->timeout = (origin_timeout >= 0 && origin_timeout > 
> >>>>>>> client_timeout)?
> >>>>>>> -  origin_timeout : client_timeout;
> >>>>>>> +/* Defaults to the smallest timeout of both connections */
> >>>>>>> +tunnel->timeout = (client_timeout >= 0 && client_timeout < 
> >>>>>>> origin_timeout ?
> >>>>>>> +   client_timeout : origin_timeout);
> >>>>>>
> >>>>>> Why?
> >>>>>
> >>>>> We discussed this (quickly) with Stefan on
> >>>>> https://github.com/apache/httpd/pull/366, but hey the commit is here
> >>>>> for review finally :)
> >>>>>
> >>>>>> It was the other way round on purpose, e.g. if Timeout is set to 5 for 
> >>>>>> a small front end timeout and ProxyTimeout is set to
> >>>>>> e.g. 600 to keep Websockets open for 10 minutes.
> >>>>>
> >>>>> It seems to me that using Timeout (5s) here is a valid case too if
> >>>>> Timeout < ProxyTimeout (as in your example) is a way to limit how long
> >>>>> a client can consume httpd resources.
> >>>>> So maybe we should only use the backend timeout which is an easy(er)
> >>>>> way for the user to control this?
> >>>>
> >>>> So, the goal is to allow someone keeping the websocket open for longer
> >>>> than we usually allow for HTTP requests, to set a long ProxyTimeout or
> >>>> timeout parameter to ProxyPass.
> >>>>
> >>>> For HTTP/1.1 that would override the connection Timeout, since the tunnel
> >>>> poll would only use the largest value.
> >>>>
> >>>> For HTTP/2 I have to check how that how to accomplish that. The working
> >>>> there is different.
> >>>
> >>> Sorry for being a bit grumpy above, but this came out of the 

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

[Yann Ylavic]

On Fri, Jun 30, 2023 at 2:35 PM Ruediger Pluem  wrote:
>
> On 6/29/23 5:08 PM, Yann Ylavic wrote:
> > On Tue, Apr 25, 2023 at 1:57 PM  wrote:
> >>
> >> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> >> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Tue Apr 25 11:57:22 2023
> >> @@ -2790,7 +2790,11 @@ ap_proxy_determine_connection(apr_pool_t
> >>   * The single DNS lookup is used once per worker.
> >>   * If dynamic change is needed then set the addr to 
> >> NULL
> >>   * inside dynamic config to force the lookup.
> >> + *
> >> + * Clear the dns_pool before to avoid a memory leak 
> >> in case
> >> + * we did the lookup again.
> >>   */
> >> +apr_pool_clear(worker->cp->dns_pool);
> >>  err = apr_sockaddr_info_get(,
> >>  conn->hostname, 
> >> APR_UNSPEC,
> >>  conn->port, 0,
> >
> > I'm not sure we can clear the dns_pool, some conn->addr allocated on
> > it may be still in use by other threads.
> > While reviewing your backport proposal, I noticed that
> > worker->cp->addr was used concurrently in several places in mod_proxy
> > with no particular care, so I started to code a follow up, but it
> > needs that apr_pool_clear() too somewhere to avoid the leak and
> > figured it may not be safe (like the above).
> > I attach the patch here to show what I mean by insecure
> > worker->cp->addr usage if we start to modify it concurrently, though
> > more work is needed it seems..
> >
> > If I am correct we need to refcount the worker->cp->addr (or rather a
> > worker->address struct). I had a patch which does that to handle a
> > proxy address_ttl parameter (above which address is renewed), I can
> > try to revive and mix it with the attached one, in a PR. WDYT?
>
> Thanks for the good catch. With regards to mod_proxy_ajp I think we should
> use conn->addr instead of worker->cp->addr. We cannot be sure that 
> worker->cp->addr is really set.
> I guess it did not cause any problems so far as in the AJP case we always 
> reuse connections by default
> and hence worker->cp->addr is set.
>
> I think using a refcount could be burdensome as we need to ensure correct 
> increase and decrease of the refcount and
> we might have 3rd party modules that do not play well. Hence I propose a copy 
> approach (but thinking of it, it might
> fail in the same or other ways with 3rd party modules using worker->cp->addr 
> directly in a similar way like mod_proxy_ajp does today)

Let me look at what I can come up with and compare with what you
propose below (which does not look trivial either from a quick look).
That is to say, I need a bit more time to have an opinion here :)

>
> I modified some code from your patch especially ap_proxy_worker_addr_get:

Thanks;
Yann.

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

[Yann Ylavic]

On Fri, Jun 30, 2023 at 2:44 PM Ruediger Pluem  wrote:
>
> On 6/30/23 11:08 AM, ic...@apache.org wrote:
> > Author: icing
> > Date: Fri Jun 30 09:08:23 2023
> > New Revision: 1910704
> >
> > URL: http://svn.apache.org/viewvc?rev=1910704=rev
> > Log:
> > proxy: in proxy tunnels, use the smaller timeout value of
> >client and origin as timeout for polling the tunnel.
> >
> >
> > 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=1910704=1910703=1910704=diff
> > ==
> > --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> > +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Fri Jun 30 09:08:23 2023
> > @@ -4921,9 +4921,9 @@ PROXY_DECLARE(apr_status_t) ap_proxy_tun
> >  apr_socket_timeout_get(tunnel->origin->pfd->desc.s, _timeout);
> >  apr_socket_opt_set(tunnel->origin->pfd->desc.s, APR_SO_NONBLOCK, 1);
> >
> > -/* Defaults to the biggest timeout of both connections */
> > -tunnel->timeout = (origin_timeout >= 0 && origin_timeout > 
> > client_timeout)?
> > -  origin_timeout : client_timeout;
> > +/* Defaults to the smallest timeout of both connections */
> > +tunnel->timeout = (client_timeout >= 0 && client_timeout < 
> > origin_timeout ?
> > +   client_timeout : origin_timeout);
>
> Why?

We discussed this (quickly) with Stefan on
https://github.com/apache/httpd/pull/366, but hey the commit is here
for review finally :)

> It was the other way round on purpose, e.g. if Timeout is set to 5 for a 
> small front end timeout and ProxyTimeout is set to
> e.g. 600 to keep Websockets open for 10 minutes.

It seems to me that using Timeout (5s) here is a valid case too if
Timeout < ProxyTimeout (as in your example) is a way to limit how long
a client can consume httpd resources.
So maybe we should only use the backend timeout which is an easy(er)
way for the user to control this?

Regards;
Yann.

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

[Yann Ylavic]

On Tue, Apr 25, 2023 at 1:57 PM  wrote:
>
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Tue Apr 25 11:57:22 2023
> @@ -2790,7 +2790,11 @@ ap_proxy_determine_connection(apr_pool_t
>   * The single DNS lookup is used once per worker.
>   * If dynamic change is needed then set the addr to NULL
>   * inside dynamic config to force the lookup.
> + *
> + * Clear the dns_pool before to avoid a memory leak in 
> case
> + * we did the lookup again.
>   */
> +apr_pool_clear(worker->cp->dns_pool);
>  err = apr_sockaddr_info_get(,
>  conn->hostname, APR_UNSPEC,
>  conn->port, 0,

I'm not sure we can clear the dns_pool, some conn->addr allocated on
it may be still in use by other threads.
While reviewing your backport proposal, I noticed that
worker->cp->addr was used concurrently in several places in mod_proxy
with no particular care, so I started to code a follow up, but it
needs that apr_pool_clear() too somewhere to avoid the leak and
figured it may not be safe (like the above).
I attach the patch here to show what I mean by insecure
worker->cp->addr usage if we start to modify it concurrently, though
more work is needed it seems..

If I am correct we need to refcount the worker->cp->addr (or rather a
worker->address struct). I had a patch which does that to handle a
proxy address_ttl parameter (above which address is renewed), I can
try to revive and mix it with the attached one, in a PR. WDYT?


Regards;
Yann.
diff --git a/modules/proxy/mod_proxy.h b/modules/proxy/mod_proxy.h
index 04fee22742..d55381de64 100644
--- a/modules/proxy/mod_proxy.h
+++ b/modules/proxy/mod_proxy.h
@@ -1041,6 +1041,25 @@ PROXY_DECLARE(int) ap_proxy_post_request(proxy_worker *worker,
  request_rec *r,
  proxy_server_conf *conf);
 
+/**
+ * Resolve an address, reusing the one of the worker if any.
+ * @param worker   worker the address is used for
+ * @param addrpreturned address pointer
+ * @param host host to resolve (the worker's if reusable)
+ * @param host port to resolve (the worker's if reusable)
+ * @param rcurrent request (if any)
+ * @param scurrent server (if any)
+ * @param ppool to allocate the address (ignored for reusable worker)
+ * @return APR_SUCCESS or an error
+ */
+PROXY_DECLARE(apr_status_t) ap_proxy_worker_addr_get(proxy_worker *worker,
+ apr_sockaddr_t **addrp,
+ const char *host,
+ apr_port_t port,
+ request_rec *r,
+ server_rec *s,
+ apr_pool_t *p);
+
 /**
  * Determine backend hostname and port
  * @param p   memory pool used for processing
diff --git a/modules/proxy/mod_proxy_ajp.c b/modules/proxy/mod_proxy_ajp.c
index cedf71379c..34f13cbaf1 100644
--- a/modules/proxy/mod_proxy_ajp.c
+++ b/modules/proxy/mod_proxy_ajp.c
@@ -236,8 +236,7 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r,
 if (status != APR_SUCCESS) {
 conn->close = 1;
 ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, APLOGNO(00868)
-  "request failed to %pI (%s:%d)",
-  conn->worker->cp->addr,
+  "request failed to %pI (%s:%d)", conn->addr,
   conn->worker->s->hostname_ex,
   (int)conn->worker->s->port);
 if (status == AJP_EOVERFLOW)
@@ -334,8 +333,7 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r,
 conn->close = 1;
 apr_brigade_destroy(input_brigade);
 ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, APLOGNO(00876)
-  "send failed to %pI (%s:%d)",
-  conn->worker->cp->addr,
+  "send failed to %pI (%s:%d)", conn->addr,
   conn->worker->s->hostname_ex,
   (int)conn->worker->s->port);
 /*
@@ -376,8 +374,7 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r,
 conn->close = 1;
 apr_brigade_destroy(input_brigade);
 ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, APLOGNO(00878)
-  "read response failed from %pI (%s:%d)",
-  conn->worker->cp->addr,
+  "read response failed from %pI (%s:%d)", conn->addr,
   

Re: svn commit: r1910649 - in /httpd/httpd/trunk/test: clients/h2ws.c modules/http2/test_800_websockets.py modules/http2/ws_server.py

[Yann Ylavic]

Hi Stefan,

>
> Modified:
> httpd/httpd/trunk/test/clients/h2ws.c
> httpd/httpd/trunk/test/modules/http2/test_800_websockets.py
> httpd/httpd/trunk/test/modules/http2/ws_server.py

Are the ws tests supposed to run/pass in ci? They don't currently, and
ISTR from the PR that some python libs versions prevent them from
passing so I'm wondering if disabing isn't working there or if it's a
real failure..


Regards;
Yann.

Re: svn commit: r1910267 - in /httpd/httpd/trunk: docs/log-message-tags/next-number modules/filters/mod_ext_filter.c

[Yann Ylavic]

On Thu, Jun 8, 2023 at 9:03 AM Giovanni Bechis  wrote:
>
> This should fix both issues.

+1, thanks!

Regards;
Yann.

Re: svn commit: r1910267 - in /httpd/httpd/trunk: docs/log-message-tags/next-number modules/filters/mod_ext_filter.c

[Yann Ylavic]

On Wed, Jun 7, 2023 at 4:36 PM Ruediger Pluem  wrote:
>
> On 6/7/23 1:56 PM, Yann Ylavic wrote:
> > Hi Giovanni;
> >
> > On Wed, Jun 7, 2023 at 12:02 AM  wrote:
> >>
> >> Author: gbechis
> >> Date: Tue Jun  6 22:02:37 2023
> >> New Revision: 1910267
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1910267=rev
> >> Log:
> >> mod_ext_filter: check exit status of filter processes
> > []
> >>
> >> +/* check_filter_process_on_eos():
> >> + *
> >> + * if we hit end-of-stream, check the exit status of the filter process, 
> >> and log
> >> + * an appropriate message if it failed
> >> + */
> >> +static apr_status_t check_filter_process_on_eos(ef_ctx_t *ctx, 
> >> request_rec *r)
> >> +{
> >> +if (ctx->hit_eos) {
> >> +int exitcode;
> >> +apr_exit_why_e exitwhy;
> >> +apr_status_t waitret = apr_proc_wait(ctx->proc, , 
> >> ,
> >> + APR_WAIT);
> >> +if (waitret != APR_CHILD_DONE) {
> >> +ap_log_rerror(APLOG_MARK, APLOG_ERR, waitret, r, 
> >> APLOGNO(10451)
> >> +  "apr_proc_wait() failed, uri=%s", r->uri);
> >> +return waitret;
> >> +}
> >> +else if (exitwhy != APR_PROC_EXIT) {
> >> +ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_SUCCESS, r, 
> >> APLOGNO(10452)
> >> +  "child process %s killed by signal %d, uri=%s",
> >> +  ctx->filter->command, exitcode, r->uri);
> >> +return HTTP_INTERNAL_SERVER_ERROR;
> >> +}
> >> +else if (exitcode != 0) {
> >> +ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_SUCCESS, r, 
> >> APLOGNO(10453)
> >> +  "child process %s exited with non-zero status 
> >> %d, "
> >> +  "uri=%s", ctx->filter->command, exitcode, 
> >> r->uri);
> >> +return HTTP_INTERNAL_SERVER_ERROR;
> >> +}
> >
> > HTTP_INTERNAL_SERVER_ERROR (like all HTTP_* statuses) is not an
> > apr_status_t, it shouldn't be returned by a filter (and does not print
> > well as an ap_log_rerror() error status for instance like below).
> >
> > Maybe use APR_EGENERAL? The error message could be enough to
> > distinguish them here.
> > I wouldn't return waitret for the first case either since it's in the
> > error message already, no need to forward it specifically to the
> > caller, so APR_EGENERAL still possibly.
> >
> >> +}
> >> +
> >> +return APR_SUCCESS;
> >> +}
> >> +
> >>  /* ef_unified_filter:
> >>   *
> >>   * runs the bucket brigade bb through the filter and puts the result into
> >> @@ -880,6 +914,11 @@ static apr_status_t ef_output_filter(ap_
> >>  if (rv != APR_SUCCESS) {
> >>  ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01468)
> >>"ef_unified_filter() failed");
> >> +return rv;
> >> +}
> >> +
> >> +if ((rv = check_filter_process_on_eos(ctx, r)) != APR_SUCCESS) {
> >> +return rv;
> >
> > Not correct here for a filter.
>
> I am a little bit confused. Provided that your comments on the 
> check_filter_process_on_eos are considered and the code is changed
> accordingly, why would it be incorrect for the  filter to return this?

Sorry for not being clear. I meant *if the code is not changed* that's
where a/some/this filter returns an HTTP_ error code instead of an
apr_status_t, which may confuse any upper filter or logger (not the
end of the world though, it should hardly be considered something
recoverable).

>
> >
> >>  }
> >>
> >>  if ((rv = ap_pass_brigade(f->next, bb)) != APR_SUCCESS) {
> >> @@ -939,7 +978,13 @@ static apr_status_t ef_input_filter(ap_f
> >>  }
> >>
> >>  rv = ef_unified_filter(f, bb);
> >> -return rv;
> >> +if (rv != APR_SUCCESS) {
> >> +ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r, APLOGNO(10454)
> >> +  "ef_unified_filter() failed");
> >> +return rv;
> >
> > Ditto, for both the error log status and return value.
>
> Same confusion as above: While ef_unified_filter formally returns an int it 
> looks like its content is actually an apr_status_t.
> Hence why shouldn't it be used in the log message or returned?

Oh I misread ef_unified_filter() as ef_output_filter() and thought the
error was propagating here too.
That's not the case, though now that I look at it in the code (rather
than the diff only..) it probably makes sense for ef_unified_filter()
to declare an apr_status_t as returned type (which FWICT it actually
always returns, as you said).

>
> >
> >> +}
> >> +
> >> +return check_filter_process_on_eos(ctx, f->r);
> >>  }
> >

Regards;
Yann.

Re: svn commit: r1910267 - in /httpd/httpd/trunk: docs/log-message-tags/next-number modules/filters/mod_ext_filter.c

[Yann Ylavic]

Hi Giovanni;

On Wed, Jun 7, 2023 at 12:02 AM  wrote:
>
> Author: gbechis
> Date: Tue Jun  6 22:02:37 2023
> New Revision: 1910267
>
> URL: http://svn.apache.org/viewvc?rev=1910267=rev
> Log:
> mod_ext_filter: check exit status of filter processes
[]
>
> +/* check_filter_process_on_eos():
> + *
> + * if we hit end-of-stream, check the exit status of the filter process, and 
> log
> + * an appropriate message if it failed
> + */
> +static apr_status_t check_filter_process_on_eos(ef_ctx_t *ctx, request_rec 
> *r)
> +{
> +if (ctx->hit_eos) {
> +int exitcode;
> +apr_exit_why_e exitwhy;
> +apr_status_t waitret = apr_proc_wait(ctx->proc, , ,
> + APR_WAIT);
> +if (waitret != APR_CHILD_DONE) {
> +ap_log_rerror(APLOG_MARK, APLOG_ERR, waitret, r, APLOGNO(10451)
> +  "apr_proc_wait() failed, uri=%s", r->uri);
> +return waitret;
> +}
> +else if (exitwhy != APR_PROC_EXIT) {
> +ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_SUCCESS, r, 
> APLOGNO(10452)
> +  "child process %s killed by signal %d, uri=%s",
> +  ctx->filter->command, exitcode, r->uri);
> +return HTTP_INTERNAL_SERVER_ERROR;
> +}
> +else if (exitcode != 0) {
> +ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_SUCCESS, r, 
> APLOGNO(10453)
> +  "child process %s exited with non-zero status %d, "
> +  "uri=%s", ctx->filter->command, exitcode, r->uri);
> +return HTTP_INTERNAL_SERVER_ERROR;
> +}

HTTP_INTERNAL_SERVER_ERROR (like all HTTP_* statuses) is not an
apr_status_t, it shouldn't be returned by a filter (and does not print
well as an ap_log_rerror() error status for instance like below).

Maybe use APR_EGENERAL? The error message could be enough to
distinguish them here.
I wouldn't return waitret for the first case either since it's in the
error message already, no need to forward it specifically to the
caller, so APR_EGENERAL still possibly.

> +}
> +
> +return APR_SUCCESS;
> +}
> +
>  /* ef_unified_filter:
>   *
>   * runs the bucket brigade bb through the filter and puts the result into
> @@ -880,6 +914,11 @@ static apr_status_t ef_output_filter(ap_
>  if (rv != APR_SUCCESS) {
>  ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01468)
>"ef_unified_filter() failed");
> +return rv;
> +}
> +
> +if ((rv = check_filter_process_on_eos(ctx, r)) != APR_SUCCESS) {
> +return rv;

Not correct here for a filter.

>  }
>
>  if ((rv = ap_pass_brigade(f->next, bb)) != APR_SUCCESS) {
> @@ -939,7 +978,13 @@ static apr_status_t ef_input_filter(ap_f
>  }
>
>  rv = ef_unified_filter(f, bb);
> -return rv;
> +if (rv != APR_SUCCESS) {
> +ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r, APLOGNO(10454)
> +  "ef_unified_filter() failed");
> +return rv;

Ditto, for both the error log status and return value.

> +}
> +
> +return check_filter_process_on_eos(ctx, f->r);
>  }


Regards;
Yann.

Re: svn commit: r1892450 - /httpd/httpd/trunk/server/core_filters.c

[Yann Ylavic]

On Sun, May 21, 2023 at 4:22 PM Yann Ylavic  wrote:
>
> On Sun, May 21, 2023 at 2:07 PM Christophe JAILLET
>  wrote:
> >
> > Le 19/08/2021 à 15:43, yla...@apache.org a écrit :
> > >
> > > @@ -604,7 +604,7 @@ static apr_status_t send_brigade_nonbloc
> > >*/
> > >   if (nbytes > sconf->flush_max_threshold
> > >   && next != APR_BRIGADE_SENTINEL(bb)
> > > -&& !is_in_memory_bucket(next)) {
> > > +&& next->length && !is_in_memory_bucket(next)) {
> > >   (void)apr_socket_opt_set(s, APR_TCP_NOPUSH, 1);
> > >   rv = writev_nonblocking(s, bb, ctx, nbytes, nvec, c);
> > >   if (rv != APR_SUCCESS) {
> >
> > The comment above this code is:
> >  /* Flush above max threshold, unless the brigade still contains in
> >   * memory buckets which we want to try writing in the same pass (if
> >   * we are at the end of the brigade, the write will happen outside
> >   * the loop anyway).
> >   */
> >
> > With the added next->length, IIUC, we will *also* process the bucket
> > *after* the metadata. So we could accumulate above flush_max_threshold
> > for no good reason (i.e. not processing data already in memory).
> >
> > Is it what is expected?
>
> Buckets with ->length == 0 don't contain data (in memory or not), as
> opposed to ->length == -1 (unknown/on-read data length) for instance.
> No special action is needed for empty buckets at the network level,
> they just need to be destroyed when consumed (e.g. for EOR buckets to
> terminate/cleanup the underlying request), so the loop simply ignores
> them until the flush (i.e. writev/sendfile will delete them finally).
> So presumably we can't accumulate above flush_max_threshold by not
> flushing before empty buckets? More exactly we won't continue to
> accumulate once flush_max_threshold is reached, but we might flush
> more than that if the last accumulated bucket crosses the threshold
> (that's independent from empty buckets though).

Hm, re-reading your question and the code I think I replied
orthogonally (and not accurately) here.
What the code does actually is coalescing in memory buckets (including
empty ones, barely) regardless of flush_max_threshold, precisely
because they are in memory already and we want to try sending them on
the network (up to its actual capacity) to release memory on httpd as
much as possible. Ignoring ->length == 0 barely means that metadata
buckets are in memory buckets (which they really are actually), maybe
it would be clearer to add the ->length == 0 test directly in
is_in_memory_bucket() after all..

Anyway I see what you mean now, when we are at flush_max_threshold and
the next bucket is a metadata, ignoring/continuing on that next bucket
in the next loop without re-checking for flush_max_threshold is bad.
And you are very right, good catch! Your patch looks good to me too.

This commit (r1892450) did not make it to 2.4.x it seems, maybe with
your fix it could now :) That's a good optimization I think..


Regards;
Yann.

Re: svn commit: r1892450 - /httpd/httpd/trunk/server/core_filters.c

[Yann Ylavic]

On Sun, May 21, 2023 at 2:07 PM Christophe JAILLET
 wrote:
>
> Le 19/08/2021 à 15:43, yla...@apache.org a écrit :
> > Author: ylavic
> > Date: Thu Aug 19 13:43:23 2021
> > New Revision: 1892450
> >
> > URL: http://svn.apache.org/viewvc?rev=1892450=rev
> > Log:
> > core: don't break out iovec coalescing for metadata in 
> > ap_core_output_filter().
> >
> > * server/core_filters.c (send_brigade_nonblocking):
> >Keep filling in the iovec when the next bucket has no ->length.
> >
> > Modified:
> >  httpd/httpd/trunk/server/core_filters.c
> >
> > Modified: httpd/httpd/trunk/server/core_filters.c
> > URL: 
> > http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core_filters.c?rev=1892450=1892449=1892450=diff
> > ==
> > --- httpd/httpd/trunk/server/core_filters.c (original)
> > +++ httpd/httpd/trunk/server/core_filters.c Thu Aug 19 13:43:23 2021
> > @@ -604,7 +604,7 @@ static apr_status_t send_brigade_nonbloc
> >*/
> >   if (nbytes > sconf->flush_max_threshold
> >   && next != APR_BRIGADE_SENTINEL(bb)
> > -&& !is_in_memory_bucket(next)) {
> > +&& next->length && !is_in_memory_bucket(next)) {
> >   (void)apr_socket_opt_set(s, APR_TCP_NOPUSH, 1);
> >   rv = writev_nonblocking(s, bb, ctx, nbytes, nvec, c);
> >   if (rv != APR_SUCCESS) {
> >
> >
> >
>
> Hi,
>
> The comment above this code is:
>  /* Flush above max threshold, unless the brigade still contains in
>   * memory buckets which we want to try writing in the same pass (if
>   * we are at the end of the brigade, the write will happen outside
>   * the loop anyway).
>   */
>
> With the added next->length, IIUC, we will *also* process the bucket
> *after* the metadata. So we could accumulate above flush_max_threshold
> for no good reason (i.e. not processing data already in memory).
>
> Is it what is expected?

Buckets with ->length == 0 don't contain data (in memory or not), as
opposed to ->length == -1 (unknown/on-read data length) for instance.
No special action is needed for empty buckets at the network level,
they just need to be destroyed when consumed (e.g. for EOR buckets to
terminate/cleanup the underlying request), so the loop simply ignores
them until the flush (i.e. writev/sendfile will delete them finally).
So presumably we can't accumulate above flush_max_threshold by not
flushing before empty buckets? More exactly we won't continue to
accumulate once flush_max_threshold is reached, but we might flush
more than that if the last accumulated bucket crosses the threshold
(that's independent from empty buckets though).


Regards;
Yann.

Re: PR 66499

[Yann Ylavic]

On Mon, May 15, 2023 at 9:31 AM Stefan Eissing  wrote:
>
> > Am 14.05.2023 um 21:27 schrieb Yann Ylavic :
> >
> > On Fri, May 12, 2023 at 6:35 PM Stefan Eissing via dev
> >  wrote:
> >>
> >> How is mod_http2 supposed to know which case is in play? Any ideas?
> >
> > It can't know I agree, but possibly mod_h2 could fill in r->parsed_uri
> > when creating the request_rec (as if a full uri-path were received),
> > that's what proxy_detect() needs to allow forward-proxying (in
> > addition to ProxyRequests on). Reverse-proxying won't use
> > r->parsed_uri.{scheme,hostname} anyway so ProxyRequests on/off would
> > be the only criteria, which seems fine?
>
> Hmm, sounds hacky. I think I'd rather add a directive like `H2ForwardProxy 
> on`.

Not that much hacky I think, if an HTTP/1 request is received with
full uri-path (which is allowed) then r->parsed_uri is fully populated
regardless of how the request will be handled (local, cgi, proxy
reverse/forward). That's on the handler to use it (or not) and format
an eventual upstream request according to the configured protocol. And
since :scheme and :authority are (always?) available in HTTP/2, it
makes sense to me to provide them in r->parsed_uri too. It seems that
vhost selection (and r->hostname / Host header) are based on
:authority if available already, just like HTTP/1.

>
> > PS: Since we are discussing PRs, maybe we can talk about PR 66597 :p
> > There it seems that in 2.4.x (i.e. !AP_HAS_RESPONSE_BUCKETS), the
> > H2_C2_REQUEST_IN filter will insert chunk encoding (via
> > read_and_chunk()) at AP_FTYPE_PROTOCOL, which is the same level as the
> > HTTP_IN filter supposed to consume this chunking (IIUC).
> > Could it be possible that H2_C2_REQUEST_IN gets added earlier than
> > HTTP_IN such that the order of the two is backward (i.e. the chunk
> > encoding is never consumed), which is why mod_proxy ends up
> > reading/forwarding chunked encoding as if HTTP_IN was not called?
>
> Maybe, but I do not understand the difference between mod_proxy_* and 
> mod_cgid in chunked input processing. For the latter I have test cases that 
> work.

That's mod_proxy_fcgi but yeah, it should work the same as
mod_proxy_http for instance (common code). Let's see what the logs
show in the PR and if the proposed patch helps..


Regards;
Yann.

Re: PR 66499

[Yann Ylavic]

On Fri, May 12, 2023 at 6:35 PM Stefan Eissing via dev
 wrote:
>
> Regarding https://bz.apache.org/bugzilla/show_bug.cgi?id=66499 I have a 
> question.
>
> When using httpd in a forward proxy setup ("ProxyRequests On"), we expect 
> absolute URLs in HTTP/1.1 requests. Fine.
>
> When accessing such a setup via HTTP/2, we get :scheme, :auhority and :path 
> and, usually, do NOT want to convert that to absolute URIs in the reuqest 
> line. That is what 66499 complained about. However, in the forward proxy 
> case, we need it.
>
> How is mod_http2 supposed to know which case is in play? Any ideas?

It can't know I agree, but possibly mod_h2 could fill in r->parsed_uri
when creating the request_rec (as if a full uri-path were received),
that's what proxy_detect() needs to allow forward-proxying (in
addition to ProxyRequests on). Reverse-proxying won't use
r->parsed_uri.{scheme,hostname} anyway so ProxyRequests on/off would
be the only criteria, which seems fine?


PS: Since we are discussing PRs, maybe we can talk about PR 66597 :p
There it seems that in 2.4.x (i.e. !AP_HAS_RESPONSE_BUCKETS), the
H2_C2_REQUEST_IN filter will insert chunk encoding (via
read_and_chunk()) at AP_FTYPE_PROTOCOL, which is the same level as the
HTTP_IN filter supposed to consume this chunking (IIUC).
Could it be possible that H2_C2_REQUEST_IN gets added earlier than
HTTP_IN such that the order of the two is backward (i.e. the chunk
encoding is never consumed), which is why mod_proxy ends up
reading/forwarding chunked encoding as if HTTP_IN was not called?
So maybe the below patch would make the ordering more robust:

Index: modules/http2/h2_c2.c
===
--- modules/http2/h2_c2.c(revision 1909607)
+++ modules/http2/h2_c2.c(working copy)
@@ -854,7 +854,7 @@ void h2_c2_register_hooks(void)
   NULL, AP_FTYPE_NETWORK);

 ap_register_input_filter("H2_C2_REQUEST_IN", h2_c2_filter_request_in,
- NULL, AP_FTYPE_PROTOCOL);
+ NULL, AP_FTYPE_PROTOCOL - 1);
 ap_register_output_filter("H2_C2_RESPONSE_OUT", h2_c2_filter_response_out,
   NULL, AP_FTYPE_PROTOCOL);
 ap_register_output_filter("H2_C2_TRAILERS_OUT", h2_c2_filter_trailers_out,
?


Regards;
Yann.

Re: Fwd: [apache/httpd] don't forward invalid query strings (d78a166)

[Yann Ylavic]

On Tue, May 9, 2023 at 2:10 PM Yann Ylavic  wrote:
>
> On Tue, May 9, 2023 at 12:55 PM Ruediger Pluem  wrote:
> >
> > On 5/9/23 12:16 PM, Eric Covener wrote:
> > > Still getting feedback in the PR about breakage. Any thoughts on options 
> > > here, like allowing spaces or encoding rather than failing?
> >
> > Allowing spaces is out of question for me as it creates an invalid request 
> > and opens the door to response splitting. At best we
> > could encode automatically. It is really a good question if we could not 
> > make BCTLS the default.
>
> BCTLS by default looks fine to me, it changes the behaviour for users
> that (used to) expect/handle decoded spaces in the query-string in
> their scripts, but it's blocked now anyway..

Hm, actually we don't really know where the backref is placed (either
in the uri-path or in the query-string), so escaping unconditionally
might lead to double-escaping in the uri-path. Maybe it's simpler to
remove the check and leave it to mod_proxy only..

>
>
> Regards;
> Yann.

Re: Fwd: [apache/httpd] don't forward invalid query strings (d78a166)

[Yann Ylavic]

On Tue, May 9, 2023 at 12:55 PM Ruediger Pluem  wrote:
>
> On 5/9/23 12:16 PM, Eric Covener wrote:
> > Still getting feedback in the PR about breakage. Any thoughts on options 
> > here, like allowing spaces or encoding rather than failing?
>
> Allowing spaces is out of question for me as it creates an invalid request 
> and opens the door to response splitting. At best we
> could encode automatically. It is really a good question if we could not make 
> BCTLS the default.

BCTLS by default looks fine to me, it changes the behaviour for users
that (used to) expect/handle decoded spaces in the query-string in
their scripts, but it's blocked now anyway..


Regards;
Yann.

Re: ci jobs

[Yann Ylavic]

On Wed, May 3, 2023 at 10:43 AM Stefan Eissing via dev
 wrote:
>
> FYI, github workflows: mod_http2 and mod_tls CI jobs are re-enabled in trunk 
> and 2.4.x

Great, thanks Stefan!

Regards;
Yann.

Re: build trunk in windows

[Yann Ylavic]

On Wed, May 3, 2023 at 2:54 PM jean-frederic clere  wrote:
>
> On 4/24/23 18:25, Steffen wrote:
> > There is a howto Building Apache and dependencies using CMake at
> >
> > https://www.apachelounge.com/viewtopic.php?t=8609
> > 
> >
> >
>
> I ended fixing include/http_protocol.h see patch, did I miss something?

Looks like ap_h1_response_out_filter() is declared in
"include/mod_core.h" already, but without AP_CORE_DECLARE_NONSTD().
Not sure if we should remove the AP_CORE_DECLARE_NONSTD() in
"modules/http/http_filters.c" (where it's implemented) or add it in
the declaration. For instance ap_http_outerror_filter() has no
AP_CORE_DECLARE_NONSTD() anywhere..

Regards;
Yann.

Re: [VOTE] Switch read/write repository from Subversion to Git

[Yann Ylavic]

On Thu, May 4, 2023 at 10:34 AM Ruediger Pluem  wrote:
>
> This is a formal vote on whether we should move our read/write repository 
> from Subversion to Git.
> This means that our latest read/write repository will be no longer available 
> via svn.apache.org. It
> will be available via Git at 
> https://gitbox.apache.org/repos/asf/httpd-site.git and 
> https://github.com/apache/httpd.git.
> Github also offers the possibility to use a Subversion client:
> https://docs.github.com/en/get-started/working-with-subversion-on-github/support-for-subversion-clients
>

[X]: Move the read/write repository from Subversion to Git and
leverage the features of Github (for now Actions and PR).

Regards;
Yann.

Re: svn commit: r1909135 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h server/core.c

[Yann Ylavic]

On Fri, Apr 14, 2023 at 4:02 PM  wrote:
>
> Author: minfrin
> Date: Fri Apr 14 14:02:11 2023
> New Revision: 1909135
>
> URL: http://svn.apache.org/viewvc?rev=1909135=rev
> Log:
> core: Be explicit if an enclosing directive contains a path or a
> regex.

Currently all the tests (framework) are broken due to cmd->regex not
being properly stacked/scoped.
If cmd->regex is to be used to store the enclosing section's regex, it
should be saved and restored by *all* the the sections (much like
cmd->override is saved/restored using the old_overrides stack variable
in most sections). It should also be set to NULL when parsing
(sub-)sections with no possible regex, unless we want to inherit
cmd->regex there.

But for instance some like:
   
  
# use /fake from 
Alias /real
  
   
won't work for Alias because  overwrites cmd->path with "*If".

Also, what about:
   
  
# which regex here?
  
   
Or:
   
  
# 's regex usable here?
  
   
?

Or third-party sections (unaware of cmd->regex) which contain
directives that depend on cmd->regex?

I'm not saying it's a bad idea but it needs more changes than this
commit, changes that spread all over the code base it seems (modules
can have their sections too), something like:
$ grep -r 'AP_INIT\w\+("<' server/ modules/ 2>/dev/null
server/core.c:AP_INIT_RAW_ARGS("
with Alias for now?


Regards;
Yann.

Re: svn commit: r1909137 - in /httpd/httpd/trunk: CHANGES modules/mappers/mod_alias.c

[Yann Ylavic]

On Mon, Apr 17, 2023 at 9:08 AM Ruediger Pluem  wrote:
>
> On 4/16/23 12:20 PM, Graham Leggett via dev wrote:
> > On 14 Apr 2023, at 17:18, Ruediger Pluem  > > wrote:
> >
> >> Would that break configs like
> >>
> >> 
> >>
> >> Alias /filesystemprefix/%{HTTP:X-example-header}
> >>
> >> 
> >>
> >> where the expression evaluation determines the complete filesystem path 
> >> without adding the remainder of the URL?
> >
> > It would, which alas might mean we can't backport this, which isn’t great.
> >
> >> I admit that the above looks like a strange setup and is probably a bad 
> >> example, but the parameter to Alias could be an arbitrary
> >> complex expression that evaluates to the final filesystem resource (like 
> >> AliasMatch). Wouldn't we need a kind of way to figure out
> >> if the users wants the remainder of the URI added or not even if we do not 
> >> use a regular expression in the surrounding Location
> >> block?
> >
> > LocationMatch is the correct way to do this - the config explicitly 
> > declares the exact URL to match, and the Alias gives the exact
> > unambiguous mapping.
> >
> > Having a prefix in the “Alias /foo /bar” case and then arbitrarily not 
> > having a prefix in the “Alias /bar” case sent me on a huge
> > wild goose chase - what made it worse was the client was a webdav client, 
> > so the behaviour just made no sense. I am seeing people
> > asking why they’re getting a 405 and not getting answers, so I think this 
> > is tripping up other people too.
>
> I completely agree that Alias in a Location should behave as it does after 
> r1909137 and the behavior change is not a problem in
> trunk, but I think due this behavior change it is not possible to backport it 
> as is.

If we change the behaviour in trunk such that:
  1. Alias /fake /real
is the same as:
  2. 
   Alias /real
 
I'd suggest that we treat the /real part the same too (currently /real
is a plain path in 1. and an expr in 2).
It seems that:
  3. 
   Alias /fake
 
could be a thing too (though /real would never be an expr here).

Likewise "AliasMatch /fake /real" and LocationMatch + "AliasMatch
/real" only should probably work the same, where /real could use
captures from the LocationMatch (just like
LocationMatch+ProxyPassMatch if I'm not mistaken).

I also find the names in alias_dir_conf quite confusing, the "alias"
corresponds to "real" in alias_entry used by alias_server_conf, while
"alias_fake" corresponds to "alias". ISTM that alias_dir_conf should
embed an alias_entry rather than duplicating all/most of its fields.
Maybe we could even get rid of alias_server_conf actually and handle
everything as perdir merging?

> The question is if we find an approach to
> make it backportable e.g. by adding some kind of 2.4.x only configuration 
> switch to get the behavior of r1909137.

Maybe "AliasPerDirRelative on" (default: "off", context: "server
config, virtual host, directory").
And then we change the default to "on" in trunk.


Regards;
Yann.

Re: ci vs PR approvals? (was: [apache/httpd] Fix a possible NULL pointer dereference in hook_uri2file (PR #355))

[Yann Ylavic]

On Wed, Apr 12, 2023 at 1:31 PM Eric Covener  wrote:
>
> On Wed, Apr 12, 2023 at 6:36 AM Yann Ylavic  wrote:
> >
> > On Wed, Apr 12, 2023 at 12:26 PM Yann Ylavic  wrote:
> > >
> > > On Wed, Apr 12, 2023 at 12:18 PM ylavic  wrote:
> > > >
> > > > @ylavic approved this pull request.
> > > >
> > > > Three approvals to get ci started?
> > >
> > > Nope.. It seems that gh actions don't run for PRs whatever we do.
> > > The docs[1] say that there should be an "Approve and run" button near
> > > the "workflow awaiting approval" text, but it's not the case for httpd
> > > mirror, while approving the whole PR looks inefficient..
> >
> > We (PMC/committers) once had the right to close any PRs, but that
> > seems to not be the case anymore either.
> > Something changed since
> > https://lists.apache.org/thread/g7bb70ymlmkzjlx1rpvq46dwz54qcpdb
> > probably.
> >
> > >
> > > Any more ideas? Help from infra needed?
> > >
> > > Regards;
> > > Yann.
> > >
> > > [1] 
> > > https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks
>
>
> We are chatting with Daniel about it on ASF slack.

Ah ok, I created https://issues.apache.org/jira/browse/INFRA-24457 FWIW..

Regards;
Yann.