Re: age in proxy_balancer_method
On 1/2/24 14:47, Jim Jagielski wrote: Yeah, that sounds about right. I'd say that whatever changes need (or should be) made are fine Thanks I will keep experimenting ;-) On Dec 21, 2023, at 1:32 PM, Rainer Jung wrote: I guess it could be like this: when Mladen originally implemented the by requests load balancing method in mod_jk he used the count and subtract method for the counters. He then ported this to mod_proxy_balancer and I think it is still, how by requests counting woorks there. There are pros and cons, e.g. in case a worker goes down for some time. A bit later we switched in mod_jk to a count and divide, where division by 2 was done roughly every 60 seconds (configurable). I think the idea of the age method was roughly, that you could implement a balanvcer method, that registers a mod_watchdog task, that regularly ages the balancing counters. Aging because you want to give the past a smaller influence on the balancing decision than the more recent activity. I hope that's understandable and maybe Jim remembers something similar to that. Best regards, Rainer Am 21.12.23 um 08:23 schrieb jean-frederic clere: On 12/20/23 21:22, Jim Jagielski wrote: I'll have to go back through my notes... I do recall adding fields that although were not being used at the time, were _going to be used_ as some point, and I didn't want to have to worry about ABI compatibility. Cool I will wait before implementing something that breaks your design ;-) On Dec 14, 2023, at 8:27 AM, jean-frederic clere wrote: Hi, Any examples or docs about: apr_status_t (*age)(proxy_balancer *balancer, server_rec *s); In struct proxy_balancer_method? -- Cheers Jean-Frederic -- Cheers Jean-Frederic
Re: process_regexp bug, infinite recursion
On 1/8/24 1:37 PM, Yann Ylavic wrote:
> 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..
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.
Regards
Rüdiger
Re: Module file access problem
On 12/25/23 9:55 AM, MIIM via dev wrote:
>
> I apologize in advance for posting this question to the development
> forum, but unfortunately the modules-development list is dead
> according to the list server.
>
>
> I have a module attempting to write a log file --
> not one of the Apache log files -- for its own use.
>
> I adopted the code from a similar module which uses
> it to open-for-read a whitelist file in the default
> configuration directory; it works in that module.
>
> When opening for create-or-append a permissions
> failure occurs. The module reports the error as follows:
>
>
> Dec 24 22:11:58 mod_botlist: logfile
> Dec 24 22:11:58 mod_botlist: Log file
> Dec 24 22:11:58 mod_botlist: Error opening log file
>
>
>
> Protection for the /var/log/apache directory is the default.
> This is the directory where Apache keeps its log files:
>
>
> /var/log ...
> drwxr-xr-x 2 root root 4096 Dec 24 22:11 apache
>
>
> The code for the file open is below. The module gets
> through the stat checking section but fails to open
> the (nonexistent at the first request) file for append.
>
>
> /* Stat a possibly existing file in case there's a problem */
>
> errno = 0;
> logstat = stat(logfilename, );
> if ( (logstat < 0) && (errno != ENOENT) ) {
> filerr = errno;
> ap_log_rerror(APLOG_MARK, APLOG_NOTICE, 0, r,
> "mod_botlist: Error on log file <%s>",
> logfilename);
> ap_log_rerror(APLOG_MARK, APLOG_NOTICE, 0, r,
> " %s",
> strerror(filerr));
> bl_unlock_mutex(r, bl_scfg);
> return DECLINED;
> }
>
> /* Open the file for append, or if none, create it */
>
> errno = 0;
> logfile = fopen(logfilename, "a+");
> if (errno != 0) {
> filerr = errno;
> ap_log_rerror(APLOG_MARK, APLOG_NOTICE, 0, r,
> "mod_botlist: Error opening log file <%s>",
> logfilename);
> ap_log_rerror(APLOG_MARK, APLOG_NOTICE, 0, r,
> " %s",
> strerror(filerr));
> bl_unlock_mutex(r, bl_scfg);
> return DECLINED;
> }
>
>
> 1) Is this problem due to the core giving up its privilege
> after the individual servers are started? Clearly some
> part of the server had enough privilege to create those
> files.
It depends in which phase you try to open the logfile whether the privileges
have already been dropped.
I cannot tell this from the code snippets above. Apart from this it is
advisable to use the respective
functions from the APR library instead of the POSIX / stdio.h functions above
as this keeps the module
much more portable.
Another test that you can do is to write your logfile to a directory where
everyone can write (e.g. /tmp)
Then you can see which user owns the file and if it is the user specified via
the User directive.
Regards
Rüdiger
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_regexp(header_entry *hdr, const char *value,
- request_rec *r)
+
+static const char *process_regexp_core(header_entry *hdr,
+ const char *val, apr_size_t val_len,
+ 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 *sub, *rem;
+apr_size_t
Re: process_regexp bug, infinite recursion
On 1/5/24 3:08 PM, Yann Ylavic wrote:
> 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.
>
>
> 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?
> +}
> +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 ?
> + 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;
> }
Regards
Rüdiger
