Re: "Forbid" directive in core?
> On 27 Apr 2020, at 16:37, Eric Covener wrote: > > > Bumping a very old thread. tl;dr people are often surprised that when > Location sections have access control directives and overlap with the > filesystem it undoes the default > >Require all denied > We always warn against mixing with filesystem. That's just one of many gotchas that may arise from it. I wonder if a better solution would be to be much stricter about the division? Ideally make it a config error (switchable to a warning so as not to break too many old configs) to overlap them? A first-pass implementation might be for the Location directive to issue a warning if it matches a directory in the filesystem. -- Nick Kew
Re: "Forbid" directive in core?
On Mon, Apr 27, 2020 at 12:14 PM Yann Ylavic wrote: > > On Mon, Apr 27, 2020 at 5:37 PM Eric Covener wrote: > > > > Bumping a very old thread. tl;dr people are often surprised that when > > Location sections have access control directives and overlap with the > > filesystem it undoes the default > > > > Require all denied > > > > Thanks for pointing at this, I was wondering what baptx was talking > about on users@. TIL... > > What about upper sf proposal to default AuthMerging to "and"? > Would that be too disruptive? Or at least, if this is a direction, > could we do that in our default httpd.conf (and docs)? AuthMerging in default conf for at least the .ht* case seems to make sense. Haven't tested, never occurred to me before. Maybe because it was not analagous for 2.2 back then. I don't think we can change the compiled-in default in 2.4. Maybe in trunk?
Re: Move FILE buckets on setaside (like/as a morphing bucket)?
On Mon, Apr 27, 2020 at 04:27:56PM +0200, Yann Ylavic wrote: > On Mon, Apr 27, 2020 at 4:11 PM Joe Orton wrote: > > > > +1 from me for using the term "opaque buckets" as a synonym for > > "e->length == (apr_size_t)-1" aka "buckets with indeterminate length". > > OK thanks, just committed "something" in r1877077 because I keep > messing up with my attachments today. > Let's discuss from "that" if needed.. That change is definitely better. Isn't the code still making the assumption that FILE is the only possible non-opaque morphing bucket type? http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_filter.c?revision=1877077&view=markup#l1108 ...and by "non-opaque morphing bucket type" the distinction intended here is... "representing a determinate length bucket type which is not fully mapped into RAM until you try read()ing it until exhaustion". (!)
Re: "Forbid" directive in core?
On Mon, Apr 27, 2020 at 5:37 PM Eric Covener wrote: > > Bumping a very old thread. tl;dr people are often surprised that when > Location sections have access control directives and overlap with the > filesystem it undoes the default > > Require all denied > Thanks for pointing at this, I was wondering what baptx was talking about on users@. TIL... What about upper sf proposal to default AuthMerging to "and"? Would that be too disruptive? Or at least, if this is a direction, could we do that in our default httpd.conf (and docs)?
Re: Providing near-time averaged monitoring data for mod_systemd and mod_status
Am 27.04.2020 um 15:57 schrieb Joe Orton: On Sat, Apr 25, 2020 at 08:10:40PM +0200, Rainer Jung wrote: Patch available at home.apache.org/~rjung/patches/httpd-trunk-mon-snaps-v1_2.patch Very nice! +1 from me. Does the times_per_thread logic still make any sense? It's always been wrong for Linux AFAICT so maybe can just be dropped. I will do some code archeology to find out who added it and why. A minor nit, I think the snap_index should be read/written using apr_atomic_t since it's going to be accessed concurrently across threads? Yup. Since we only have concurrent reads, it should suffice to check the value with apr_atomic_read32 and then use apr_atomic_set32 to set it the 0 or 1. I wonder how we best make sure, that the new monitor hook is not called from modules? Currently it is no AP_DECLARE'd but contained in httpd.h. And the hook registration is in server/core.c, but the implementation in server/scoreboard.c. No idea (not tried) whether it would be better to have it in server/core.c as a static function or whether that's not needed and it would suffice to move the function declaration to a private header file. Thanks and regards, Rainer Some notes: - in order to use the data from mod_systemd (monitor hook), which runs in the maim process, and also from mod_status, so from child processes, it needs to sit in shared memory. Since most of the data is tightly coupled to the scoreboard, I added the new cached data to the global part of the scoreboard. - it was then IMHO a good fit to move the existing ap_get_sload(ap_sload_t *ld) from server/util.c to server/scoreboard.c. - ap_sload_t is used as a collection of data which can be used to generate averaged data from a pair of ap_sload_t structs. It was extended to also contain the total request duration plus total usr and sys CPU and the timestamp when the data was taken. So it should now contain all data from which mod-systemd and mod_status currently like to display derived averaged values. - busy and idle in ap_sload_t have been changed from int to float, because they were already only used a percentages. IMHO it doesn't make sense to express idle and busy as percentages based on a total of busy plus idle, because that sum is not a meaningful total. The server can grow by creating new processes and suddenly your busy percentage might shrink, although the absolute number of busy threads increases. That's counter intuitive. So I added the unused process slots to the total count, and we have three counters that sum up to this total, busy, idle and dead. We need a better name than "dead" for these unused process slots that might get used later. "unused" is to close to "idle" and I don't have a good name yet. - the new ap_mon_snap_t contains a pointer to an ap_sload_t, six averaged values generated from two ap_sload_t and a member that conatins the time delta between those two ap_sload_t. The ap_sload_t referenced by ap_mon_snap_t contains the data at time t1. During the next monitor run, new t1 data will be collected and the previous t1 data will be used as t0 data to generate the new averages. - the scoreboard contains two ap_mon_snap_t (plus the two ap_sload_t referenced by them) to be able to update one of them without breaking access by consumers to the other one. After the update the roles get switched. - both structs, ap_sload_t and ap_mon_snap_t are declared in httpd.h, because ap_sload_t already had been there. t might be a better fit to move them to scoreboard.h, but I'm not sure about that and I don't know if that would be allowed by the compatibility rules. - also in httpd.h there are now three new function declarations. Two only used by server/core.c as hook functions: int ap_scoreboard_monitor(apr_pool_t *p, server_rec *s); void ap_scoreboard_child_init(apr_pool_t *p, server_rec *s); and one for public consumption: AP_DECLARE(void) ap_get_mon_snap(ap_mon_snap_t *ms); - mod_systemd and mod_status now call ap_get_mon_snap(&snap) to retrieve the latest averaged data. mod_status still uses the scoreboard in addition to retrieve up-to-date current scalar metrics. Small adjustments to the mod_status output plus additions to mod_systemd notification messages. Tne STATUS in the notification of mod_systemd now looks liek this: STATUS=Pct Idle workers 28.4; Pct Busy workers 10.6; Pct Dead workers 60.9; Requests/sec: 5030; Bytes served/sec: 2.9MB/sec; Bytes served/request: 596 B/req; Avg Duration(ms): 5.78; Avg Concurrency: 29.1; Cpu Pct: 40.5 - scoreboard.c changes: - take over ap_get_sload(ap_sload_t *sl) from server/util.c. Added copied code from mod_status to that function to also add up the request duration and the usr and sys CPU times. - ap_scoreboard_monitor() runs in the monitor hook. It calls ap_get_sload() and then a static utility function calc_mon_data() to calculate the new averages. - Minor mmn change not yet part of the patch. It compiles fine (maintainer mode) on RHEL 7 x8
Re: Providing near-time averaged monitoring data for mod_systemd and mod_status
Am 27.04.2020 um 17:28 schrieb Yann Ylavic: On Mon, Apr 27, 2020 at 5:18 PM Rainer Jung wrote: Hi Yann, Am 27.04.2020 um 16:40 schrieb Yann Ylavic: Hi Rainer, On Mon, Apr 27, 2020 at 3:17 PM Rainer Jung wrote: Thanks for this. Could you please create this as a PR on github as well? This ensures that all the Travis tests are run for your patch. Thanks Rüdiger. Done and indeed Travis found one not that I fixed but we need to discuss. Thanks Rainer, looks very nice. Could we avoid the "ap_sload_t *sload" pointer in ap_mon_snap_t somehow? I'm asking because snap0 and snap1 in global_score (SHM) end up containing a pointer now. My initial version had the fields of ap_sload_t directly inside ap_mon_snap_t instead of the pointer. I changed it, because - it allowed to more safely copy the whole ap_sload_t without relying on keeping two struct definitions in sync - it allowed to extend ap_sload_t and ap_mon_snap_t at their respective ends and stay compatible. You have a problem with the pointers because of possible alignment issues or what kind of trouble might we run into? Or is it just hard to understand (and more comments in the code will help)? My concern is more about security, e.g. CVE-2019-0211 kind of things, with an unprivileged user (child) playing with this pointer and making the main process (monitor?) at best crash, at worst do anything they want (root privilege escalation). I see. Although I don't see a way how we would execute code at the derefenced address, as a safety guard for future code changes we can easily avoid using the pointers in the scoreboard and instead use sload0 / snap0 and sload1 / snap1 when doing the calculations. Since the pointers (after init) would have been constant, we can hard code it into trivial logic instead. One could then use a separate type for snap0 and snap1 in the scoreboard, but I would find it better to stay with the ap_mon_snap_t type, even if we do not use the pointer there. For the struct passed in from the consumer, I would still use the pointer as-is. Will provide a change on top of the PR for further discussion. Thanks for the heads up. Didn't think about using the scoreboard and the monitor hook running in the parent havig security implications. One really needs to be careful about assumptions for the scoreboard data! Regards, Rainer
Re: "Forbid" directive in core?
On Mon, Apr 27, 2020 at 11:37 AM Eric Covener wrote: > On Sat, Sep 28, 2013 at 12:21 PM Tim Bannister > wrote: > > The second time in a few days, I'm going to suggest adding an optional > parameter to a directive. > > > > Taking a leaf out of cascading stylesheets, how about “Forbidden On > Level=Important” and perhaps “Forbidden On Level=Indelible”? > > > > (the idea being that the “Indelible” level can't be removed). > > > > > > This lets distributions ship a fairly safe default configuration but > gives users enough scope to hang themselves. With this, “forbidden OFF” > isn't so risky and “Forbidden Off Level=Important” can carry a health > warning (and perhaps an ErrorLog warning as well). > > > > > > Too complex or worth having? What do people think? If there's appetite > for it then I will have a go at providing a patch. > > What do currently active people think of the original basic "Forbid" > or the one with tags/levels? > Most CSS experts will tell you that "!important" is bad and if you are using it, you didn't design your site properly. As someone who does a lot of config support, I also think this is overly complicated. - Y
Re: "Forbid" directive in core?
On Sat, Sep 28, 2013 at 12:21 PM Tim Bannister wrote: > > On 28 Sep 2013, at 14:19, Eric Covener wrote: > > > I've come back to this because I've struggled in another area with > > access_checker vs. access_checker_ex. I really think we need basic access > > control outside of Require and Satisfy. > > > > I have a copy of the "Forbidden" directive in mod_authz_core and I am > > currrently allowing ON/OFF flags. > > > > * using a new directive means someone won't casually add "forbidden OFF" > > when they think they're turnong on more access control with Require > > * we can document that "forbidden OFF" is extreme from the start. > > > > I am on the fence about having an argument at all. My fear is that it will > > evolve into a misguided FAQ of 'try forbidden OFF if you get a 403' then > > we're right back to > > > > > > Forbidden > > > > > > ... > > > > > > ... > > Require ldap-group cn=foo > > Forbidden OFF > > > > The second time in a few days, I'm going to suggest adding an optional > parameter to a directive. > > Taking a leaf out of cascading stylesheets, how about “Forbidden On > Level=Important” and perhaps “Forbidden On Level=Indelible”? > > (the idea being that the “Indelible” level can't be removed). > > > This lets distributions ship a fairly safe default configuration but gives > users enough scope to hang themselves. With this, “forbidden OFF” isn't so > risky and “Forbidden Off Level=Important” can carry a health warning (and > perhaps an ErrorLog warning as well). > > > Too complex or worth having? What do people think? If there's appetite for it > then I will have a go at providing a patch. Bumping a very old thread. tl;dr people are often surprised that when Location sections have access control directives and overlap with the filesystem it undoes the default Require all denied What do currently active people think of the original basic "Forbid" or the one with tags/levels?
Re: Providing near-time averaged monitoring data for mod_systemd and mod_status
On Mon, Apr 27, 2020 at 5:18 PM Rainer Jung wrote: > > Hi Yann, > > Am 27.04.2020 um 16:40 schrieb Yann Ylavic: > > Hi Rainer, > > > > On Mon, Apr 27, 2020 at 3:17 PM Rainer Jung wrote: > >>> > >>> Thanks for this. > >>> Could you please create this as a PR on github as well? This ensures that > >>> all the Travis tests are run for your patch. > >> > >> Thanks Rüdiger. Done and indeed Travis found one not that I fixed but we > >> need to discuss. > > > > Thanks Rainer, looks very nice. > > > > Could we avoid the "ap_sload_t *sload" pointer in ap_mon_snap_t somehow? > > I'm asking because snap0 and snap1 in global_score (SHM) end up > > containing a pointer now. > > My initial version had the fields of ap_sload_t directly inside > ap_mon_snap_t instead of the pointer. > > I changed it, because > > - it allowed to more safely copy the whole ap_sload_t without relying on > keeping two struct definitions in sync > > - it allowed to extend ap_sload_t and ap_mon_snap_t at their respective > ends and stay compatible. > > You have a problem with the pointers because of possible alignment > issues or what kind of trouble might we run into? Or is it just hard to > understand (and more comments in the code will help)? My concern is more about security, e.g. CVE-2019-0211 kind of things, with an unprivileged user (child) playing with this pointer and making the main process (monitor?) at best crash, at worst do anything they want (root privilege escalation). Regards, Yann.
Re: Providing near-time averaged monitoring data for mod_systemd and mod_status
Hi Yann, Am 27.04.2020 um 16:40 schrieb Yann Ylavic: Hi Rainer, On Mon, Apr 27, 2020 at 3:17 PM Rainer Jung wrote: Thanks for this. Could you please create this as a PR on github as well? This ensures that all the Travis tests are run for your patch. Thanks Rüdiger. Done and indeed Travis found one not that I fixed but we need to discuss. Thanks Rainer, looks very nice. Could we avoid the "ap_sload_t *sload" pointer in ap_mon_snap_t somehow? I'm asking because snap0 and snap1 in global_score (SHM) end up containing a pointer now. My initial version had the fields of ap_sload_t directly inside ap_mon_snap_t instead of the pointer. I changed it, because - it allowed to more safely copy the whole ap_sload_t without relying on keeping two struct definitions in sync - it allowed to extend ap_sload_t and ap_mon_snap_t at their respective ends and stay compatible. You have a problem with the pointers because of possible alignment issues or what kind of trouble might we run into? Or is it just hard to understand (and more comments in the code will help)? Or because it is harder for consumers to use the API in a correct way (the pointer in ap_mon_snap_t might be uninitialized)? In the latter case: one could also see this as a feature instead of as a bug: as long as one is only interested in the averaged data and not in the sload data, you can pass in an ap_mon_snap_t with a null pointer to the ap_sload_t. Regards, Rainer
Re: Providing near-time averaged monitoring data for mod_systemd and mod_status
Hi Rainer, On Mon, Apr 27, 2020 at 3:17 PM Rainer Jung wrote: > > > > Thanks for this. > > Could you please create this as a PR on github as well? This ensures that > > all the Travis tests are run for your patch. > > Thanks Rüdiger. Done and indeed Travis found one not that I fixed but we > need to discuss. Thanks Rainer, looks very nice. Could we avoid the "ap_sload_t *sload" pointer in ap_mon_snap_t somehow? I'm asking because snap0 and snap1 in global_score (SHM) end up containing a pointer now. Regards, Yann.
Re: Move FILE buckets on setaside (like/as a morphing bucket)?
On Mon, Apr 27, 2020 at 4:11 PM Joe Orton wrote: > > +1 from me for using the term "opaque buckets" as a synonym for > "e->length == (apr_size_t)-1" aka "buckets with indeterminate length". OK thanks, just committed "something" in r1877077 because I keep messing up with my attachments today. Let's discuss from "that" if needed..
Re: Move FILE buckets on setaside (like/as a morphing bucket)?
On Mon, Apr 27, 2020 at 03:57:37PM +0200, Yann Ylavic wrote: > On Mon, Apr 27, 2020 at 3:43 PM Yann Ylavic wrote: > > > > On Mon, Apr 27, 2020 at 2:09 PM Yann Ylavic wrote: > > > > > > https://github.com/apache/httpd/pull/117 > > > > I closed it, how about the second patch there though? > > > > https://github.com/apache/httpd/pull/117/commits/cdf4c4a3c61340f061267cc70c456d9469e9faac > > Actually the attached one (with no mention of morphing since we are > not talking about FILE buckets anymore). > I have chosen the term "opaque" because it's concise (e.g. > opaque_buckets_in_brigade), but it's just in comments now (and > accompanied by "(length == -1)" to specify what it means). The goal > being to axe AP_BUCKET_IS_MORPHING() which I introduced recently.. +1 from me for using the term "opaque buckets" as a synonym for "e->length == (apr_size_t)-1" aka "buckets with indeterminate length". Regards, Joe
Re: Move FILE buckets on setaside (like/as a morphing bucket)?
On Mon, Apr 27, 2020 at 3:57 PM Yann Ylavic wrote:
>
> On Mon, Apr 27, 2020 at 3:43 PM Yann Ylavic wrote:
> >
> > On Mon, Apr 27, 2020 at 2:09 PM Yann Ylavic wrote:
> > >
> > > https://github.com/apache/httpd/pull/117
> >
> > I closed it, how about the second patch there though?
> >
> > https://github.com/apache/httpd/pull/117/commits/cdf4c4a3c61340f061267cc70c456d9469e9faac
>
> Actually the attached one (with no mention of morphing since we are
> not talking about FILE buckets anymore).
> I have chosen the term "opaque" because it's concise (e.g.
> opaque_buckets_in_brigade), but it's just in comments now (and
> accompanied by "(length == -1)" to specify what it means). The goal
> being to axe AP_BUCKET_IS_MORPHING() which I introduced recently..
Argh.. now attached.
Index: include/http_request.h
===
--- include/http_request.h (revision 1876933)
+++ include/http_request.h (working copy)
@@ -589,15 +589,6 @@ AP_DECLARE(int) ap_if_walk(request_rec *r);
AP_DECLARE_DATA extern const apr_bucket_type_t ap_bucket_type_eor;
/**
- * Determine if a bucket is morphing, that is which changes its
- * type on read (usually to "heap" allocated data), while moving
- * itself at the next position to remain plugged until exhausted.
- * @param e The bucket to inspect
- * @return true or false
- */
-#define AP_BUCKET_IS_MORPHING(e)((e)->length == (apr_size_t)-1)
-
-/**
* Determine if a bucket is an End Of REQUEST (EOR) bucket
* @param e The bucket to inspect
* @return true or false
Index: server/util_filter.c
===
--- server/util_filter.c (revision 1876933)
+++ server/util_filter.c (working copy)
@@ -976,11 +976,11 @@ AP_DECLARE(apr_status_t) ap_filter_setaside_brigad
e = next) {
next = APR_BUCKET_NEXT(e);
-/* Morphing buckets are moved, so assumed to have next EOR's
- * lifetime or at least the lifetime of the connection.
+/* Opaque buckets (length == -1) are moved, so assumed to have
+ * next EOR's lifetime or at least the lifetime of the connection.
*/
-if (AP_BUCKET_IS_MORPHING(e)) {
-/* Save buckets batched below? */
+if (e->length == (apr_size_t)-1 || APR_BUCKET_IS_FILE(e)) {
+/* First save buckets batched below, if any. */
if (batched_buckets) {
batched_buckets = 0;
if (!tmp_bb) {
@@ -1058,7 +1058,7 @@ AP_DECLARE(apr_status_t) ap_filter_reinstate_briga
{
apr_bucket *bucket, *next;
apr_size_t bytes_in_brigade, memory_bytes_in_brigade;
-int eor_buckets_in_brigade, morphing_buckets_in_brigade;
+int eor_buckets_in_brigade, opaque_buckets_in_brigade;
struct ap_filter_private *fp = f->priv;
core_server_config *conf;
@@ -1094,7 +1094,7 @@ AP_DECLARE(apr_status_t) ap_filter_reinstate_briga
* of everything up to the last one.
*
* b) The brigade contains at least flush_max_threshold bytes in memory,
- * that is non-file and non-morphing buckets: do blocking writes of
+ * that is non-opaque buckets (length != -1) : do blocking writes of
* everything up the last bucket above flush_max_threshold.
* (The point of this rule is to provide flow control, in case a
* handler is streaming out lots of data faster than the data can be
@@ -1105,9 +1105,9 @@ AP_DECLARE(apr_status_t) ap_filter_reinstate_briga
* (The point of this rule is to prevent too many FDs being kept open
* by pipelined requests, possibly allowing a DoS).
*
- * Note that morphing buckets use no memory until read, so they don't
+ * Note that opaque buckets use no memory until read, so they don't
* account for point b) above. Both ap_filter_reinstate_brigade() and
- * ap_filter_setaside_brigade() assume that morphing buckets have an
+ * ap_filter_setaside_brigade() assume that opaque buckets have an
* appropriate lifetime (until next EOR for instance), so they are simply
* setaside or reinstated by moving them from/to fp->bb to/from bb.
*/
@@ -1115,7 +1115,7 @@ AP_DECLARE(apr_status_t) ap_filter_reinstate_briga
bytes_in_brigade = 0;
memory_bytes_in_brigade = 0;
eor_buckets_in_brigade = 0;
-morphing_buckets_in_brigade = 0;
+opaque_buckets_in_brigade = 0;
conf = ap_get_core_module_config(f->c->base_server->module_config);
@@ -1126,8 +1126,8 @@ AP_DECLARE(apr_status_t) ap_filter_reinstate_briga
if (AP_BUCKET_IS_EOR(bucket)) {
eor_buckets_in_brigade++;
}
-else if (AP_BUCKET_IS_MORPHING(bucket)) {
-morphing_buckets_in_brigade++;
+else if (bucket->length == (apr_size_t)-1) {
+opaque_buckets_in_brigade++;
}
else if (bucket->length) {
bytes_
Re: Move FILE buckets on setaside (like/as a morphing bucket)?
On Mon, Apr 27, 2020 at 3:43 PM Yann Ylavic wrote: > > On Mon, Apr 27, 2020 at 2:09 PM Yann Ylavic wrote: > > > > https://github.com/apache/httpd/pull/117 > > I closed it, how about the second patch there though? > > https://github.com/apache/httpd/pull/117/commits/cdf4c4a3c61340f061267cc70c456d9469e9faac Actually the attached one (with no mention of morphing since we are not talking about FILE buckets anymore). I have chosen the term "opaque" because it's concise (e.g. opaque_buckets_in_brigade), but it's just in comments now (and accompanied by "(length == -1)" to specify what it means). The goal being to axe AP_BUCKET_IS_MORPHING() which I introduced recently..
Re: Providing near-time averaged monitoring data for mod_systemd and mod_status
On Sat, Apr 25, 2020 at 08:10:40PM +0200, Rainer Jung wrote: > Patch available at > > home.apache.org/~rjung/patches/httpd-trunk-mon-snaps-v1_2.patch Very nice! +1 from me. Does the times_per_thread logic still make any sense? It's always been wrong for Linux AFAICT so maybe can just be dropped. A minor nit, I think the snap_index should be read/written using apr_atomic_t since it's going to be accessed concurrently across threads? Regards, Joe > Some notes: > > - in order to use the data from mod_systemd (monitor hook), which runs in > the maim process, and also from mod_status, so from child processes, it > needs to sit in shared memory. Since most of the data is tightly coupled to > the scoreboard, I added the new cached data to the global part of the > scoreboard. > > - it was then IMHO a good fit to move the existing ap_get_sload(ap_sload_t > *ld) from server/util.c to server/scoreboard.c. > > - ap_sload_t is used as a collection of data which can be used to generate > averaged data from a pair of ap_sload_t structs. It was extended to also > contain the total request duration plus total usr and sys CPU and the > timestamp when the data was taken. So it should now contain all data from > which mod-systemd and mod_status currently like to display derived averaged > values. > > - busy and idle in ap_sload_t have been changed from int to float, because > they were already only used a percentages. IMHO it doesn't make sense to > express idle and busy as percentages based on a total of busy plus idle, > because that sum is not a meaningful total. The server can grow by creating > new processes and suddenly your busy percentage might shrink, although the > absolute number of busy threads increases. That's counter intuitive. So I > added the unused process slots to the total count, and we have three > counters that sum up to this total, busy, idle and dead. We need a better > name than "dead" for these unused process slots that might get used later. > "unused" is to close to "idle" and I don't have a good name yet. > > - the new ap_mon_snap_t contains a pointer to an ap_sload_t, six averaged > values generated from two ap_sload_t and a member that conatins the time > delta between those two ap_sload_t. The ap_sload_t referenced by > ap_mon_snap_t contains the data at time t1. During the next monitor run, new > t1 data will be collected and the previous t1 data will be used as t0 data > to generate the new averages. > > - the scoreboard contains two ap_mon_snap_t (plus the two ap_sload_t > referenced by them) to be able to update one of them without breaking access > by consumers to the other one. After the update the roles get switched. > > - both structs, ap_sload_t and ap_mon_snap_t are declared in httpd.h, > because ap_sload_t already had been there. t might be a better fit to move > them to scoreboard.h, but I'm not sure about that and I don't know if that > would be allowed by the compatibility rules. > > - also in httpd.h there are now three new function declarations. Two only > used by server/core.c as hook functions: > > int ap_scoreboard_monitor(apr_pool_t *p, server_rec *s); > void ap_scoreboard_child_init(apr_pool_t *p, server_rec *s); > > and one for public consumption: > > AP_DECLARE(void) ap_get_mon_snap(ap_mon_snap_t *ms); > > - mod_systemd and mod_status now call ap_get_mon_snap(&snap) to retrieve the > latest averaged data. mod_status still uses the scoreboard in addition to > retrieve up-to-date current scalar metrics. Small adjustments to the > mod_status output plus additions to mod_systemd notification messages. Tne > STATUS in the notification of mod_systemd now looks liek this: > > STATUS=Pct Idle workers 28.4; Pct Busy workers 10.6; Pct Dead workers 60.9; > Requests/sec: 5030; Bytes served/sec: 2.9MB/sec; Bytes served/request: 596 > B/req; Avg Duration(ms): 5.78; Avg Concurrency: 29.1; Cpu Pct: 40.5 > > - scoreboard.c changes: > > - take over ap_get_sload(ap_sload_t *sl) from server/util.c. > Added copied code from mod_status to that function to also add up the > request duration and the usr and sys CPU times. > > - ap_scoreboard_monitor() runs in the monitor hook. It calls > ap_get_sload() and then a static utility function calc_mon_data() to > calculate the new averages. > > - Minor mmn change not yet part of the patch. > > It compiles fine (maintainer mode) on RHEL 7 x86_64 and on Solaris 10 Sparc > and I did some tests with mod_status and mod_systemd. > > Regards, > > Rainer > > Am 24.04.2020 um 18:32 schrieb Rainer Jung: > > Am 24.04.2020 um 16:21 schrieb Joe Orton: > > > On Fri, Apr 24, 2020 at 12:17:19PM +0200, Rainer Jung wrote: > > > > Thinking further: I think it would make sense to have a module or core > > > > implement the monitor hook to generate that derived data (requests/sec, > > > > bytes/sec, durationMs/request, avgConcurrency) in the last > > > > monitor interval > > > > and to provide that data to consumers like mod_
Re: Move FILE buckets on setaside (like/as a morphing bucket)?
On Mon, Apr 27, 2020 at 2:09 PM Yann Ylavic wrote: > > https://github.com/apache/httpd/pull/117 I closed it, how about the second patch there though? https://github.com/apache/httpd/pull/117/commits/cdf4c4a3c61340f061267cc70c456d9469e9faac
Re: Move FILE buckets on setaside (like/as a morphing bucket)?
On Mon, Apr 27, 2020 at 3:04 PM Joe Orton wrote: > > On Mon, Apr 27, 2020 at 02:29:55PM +0200, Yann Ylavic wrote: > > The call chain is: > > apr_bucket_setaside(bucket, newpool) > > => file_bucket_setaside(bucket, newpool) > > => apr_file_setaside(&newfd, bucket->data->fd, newpool) > > => copy bucket->data->fd to newfd, move cleanup to newpool > > => bucket->data->fd->filedes = -1 > > => bucket->data->fd = newfd > > > > And since bucket->data is shared, all other buckets with the same data > > get their filedes set to -1. > > You mentioned this was a problem for split buckets, which I don't get. > For a split FILE bucket bucket->data is common across copies, so this > seems fine, the last "bucket->data->fd = newfd;" updates _all_ the split > buckets. Ah yes indeed, you are obviously correct. I just extrapolated my issue (the file mangled by mod_proxy), and thought the same could happen with splitted file buckets.. Sorry for the noise and taking your time, and thank you/Rüdiger for your patience ;) > > That is clearly the implication of the apr_file_setaside() API warning > which extends to apr_bucket_setaside(). Alternatively you could say the > apr_file_setaside() implementation is broken and should work like > apr_mmap_dup(). Actually what I did for my proxy case is an apr_os_file_get() + apr_os_file_set() to create a file that mod_proxy can mangle at wish. The goal was also that sendfile() be finally used. > > (I think treating setaside as special for FILE is a bad road to start > down - I should be able copy and paste the FILE implementation as a new > MYFILE bucket type and have the core behave correctly, if not optimally) +1 Thanks, Yann.
Re: Providing near-time averaged monitoring data for mod_systemd and mod_status
Am 27.04.2020 um 08:57 schrieb Ruediger Pluem: On 4/25/20 8:10 PM, Rainer Jung wrote: Patch available at home.apache.org/~rjung/patches/httpd-trunk-mon-snaps-v1_2.patch Thanks for this. Could you please create this as a PR on github as well? This ensures that all the Travis tests are run for your patch. Thanks Rüdiger. Done and indeed Travis found one not that I fixed but we need to discuss. I changed the data type of the idle and busy fields in the sload struct from int to float, because they were used as floats. But that broke mod_headers which uses the data and will break other consumers as well. For compatibility reasons, we could keep the int type though. Regards, Rainer
Re: Move FILE buckets on setaside (like/as a morphing bucket)?
On Mon, Apr 27, 2020 at 02:29:55PM +0200, Yann Ylavic wrote: > The call chain is: > apr_bucket_setaside(bucket, newpool) > => file_bucket_setaside(bucket, newpool) > => apr_file_setaside(&newfd, bucket->data->fd, newpool) > => copy bucket->data->fd to newfd, move cleanup to newpool > => bucket->data->fd->filedes = -1 > => bucket->data->fd = newfd > > And since bucket->data is shared, all other buckets with the same data > get their filedes set to -1. You mentioned this was a problem for split buckets, which I don't get. For a split FILE bucket bucket->data is common across copies, so this seems fine, the last "bucket->data->fd = newfd;" updates _all_ the split buckets. I can see this is a problem *outside* of _split, so if you do something like: e1 = apr_brigade_insert_file(bb, fd, ...); e2 = apr_brigade_insert_file(bb, fd, ...); and then a later apr_bucket_setaside(e1) will break e2 as you describe, because e1 and e2 _don't_ have a common ->data. I can see in the list archive a discussion of apr_file_setaside() where cliffw suggested apr_file_setaside() should work like apr_mmap_dup() which doesn't appear to have this problem. > > > The case I'm facing is a bit different but similar. A handler is > > > saving large bodies into a file and inserts an input filter for > > > mod_proxy to use the file bucket as request body (much like > > > mod_request). The file is still needed/used after mod_proxy (in an > > > output filter), so I don't expect the file to be invalidated by > > > mod_proxy's output filtering. > > > > I do agree that it is surprising behaviour that the apr_file_t * is > > potentially invalidated after being wrapped in a FILE bucket though. Not > > sure how to avoid it, IIRC this is some optimisation to avoid excessive > > dup2() calls? > > Yes I think so, that plus a new fd to take into account for ulimit nofile. > Alternatively we could play with apr_os_file_get()+apr_os_file_set(), > which will leave alone the fd and cleanup from the original pool, but > at that point I think we'd better let the caller responsible for the > lifetime.. I think within the constraint of the current APR/httpd buckets/filtering API it is probably necessary to say that you cannot insert a FILE bucket referring to the same apr_file_t * twice. Or you should dup2 inbetween. That is clearly the implication of the apr_file_setaside() API warning which extends to apr_bucket_setaside(). Alternatively you could say the apr_file_setaside() implementation is broken and should work like apr_mmap_dup(). (I think treating setaside as special for FILE is a bad road to start down - I should be able copy and paste the FILE implementation as a new MYFILE bucket type and have the core behave correctly, if not optimally) Regards, Joe
Re: Move FILE buckets on setaside (like/as a morphing bucket)?
On 4/27/20 2:29 PM, Yann Ylavic wrote: > On Mon, Apr 27, 2020 at 11:11 AM Joe Orton wrote: >> >> On Sun, Apr 26, 2020 at 05:26:02PM +0200, Yann Ylavic wrote: >>> For FILE buckets, the behaviour of apr_bucket_setaside() is to take >>> *full* ownership of the underlying apr_file, that is: allocate/move >>> the file/cleanup to the new pool AND set the old file's fd to -1 (see >>> apr_file_setaside, [1]). >>> >>> This can be problematic in an output filters chain whenever a FILE >>> bucket gets splitted. Suppose that an FTYPE_RESOURCE output filter >>> splits a FILE bucket in two and sends the first one through the chain, >>> and then due to network congestion the core output filter needs to set >>> the FILE bucket aside. It happens that the second FILE bucket becomes >>> a dangling bucket (with fd == -1). >> >> Hang on, I don't follow this, how does data->fd end up as -1 in this >> case? The buckets split from the original FILE bucket have a common >> ->data since it is a refcounted bucket type. If any of them are >> setaside the common ->data->fd is replaced with a new apr_file_t >> allocated from a new (longer-lived) pool. No? > > The call chain is: > apr_bucket_setaside(bucket, newpool) > => file_bucket_setaside(bucket, newpool) > => apr_file_setaside(&newfd, bucket->data->fd, newpool) > => copy bucket->data->fd to newfd, move cleanup to newpool > => bucket->data->fd->filedes = -1 > => bucket->data->fd = newfd But isn't bucket->data->fd the same for the old and the new one? So after bucket->data->fd = newfd bucket->data->fd(aka newfd)->filedes will have the correct value for all buckets? Regards Rüdiger
Re: Move FILE buckets on setaside (like/as a morphing bucket)?
On Mon, Apr 27, 2020 at 11:11 AM Joe Orton wrote: > > On Sun, Apr 26, 2020 at 05:26:02PM +0200, Yann Ylavic wrote: > > For FILE buckets, the behaviour of apr_bucket_setaside() is to take > > *full* ownership of the underlying apr_file, that is: allocate/move > > the file/cleanup to the new pool AND set the old file's fd to -1 (see > > apr_file_setaside, [1]). > > > > This can be problematic in an output filters chain whenever a FILE > > bucket gets splitted. Suppose that an FTYPE_RESOURCE output filter > > splits a FILE bucket in two and sends the first one through the chain, > > and then due to network congestion the core output filter needs to set > > the FILE bucket aside. It happens that the second FILE bucket becomes > > a dangling bucket (with fd == -1). > > Hang on, I don't follow this, how does data->fd end up as -1 in this > case? The buckets split from the original FILE bucket have a common > ->data since it is a refcounted bucket type. If any of them are > setaside the common ->data->fd is replaced with a new apr_file_t > allocated from a new (longer-lived) pool. No? The call chain is: apr_bucket_setaside(bucket, newpool) => file_bucket_setaside(bucket, newpool) => apr_file_setaside(&newfd, bucket->data->fd, newpool) => copy bucket->data->fd to newfd, move cleanup to newpool => bucket->data->fd->filedes = -1 => bucket->data->fd = newfd And since bucket->data is shared, all other buckets with the same data get their filedes set to -1. > > > The case I'm facing is a bit different but similar. A handler is > > saving large bodies into a file and inserts an input filter for > > mod_proxy to use the file bucket as request body (much like > > mod_request). The file is still needed/used after mod_proxy (in an > > output filter), so I don't expect the file to be invalidated by > > mod_proxy's output filtering. > > I do agree that it is surprising behaviour that the apr_file_t * is > potentially invalidated after being wrapped in a FILE bucket though. Not > sure how to avoid it, IIRC this is some optimisation to avoid excessive > dup2() calls? Yes I think so, that plus a new fd to take into account for ulimit nofile. Alternatively we could play with apr_os_file_get()+apr_os_file_set(), which will leave alone the fd and cleanup from the original pool, but at that point I think we'd better let the caller responsible for the lifetime.. Regards, Yann. > > Regards, Joe >
Re: Move FILE buckets on setaside (like/as a morphing bucket)?
On Mon, Apr 27, 2020 at 9:24 AM Ruediger Pluem wrote: > > On 4/26/20 5:26 PM, Yann Ylavic wrote: > > > So, how about we do that? > > Would look like the attached patch (which also removes the misleading > > Did I miss the attachment? :-) Argh :/ > Or even better, can we have this as a PR against trunk on github? This ensure > that it has run already through all the Travis stuff. https://github.com/apache/httpd/pull/117 Thanks, Yann.
Re: Move FILE buckets on setaside (like/as a morphing bucket)?
On Sun, Apr 26, 2020 at 05:26:02PM +0200, Yann Ylavic wrote: > For FILE buckets, the behaviour of apr_bucket_setaside() is to take > *full* ownership of the underlying apr_file, that is: allocate/move > the file/cleanup to the new pool AND set the old file's fd to -1 (see > apr_file_setaside, [1]). > > This can be problematic in an output filters chain whenever a FILE > bucket gets splitted. Suppose that an FTYPE_RESOURCE output filter > splits a FILE bucket in two and sends the first one through the chain, > and then due to network congestion the core output filter needs to set > the FILE bucket aside. It happens that the second FILE bucket becomes > a dangling bucket (with fd == -1). Hang on, I don't follow this, how does data->fd end up as -1 in this case? The buckets split from the original FILE bucket have a common ->data since it is a refcounted bucket type. If any of them are setaside the common ->data->fd is replaced with a new apr_file_t allocated from a new (longer-lived) pool. No? > The case I'm facing is a bit different but similar. A handler is > saving large bodies into a file and inserts an input filter for > mod_proxy to use the file bucket as request body (much like > mod_request). The file is still needed/used after mod_proxy (in an > output filter), so I don't expect the file to be invalidated by > mod_proxy's output filtering. I do agree that it is surprising behaviour that the apr_file_t * is potentially invalidated after being wrapped in a FILE bucket though. Not sure how to avoid it, IIRC this is some optimisation to avoid excessive dup2() calls? Regards, Joe
Re: Move FILE buckets on setaside (like/as a morphing bucket)?
> Am 26.04.2020 um 17:26 schrieb Yann Ylavic : > > After a look at our codebase, this would work as is, all of the FILE > buckets we pass through output filters are from r->pool, so the > lifetime is within the EOR scope (I'm not totally sure about the http2 > beam case, Stefan?). Lacking a name to describe the pool situtation in mod_http2, yet, let's use "crossing conntections" for now. The "main" connection (to the h2 client) and the one processing the h2 streams (requests) are side-by-side, living mostly in separate threads even. Buckets in request brigades need to "cross" these connections. In my delusion of having understood all about pools, threads and filters, I made h2_bucket_beam for the purpose of avoiding copying bucket data, especially files, across connections. And for files, it needs to apr_file_setaside() into the other connection. Which, I guess, only works for non-split file buckets. But I am no longer certain. In an alternate universe, were I to do it again, I would not place HTTP/2 into the httpd process itself. Not only because it is tricky, but it also affects further optimizations of the HTTP/1.x case more difficult. As to be seen from this discussion. - Stefan
Re: Move FILE buckets on setaside (like/as a morphing bucket)?
On 4/26/20 5:26 PM, Yann Ylavic wrote: > So, how about we do that? > Would look like the attached patch (which also removes the misleading Did I miss the attachment? :-) Or even better, can we have this as a PR against trunk on github? This ensure that it has run already through all the Travis stuff. Regards Rüdiger
