Re: svn commit: r1919628 - in /httpd/httpd/trunk: changes-entries/bz69203.txt modules/proxy/mod_proxy.c modules/proxy/mod_proxy_fcgi.c
On Wed, Aug 7, 2024 at 11:44 AM Ruediger Pluem wrote:
>
> On 8/2/24 2:53 AM, yla...@apache.org wrote:
> >
> > --- httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c (original)
> > +++ httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c Fri Aug 2 00:53:53
> > 2024
>
> > @@ -93,20 +95,40 @@ static int proxy_fcgi_canon(request_rec
> > host = apr_pstrcat(r->pool, "[", host, "]", NULL);
> > }
> >
> > -/* We do not call ap_proxy_canonenc_ex() on the path here because the
> > CGI
> > - * environment variable SCRIPT_FILENAME based on it want the
> > decoded/local
> > - * path, don't let control characters pass still.
> > - *
> > - * XXX: should we encode based on dconf->backend_type though?
> > - */
> > -for (pc = path; *pc; pc++) {
> > -if (apr_iscntrl(*pc)) {
> > +from_handler = apr_table_get(r->notes, "proxy-sethandler") != NULL;
>
> Why do we need the from_handler variable?
Axed in r1921237.
Thanks;
Yann.
Re: svn commit: r1919547 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
On Thu, Aug 8, 2024 at 11:45 AM Ruediger Pluem wrote:
>
> On 7/27/24 3:54 PM, yla...@apache.org wrote:
> >
> > --- httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c (original)
> > +++ httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c Sat Jul 27 13:54:08
> > 2024
> > @@ -133,7 +133,6 @@ static int proxy_fcgi_canon(request_rec
> > /* It has to be on disk for this to work */
> > if (!strcasecmp(pathinfo_type, "full")) {
> > rconf->need_dirwalk = 1;
> > -ap_unescape_url_keep2f(path, 0);
> > }
> > else if (!strcasecmp(pathinfo_type, "first-dot")) {
> > char *split = ap_strchr(path, '.');
> > @@ -348,10 +347,11 @@ static apr_status_t send_environment(pro
> > fcgi_req_config_t *rconf = ap_get_module_config(r->request_config,
> > &proxy_fcgi_module);
> > fcgi_dirconf_t *dconf = ap_get_module_config(r->per_dir_config,
> > &proxy_fcgi_module);
> >
> > -if (rconf) {
> > - if (rconf->need_dirwalk) {
> > - ap_directory_walk(r);
> > - }
> > +if (rconf && rconf->need_dirwalk) {
> > +char *saved_filename = r->filename;
> > +r->filename = r->uri;
>
> Why not using the result of the below Strip proxy: prefixes for r->filename?
> In case of a Rewriterule I guess r->uri and
> r->filename could be fundamentally different path wise.
Did this in r1921238, as well as setting up the expected request
fields for ap_directory_walk().
Regards;
Yann.
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
On Wed, Sep 11, 2024 at 5:59 PM Yann Ylavic wrote: > > On Mon, Sep 9, 2024 at 12:20 PM Ruediger Pluem wrote: > > > > Looks like there is no further feedback on the below. > > @Yann: Care to commit > > https://bz.apache.org/bugzilla/attachment.cgi?id=39832 ? > > I would then commit my stuff below. > > Done (r1920570). > > I suppose we need a single backport proposal for both PRs 69235 and 69260? > If so maybe amend "changes-entries/pr69235.txt" with your follow up commit? Nevermind, just saw that PR 69260 is now a duplicate. > > Regards; > Yann.
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
On Mon, Sep 9, 2024 at 12:20 PM Ruediger Pluem wrote: > > Looks like there is no further feedback on the below. > @Yann: Care to commit https://bz.apache.org/bugzilla/attachment.cgi?id=39832 ? > I would then commit my stuff below. Done (r1920570). I suppose we need a single backport proposal for both PRs 69235 and 69260? If so maybe amend "changes-entries/pr69235.txt" with your follow up commit? Regards; Yann.
Re: svn commit: r1919325 - in /httpd/httpd/trunk: changes-entries/pr69197.txt modules/mappers/mod_rewrite.c
On Thu, Aug 8, 2024 at 10:19 AM Ruediger Pluem wrote:
Sorry for the late reply, I was quite busy at work$ back from vacations..
>
> On 7/17/24 10:50 PM, yla...@apache.org wrote:
> > --- httpd/httpd/trunk/modules/mappers/mod_rewrite.c (original)
> > +++ httpd/httpd/trunk/modules/mappers/mod_rewrite.c Wed Jul 17 20:50:12 2024
> >
> > +span = strcspn(input, EXPAND_SPECIALS "?");
> > +if (input[span] == '?') {
> > +/* this qmark is not from an expansion thus safe */
> > +*unsafe_qmark = 0;
> > +
> > +/* keep tracking only if interested in the last qmark */
> > +if (entry && (entry->flags & RULEFLAG_QSLAST)) {
> > +do {
> > +span++;
> > +span += strcspn(input + span, EXPAND_SPECIALS "?");
> > +} while (input[span] == '?');
>
> Why do we need this loop? Isn't that the same as
>
> span += strcspn(input + span, EXPAND_SPECIALS);
Yes correct, I first thought of returning the offset of the qmark but
a bool is enough, didn't change the loop though.
Changed both here and below to what you propose, in r1920566.
>
> > +}
> > +else {
> > +unsafe_qmark = NULL;
> > +span += strcspn(input + span, EXPAND_SPECIALS);
> > +}
> > +}
> > +}
> > +
> > /* check the remainder */
> > -if (*p && (span = strcspn(p, "\\$%")) > 0) {
>
> Can't we just do a break here in the !*p case?
We could, but we are not on the fast path here and I don't think one
more strcspn() on an empty string is going to hurt compared to the
expansion work. We could break out earlier in other places in this
loop too, not worth complicating the code imho.
Thanks for reviewing!
Regards;
Yann.
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 3:26 PM Eric Covener wrote:
>
> On Fri, Aug 2, 2024 at 9:10 AM Yann Ylavic wrote:
> >
> > On Fri, Aug 2, 2024 at 1:06 PM Eric Covener wrote:
> > >
> > > > Yeah, if not under DocumentRoot I don't see how ProxyPass could work,
> > > > but SetHandler should since it's following the whole request
> > > > processing to resolve the filesystem r->filename?
> > >
> > > In the examples I am seeing spot-checking google results, people who
> > > use ProxyPass + FPM hard-code the document root (or wherever the stuff
> > > is) in the 2nd parameter. This includes our own manual and the
> > > unofficial confluence wiki.
> >
> > Ah ok, I think we can do something like this:
> > if (rconf->need_dirwalk) {
> > const char *docroot = ap_document_root(r);
> > if (strncmp(docroot, r->filename, strlen(docroot)) != 0) {
> > r->proxyreq = PROXYREQ_NONE;
> > ap_core_translate(r);
> > }
> > ap_directory_walk(r);
> > }
> > ?
>
> I think users could be happily humming along with some other absolute
> filesystem path in the ProxyPass 2nd arg and would now see it
> docroot-prefixed.
> Are you trying to make the ProxyPass+FPM vars better because they will
> no longer be fixed up by apache_was_here side effects? I think it is
> probably bes to just retain/restore some of the historical bogus
> values if MAY_BE_FPM -- maybe doing them at the last moment where we'd
> do the above.
Maybe this:
if (rconf) {
if (!rconf->need_dirwalk) {
r->filename = rconf->filename;
}
else {
char *saved_uri = r->uri;
char *saved_path_info = r->path_info;
int i = 0;
/* Try to find the script without DocumentRoot than with */
do {
r->path_info = NULL;
r->filename = r->uri = rconf->filename;
if (i) {
r->proxyreq = PROXYREQ_NONE;
ap_core_translate(r);
r->proxyreq = PROXYREQ_REVERSE;
}
ap_directory_walk(r);
} while (r->finfo.filetype != APR_REG && ++i < 2);
/* If no actual script was found, fall back to the "proxy:"
* SCRIPT_FILENAME dealt with below or by php-fpm directly.
*/
if (i == 2) {
r->path_info = saved_path_info;
r->filename = proxy_filename;
}
/* Restore REQUEST_URI in any case */
r->uri = saved_uri;
}
}
?
Index: modules/proxy/mod_proxy_fcgi.c
===
--- modules/proxy/mod_proxy_fcgi.c (revision 1919628)
+++ modules/proxy/mod_proxy_fcgi.c (working copy)
@@ -29,6 +29,7 @@ typedef struct {
typedef struct {
int need_dirwalk;
+char *filename;
} fcgi_req_config_t;
/* We will assume FPM, but still differentiate */
@@ -141,8 +142,10 @@ static int proxy_fcgi_canon(request_rec *r, char *
rconf = apr_pcalloc(r->pool, sizeof(fcgi_req_config_t));
ap_set_module_config(r->request_config, &proxy_fcgi_module, rconf);
}
+rconf->filename = apr_pstrcat(r->pool, "/", url, NULL);
-if (NULL != (pathinfo_type = apr_table_get(r->subprocess_env, "proxy-fcgi-pathinfo"))) {
+pathinfo_type = apr_table_get(r->subprocess_env, "proxy-fcgi-pathinfo");
+if (pathinfo_type) {
/* It has to be on disk for this to work */
if (!strcasecmp(pathinfo_type, "full")) {
rconf->need_dirwalk = 1;
@@ -181,6 +184,9 @@ static int proxy_fcgi_canon(request_rec *r, char *
"set r->path_info to %s", r->path_info);
}
}
+else if (FCGI_MAY_BE_FPM(dconf) && !from_handler) {
+rconf->need_dirwalk = 1;
+}
return OK;
}
@@ -359,16 +365,43 @@ static apr_status_t send_environment(proxy_conn_re
int next_elem, starting_elem;
fcgi_req_config_t *rconf = ap_get_module_config(r->request_config, &proxy_fcgi_module);
fcgi_dirconf_t *dconf = ap_get_module_config(r->per_dir_config, &proxy_fcgi_module);
+char *proxy_filename = r->filename;
-if (rconf && rconf->need_dirwalk) {
-char *saved_filename = r->filename;
-r->filename = r->uri;
-ap_directory_walk(r);
-r->filename = saved_filename;
+if (rconf) {
+if (!rconf->need_dirwalk) {
+r->filename = rconf->filename;
+}
+else {
+char
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 3:10 PM Yann Ylavic wrote:
>
> On Fri, Aug 2, 2024 at 1:06 PM Eric Covener wrote:
> >
> > > Yeah, if not under DocumentRoot I don't see how ProxyPass could work,
> > > but SetHandler should since it's following the whole request
> > > processing to resolve the filesystem r->filename?
> >
> > In the examples I am seeing spot-checking google results, people who
> > use ProxyPass + FPM hard-code the document root (or wherever the stuff
> > is) in the 2nd parameter. This includes our own manual and the
> > unofficial confluence wiki.
>
> Ah ok, I think we can do something like this:
> if (rconf->need_dirwalk) {
> const char *docroot = ap_document_root(r);
> if (strncmp(docroot, r->filename, strlen(docroot)) != 0) {
> r->proxyreq = PROXYREQ_NONE;
> ap_core_translate(r);
> }
> ap_directory_walk(r);
> }
> ?
Full patch (v2).
Index: modules/proxy/mod_proxy_fcgi.c
===
--- modules/proxy/mod_proxy_fcgi.c (revision 1919628)
+++ modules/proxy/mod_proxy_fcgi.c (working copy)
@@ -29,6 +29,7 @@ typedef struct {
typedef struct {
int need_dirwalk;
+char *filename;
} fcgi_req_config_t;
/* We will assume FPM, but still differentiate */
@@ -141,8 +142,10 @@ static int proxy_fcgi_canon(request_rec *r, char *
rconf = apr_pcalloc(r->pool, sizeof(fcgi_req_config_t));
ap_set_module_config(r->request_config, &proxy_fcgi_module, rconf);
}
+rconf->filename = apr_pstrcat(r->pool, "/", url, NULL);
-if (NULL != (pathinfo_type = apr_table_get(r->subprocess_env, "proxy-fcgi-pathinfo"))) {
+pathinfo_type = apr_table_get(r->subprocess_env, "proxy-fcgi-pathinfo");
+if (pathinfo_type) {
/* It has to be on disk for this to work */
if (!strcasecmp(pathinfo_type, "full")) {
rconf->need_dirwalk = 1;
@@ -181,6 +184,9 @@ static int proxy_fcgi_canon(request_rec *r, char *
"set r->path_info to %s", r->path_info);
}
}
+else if (FCGI_MAY_BE_FPM(dconf) && !from_handler) {
+rconf->need_dirwalk = 1;
+}
return OK;
}
@@ -359,18 +365,26 @@ static apr_status_t send_environment(proxy_conn_re
int next_elem, starting_elem;
fcgi_req_config_t *rconf = ap_get_module_config(r->request_config, &proxy_fcgi_module);
fcgi_dirconf_t *dconf = ap_get_module_config(r->per_dir_config, &proxy_fcgi_module);
+char *saved_filename = r->filename;
-if (rconf && rconf->need_dirwalk) {
-char *saved_filename = r->filename;
-r->filename = r->uri;
-ap_directory_walk(r);
-r->filename = saved_filename;
+if (rconf) {
+r->filename = rconf->filename;
+
+/* To fix/set r->filename/path_info, do now what ProxyPass skipped */
+if (rconf->need_dirwalk) {
+const char *docroot = ap_document_root(r);
+if (strncmp(r->filename, docroot, strlen(docroot)) != 0) {
+r->proxyreq = PROXYREQ_NONE;
+ap_core_translate(r);
+}
+ap_directory_walk(r);
+}
}
-
-/* Strip proxy: prefixes */
-if (r->filename) {
+else if (r->filename) {
char *newfname = NULL;
+/* Strip proxy: prefixes */
+
if (!strncmp(r->filename, "proxy:balancer://", 17)) {
newfname = apr_pstrdup(r->pool, r->filename+17);
}
@@ -401,6 +415,9 @@ static apr_status_t send_environment(proxy_conn_re
ap_add_common_vars(r);
ap_add_cgi_vars(r);
+r->filename = saved_filename;
+r->proxyreq = PROXYREQ_REVERSE;
+
/* XXX are there any FastCGI specific env vars we need to send? */
/* Give admins final option to fine-tune env vars */
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 1:06 PM Eric Covener wrote:
>
> > Yeah, if not under DocumentRoot I don't see how ProxyPass could work,
> > but SetHandler should since it's following the whole request
> > processing to resolve the filesystem r->filename?
>
> In the examples I am seeing spot-checking google results, people who
> use ProxyPass + FPM hard-code the document root (or wherever the stuff
> is) in the 2nd parameter. This includes our own manual and the
> unofficial confluence wiki.
Ah ok, I think we can do something like this:
if (rconf->need_dirwalk) {
const char *docroot = ap_document_root(r);
if (strncmp(docroot, r->filename, strlen(docroot)) != 0) {
r->proxyreq = PROXYREQ_NONE;
ap_core_translate(r);
}
ap_directory_walk(r);
}
?
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 11:33 AM Yann Ylavic wrote: > > On Fri, Aug 2, 2024 at 6:02 AM Eric Covener wrote: > > > > On Thu, Aug 1, 2024 at 9:22 PM Yann Ylavic wrote: > > > > > > > > For this how about this attached patch? > > > > With it I can get the correct env vars (I think), and since we'd not > > > > send a "proxy:" SCRIPT_FILENAME anymore, php-fpm would not flag > > > > "apache_was_there" and work straight with the raw vars? I'm probably > > > > having a sweet dream :) > > > > Just in case.. > > > > > > PS: the script needs to exist in the DOCUMENT_ROOT for this to work, > > > but that's how php-fpm works I suppose. > > > > This is somewhat over my head (despite writing and forgetting some of > > those fcgi kludges) but tell me if I am close. > > > > - proxy-fcgi-pathinfo was only meant to be used with ProxyPass, not > > SetHandler, but this is not explicit in the code. > > Yeah, I didn't change that part. > > > - The current code to generate SCRIPT_FILENAME (supposed to be a > > filename) and PATH_INFO probably doesn't work for the ProxyPass path > > where it's actually needed. This is because r->filename won't even be > > docroot-prefixed if mod_proxy handles translate_name early. > > Correct. > > > - Your addition gets it to at least work for stuff that is under the > > DocumentRoot, as the directory walk will now split into r->filename > > and r->path_info based on what was on disk > > Yeah, if not under DocumentRoot I don't see how ProxyPass could work, > but SetHandler should since it's following the whole request > processing to resolve the filesystem r->filename? > > > - Your addition also causes all ProxyPass configs that didn't tell us > > non-FPM backend type explicitly to act like proxy-fcgi-pathinfo=full > > (dirwalk). > > Yeah, not really thought deeply but I don't get what ProxyPass without > proxy-fcgi-pathinfo is meant for.. > > > > > Does the latest patch still leave us with proxy:fcgi:// in the env for > > FPM or Unknown or is it part of the dream scenario? > > No more proxy:fcgi://, that's the bet :) And we probably can stop blocking '?' then.
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 6:02 AM Eric Covener wrote: > > On Thu, Aug 1, 2024 at 9:22 PM Yann Ylavic wrote: > > > > > > For this how about this attached patch? > > > With it I can get the correct env vars (I think), and since we'd not > > > send a "proxy:" SCRIPT_FILENAME anymore, php-fpm would not flag > > > "apache_was_there" and work straight with the raw vars? I'm probably > > > having a sweet dream :) > > > Just in case.. > > > > PS: the script needs to exist in the DOCUMENT_ROOT for this to work, > > but that's how php-fpm works I suppose. > > This is somewhat over my head (despite writing and forgetting some of > those fcgi kludges) but tell me if I am close. > > - proxy-fcgi-pathinfo was only meant to be used with ProxyPass, not > SetHandler, but this is not explicit in the code. Yeah, I didn't change that part. > - The current code to generate SCRIPT_FILENAME (supposed to be a > filename) and PATH_INFO probably doesn't work for the ProxyPass path > where it's actually needed. This is because r->filename won't even be > docroot-prefixed if mod_proxy handles translate_name early. Correct. > - Your addition gets it to at least work for stuff that is under the > DocumentRoot, as the directory walk will now split into r->filename > and r->path_info based on what was on disk Yeah, if not under DocumentRoot I don't see how ProxyPass could work, but SetHandler should since it's following the whole request processing to resolve the filesystem r->filename? > - Your addition also causes all ProxyPass configs that didn't tell us > non-FPM backend type explicitly to act like proxy-fcgi-pathinfo=full > (dirwalk). Yeah, not really thought deeply but I don't get what ProxyPass without proxy-fcgi-pathinfo is meant for.. > > Does the latest patch still leave us with proxy:fcgi:// in the env for > FPM or Unknown or is it part of the dream scenario? No more proxy:fcgi://, that's the bet :)
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 5:22 AM Eric Covener wrote: > > On Thu, Aug 1, 2024 at 9:12 PM Yann Ylavic wrote: > > > > On Fri, Aug 2, 2024 at 12:18 AM Yann Ylavic wrote: > > > > > > So we probably should keep encoding r->filename with ProxyPass, and > > > come back to my previous patch which skipped it only for SetHandler? > > > Possibly FCGI_MAY_BE_FPM() only too because for "ProxyFCGIBackendType > > > GENERIC" we don't send the "proxy:scheme://host" part and > > > SCRIPT_NAME/FILENAME are supposed to be the real decoded paths? > > > > So I did this in r1919629. > > Just to level-set, IIUC: > > - 2.4.59 encoded SCRIPT_FILENAME for ProxyPass but not SetHandler > didn't and this same fragile stuff in php-fpm is CVE-2024-38473 > - 2.4.60 started encoding these, for CVE-2024-38473 + general safety > - PR69203 reports a few different symptoms where SCRIPT_FILENAME has > URL-encoded file names that are actually spaces, utf-8, etc. > - https://github.com/apache/httpd/pull/470 undoes the re-encoding > introduced in 2.4.60 but makes sure controls and '?' aren't in the > SCRIPT_FILENAME (for CVE-2024-38473 and related funny business). Yeah correct, just '?' is not forbidden when !FCGI_MAY_BE_FPM because in this case SCRIPT_FILENAME is not the "proxy:" URL (https://github.com/apache/httpd/commit/cab0bfbb2645bb8f689535e5e2834e2dbc23f5a5 applies). > > So this sounds reasonable to me without upsetting the fragile link > between php-fpm and proxy_fcgi. > > > > But it's going to be an endless issue if we can't fix or align > > > ProxyPass and SetHandler because of workarounds there, we have to > > > remain bug compatible.. > > I wonder does ProxyPass just not work with php-fpm and these > spaces/utf-8 scenarios? Possibly because php-fpm seems to do some decoding itself when it thinks apache_was_there with ProxyPass, but I didn't test nor fully follow the code. > > > For this how about this attached patch? > > With it I can get the correct env vars (I think), and since we'd not > > send a "proxy:" SCRIPT_FILENAME anymore, php-fpm would not flag > > "apache_was_there" and work straight with the raw vars? I'm probably > > having a sweet dream :) > > It sounds the most rational, but it also sounds very similar to the > breakage the t/modules/proxy_fcgi.t points to here: > https://github.com/apache/httpd/commit/cab0bfbb2645bb8f689535e5e2834e2dbc23f5a5 Not really the same because we don't just skip the "proxy:scheme://host" for SCRIPT_FILENAME, we really "resolve" the rest through ap_core_translate() (which adds DOCUMENT_ROOT) and ap_directory_walk() (which splits, provided the script exists). Eg. by requesting "/sympa/index.php/blah" I get: (gdb) dump_table r->subprocess_env [11] 'DOCUMENT_ROOT'='/var/www/htdocs' [0x556c4e98] [16] 'SCRIPT_FILENAME'='/var/www/htdocs/sympa/index.php' [0x7fffe8007378] [22] 'REQUEST_URI'='/sympa/index.php/blah' [0x7fffe8007cc8] [23] 'SCRIPT_NAME'='/sympa/index.php' [0x7fffe8007ce0] [24] 'PATH_INFO'='/blah' [0x7fffe80067ef] [25] 'PATH_TRANSLATED'='/var/www/htdocs/blah' [0x7fffe8007d10] > > As the thread talks about, maybe we can get them to accept some other > way to signal Apache. Or with the real SCRIPT_FILENAME get them to not think apache_was_there..
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 3:12 AM Yann Ylavic wrote: > > On Fri, Aug 2, 2024 at 12:18 AM Yann Ylavic wrote: > > > > So we probably should keep encoding r->filename with ProxyPass, and > > come back to my previous patch which skipped it only for SetHandler? > > Possibly FCGI_MAY_BE_FPM() only too because for "ProxyFCGIBackendType > > GENERIC" we don't send the "proxy:scheme://host" part and > > SCRIPT_NAME/FILENAME are supposed to be the real decoded paths? > > So I did this in r1919629. > > > > > But it's going to be an endless issue if we can't fix or align > > ProxyPass and SetHandler because of workarounds there, we have to > > remain bug compatible.. > > For this how about this attached patch? > With it I can get the correct env vars (I think), and since we'd not > send a "proxy:" SCRIPT_FILENAME anymore, php-fpm would not flag > "apache_was_there" and work straight with the raw vars? I'm probably > having a sweet dream :) > Just in case.. PS: the script needs to exist in the DOCUMENT_ROOT for this to work, but that's how php-fpm works I suppose.
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 12:18 AM Yann Ylavic wrote:
>
> So we probably should keep encoding r->filename with ProxyPass, and
> come back to my previous patch which skipped it only for SetHandler?
> Possibly FCGI_MAY_BE_FPM() only too because for "ProxyFCGIBackendType
> GENERIC" we don't send the "proxy:scheme://host" part and
> SCRIPT_NAME/FILENAME are supposed to be the real decoded paths?
So I did this in r1919629.
>
> But it's going to be an endless issue if we can't fix or align
> ProxyPass and SetHandler because of workarounds there, we have to
> remain bug compatible..
For this how about this attached patch?
With it I can get the correct env vars (I think), and since we'd not
send a "proxy:" SCRIPT_FILENAME anymore, php-fpm would not flag
"apache_was_there" and work straight with the raw vars? I'm probably
having a sweet dream :)
Just in case..
Index: modules/proxy/mod_proxy_fcgi.c
===
--- modules/proxy/mod_proxy_fcgi.c (revision 1919623)
+++ modules/proxy/mod_proxy_fcgi.c (working copy)
@@ -29,6 +29,7 @@ typedef struct {
typedef struct {
int need_dirwalk;
+char *filename;
} fcgi_req_config_t;
/* We will assume FPM, but still differentiate */
@@ -119,8 +142,10 @@ static int proxy_fcgi_canon(request_rec *r, char *
rconf = apr_pcalloc(r->pool, sizeof(fcgi_req_config_t));
ap_set_module_config(r->request_config, &proxy_fcgi_module, rconf);
}
+rconf->filename = apr_pstrcat(r->pool, "/", url, NULL);
-if (NULL != (pathinfo_type = apr_table_get(r->subprocess_env, "proxy-fcgi-pathinfo"))) {
+pathinfo_type = apr_table_get(r->subprocess_env, "proxy-fcgi-pathinfo");
+if (pathinfo_type) {
/* It has to be on disk for this to work */
if (!strcasecmp(pathinfo_type, "full")) {
rconf->need_dirwalk = 1;
@@ -159,6 +184,9 @@ static int proxy_fcgi_canon(request_rec *r, char *
"set r->path_info to %s", r->path_info);
}
}
+else if (FCGI_MAY_BE_FPM(dconf) && !from_handler) {
+rconf->need_dirwalk = 1;
+}
return OK;
}
@@ -337,12 +365,17 @@ static apr_status_t send_environment(proxy_conn_re
int next_elem, starting_elem;
fcgi_req_config_t *rconf = ap_get_module_config(r->request_config, &proxy_fcgi_module);
fcgi_dirconf_t *dconf = ap_get_module_config(r->per_dir_config, &proxy_fcgi_module);
+char *saved_filename = r->filename;
-if (rconf && rconf->need_dirwalk) {
-char *saved_filename = r->filename;
-r->filename = r->uri;
-ap_directory_walk(r);
-r->filename = saved_filename;
+if (rconf) {
+if (rconf->filename) {
+r->filename = rconf->filename;
+}
+if (rconf->need_dirwalk) {
+r->proxyreq = PROXYREQ_NONE;
+ap_core_translate(r);
+ap_directory_walk(r);
+}
}
/* Strip proxy: prefixes */
@@ -379,6 +412,9 @@ static apr_status_t send_environment(proxy_conn_re
ap_add_common_vars(r);
ap_add_cgi_vars(r);
+r->filename = saved_filename;
+r->proxyreq = PROXYREQ_REVERSE;
+
/* XXX are there any FastCGI specific env vars we need to send? */
/* Give admins final option to fine-tune env vars */
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Thu, Aug 1, 2024 at 9:53 PM Eric Covener wrote: > > On Thu, Aug 1, 2024 at 2:47 PM Yann Ylavic wrote: > > > > On Thu, Aug 1, 2024 at 7:57 PM Eric Covener wrote: > > > > > > On Thu, Aug 1, 2024 at 1:37 PM Yann Ylavic wrote: > > > > > > > > On Thu, Aug 1, 2024 at 5:51 PM Eric Covener wrote: > > > > > > > > > > But does it leave the splitting problem with decoded %3F? > > > > > > > > Yeah but I'm not sure that it's _our_ problem, a "proxy:" r->filename > > > > does never contain the query-string in the first place, so any '?' in > > > > there (hence in SCRIPT_FILENAME) is part of the actual file path > > > > (which we'd encode for proxying any other scheme than fcgi). And the > > > > '?' will be in SCRIPT_NAME/PATH_INFO/etc too. If the scripts want the > > > > decoded uri-path they have to be consistent and consider that > > > > SCRIPT_FILENAME is nothing else than a path (no query-string, which is > > > > in ... QUERY_STRING). > > > > > > Just to recap, FPM doesn't want to find the query it in > > > SCRIPT_FILENAME, it wants to toss it away because it used to > > > accidentally end up in there (via mod_rewrite?) But this is where the > > > mismatch between what we've walked/mapped/authorized and what will be > > > executed is. > > > > If FPM wants a decoded SCRIPT_FILENAME but no '?' character? > > Decoding a path with %3f will inevitably give '?', even though it's > > still the path, why would FPM decode it as a URL and find a query > > string in there? > > I think as a workaround for what we can (or used to?) send: > https://github.com/php/php-src/blob/master/sapi/fpm/fpm/fpm_main.c#L1043 > It also means an actual file with a literal question mark cannot be > run through php-fpm. OK, so php-fpm will parse the given "proxy:" SCRIPT_FILENAME as an URL, trimming the query-string, and determine that "apache_was_there". So there's never been a way to pass a decoded path with '?' to php-fpm using SetHandler (which before the latest changes never re-encoded the filename). So we probably can just forbid '?' like controls. But reading that code (wow..), php-fpm is also able to differentiate ProxyPass vs Sethandler and do things like: https://github.com/php/php-src/blob/master/sapi/fpm/fpm/fpm_main.c#L1172 which %-decode the PATH_INFO, supposedly extracted from SCRIPT_FILENAME. So we probably should keep encoding r->filename with ProxyPass, and come back to my previous patch which skipped it only for SetHandler? Possibly FCGI_MAY_BE_FPM() only too because for "ProxyFCGIBackendType GENERIC" we don't send the "proxy:scheme://host" part and SCRIPT_NAME/FILENAME are supposed to be the real decoded paths? But it's going to be an endless issue if we can't fix or align ProxyPass and SetHandler because of workarounds there, we have to remain bug compatible.. At some point we'll have to coordinate with them to remove that "apache_was_there"..
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Thu, Aug 1, 2024 at 7:57 PM Eric Covener wrote: > > On Thu, Aug 1, 2024 at 1:37 PM Yann Ylavic wrote: > > > > On Thu, Aug 1, 2024 at 5:51 PM Eric Covener wrote: > > > > > > But does it leave the splitting problem with decoded %3F? > > > > Yeah but I'm not sure that it's _our_ problem, a "proxy:" r->filename > > does never contain the query-string in the first place, so any '?' in > > there (hence in SCRIPT_FILENAME) is part of the actual file path > > (which we'd encode for proxying any other scheme than fcgi). And the > > '?' will be in SCRIPT_NAME/PATH_INFO/etc too. If the scripts want the > > decoded uri-path they have to be consistent and consider that > > SCRIPT_FILENAME is nothing else than a path (no query-string, which is > > in ... QUERY_STRING). > > Just to recap, FPM doesn't want to find the query it in > SCRIPT_FILENAME, it wants to toss it away because it used to > accidentally end up in there (via mod_rewrite?) But this is where the > mismatch between what we've walked/mapped/authorized and what will be > executed is. If FPM wants a decoded SCRIPT_FILENAME but no '?' character? Decoding a path with %3f will inevitably give '?', even though it's still the path, why would FPM decode it as a URL and find a query string in there?
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Thu, Aug 1, 2024 at 5:51 PM Eric Covener wrote: > > But does it leave the splitting problem with decoded %3F? Yeah but I'm not sure that it's _our_ problem, a "proxy:" r->filename does never contain the query-string in the first place, so any '?' in there (hence in SCRIPT_FILENAME) is part of the actual file path (which we'd encode for proxying any other scheme than fcgi). And the '?' will be in SCRIPT_NAME/PATH_INFO/etc too. If the scripts want the decoded uri-path they have to be consistent and consider that SCRIPT_FILENAME is nothing else than a path (no query-string, which is in ... QUERY_STRING). There is the "proxy-noquery" ->notes which seems to influence whether SCRIPT_FILENAME should include the query-string or not, but that's probably for the "proxy-nocanon" case which uses r->unparsed_uri. And at that "nocanon" point I also think that the users have to keep the pieces.. There is also send_environment() which will strip any query-string (supposedly) from r->filename if it matches r->args (because "Query string in environment only", meaning QUERY_STRING I guess). That's guarded by !FCGI_MAY_BE_FPM though (i.e. never), because FPM seems to have some expectations/workarounds around SCRIPT_FILENAME to address ... who knows. So in this whack-a-mole game I'm afraid we'll have to have an opt-out for every single thing we are supposed to fix :/
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Wed, Jul 31, 2024 at 7:34 PM Eric Covener wrote: > > On Tue, Jul 23, 2024 at 5:35 AM Yann Ylavic wrote: > > > > On Wed, Jul 17, 2024 at 6:22 PM wrote: > > > > > > https://bz.apache.org/bugzilla/show_bug.cgi?id=69203 > > > > > > --- Comment #6 from Yann Ylavic --- > > > Created attachment 39817 > > > --> https://bz.apache.org/bugzilla/attachment.cgi?id=39817&action=edit > > > Proxy FCGI nocanon from SetHandler > > > > I'm not sure how we should proceed here, this patch would avoid > > encoding SCRIPT_FILENAME for "SetHandler proxy:fcgi:..." but not > > "ProxyPass fcgi:..." which looks awkward. SetHandler is the > > recommended/most used way to proxy fcgi which is probably why people > > start noticing now that we do the same as with ProxyPass, but I don't > > see why they should be different in this regard.. > > > > If SCRIPT_FILENAME should be decoded (per the spec) I wonder if > > proxy_fcgi_canon() should not encode at all, or maybe only when > > FCGI_MAY_BE_FPM() (so to have an opt-out)? > > > And like in the above patch forbid controls still but not space/tab, WDYT? > > Based on the bug and the japanese path, maybe set the bar even lower > and just ratchet it all the way back to the character we know is > problematic? So I went with r1919620 which makes the most sense for me. This will forbid control characters (hence allow for space only, not tab anymore) but mainly will *not* re-encode r->filename, irrespective of ProxyPass vs SetHandler (I think that if users want the encoding we should have an opt with ProxyFCGIBackendType, ProxyPass vs SetHandler makes no sense here..). WDYT? Regards; Yann.
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
On Wed, Jul 31, 2024 at 6:57 PM wrote: > > https://bz.apache.org/bugzilla/show_bug.cgi?id=69235 > > --- Comment #2 from Yann Ylavic --- > Created attachment 39832 > --> https://bz.apache.org/bugzilla/attachment.cgi?id=39832&action=edit > mod_proxy fixup after mod_rewrite's What could be the issue with mod_proxy fixing up / canonicalizing all mod_rewrite [P] URLs (including perdir)? Looks sensible to me..
Re: svn commit: r1919534 - in /httpd/test/framework/trunk/t: conf/extra.conf.in modules/rewrite.t
On Fri, Jul 26, 2024 at 6:10 PM Joe Orton wrote: > > On Fri, Jul 26, 2024 at 03:52:40PM -, yla...@apache.org wrote: > > Author: ylavic > > Date: Fri Jul 26 15:52:40 2024 > > New Revision: 1919534 > > > > URL: http://svn.apache.org/viewvc?rev=1919534&view=rev > > Log: > > Is that what's wrong in ci? > > You mean for 2.4.x? Trunk is (was) passing, though > https://github.com/apache/httpd/actions/runs/10113115333 looks like some > transient/random failure. Ah ok. > > 2.4.x has been failing since > https://github.com/apache/httpd-tests/commit/f1044d936f0a842bf9f56af54546d4d6001b7242 > and I had assumed it would start passing with the backport of r1919325. Right, I thought that the new tests were disabled for >2.4.62 but missed that 2.4.x was AP_SERVER_PATCHLEVEL_NUMBER=63 already.. r1919325 backported now and all is good it seems. Thanks!
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Wed, Jul 17, 2024 at 6:22 PM wrote: > > https://bz.apache.org/bugzilla/show_bug.cgi?id=69203 > > --- Comment #6 from Yann Ylavic --- > Created attachment 39817 > --> https://bz.apache.org/bugzilla/attachment.cgi?id=39817&action=edit > Proxy FCGI nocanon from SetHandler I'm not sure how we should proceed here, this patch would avoid encoding SCRIPT_FILENAME for "SetHandler proxy:fcgi:..." but not "ProxyPass fcgi:..." which looks awkward. SetHandler is the recommended/most used way to proxy fcgi which is probably why people start noticing now that we do the same as with ProxyPass, but I don't see why they should be different in this regard.. If SCRIPT_FILENAME should be decoded (per the spec) I wonder if proxy_fcgi_canon() should not encode at all, or maybe only when FCGI_MAY_BE_FPM() (so to have an opt-out)? And like in the above patch forbid controls still but not space/tab, WDYT? Regards; Yann.
Re: [VOTE] Release httpd-2.4.62-rc1 as httpd-2.4.62
On Mon, Jul 15, 2024 at 2:14 PM Eric Covener wrote: > > I would like to call a VOTE over the next few days to release > this candidate tarball httpd-2.4.62-rc1 as 2.4.62: [X] +1: It's not just good, it's good enough! Tested on Debian stable and testing/sid, all passes. Sigs/checksums OK. Thanks again Eric!
Re: svn commit: r1919142 - /httpd/httpd/branches/2.4.x/STATUS
On Thu, Jul 11, 2024 at 4:19 PM wrote: > --- httpd/httpd/branches/2.4.x/STATUS (original) > +++ httpd/httpd/branches/2.4.x/STATUS Thu Jul 11 14:19:08 2024 > @@ -166,6 +166,34 @@ PATCHES ACCEPTED TO BACKPORT FROM TRUNK: > + > + *) mod_http2: sync with github module. Add async handling > + on newer httpd for blocked connections, fix yield handling. > + Trunk version of patch: > +https://svn.apache.org/r1918003 > +https://svn.apache.org/r1918022 > +https://svn.apache.org/r1918035 > +https://svn.apache.org/r1918078 > +https://svn.apache.org/r1918098 > +https://svn.apache.org/r1918099 > +https://svn.apache.org/r1918257 > +https://svn.apache.org/r1918482 > +https://svn.apache.org/r1918483 > +https://svn.apache.org/r1918491 > +https://svn.apache.org/r1919141 > +2.4.x version of patch: > + > https://patch-diff.githubusercontent.com/raw/apache/httpd/pull/449.diff > +PR: https://github.com/apache/httpd/pull/449 > ++1: ylavic, icing, gbechis > +rpluem says: PR currently has merge conflicts that need to be resolved > +ylavic says: PR rebased and r1919141 added (colspan adjustment in > mod_status) > + Tiny fix which needs vote reset? This possibly can wait for the next release too, maybe Eric wanted to have a look IIRC..
Re: svn commit: r1919139 - /httpd/httpd/branches/2.4.x/STATUS
On Thu, Jul 11, 2024 at 3:59 PM wrote: > > Author: ylavic > Date: Thu Jul 11 13:58:47 2024 > New Revision: 1919139 > > URL: http://svn.apache.org/viewvc?rev=1919139&view=rev > Log: > Backported. > > - *) mod_proxy: Backport for PRs 69160 and 69168 > - Trunk version of patch: > -https://svn.apache.org/r1919015 > -https://svn.apache.org/r1919019 > -https://svn.apache.org/r1919022 > -https://svn.apache.org/r1919023 > -https://svn.apache.org/r1919024 > -2.4.x version of patch: > - > https://patch-diff.githubusercontent.com/raw/apache/httpd/pull/460.diff > -PR: https://github.com/apache/httpd/pull/460 > -+1: rpluem, covener, ylavic > -covener: needs manual MMN bump after backport The added functions are in "proxy_util.h" which is a private header, do we need a MMN bump? Regards; Yann.
Re: mod_md in 2.4.61 fails to compile with openssl < 1.1.1
On Fri, Jul 5, 2024 at 5:59 PM Yann Ylavic wrote: > > On Fri, Jul 5, 2024 at 5:08 PM Ruediger Pluem wrote: > > > > On 7/5/24 4:09 PM, Stefan Eissing via dev wrote: > > > > > > The patches look good to me. I have not tested them as I have no old > > > openssl lying around, but I trust in your build tests. > > > > Rebuild 2.4.61 with both patches from Yann on RedHat 7 - 9. All good now, > > even on 7 with openssl 1.0.2 (means it compiles and no > > more implicit declaration warnings). > > @Yann: Care to commit the patches? > > Will do on the weekend if/when possible, feel free to beat me to it if > you can ;) r1919026. > > > Regards; > Yann.
Re: mod_md in 2.4.61 fails to compile with openssl < 1.1.1
On Fri, Jul 5, 2024 at 5:08 PM Ruediger Pluem wrote: > > On 7/5/24 4:09 PM, Stefan Eissing via dev wrote: > > > > > >> Am 05.07.2024 um 15:44 schrieb Ruediger Pluem : > >> > >> > >> > >> On 7/5/24 3:40 PM, Yann Ylavic wrote: > >>> On Fri, Jul 5, 2024 at 3:35 PM Yann Ylavic wrote: > >>>> > >>>> On Fri, Jul 5, 2024 at 3:05 PM Ruediger Pluem wrote: > >>>>> > >>>>>>>>> md_crypt.c: In function 'md_cert_get_ct_scts': > >>>>>>>>> md_crypt.c:2071:5: error: unknown type name 'SCT' > >>>>>>>>>SCT *sct_handle; > >>>>> > >>>>> This one is caused by r1918195 in >= 2.4.60. Before r1918195 > >>>>> OPENSSL_NO_CT was defined when openssl was < 1.1.1. Now it is not any > >>>>> longer and hence md_cert_get_ct_scts gets a real function body as > >>>>> > >>>>> #ifndef OPENSSL_NO_CT > >>>>> > >>>>> (line 2068) is now true. Hence we error out on the non presence of the > >>>>> SCT struct (line 2071). > >>>> > >>>> Maybe something like the attached patch for this one too (which could > >>>> avoid configure tricks for both..). > >>> > >>> Or rather this one. > >>> > >> > >> > >> Looks good to me. Waiting for Stefan's feedback. > > > > The patches look good to me. I have not tested them as I have no old > > openssl lying around, but I trust in your build tests. > > Rebuild 2.4.61 with both patches from Yann on RedHat 7 - 9. All good now, > even on 7 with openssl 1.0.2 (means it compiles and no > more implicit declaration warnings). > @Yann: Care to commit the patches? Will do on the weekend if/when possible, feel free to beat me to it if you can ;) Regards; Yann.
Re: mod_md in 2.4.61 fails to compile with openssl < 1.1.1
On Fri, Jul 5, 2024 at 3:35 PM Yann Ylavic wrote:
>
> On Fri, Jul 5, 2024 at 3:05 PM Ruediger Pluem wrote:
> >
> > >>>> md_crypt.c: In function 'md_cert_get_ct_scts':
> > >>>> md_crypt.c:2071:5: error: unknown type name 'SCT'
> > >>>> SCT *sct_handle;
> >
> > This one is caused by r1918195 in >= 2.4.60. Before r1918195 OPENSSL_NO_CT
> > was defined when openssl was < 1.1.1. Now it is not any
> > longer and hence md_cert_get_ct_scts gets a real function body as
> >
> > #ifndef OPENSSL_NO_CT
> >
> > (line 2068) is now true. Hence we error out on the non presence of the SCT
> > struct (line 2071).
>
> Maybe something like the attached patch for this one too (which could
> avoid configure tricks for both..).
Or rather this one.
Index: modules/md/md_crypt.c
===
--- modules/md/md_crypt.c (revision 1918881)
+++ modules/md/md_crypt.c (working copy)
@@ -63,7 +63,11 @@
|| LIBRESSL_VERSION_NUMBER >= 0x305fL)
/* Missing from LibreSSL < 3.5.0 and only available since OpenSSL v1.1.x */
#include
+#define MD_HAVE_CT 1
#endif
+#ifndef MD_HAVE_CT
+#define MD_HAVE_CT 0
+#endif
static int initialized;
@@ -2037,11 +2061,10 @@ out:
return rv;
}
+#if MD_HAVE_CT
#define MD_OID_CT_SCTS_NUM "1.3.6.1.4.1.11129.2.4.2"
#define MD_OID_CT_SCTS_SNAME"CT-SCTs"
#define MD_OID_CT_SCTS_LNAME"CT Certificate SCTs"
-
-#ifndef OPENSSL_NO_CT
static int get_ct_scts_nid(void)
{
int nid = OBJ_txt2nid(MD_OID_CT_SCTS_NUM);
@@ -2065,7 +2088,7 @@ const char *md_nid_get_lname(int nid)
apr_status_t md_cert_get_ct_scts(apr_array_header_t *scts, apr_pool_t *p, const md_cert_t *cert)
{
-#ifndef OPENSSL_NO_CT
+#if MD_HAVE_CT
int nid, i, idx, critical;
STACK_OF(SCT) *sct_list;
SCT *sct_handle;
Re: mod_md in 2.4.61 fails to compile with openssl < 1.1.1
On Fri, Jul 5, 2024 at 3:05 PM Ruediger Pluem wrote:
>
> md_crypt.c: In function 'md_cert_get_ct_scts':
> md_crypt.c:2071:5: error: unknown type name 'SCT'
> SCT *sct_handle;
>
> This one is caused by r1918195 in >= 2.4.60. Before r1918195 OPENSSL_NO_CT
> was defined when openssl was < 1.1.1. Now it is not any
> longer and hence md_cert_get_ct_scts gets a real function body as
>
> #ifndef OPENSSL_NO_CT
>
> (line 2068) is now true. Hence we error out on the non presence of the SCT
> struct (line 2071).
Maybe something like the attached patch for this one too (which could
avoid configure tricks for both..).
Index: modules/md/md_crypt.c
===
--- modules/md/md_crypt.c (revision 1918881)
+++ modules/md/md_crypt.c (working copy)
@@ -57,12 +57,14 @@
#include
#endif
-#if !defined(OPENSSL_NO_CT) \
-&& OPENSSL_VERSION_NUMBER >= 0x1010L \
-&& (!defined(LIBRESSL_VERSION_NUMBER) \
-|| LIBRESSL_VERSION_NUMBER >= 0x305fL)
+#if defined(OPENSSL_NO_CT)
+#define MD_NO_CT
+#elif (OPENSSL_VERSION_NUMBER >= 0x1010L \
+ && (!defined(LIBRESSL_VERSION_NUMBER) \
+ || LIBRESSL_VERSION_NUMBER >= 0x305fL))
/* Missing from LibreSSL < 3.5.0 and only available since OpenSSL v1.1.x */
#include
+#undef MD_NO_CT
#endif
static int initialized;
@@ -2037,11 +2059,10 @@ out:
return rv;
}
+#ifndef MD_NO_CT
#define MD_OID_CT_SCTS_NUM "1.3.6.1.4.1.11129.2.4.2"
#define MD_OID_CT_SCTS_SNAME"CT-SCTs"
#define MD_OID_CT_SCTS_LNAME"CT Certificate SCTs"
-
-#ifndef OPENSSL_NO_CT
static int get_ct_scts_nid(void)
{
int nid = OBJ_txt2nid(MD_OID_CT_SCTS_NUM);
@@ -2065,7 +2086,7 @@ const char *md_nid_get_lname(int nid)
apr_status_t md_cert_get_ct_scts(apr_array_header_t *scts, apr_pool_t *p, const md_cert_t *cert)
{
-#ifndef OPENSSL_NO_CT
+#ifndef MD_NO_CT
int nid, i, idx, critical;
STACK_OF(SCT) *sct_list;
SCT *sct_handle;
Re: mod_md in 2.4.61 fails to compile with openssl < 1.1.1
On Fri, Jul 5, 2024 at 3:16 PM Yann Ylavic wrote:
>
> On Fri, Jul 5, 2024 at 3:05 PM Ruediger Pluem wrote:
> >
> >
> >
> > On 7/5/24 2:14 PM, Ruediger Pluem wrote:
> > >
> > >
> > > On 7/5/24 2:11 PM, Ruediger Pluem wrote:
> > >>
> > >>
> > >> On 7/5/24 2:04 PM, Stefan Eissing via dev wrote:
> > >>>
> > >>>
> > >>>> Am 05.07.2024 um 13:51 schrieb Ruediger Pluem :
> > >>>>
> > >>>> I just noticed that mod_md in 2.4.61 fails to compile with openssl <
> > >>>> 1.1.1. Below is the output against openssl 1.0.2 on RedHat 7:
> > >>>>
> > >>>> md_crypt.c: In function 'md_pkey_get_rsa_e64':
> > >>>> md_crypt.c:982:5: warning: implicit declaration of function
> > >>>> 'EVP_PKEY_get0_RSA' [-Wimplicit-function-declaration]
> > >>>> const RSA *rsa = EVP_PKEY_get0_RSA(pkey->pkey);
> > >>>> ^
> > >>>> md_crypt.c:982:22: warning: initialization makes pointer from integer
> > >>>> without a cast [enabled by default]
> > >>>> const RSA *rsa = EVP_PKEY_get0_RSA(pkey->pkey);
> > >>>> ^
> > >>>> md_crypt.c: In function 'md_pkey_get_rsa_n64':
> > >>>> md_crypt.c:1002:22: warning: initialization makes pointer from integer
> > >>>> without a cast [enabled by default]
> > >>>> const RSA *rsa = EVP_PKEY_get0_RSA(pkey->pkey);
> > >>>> ^
> >
> > This was already the case with 2.4.59 and openssl 1.0.2. Hence we did not
> > fail to compile but loading of mod_md likely would fail
> > as the symbol EVP_PKEY_get0_RSA is not available with openssl 1.0.2.
>
> This probably comes from r1913912 (2.4.x) which backported r1913616
> (trunk) which changed EVP_PKEY_get1_RSA() => EVP_PKEY_get0_RSA(), the
> former being probably available in < 1.1.1.
> So the check for using EVP_PKEY_get{0,1}_RSA() or the new openssl >= 3
> API should probably be something like:
>
> #if OPENSSL_VERSION_NUMBER < 0x10101000L
> RSA *rsa = EVP_PKEY_get1_RSA(pkey->pkey);
> if (rsa) {
> const char *ret;
> const BIGNUM *e;
> RSA_get0_key(rsa, NULL, &e, NULL);
> ret = bn64(e, p);
> RSA_free(rsa);
> return ret;
> }
> #elif OPENSSL_VERSION_NUMBER < 0x3000L
> ...
> #else
> ...
> #endif
>
> ?
Patch attached.
>
>
> Regards;
> Yann.
Index: modules/md/md_crypt.c
===
--- modules/md/md_crypt.c (revision 1918881)
+++ modules/md/md_crypt.c (working copy)
@@ -978,7 +978,17 @@ static const char *bn64(const BIGNUM *b, apr_pool_
const char *md_pkey_get_rsa_e64(md_pkey_t *pkey, apr_pool_t *p)
{
-#if OPENSSL_VERSION_NUMBER < 0x3000L
+#if OPENSSL_VERSION_NUMBER < 0x10101000L
+RSA *rsa = EVP_PKEY_get1_RSA(pkey->pkey);
+if (rsa) {
+const char *ret;
+const BIGNUM *e;
+RSA_get0_key(rsa, NULL, &e, NULL);
+ret = bn64(e, p);
+RSA_free(rsa);
+return ret;
+}
+#elif OPENSSL_VERSION_NUMBER < 0x3000L
const RSA *rsa = EVP_PKEY_get0_RSA(pkey->pkey);
if (rsa) {
const BIGNUM *e;
@@ -998,7 +1008,17 @@ const char *md_pkey_get_rsa_e64(md_pkey_t *pkey, a
const char *md_pkey_get_rsa_n64(md_pkey_t *pkey, apr_pool_t *p)
{
-#if OPENSSL_VERSION_NUMBER < 0x3000L
+#if OPENSSL_VERSION_NUMBER < 0x10101000L
+RSA *rsa = EVP_PKEY_get1_RSA(pkey->pkey);
+if (rsa) {
+const char *ret;
+const BIGNUM *n;
+RSA_get0_key(rsa, &n, NULL, NULL);
+ret = bn64(n, p);
+RSA_free(rsa);
+return ret;
+}
+#elif OPENSSL_VERSION_NUMBER < 0x3000L
const RSA *rsa = EVP_PKEY_get0_RSA(pkey->pkey);
if (rsa) {
const BIGNUM *n;
Re: mod_md in 2.4.61 fails to compile with openssl < 1.1.1
On Fri, Jul 5, 2024 at 3:05 PM Ruediger Pluem wrote:
>
>
>
> On 7/5/24 2:14 PM, Ruediger Pluem wrote:
> >
> >
> > On 7/5/24 2:11 PM, Ruediger Pluem wrote:
> >>
> >>
> >> On 7/5/24 2:04 PM, Stefan Eissing via dev wrote:
> >>>
> >>>
> Am 05.07.2024 um 13:51 schrieb Ruediger Pluem :
>
> I just noticed that mod_md in 2.4.61 fails to compile with openssl <
> 1.1.1. Below is the output against openssl 1.0.2 on RedHat 7:
>
> md_crypt.c: In function 'md_pkey_get_rsa_e64':
> md_crypt.c:982:5: warning: implicit declaration of function
> 'EVP_PKEY_get0_RSA' [-Wimplicit-function-declaration]
> const RSA *rsa = EVP_PKEY_get0_RSA(pkey->pkey);
> ^
> md_crypt.c:982:22: warning: initialization makes pointer from integer
> without a cast [enabled by default]
> const RSA *rsa = EVP_PKEY_get0_RSA(pkey->pkey);
> ^
> md_crypt.c: In function 'md_pkey_get_rsa_n64':
> md_crypt.c:1002:22: warning: initialization makes pointer from integer
> without a cast [enabled by default]
> const RSA *rsa = EVP_PKEY_get0_RSA(pkey->pkey);
> ^
>
> This was already the case with 2.4.59 and openssl 1.0.2. Hence we did not
> fail to compile but loading of mod_md likely would fail
> as the symbol EVP_PKEY_get0_RSA is not available with openssl 1.0.2.
This probably comes from r1913912 (2.4.x) which backported r1913616
(trunk) which changed EVP_PKEY_get1_RSA() => EVP_PKEY_get0_RSA(), the
former being probably available in < 1.1.1.
So the check for using EVP_PKEY_get{0,1}_RSA() or the new openssl >= 3
API should probably be something like:
#if OPENSSL_VERSION_NUMBER < 0x10101000L
RSA *rsa = EVP_PKEY_get1_RSA(pkey->pkey);
if (rsa) {
const char *ret;
const BIGNUM *e;
RSA_get0_key(rsa, NULL, &e, NULL);
ret = bn64(e, p);
RSA_free(rsa);
return ret;
}
#elif OPENSSL_VERSION_NUMBER < 0x3000L
...
#else
...
#endif
?
Regards;
Yann.
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
On Wed, Jul 3, 2024 at 4:40 PM Ruediger Pluem wrote: > > On 7/3/24 3:23 PM, Yann Ylavic wrote: > > On Wed, Jul 3, 2024 at 2:34 PM Ruediger Pluem wrote: > >> > >> Thanks for the intense review. I hope I captured all stuff in the below. > > > > Thanks for working on this. > > Committed in r1918880 considering your comments. Feel free to add the patch > you provided > with regards to the move of the trace level checking. Done in r1918883.
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
On Wed, Jul 3, 2024 at 4:40 PM Ruediger Pluem wrote: > > On 7/3/24 3:23 PM, Yann Ylavic wrote: > > On Wed, Jul 3, 2024 at 2:34 PM Ruediger Pluem wrote: > >> > >> Thanks for the intense review. I hope I captured all stuff in the below. > > > > Thanks for working on this. > > Committed in r1918880 considering your comments. Feel free to add the patch > you provided > with regards to the move of the trace level checking. Great, thanks Rüdiger!
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
On Wed, Jul 3, 2024 at 2:34 PM Ruediger Pluem wrote:
>
> Thanks for the intense review. I hope I captured all stuff in the below.
Thanks for working on this.
> I intentionally left the 'len' parameter of ssl_io_data_dump an apr_size_t in
> case we ever get to the conclusion that we want to
> make MODSSL_IO_DUMP_MAX larger. It will fit then automatically.
>
> Index: modules/ssl/ssl_engine_io.c
> ===
> --- modules/ssl/ssl_engine_io.c (revision 1918869)
> +++ modules/ssl/ssl_engine_io.c (working copy)
> @@ -2308,7 +2308,7 @@
> #define DUMP_WIDTH 16
>
> static void ssl_io_data_dump(conn_rec *c, server_rec *s,
> - const char *b, long len)
> + const char *b, apr_size_t len)
> {
> char buf[256];
> int i, j, rows, trunc, pos;
The problem is that some of the i,j,.. should apr_size_t too then.
I'd rather we use "int len" here because APR_INT32_MAX is really a
hard limit IMHO, we shouldn't take seconds/minutes to log things to
disk anyway, and it'd avoid thinking about overflows here and keep
things simple.
> @@ -2361,11 +2361,13 @@
> }
> if (trunc > 0)
> ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
> -"| %04ld - ", len + trunc);
> +"| %04" APR_SIZE_T_FMT " - ", len +
> (size_t)trunc);
> ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
>
> "+-+");
> }
>
> +#define MODSSL_IO_DUMP_MAX APR_UINT16_MAX
> +
> #if OPENSSL_VERSION_NUMBER >= 0x3000L
> static long modssl_io_cb(BIO *bio, int cmd, const char *argp,
> size_t len, int argi, long argl, int rc,
> @@ -2379,9 +2381,9 @@
> conn_rec *c;
> server_rec *s;
> #if OPENSSL_VERSION_NUMBER >= 0x3000L
> -(void)len;
> -(void)processed;
> +(void)argi;
> #endif
> +(void)argl;
>
> if ((ssl = (SSL *)BIO_get_callback_arg(bio)) == NULL)
> return rc;
> @@ -2391,27 +2393,58 @@
>
> if ( cmd == (BIO_CB_WRITE|BIO_CB_RETURN)
> || cmd == (BIO_CB_READ |BIO_CB_RETURN) ) {
> -if (rc >= 0) {
> +#if OPENSSL_VERSION_NUMBER >= 0x3000L
> +apr_size_t requested_len = len;
> +/*
> + * On OpenSSL >= 3 rc uses the meaning of the BIO_read_ex and
> + * BIO_write_ex functions return value and not the one of
> + * BIO_read and BIO_write. Hence 0 indicates an error.
> + */
> +int ok = (rc > 0);
> +#else
> +apr_size_t requested_len = (apr_size_t)argi;
> +int ok = (rc >= 0);
> +#endif
> +if (ok) {
> +#if OPENSSL_VERSION_NUMBER >= 0x3000L
> +apr_size_t actual_len = *processed;
> +#else
> +apr_size_t actual_len = (apr_size_t)rc;
> +#endif
> const char *dump = "";
> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
> -if (argp != NULL)
> +if (argp == NULL)
> +dump = "(Oops, no memory buffer?)";
> +else if (actual_len > MODSSL_IO_DUMP_MAX)
> +dump = "(BIO dump follows, truncated to "
> + APR_STRINGIFY(MODSSL_IO_DUMP_MAX) ")";
> +else
> dump = "(BIO dump follows)";
> -else
> -dump = "(Oops, no memory buffer?)";
> }
> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> -"%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
> +"%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
> +" bytes %s BIO#%pp [mem: %pp] %s",
> MODSSL_LIBRARY_NAME,
> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
> -(long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ?
> "to" : "from"),
> +actual_len, requested_len,
> +(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> bio, argp, dump);
> -if (*dump != '\0' && argp != NULL)
> -ssl_io_data_dump(c, s, argp, rc);
> +/*
> + * *dump will only be != '\0' if
> + * APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)
> + */
> +if (*dump != '\0' && argp != NULL) {
> +apr_size_t dump_len = (actual_len >= MODSSL_IO_DUMP_MAX
> + ? MODSSL_IO_DUMP_MAX
> + : actual_len);
So "int dump_len" here should be (more than) enough?
> +ssl_io_data_dump(c, s, argp, dump_len);
> +}
> }
> else {
> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> -"%s: I/O error, %d bytes expected to %s on BIO#%pp [mem:
> %pp]",
> -MODSSL_LIBRARY_NAME, argi,
> +
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
On Wed, Jul 3, 2024 at 11:01 AM Yann Ylavic wrote:
>
> On Wed, Jul 3, 2024 at 10:57 AM Yann Ylavic wrote:
> >
> > On Wed, Jul 3, 2024 at 8:58 AM Ruediger Pluem wrote:
> > >
> > > On 7/3/24 2:59 AM, Yann Ylavic wrote:
> > > > On Tue, Jul 2, 2024 at 10:57 AM Ruediger Pluem
> > > > wrote:
> > > >>
> > > >> Updated patch.
> > []
> > > >> const char *dump = "";
> > > >> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
> > > >> if (argp != NULL)
> > > >> @@ -2400,23 +2413,28 @@
> > > >> dump = "(Oops, no memory buffer?)";
> > > >> }
> > > >> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> > > >> -"%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
> > > >> +"%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
> > > >> +" bytes %s BIO#%pp [mem: %pp] %s",
> > > >> MODSSL_LIBRARY_NAME,
> > > >> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" :
> > > >> "read"),
> > > >> -(long)rc, argi, (cmd ==
> > > >> (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> > > >> +actual_len, requested_len,
> > > >> +(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" :
> > > >> "from"),
> > > >> bio, argp, dump);
> > > >> if (*dump != '\0' && argp != NULL)
>
> Here we could check for APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7) too, to
> save the call when ssl_io_data_dump() is a noop anyway.
In this area, maybe we want to move the check for APLOG_TRACE4 from
the caller of modssl_set_io_callbacks() to the helper itself (which
knows better). Something like the attached eventually, feel free to
use since you are touching this code ;)
Index: modules/ssl/ssl_engine_io.c
===
--- modules/ssl/ssl_engine_io.c (revision 1918653)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -2281,9 +2281,7 @@ apr_status_t ssl_io_filter_init(conn_rec *c, reque
apr_pool_cleanup_register(c->pool, (void*)filter_ctx,
ssl_io_filter_cleanup, apr_pool_cleanup_null);
-if (APLOG_CS_IS_LEVEL(c, mySrvFromConn(c), APLOG_TRACE4)) {
-modssl_set_io_callbacks(ssl);
-}
+modssl_set_io_callbacks(ssl, c, mySrvFromConn(c));
return APR_SUCCESS;
}
@@ -2429,10 +2450,15 @@ static APR_INLINE void set_bio_callback(BIO *bio,
BIO_set_callback_arg(bio, arg);
}
-void modssl_set_io_callbacks(SSL *ssl)
+void modssl_set_io_callbacks(SSL *ssl, conn_rec *c, server_rec *s)
{
-BIO *rbio = SSL_get_rbio(ssl),
-*wbio = SSL_get_wbio(ssl);
+BIO *rbio, *wbio;
+
+if (!APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE4))
+return;
+
+rbio = SSL_get_rbio(ssl);
+wbio = SSL_get_wbio(ssl);
if (rbio) {
set_bio_callback(rbio, ssl);
}
Index: modules/ssl/ssl_engine_kernel.c
===
--- modules/ssl/ssl_engine_kernel.c (revision 1918653)
+++ modules/ssl/ssl_engine_kernel.c (working copy)
@@ -2607,9 +2607,7 @@ static int ssl_find_vhost(void *servername, conn_r
* (and the first vhost doesn't use APLOG_TRACE4), then
* we need to set that callback here.
*/
-if (APLOGtrace4(s)) {
-modssl_set_io_callbacks(ssl);
-}
+modssl_set_io_callbacks(ssl, c, s);
return 1;
}
Index: modules/ssl/ssl_private.h
===
--- modules/ssl/ssl_private.h (revision 1918653)
+++ modules/ssl/ssl_private.h (working copy)
@@ -1053,7 +1053,7 @@ void modssl_callback_keylog(const SSL *ssl
/** I/O */
apr_status_t ssl_io_filter_init(conn_rec *, request_rec *r, SSL *);
void ssl_io_filter_register(apr_pool_t *);
-void modssl_set_io_callbacks(SSL *ssl);
+void modssl_set_io_callbacks(SSL *ssl, conn_rec *c, server_rec *s);
/* ssl_io_buffer_fill fills the setaside buffering of the HTTP request
* to allow an SSL renegotiation to take place. */
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
On Wed, Jul 3, 2024 at 10:57 AM Yann Ylavic wrote:
>
> On Wed, Jul 3, 2024 at 8:58 AM Ruediger Pluem wrote:
> >
> > On 7/3/24 2:59 AM, Yann Ylavic wrote:
> > > On Tue, Jul 2, 2024 at 10:57 AM Ruediger Pluem wrote:
> > >>
> > >> Updated patch.
> []
> > >> const char *dump = "";
> > >> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
> > >> if (argp != NULL)
> > >> @@ -2400,23 +2413,28 @@
> > >> dump = "(Oops, no memory buffer?)";
> > >> }
> > >> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> > >> -"%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
> > >> +"%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
> > >> +" bytes %s BIO#%pp [mem: %pp] %s",
> > >> MODSSL_LIBRARY_NAME,
> > >> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" :
> > >> "read"),
> > >> -(long)rc, argi, (cmd ==
> > >> (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> > >> +actual_len, requested_len,
> > >> +(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" :
> > >> "from"),
> > >> bio, argp, dump);
> > >> if (*dump != '\0' && argp != NULL)
Here we could check for APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7) too, to
save the call when ssl_io_data_dump() is a noop anyway.
> > >> -ssl_io_data_dump(c, s, argp, rc);
> > >> +ssl_io_data_dump(c, s, argp, actual_len);
> > > And here:
> > >long dump_len = (actual_len >= APR_UINT16_MAX
> > > ? APR_UINT16_MAX
> > > : actual_len);
> >
> > Hm. As you point out: SSL records should be of limited length anyway, but
> > if we really truncate the output shouldn't we log this?
> > A log message like: Dump truncated to first XXX bytes?
>
> Something like this to initialize "dump" above?
>
> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
> if (argp == NULL)
> dump = "(Oops, no memory buffer?)";
> else if (actual_len > MODSSL_IO_DUMP_MAX)
> dump = "(BIO dump follows, truncated to "
> APR_STRINGIFY(MODSSL_IO_DUMP_MAX) ")";
> else
> dump = "(BIO dump follows)";
> }
>
> where we'd "#define MODSSL_IO_DUMP_MAX APR_UINT16_MAX" and use it
> everywhere eventually.
>
> >
> > >ssl_io_data_dump(c, s, argp, dump_len);
> > >> }
> > >> else {
> > >> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> > >> -"%s: I/O error, %d bytes expected to %s on BIO#%pp
> > >> [mem: %pp]",
> > >> -MODSSL_LIBRARY_NAME, argi,
> > >> +"%s: I/O error, %" APR_SIZE_T_FMT
> > >> +" bytes expected to %s on BIO#%pp [mem: %pp]",
> > >> +MODSSL_LIBRARY_NAME, requested_len,
> > >> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" :
> > >> "read"),
> > >> bio, argp);
> > >> }
> > >> }
> > >> return rc;
> > >> +#undef requested_len
> > >> +#undef actual_len
> > >> }
> > > ?
>
>
> Cheers;
> Yann.
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
On Wed, Jul 3, 2024 at 8:58 AM Ruediger Pluem wrote:
>
> On 7/3/24 2:59 AM, Yann Ylavic wrote:
> > On Tue, Jul 2, 2024 at 10:57 AM Ruediger Pluem wrote:
> >>
> >> Updated patch.
[]
> >> const char *dump = "";
> >> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
> >> if (argp != NULL)
> >> @@ -2400,23 +2413,28 @@
> >> dump = "(Oops, no memory buffer?)";
> >> }
> >> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> >> -"%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
> >> +"%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
> >> +" bytes %s BIO#%pp [mem: %pp] %s",
> >> MODSSL_LIBRARY_NAME,
> >> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" :
> >> "read"),
> >> -(long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN)
> >> ? "to" : "from"),
> >> +actual_len, requested_len,
> >> +(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> >> bio, argp, dump);
> >> if (*dump != '\0' && argp != NULL)
> >> -ssl_io_data_dump(c, s, argp, rc);
> >> +ssl_io_data_dump(c, s, argp, actual_len);
> > And here:
> >long dump_len = (actual_len >= APR_UINT16_MAX
> > ? APR_UINT16_MAX
> > : actual_len);
>
> Hm. As you point out: SSL records should be of limited length anyway, but if
> we really truncate the output shouldn't we log this?
> A log message like: Dump truncated to first XXX bytes?
Something like this to initialize "dump" above?
if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
if (argp == NULL)
dump = "(Oops, no memory buffer?)";
else if (actual_len > MODSSL_IO_DUMP_MAX)
dump = "(BIO dump follows, truncated to "
APR_STRINGIFY(MODSSL_IO_DUMP_MAX) ")";
else
dump = "(BIO dump follows)";
}
where we'd "#define MODSSL_IO_DUMP_MAX APR_UINT16_MAX" and use it
everywhere eventually.
>
> >ssl_io_data_dump(c, s, argp, dump_len);
> >> }
> >> else {
> >> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> >> -"%s: I/O error, %d bytes expected to %s on BIO#%pp
> >> [mem: %pp]",
> >> -MODSSL_LIBRARY_NAME, argi,
> >> +"%s: I/O error, %" APR_SIZE_T_FMT
> >> +" bytes expected to %s on BIO#%pp [mem: %pp]",
> >> +MODSSL_LIBRARY_NAME, requested_len,
> >> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" :
> >> "read"),
> >> bio, argp);
> >> }
> >> }
> >> return rc;
> >> +#undef requested_len
> >> +#undef actual_len
> >> }
> > ?
Cheers;
Yann.
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
On Tue, Jul 2, 2024 at 10:57 AM Ruediger Pluem wrote:
>
> On 7/1/24 3:01 PM, Ruediger Pluem wrote:
> >
> >
> > On 6/27/24 3:48 PM, Ruediger Pluem wrote:
> >>
> >>
> >> On 6/27/24 12:47 PM, Yann Ylavic wrote:
> >>> On Thu, Jun 27, 2024 at 12:38 PM Yann Ylavic wrote:
> >>>>
> >>>> On Thu, Jun 27, 2024 at 12:07 PM Ruediger Pluem
> >>>> wrote:
> >>>>>
> >>>>> How about the below? I am yet undecided if I should follow the below
> >>>>> approach to have
> >>>>> two completely separate callbacks depending on the OpenSSL version or
> >>>>> one with a lot of
> >>>>> #If statements in it, but as much shared code as possible. Thoughts?
> >>>>
> >>>> Hm, I wouldn't duplicate the whole thing depending on openssl version.
> >>>>
> >>>> Something like the attached maybe? modssl_io_cb() will just consider
> >>>> up to APR_INT32_MAX bytes, more shouldn't happen anyway and it's more
> >>>> than enough for debug logs..
> >>>
> >>> We could even log the real length still if it matters, see attached v2.
> >>
> >> Thanks. Unfortunately the meaning of rc varies depending on if we have the
> >> ex or the non ex callback.
> >> This is not in the documentation but only in the OpenSSL code :-(.
> >> Furthermore the processed amount of data is in *processed in the ex case
> >> whereas in the non ex case it is in
> >> rc. The intended amount of data to be processed is in len in the ex case
> >> and in argi in the non ex case.
> >> Hence I propose the patch below. Of course we can have a longer debate if
> >> the len parameter to ssl_io_data_dump
> >> should be int or size_t :-). And yes dumping more than APR_INT32_MAX bytes
> >> to the log might be bad.
> >
> > Any further comments on whether we should limit the dump to APR_INT32_MAX
> > or not? I would guess that it will not matter
> > in practice, but I am still open to it.
> > BTW: I guess
> >
> > int len = data_len & APR_INT32_MAX;
> >
> > would be wrong. It would need to be
> >
> > int len = data_len > APR_INT32_MAX ? APR_INT32_MAX : (int)data_len;
> >
> > instead.
Right! Maybe APR_UINT16_MAX is enough, I don't think we should log
more undecipherable text than that :)
IIRC an SSL/TLS payload is 16K (still?) anyway so we shouldn't be
called for more than that (though multiple times)?
>
> Updated patch.
>
> Index: modules/ssl/ssl_engine_io.c
> ===
> --- modules/ssl/ssl_engine_io.c (revision 1918813)
> +++ modules/ssl/ssl_engine_io.c (working copy)
> @@ -2308,7 +2308,7 @@
> #define DUMP_WIDTH 16
>
> static void ssl_io_data_dump(conn_rec *c, server_rec *s,
> - const char *b, long len)
> + const char *b, size_t len)
> {
> char buf[256];
> int i, j, rows, trunc, pos;
> @@ -2361,7 +2361,7 @@
> }
> if (trunc > 0)
> ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
> -"| %04ld - ", len + trunc);
> +"| %04" APR_SIZE_T_FMT " - ", len +
> (size_t)trunc);
> ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
>
> "+-+");
> }
> @@ -2379,9 +2379,9 @@
> conn_rec *c;
> server_rec *s;
> #if OPENSSL_VERSION_NUMBER >= 0x3000L
> -(void)len;
> -(void)processed;
> +(void)argi;
> #endif
> +(void)argl;
>
> if ((ssl = (SSL *)BIO_get_callback_arg(bio)) == NULL)
> return rc;
> @@ -2391,7 +2391,20 @@
>
> if ( cmd == (BIO_CB_WRITE|BIO_CB_RETURN)
> || cmd == (BIO_CB_READ |BIO_CB_RETURN) ) {
> +#if OPENSSL_VERSION_NUMBER >= 0x3000L
> +#define requested_len (len)
> +#define actual_len (*processed)
requested_len = len
> +/*
> + * On OpenSSL >= 3 rc uses the meaning of the BIO_read_ex and
> + * BIO_write_ex functions return value and not the one of
> + * BIO_read and BIO_write. Hence 0 indicates an error.
> + */
> +if (rc > 0) {
> +#else
> +#define requested_len ((size_t)argi)
> +#define actual_len ((size_t)rc)
> if (rc >= 0) {
> +#endif
Maybe use variables like:
#if OPENSSL_VERSION_NUMBER >= 0x3000L
apr_size_t requ
Re: [VOTE] Release httpd-2.4.61-rc1 as httpd-2.4.61
On Tue, Jul 2, 2024 at 3:45 PM Eric Covener wrote: > > Hi all, > > Please find below the proposed release tarball and signatures: > > https://dist.apache.org/repos/dist/dev/httpd/ > > === Different from template === > I would like to call an expedited VOTE (due to regression in 2.4.60) > over the next 1-2 days to release this candidate tarball > httpd-2.4.61-rc1 as 2.4.61: > > (sorry) > === end not from a template [X] +1: It's not just good, it's good enough! Tested on Debian stable and testing/sid, all passes. Thanks Eric!
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
On Thu, Jun 27, 2024 at 12:38 PM Yann Ylavic wrote:
>
> On Thu, Jun 27, 2024 at 12:07 PM Ruediger Pluem wrote:
> >
> > How about the below? I am yet undecided if I should follow the below
> > approach to have
> > two completely separate callbacks depending on the OpenSSL version or one
> > with a lot of
> > #If statements in it, but as much shared code as possible. Thoughts?
>
> Hm, I wouldn't duplicate the whole thing depending on openssl version.
>
> Something like the attached maybe? modssl_io_cb() will just consider
> up to APR_INT32_MAX bytes, more shouldn't happen anyway and it's more
> than enough for debug logs..
We could even log the real length still if it matters, see attached v2.
>
> Regards;
> Yann.
Index: modules/ssl/ssl_engine_io.c
===
--- modules/ssl/ssl_engine_io.c (revision 1918653)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -2308,7 +2308,7 @@ void ssl_io_filter_register(apr_pool_t *p)
#define DUMP_WIDTH 16
static void ssl_io_data_dump(conn_rec *c, server_rec *s,
- const char *b, long len)
+ const char *b, int len)
{
char buf[256];
int i, j, rows, trunc, pos;
@@ -2361,17 +2361,18 @@ static void ssl_io_data_dump(conn_rec *c, server_r
}
if (trunc > 0)
ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
-"| %04ld - ", len + trunc);
+"| %04d - ", len + trunc);
ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
"+-+");
}
#if OPENSSL_VERSION_NUMBER >= 0x3000L
-static long modssl_io_cb(BIO *bio, int cmd, const char *argp,
- size_t len, int argi, long argl, int rc,
+static long modssl_io_cb(BIO *bio, int cmd,
+ const char *data, size_t data_len,
+ int argi, long argl, int rc,
size_t *processed)
#else
-static long modssl_io_cb(BIO *bio, int cmd, const char *argp,
+static long modssl_io_cb(BIO *bio, int cmd, const char *data,
int argi, long argl, long rc)
#endif
{
@@ -2378,8 +2379,10 @@ static void ssl_io_data_dump(conn_rec *c, server_r
SSL *ssl;
conn_rec *c;
server_rec *s;
+
+/* unused */
+(void)argl;
#if OPENSSL_VERSION_NUMBER >= 0x3000L
-(void)len;
(void)processed;
#endif
@@ -2392,28 +2395,35 @@ static void ssl_io_data_dump(conn_rec *c, server_r
if ( cmd == (BIO_CB_WRITE|BIO_CB_RETURN)
|| cmd == (BIO_CB_READ |BIO_CB_RETURN) ) {
if (rc >= 0) {
+#if OPENSSL_VERSION_NUMBER >= 0x3000L
+apr_size_t real_len = data_len;
+int len = data_len & APR_INT32_MAX;
+#else
+apr_size_t real_len = rc;
+int len = rc & APR_INT32_MAX;
+#endif
const char *dump = "";
if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
-if (argp != NULL)
+if (data != NULL)
dump = "(BIO dump follows)";
else
dump = "(Oops, no memory buffer?)";
}
ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
-"%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
+"%s: %s %" APR_SIZE_T_FMT "/%d bytes %s BIO#%pp [mem: %pp] %s",
MODSSL_LIBRARY_NAME,
(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
-(long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
-bio, argp, dump);
-if (*dump != '\0' && argp != NULL)
-ssl_io_data_dump(c, s, argp, rc);
+real_len, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
+bio, data, dump);
+if (*dump != '\0' && data != NULL)
+ssl_io_data_dump(c, s, data, len);
}
else {
ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
-"%s: I/O error, %d bytes expected to %s on BIO#%pp [mem: %pp]",
-MODSSL_LIBRARY_NAME, argi,
+"%s: I/O error %ld, %d bytes expected to %s on BIO#%pp [mem: %pp]",
+MODSSL_LIBRARY_NAME, (long)rc, argi,
(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
-bio, argp);
+bio, data);
}
}
return rc;
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
On Thu, Jun 27, 2024 at 12:07 PM Ruediger Pluem wrote:
>
> How about the below? I am yet undecided if I should follow the below approach
> to have
> two completely separate callbacks depending on the OpenSSL version or one
> with a lot of
> #If statements in it, but as much shared code as possible. Thoughts?
Hm, I wouldn't duplicate the whole thing depending on openssl version.
Something like the attached maybe? modssl_io_cb() will just consider
up to APR_INT32_MAX bytes, more shouldn't happen anyway and it's more
than enough for debug logs..
Regards;
Yann.
Index: modules/ssl/ssl_engine_io.c
===
--- modules/ssl/ssl_engine_io.c (revision 1918653)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -2308,7 +2308,7 @@ void ssl_io_filter_register(apr_pool_t *p)
#define DUMP_WIDTH 16
static void ssl_io_data_dump(conn_rec *c, server_rec *s,
- const char *b, long len)
+ const char *b, int len)
{
char buf[256];
int i, j, rows, trunc, pos;
@@ -2361,17 +2361,18 @@ static void ssl_io_data_dump(conn_rec *c, server_r
}
if (trunc > 0)
ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
-"| %04ld - ", len + trunc);
+"| %04d - ", len + trunc);
ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
"+-+");
}
#if OPENSSL_VERSION_NUMBER >= 0x3000L
-static long modssl_io_cb(BIO *bio, int cmd, const char *argp,
- size_t len, int argi, long argl, int rc,
+static long modssl_io_cb(BIO *bio, int cmd,
+ const char *data, size_t data_len,
+ int argi, long argl, int rc,
size_t *processed)
#else
-static long modssl_io_cb(BIO *bio, int cmd, const char *argp,
+static long modssl_io_cb(BIO *bio, int cmd, const char *data,
int argi, long argl, long rc)
#endif
{
@@ -2378,11 +2379,11 @@ static void ssl_io_data_dump(conn_rec *c, server_r
SSL *ssl;
conn_rec *c;
server_rec *s;
+
#if OPENSSL_VERSION_NUMBER >= 0x3000L
-(void)len;
(void)processed;
#endif
-
+
if ((ssl = (SSL *)BIO_get_callback_arg(bio)) == NULL)
return rc;
if ((c = (conn_rec *)SSL_get_app_data(ssl)) == NULL)
@@ -2392,28 +2393,33 @@ static void ssl_io_data_dump(conn_rec *c, server_r
if ( cmd == (BIO_CB_WRITE|BIO_CB_RETURN)
|| cmd == (BIO_CB_READ |BIO_CB_RETURN) ) {
if (rc >= 0) {
+#if OPENSSL_VERSION_NUMBER >= 0x3000L
+int len = data_len & APR_INT32_MAX;
+#else
+int len = rc & APR_INT32_MAX;
+#endif
const char *dump = "";
if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
-if (argp != NULL)
+if (data != NULL)
dump = "(BIO dump follows)";
else
dump = "(Oops, no memory buffer?)";
}
ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
-"%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
+"%s: %s %d/%d bytes %s BIO#%pp [mem: %pp] %s",
MODSSL_LIBRARY_NAME,
(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
-(long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
-bio, argp, dump);
-if (*dump != '\0' && argp != NULL)
-ssl_io_data_dump(c, s, argp, rc);
+len, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
+bio, data, dump);
+if (*dump != '\0' && data != NULL)
+ssl_io_data_dump(c, s, data, len);
}
else {
ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
-"%s: I/O error, %d bytes expected to %s on BIO#%pp [mem: %pp]",
-MODSSL_LIBRARY_NAME, argi,
+"%s: I/O error %ld, %d bytes expected to %s on BIO#%pp [mem: %pp]",
+MODSSL_LIBRARY_NAME, (long)rc, argi,
(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
-bio, argp);
+bio, data);
}
}
return rc;
Re: [VOTE] Release httpd-2.4.60-rc4 as httpd-2.4.60
On Wed, Jun 26, 2024 at 7:10 PM Eric Covener wrote: > > I would like to call a VOTE over the next few days to release > this candidate tarball httpd-2.4.60-rc4 as 2.4.60: [X] +1: It's not just good, it's good enough! Tested on Debian stable and testing/sid, all passes. Sigs/checksums OK too. Great job Eric, thank you very much!
Re: svn commit: r1918651 - in /httpd/httpd/trunk: include/ap_mmn.h include/httpd.h modules/mappers/mod_rewrite.c server/util.c
On Wed, Jun 26, 2024 at 12:09 PM wrote:
>
> --- httpd/httpd/trunk/modules/mappers/mod_rewrite.c (original)
> +++ httpd/httpd/trunk/modules/mappers/mod_rewrite.c Wed Jun 26 10:09:29 2024
> @@ -4413,10 +4410,15 @@ static rule_return_type apply_rewrite_ru
> && !is_absolute_path(newuri)
> && !is_absolute_uri(newuri, NULL)) {
> if (ctx->perdir) {
> -rewritelog(r, 3, ctx->perdir, "add per-dir prefix: %s -> %s%s",
> - newuri, ctx->perdir, newuri);
> +if (!AP_IS_SLASH(*newuri)) {
> +/* perdir, the newuri will be internally redirected, so
> + * leading slash is enough even if it's an ambiguous fs path
> + */
> +rewritelog(r, 3, ctx->perdir, "add per-dir prefix: %s ->
> %s%s",
> + newuri, ctx->perdir, newuri);
>
> -newuri = apr_pstrcat(r->pool, ctx->perdir, newuri, NULL);
> +newuri = apr_pstrcat(r->pool, ctx->perdir, newuri, NULL);
> +}
> }
> else if (!(p->flags & (RULEFLAG_PROXY | RULEFLAG_FORCEREDIRECT))) {
> /* Not an absolute URI-path and the scheme (if any) is unknown,
Maybe the test for !AP_IS_SLASH() could be moved up (i.e.
!AP_IS_SLASH() && !is_absolute_path()) if we don't want to add a '/'
before anything Windows considers a slash too?
Regards;
Yann.
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
On Wed, Jun 26, 2024 at 10:28 AM Ruediger Pluem wrote:
>
> On 3/19/23 10:30 PM, yla...@apache.org wrote:
> >
> > --- httpd/httpd/trunk/modules/ssl/ssl_engine_io.c (original)
> > +++ httpd/httpd/trunk/modules/ssl/ssl_engine_io.c Sun Mar 19 21:30:47 2023
>
> > @@ -2402,7 +2403,7 @@ long ssl_io_data_cb(BIO *bio, int cmd,
> > "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
> > MODSSL_LIBRARY_NAME,
> > (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" :
> > "read"),
> > -rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to"
> > : "from"),
> > +(long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ?
> > "to" : "from"),
>
> I think rc has a different meaning with OpenSSL 3. I think we need to use len
> in case of OpenSSL 3.
> I noticed that with OpenSSL 3 only single bytes get dumped and no longer the
> whole buffer.
>
> > bio, argp, dump);
> > if (*dump != '\0' && argp != NULL)
> > ssl_io_data_dump(c, s, argp, rc);
>
> Similar to above. I think we need to use len here in case of OpenSSL 3.
> If my findings are seen as correct I could work on a patch :-).
I think you are right from the man page of BIO_set_callback{,_ex}(),
please go ahead ;)
Regards;
Yann.
Re: svn commit: r1918626 - in /httpd/httpd/trunk: include/ap_mmn.h modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h modules/proxy/proxy_util.c
On Wed, Jun 26, 2024 at 9:05 AM Ruediger Pluem wrote:
>
> On 6/26/24 1:49 AM, yla...@apache.org wrote:
> >
> > --- httpd/httpd/trunk/modules/proxy/mod_proxy.c (original)
> > +++ httpd/httpd/trunk/modules/proxy/mod_proxy.c Tue Jun 25 23:49:09 2024
>
> > @@ -1319,22 +1320,29 @@ static int proxy_handler(request_rec *r)
> > return DECLINED;
> > }
> >
> > -if (!r->proxyreq) {
> > -rc = DECLINED;
> > -/* We may have forced the proxy handler via config or .htaccess */
> > -if (r->handler &&
> > -strncmp(r->handler, "proxy:", 6) == 0 &&
> > -strncmp(r->filename, "proxy:", 6) != 0) {
> > -r->proxyreq = PROXYREQ_REVERSE;
> > -r->filename = apr_pstrcat(r->pool, r->handler, r->filename,
> > NULL);
> > -/* Still need to fixup/canonicalize r->filename */
> > +/* We may have forced the proxy handler via config or .htaccess */
> > +if (!r->proxyreq && r->handler && strncmp(r->handler, "proxy:", 6) ==
> > 0) {
> > +char *old_filename = r->filename;
> > +
> > +r->proxyreq = PROXYREQ_REVERSE;
> > +r->filename = apr_pstrcat(r->pool, r->handler, r->filename, NULL);
> > +
> > +/* Still need to fixup/canonicalize r->filename */
> > +uri = r->filename + 6;
> I don't think that we need to set uri to anything here. It either gets set
> below in
> ap_proxy_fixup_uds_filename below or if not we set it to r->filename + 6
> later (line 1427).
I simplified ap_proxy_fixup_uds_filename() in r1918647, it takes only
the request now and those who want the new url can use r->filename + 6
(like ap_proxy_pre_request() does).
>
> > +rc = ap_proxy_fixup_uds_filename(r, &uri);
> > +if (rc <= OK) {
> > rc = proxy_fixup(r);
> > }
> > if (rc != OK) {
> > -return rc;
> > +r->filename = old_filename;
> > +r->proxyreq = 0;
> > }
> > -}
Regards;
Yann.
Re: [VOTE] Release httpd-2.4.60-rc1 as httpd-2.4.60
On Wed, Jun 26, 2024 at 12:02 AM Helmut K. C. Tessarek wrote: > > I tried with rc2, since rc3 is not available yet. > > Same issue. Should be fixed by https://github.com/apache/httpd/commit/6937b985ae112de0ad60f12a3cb522b608a4d501.diff Regards; Yann.
Re: svn commit: r1918606 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On Wed, Jun 26, 2024 at 2:02 AM Yann Ylavic wrote: > > On Tue, Jun 25, 2024 at 7:29 PM wrote: > > > > Author: covener > > Date: Tue Jun 25 17:29:06 2024 > > New Revision: 1918606 > > > > URL: http://svn.apache.org/viewvc?rev=1918606&view=rev > > Log: > > validate hostname > > Seems this broke something in the http2 tests. Will have a look tomorrow. Was in mod_proxy_http2 code, fixed in r1918627 supposedly.
Re: svn commit: r1918606 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On Tue, Jun 25, 2024 at 7:29 PM wrote: > > Author: covener > Date: Tue Jun 25 17:29:06 2024 > New Revision: 1918606 > > URL: http://svn.apache.org/viewvc?rev=1918606&view=rev > Log: > validate hostname Seems this broke something in the http2 tests. Will have a look tomorrow.
Re: [VOTE] Release httpd-2.4.60-rc1 as httpd-2.4.60
On Wed, Jun 26, 2024 at 1:17 AM Eric Covener wrote: > > On Tue, Jun 25, 2024 at 7:03 PM Yann Ylavic wrote: > > > > On Wed, Jun 26, 2024 at 12:37 AM Eric Covener wrote: > > > > > > > The attached might work, currently testing but sending early if you > > > > want to try too. > > > > > > looks like proxy: is stripped off after the new call and needs to be > > > added back in? > > > > Yeah, the new call to ap_proxy_fixup_uds_filename() should not take > > &r->filename as argument. > > Fixed in this new version. > > +1 with basic "fakefpm" and real fpm hello-world. Please commit to > trunk when practical. Done in r1918626. Looks good in my manual testing too, there will be two calls to ap_proxy_fixup_uds_filename() when proxying from r->handler, first in proxy_handler() and second in ap_proxy_pre_request(), but since the first will strip the UDS already the other call is a noop. We can eventually rework this after the release. > > If we can get another review for 2.4.x I can add it and roll an rc so > it can be more easily picked up for testing. +1 Regards; Yann.
Re: [VOTE] Release httpd-2.4.60-rc1 as httpd-2.4.60
On Wed, Jun 26, 2024 at 12:37 AM Eric Covener wrote:
>
> > The attached might work, currently testing but sending early if you
> > want to try too.
>
> looks like proxy: is stripped off after the new call and needs to be
> added back in?
Yeah, the new call to ap_proxy_fixup_uds_filename() should not take
&r->filename as argument.
Fixed in this new version.
Index: include/ap_mmn.h
===
--- include/ap_mmn.h (revision 1918625)
+++ include/ap_mmn.h (working copy)
@@ -601,6 +601,7 @@
* 20120211.131 (2.4.59-dev) Add DAV_WALKTYPE_TOLERANT
* 20120211.131 (2.4.60-dev) Add ap_set_content_type_ex(), ap_filepath_merge(),
* and AP_REQUEST_TRUSTED_CT BNOTE.
+ * 20120211.133 (2.4.60-dev) Add ap_proxy_fixup_uds_filename()
*/
#define MODULE_MAGIC_COOKIE 0x41503234UL /* "AP24" */
@@ -608,7 +609,7 @@
#ifndef MODULE_MAGIC_NUMBER_MAJOR
#define MODULE_MAGIC_NUMBER_MAJOR 20120211
#endif
-#define MODULE_MAGIC_NUMBER_MINOR 131 /* 0...n */
+#define MODULE_MAGIC_NUMBER_MINOR 133 /* 0...n */
/**
* Determine if the server's current MODULE_MAGIC_NUMBER is at least a
Index: modules/proxy/mod_proxy.c
===
--- modules/proxy/mod_proxy.c (revision 1918625)
+++ modules/proxy/mod_proxy.c (working copy)
@@ -1298,7 +1298,7 @@ static int proxy_handler(request_rec *r)
ap_get_module_config(sconf, &proxy_module);
apr_array_header_t *proxies = conf->proxies;
struct proxy_remote *ents = (struct proxy_remote *) proxies->elts;
-int i, rc, access_status;
+int rc = DECLINED, access_status, i;
int direct_connect = 0;
const char *str;
apr_int64_t maxfwd;
@@ -1314,22 +1314,33 @@ static int proxy_handler(request_rec *r)
}
if (!r->proxyreq) {
-rc = DECLINED;
/* We may have forced the proxy handler via config or .htaccess */
if (r->handler &&
strncmp(r->handler, "proxy:", 6) == 0 &&
strncmp(r->filename, "proxy:", 6) != 0) {
+char *old_filename = r->filename;
+
r->proxyreq = PROXYREQ_REVERSE;
r->filename = apr_pstrcat(r->pool, r->handler, r->filename, NULL);
+
/* Still need to fixup/canonicalize r->filename */
-rc = proxy_fixup(r);
+uri = r->filename + 6;
+rc = ap_proxy_fixup_uds_filename(r, &uri);
+if (rc <= OK) {
+rc = proxy_fixup(r);
+}
+if (rc != OK) {
+r->filename = old_filename;
+r->proxyreq = 0;
+}
}
-if (rc != OK) {
-return rc;
-}
-} else if (strncmp(r->filename, "proxy:", 6) != 0) {
-return DECLINED;
}
+else if (strncmp(r->filename, "proxy:", 6) == 0) {
+rc = OK;
+}
+if (rc != OK) {
+return rc;
+}
/* handle max-forwards / OPTIONS / TRACE */
if ((str = apr_table_get(r->headers_in, "Max-Forwards"))) {
Index: modules/proxy/mod_proxy.h
===
--- modules/proxy/mod_proxy.h (revision 1918625)
+++ modules/proxy/mod_proxy.h (working copy)
@@ -1003,6 +1003,16 @@ PROXY_DECLARE(proxy_balancer_shared *) ap_proxy_fi
proxy_balancer *balancer,
unsigned int *index);
+/*
+ * In the case of the reverse proxy, we need to see if we
+ * were passed a UDS url (eg: from mod_proxy) and adjust uds_path
+ * as required.
+ * @param rcurrent request
+ * @param url request url to be fixed
+ * @return OK if fixed up, DECLINED if not UDS, or an HTTP_XXX error
+ */
+PROXY_DECLARE(int) ap_proxy_fixup_uds_filename(request_rec *r, char **url);
+
/**
* Get the most suitable worker and/or balancer for the request
* @param worker worker used for processing request
Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c (revision 1918625)
+++ modules/proxy/proxy_util.c (working copy)
@@ -2429,7 +2429,7 @@ static int ap_proxy_retry_worker(const char *proxy
* were passed a UDS url (eg: from mod_proxy) and adjust uds_path
* as required.
*/
-static int fix_uds_filename(request_rec *r, char **url)
+PROXY_DECLARE(int) ap_proxy_fixup_uds_filename(request_rec *r, char **url)
{
char *uds_url = r->filename + 6, *origin_url;
@@ -2452,7 +2452,7 @@ static int ap_proxy_retry_worker(const char *proxy
if (!uds_path) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10292)
"Invalid proxy UDS filename (%s)", r->filename);
-return 0;
+return HTTP_BAD_REQUEST;
}
apr_table_setn(r->notes, "uds_path", uds_path);
@@ -2464,8 +2464,1
Re: [VOTE] Release httpd-2.4.60-rc1 as httpd-2.4.60
On Tue, Jun 25, 2024 at 11:35 PM Eric Covener wrote:
>
> On Tue, Jun 25, 2024 at 5:22 PM Eric Covener wrote:
> >
> > On Tue, Jun 25, 2024 at 5:06 PM Eric Covener wrote:
> > >
> > > On Tue, Jun 25, 2024 at 4:35 PM Helmut K. C. Tessarek
> > > wrote:
> > > >
> > > > On 2024-06-25 02:53, Ruediger Pluem wrote:
> > > > > Can you provide more details on your configuration how you forward
> > > > > stuff to fcgi and what request you made?
> > > >
> > > > Very simple:
> > > >
> > > >
> > > > SetHandler "proxy:unix:/run/php82-fpm/xxx.sock|fcgi://xxx"
> > > >
> > >
> > > I think this new path needs to account for UDS. The URL is e.g.
> > > "unix:/tmp/fake.sock|fcgi://xxx..." during proxy_fcgi_canon so it
> > > declines, then the core handler reports the error on the
> > > pseudo-filename.
> >
> > Or for now, skip the newly added proxy_fixup() and escape just
> > r->filename before it gets the handler prefixed to it?
>
> These all seem hairy. We can do something more targetted from the
> start to decrease the fallout?
The attached might work, currently testing but sending early if you
want to try too.
Index: include/ap_mmn.h
===
--- include/ap_mmn.h (revision 1918625)
+++ include/ap_mmn.h (working copy)
@@ -601,6 +601,7 @@
* 20120211.131 (2.4.59-dev) Add DAV_WALKTYPE_TOLERANT
* 20120211.131 (2.4.60-dev) Add ap_set_content_type_ex(), ap_filepath_merge(),
* and AP_REQUEST_TRUSTED_CT BNOTE.
+ * 20120211.133 (2.4.60-dev) Add ap_proxy_fixup_uds_filename()
*/
#define MODULE_MAGIC_COOKIE 0x41503234UL /* "AP24" */
@@ -608,7 +609,7 @@
#ifndef MODULE_MAGIC_NUMBER_MAJOR
#define MODULE_MAGIC_NUMBER_MAJOR 20120211
#endif
-#define MODULE_MAGIC_NUMBER_MINOR 131 /* 0...n */
+#define MODULE_MAGIC_NUMBER_MINOR 133 /* 0...n */
/**
* Determine if the server's current MODULE_MAGIC_NUMBER is at least a
Index: modules/proxy/mod_proxy.c
===
--- modules/proxy/mod_proxy.c (revision 1918625)
+++ modules/proxy/mod_proxy.c (working copy)
@@ -1298,7 +1298,7 @@ static int proxy_handler(request_rec *r)
ap_get_module_config(sconf, &proxy_module);
apr_array_header_t *proxies = conf->proxies;
struct proxy_remote *ents = (struct proxy_remote *) proxies->elts;
-int i, rc, access_status;
+int rc = DECLINED, access_status, i;
int direct_connect = 0;
const char *str;
apr_int64_t maxfwd;
@@ -1314,22 +1314,30 @@ static int proxy_handler(request_rec *r)
}
if (!r->proxyreq) {
-rc = DECLINED;
/* We may have forced the proxy handler via config or .htaccess */
if (r->handler &&
strncmp(r->handler, "proxy:", 6) == 0 &&
strncmp(r->filename, "proxy:", 6) != 0) {
+char *old_filename = r->filename;
+/* Still need to fixup/canonicalize r->filename */
r->proxyreq = PROXYREQ_REVERSE;
r->filename = apr_pstrcat(r->pool, r->handler, r->filename, NULL);
-/* Still need to fixup/canonicalize r->filename */
-rc = proxy_fixup(r);
+rc = ap_proxy_fixup_uds_filename(r, &r->filename);
+if (rc <= OK) {
+rc = proxy_fixup(r);
+}
+if (rc != OK) {
+r->filename = old_filename;
+r->proxyreq = 0;
+}
}
-if (rc != OK) {
-return rc;
-}
-} else if (strncmp(r->filename, "proxy:", 6) != 0) {
-return DECLINED;
}
+else if (strncmp(r->filename, "proxy:", 6) == 0) {
+rc = OK;
+}
+if (rc != OK) {
+return rc;
+}
/* handle max-forwards / OPTIONS / TRACE */
if ((str = apr_table_get(r->headers_in, "Max-Forwards"))) {
Index: modules/proxy/mod_proxy.h
===
--- modules/proxy/mod_proxy.h (revision 1918625)
+++ modules/proxy/mod_proxy.h (working copy)
@@ -1003,6 +1003,16 @@ PROXY_DECLARE(proxy_balancer_shared *) ap_proxy_fi
proxy_balancer *balancer,
unsigned int *index);
+/*
+ * In the case of the reverse proxy, we need to see if we
+ * were passed a UDS url (eg: from mod_proxy) and adjust uds_path
+ * as required.
+ * @param rcurrent request
+ * @param url request url to be fixed
+ * @return OK if fixed up, DECLINED if not UDS, or an HTTP_XXX error
+ */
+PROXY_DECLARE(int) ap_proxy_fixup_uds_filename(request_rec *r, char **url);
+
/**
* Get the most suitable worker and/or balancer for the request
* @param worker worker used for processing request
Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c (revision 191862
Re: [VOTE] Release httpd-2.4.60-rc1 as httpd-2.4.60
On Tue, Jun 25, 2024 at 2:50 PM Eric Covener wrote: > > - whether backing out > https://github.com/apache/httpd/commit/9494aa8d52e3c263bc0413b77ac8a73b0d524388.diff > from the candidate helps? I'm not sure r1918553 made it to rc1, did it?
Re: Planning to RM around June 21
On Wed, Jun 5, 2024 at 5:24 PM Eric Covener wrote: > > Please make sure your backport proposals/votes are in. Fix for addressTTL (PR 454) is missing one vote, would be nice to fix it in 2.4.60. Thanks!
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On Wed, Jun 19, 2024 at 4:34 PM Yann Ylavic wrote: > > On Wed, Jun 19, 2024 at 4:23 PM Joe Orton wrote: > > > > On Wed, Jun 19, 2024 at 03:41:06PM +0200, Yann Ylavic wrote: > > > Done in PR 454, will update STATUS too. > > > > In the path where keep_addr_alive and keep_conn_alive are both false, > > conn_cleanup(conn) is run, which sets conn->address = NULL, and then > > running proxy_address_cleanup() leads to a NULL pointer deference in the > > following apr_pool_cleanup_run() invocation. > > > > https://github.com/apache/httpd/blob/6990550415e0801c8e73fa961036888ba1907fc1/modules/proxy/proxy_util.c#L3063 > > Argh my bad, last minute change, I should have left the conn_cleanup() > after the apr_pool_cleanup_run() as it was before. > Will fix! Done (r1918442) and merged in the backport proposal. > > Thanks; > Yann.
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On Wed, Jun 19, 2024 at 4:23 PM Joe Orton wrote: > > On Wed, Jun 19, 2024 at 03:41:06PM +0200, Yann Ylavic wrote: > > Done in PR 454, will update STATUS too. > > In the path where keep_addr_alive and keep_conn_alive are both false, > conn_cleanup(conn) is run, which sets conn->address = NULL, and then > running proxy_address_cleanup() leads to a NULL pointer deference in the > following apr_pool_cleanup_run() invocation. > > https://github.com/apache/httpd/blob/6990550415e0801c8e73fa961036888ba1907fc1/modules/proxy/proxy_util.c#L3063 Argh my bad, last minute change, I should have left the conn_cleanup() after the apr_pool_cleanup_run() as it was before. Will fix! Thanks; Yann.
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On Wed, Jun 19, 2024 at 2:52 PM Joe Orton wrote: > > On Wed, Jun 19, 2024 at 02:04:20PM +0200, Yann Ylavic wrote: > > On Wed, Jun 19, 2024 at 1:00 PM Ruediger Pluem wrote: > > > As far as I read the code it does not. > > > > > > https://github.com/apache/apr/blob/b0a08c76ceacc2308d7cf1d5a7bb3c9b4665a432/network_io/unix/sockets.c#L423-L433 > > > > > > We copy the data (sa, salen family and port) not a pointer. > > > > Ah yes, I was looking at win32 code, while Joe fixed it 13 years ago > > for unix (r983618). > > So the pointer copy exists, but only for WIN32 and OS/2 AFAICT, what a mess. > > Sorry! How so, that's a fix :) > > > Let me fix that then ;) > > There is a test in that commit, is it not catching the bug on non-Unix? > > BTW can you add r1918228 to your backport proposal, the debug output is > corrupted for this code otherwise. Done in PR 454, will update STATUS too. Regards; Yann.
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On Wed, Jun 19, 2024 at 2:49 PM Ruediger Pluem wrote: > > On 6/19/24 2:04 PM, Yann Ylavic wrote: > > > On Wed, Jun 19, 2024 at 1:00 PM Ruediger Pluem wrote: > >> > >> As far as I read the code it does not. > >> > >> https://github.com/apache/apr/blob/b0a08c76ceacc2308d7cf1d5a7bb3c9b4665a432/network_io/unix/sockets.c#L423-L433 > >> > >> We copy the data (sa, salen family and port) not a pointer. > > > > Ah yes, I was looking at win32 code, while Joe fixed it 13 years ago > > for unix (r983618). > > And I was only looking at Unix :-) I usually grep for ") apr_socket_connect" to get to the implementation of an APR function, but somehow the apr_socket ones on unix are not APR_DECLARE()d so it confused me this time :) > > > So the pointer copy exists, but only for WIN32 and OS/2 AFAICT, what a mess. > > Indeed. What a mess. > > > > Let me fix that then ;) > > Thanks. I guess these changes are backportable in APR, but how do we deal > with this in httpd? > Do we only follow the more complex code approach that you designed in case of > WIN32 and an old version of APR > and do a simpler approach on Unix and newer APR? Yeah, for now I only handled it in httpd with an #ifdef-ery (r1918438). The socket can continue to live with its copy of the addr on unix, so the workaround is now limited to WIN32/OS2. Thanks for the review! Yann.
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On Wed, Jun 19, 2024 at 1:00 PM Ruediger Pluem wrote:
>
> On 6/19/24 12:00 PM, Yann Ylavic wrote:
> > On Wed, Jun 19, 2024 at 9:07 AM Ruediger Pluem wrote:
> >>
> >> On 6/18/24 4:20 PM, yla...@apache.org wrote:
> >>>
> >>> +/* Release the old conn address */
> >>> +if (conn->address) {
> >>> +/* conn->address->addr cannot be released while it's
> >>> used by
> >>> + * conn->sock->remote_addr, thus if the old address is
> >>> still
> >>> + * the one used at apr_socket_connect() time AND the
> >>> socket
> >>> + * shouldn't be closed because the newly resolved
> >>> address is
> >>> + * the same. In this case let's (re)attach the old
> >>> address to
> >>> + * the socket lifetime (scpool), in any other case just
> >>> release
> >>> + * it now.
> >>> + */
> >>> +int addr_alive = 0,
> >>> +conn_alive = (conn->sock && conn->addr &&
> >>> + proxy_addrs_equal(conn->addr,
> >>> address->addr));
> >>> +if (conn_alive) {
> >>> +apr_sockaddr_t *remote_addr = NULL;
> >>> +apr_socket_addr_get(&remote_addr, APR_REMOTE,
> >>> conn->sock);
> >>> +addr_alive = (conn->addr == remote_addr);
> >>
> >> How can this ever be true? The remote_addr is always allocated from
> >> conn->scpool. How can
> >> conn->addr ever be allocated from conn->scpool? I think it always gets
> >> allocated from
> >> either a subpool of worker->cp->dns_pool, from conn->pool or from
> >> conn->uds_pool.
> >>
> >>> +}
> >
> > All the complications[1] in this commit come from the fact that
> > apr_socket_connect(sock, addr) does a simple pointer copy of addr into
> > sock->remote_addr, which apr_socket_addr_get(APR_REMOTE) will return.
>
> As far as I read the code it does not.
>
> https://github.com/apache/apr/blob/b0a08c76ceacc2308d7cf1d5a7bb3c9b4665a432/network_io/unix/sockets.c#L423-L433
>
> We copy the data (sa, salen family and port) not a pointer.
Ah yes, I was looking at win32 code, while Joe fixed it 13 years ago
for unix (r983618).
So the pointer copy exists, but only for WIN32 and OS/2 AFAICT, what a mess.
Let me fix that then ;)
Thanks;
Yann.
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On Wed, Jun 19, 2024 at 9:07 AM Ruediger Pluem wrote:
>
> On 6/18/24 4:20 PM, yla...@apache.org wrote:
> >
> > +/* Release the old conn address */
> > +if (conn->address) {
> > +/* conn->address->addr cannot be released while it's used
> > by
> > + * conn->sock->remote_addr, thus if the old address is
> > still
> > + * the one used at apr_socket_connect() time AND the socket
> > + * shouldn't be closed because the newly resolved address
> > is
> > + * the same. In this case let's (re)attach the old address
> > to
> > + * the socket lifetime (scpool), in any other case just
> > release
> > + * it now.
> > + */
> > +int addr_alive = 0,
> > +conn_alive = (conn->sock && conn->addr &&
> > + proxy_addrs_equal(conn->addr,
> > address->addr));
> > +if (conn_alive) {
> > +apr_sockaddr_t *remote_addr = NULL;
> > +apr_socket_addr_get(&remote_addr, APR_REMOTE,
> > conn->sock);
> > +addr_alive = (conn->addr == remote_addr);
>
> How can this ever be true? The remote_addr is always allocated from
> conn->scpool. How can
> conn->addr ever be allocated from conn->scpool? I think it always gets
> allocated from
> either a subpool of worker->cp->dns_pool, from conn->pool or from
> conn->uds_pool.
>
> > +}
All the complications[1] in this commit come from the fact that
apr_socket_connect(sock, addr) does a simple pointer copy of addr into
sock->remote_addr, which apr_socket_addr_get(APR_REMOTE) will return.
And in ap_proxy_connect_backend() we indeed do the same[2] as
apr_socket_connect(conn->sock, conn->addr).
So I think it does not matter which pool conn->addr was allocated
from[3] when apr_socket_connect() does a pointer copy?
[1] When a new conn->[address->]addr is determined and it's the same
address as the old one and we want to keep the socket open, we cannot
simply release the old conn->address without leaving a dangling
pointer in sock->remote_addr. That's why conn->[address->]addr is
reattached to conn->scpool to be cleaned up only when the socket is
closed.
[2] Well, not really the same because we'll walk all the
conn->addr[->next] to find one that works, so the conn->addr ==
remote_addr test should actually be a loop to find one of the
conn->addr[->next]. Will fix this!
[3] From worker_address_resolve() it's actually a subpool of
worker->cp->dns_pool since worker->s->is_address_reusable here.
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
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
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
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&view=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,
> &keepalive);
>
> 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
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
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
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
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
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
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&view=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&r1=1916924&r2=1916925&view=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(&event_pollset, pollset_size, pruntime,
> > +rv = apr_pollset_create_ex(&event_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
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
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
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(&worker_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(&worker_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(&worker_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(&worker_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_threa
Re: [VOTE] Release httpd-2.4.59-rc1 as httpd-2.4.59
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
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
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
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(&threads[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
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
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
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&view=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&r1=1916242&r2=1916243&view=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
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
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
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
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&view=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
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&view=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
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&view=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
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&r1=1913814&r2=1913815&view=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
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
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
+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
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
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
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
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_
Re: process_regexp bug, infinite recursion
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
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
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&view=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
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&view=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
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
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
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
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
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&view=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&r1=1914364&r2=1914365&view=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
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&view=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&r1=1914364&r2=1914365&view=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.
