Re: Summary: Broken FastCGI with httpd

[Stefan Eissing]

Thanks for the nice summary, Jacob. 

I find the topic of cgi in our server surprisingly complex. Sometimes mod_http2 
stumbles on "details" like conn->id to identify requests that Seemed a Good 
Idea at the time. 

Would it make sense to narrow down the setups to a few blessed ones that become 
part of our tests?

Stefan

> Am 17.01.2017 um 23:16 schrieb Jacob Champion :
> 
> (This conversation is currently spread over Bugzilla, IRC, GitHub, and 
> php-internals. Here's my attempt at summarizing it for all of you. If you 
> have no interest in CGI or FastCGI, stop reading now.)
> 
> == Backstory ==
> 
> Back in May I was testing some FCGI backends with mod_proxy_fcgi, and I found 
> a backend called fcgiwrap that didn't work. That conversation is in [1]. 
> SCRIPT_FILENAME was being set to a value beginning with 
> "proxy:fcgi://host:port", and that didn't make any sense. We patched the 
> problem in 2.4.21 by stripping the weird prefix and called it good.
> 
> Fast forward to find that in *some* cases, SCRIPT_FILENAME could still 
> contain weird values that didn't make any sense (in this case, a query 
> string), because proxy canonicalization doesn't run correctly in 
> per-directory rewrites. The conversation is in [2]. We stripped the query 
> string and called it good.
> 
> Fast forward to find that in *some* cases involving mod_rewrite, PHP-FPM now 
> refuses to operate correctly with mod_proxy_fcgi. That conversation is in 
> [3]. There is no patch, and we can't call this good anymore...
> 
> == Problem ==
> 
> PHP-FPM does a lot of gymnastics to get around old CGI incompatibilities with 
> mod_fastcgi, et al. It was using our accidental "proxy:fcgi//" prefix as a 
> marker to say "this is a new Apache server with mod_proxy_fcgi; don't do some 
> of the weird fixups". Without the marker, in specific cases PHP-FPM will 
> assume it is talking to an older incompatible FCGI module and "fix" things 
> incorrectly.
> 
> I looked to see if there was a way we could keep the current behavior and 
> trick current PHP-FPM deployments into not enabling their "quirks mode" 
> fixups. I don't see any, not without breaking other valid use cases. For now, 
> I've filed a Showstopper against 2.4.x with the intention of reverting the 
> change entirely for 2.4.26.
> 
> == Moving Forward ==
> 
> I think the best way to make both us and our users happy is to talk to the 
> PHP devs and fix both sides at once. To fix our side, I think we have to do 
> two things:
> 
> 1) Fix the CGI 1.1 incompatibilities in all of our first-party FCGI modules 
> so backends don't have to perform any fixup.
> 
> Those quirks have their own (in)famous bug:
> 
>https://bz.apache.org/bugzilla/show_bug.cgi?id=51517
> 
> in which the reporter melts down after three years of inaction.
> 
> The difficulty here is that there are a bunch of supported ways to invoke 
> FCGI (Action, SetHandler, ProxyPass, RewriteRule), all of which seem to 
> define CGI variables to different values in different situations.
> 
> 2) Define what SCRIPT_FILENAME means.
> 
> SCRIPT_FILENAME isn't actually a CGI 1.1 standard variable. We appear to have 
> defined it as "whatever r->filename contains", so we've effectively coupled 
> our implementation details to external clients. PHP-FPM and fcgiwrap, for 
> example, assume that SCRIPT_FILENAME should point to the script that should 
> be executed to handle the request. We need to standardize it.
> 
> These fixes probably can't be enabled by default until we go through a 
> compatibility version bump. But I've started chatting with people on the PHP 
> side [4] and we can hopefully fix these sorts of things once and for all.
> 
> --Jacob
> 
> [1] https://bz.apache.org/bugzilla/show_bug.cgi?id=59618
> [2] 
> https://lists.apache.org/thread.html/87fd8d1dc208b7d74fadf7b3929e71cfd7279c5c5b9b2566386cb684@%3Cdev.httpd.apache.org%3E
> [3] https://github.com/apache/httpd/commit/cab0bfbb#commitcomment-20393588
> [4] https://github.com/php/php-src/pull/694#issuecomment-271963956

Summary: Broken FastCGI with httpd

[Jacob Champion]

(This conversation is currently spread over Bugzilla, IRC, GitHub, and 
php-internals. Here's my attempt at summarizing it for all of you. If 
you have no interest in CGI or FastCGI, stop reading now.)


== Backstory ==

Back in May I was testing some FCGI backends with mod_proxy_fcgi, and I 
found a backend called fcgiwrap that didn't work. That conversation is 
in [1]. SCRIPT_FILENAME was being set to a value beginning with 
"proxy:fcgi://host:port", and that didn't make any sense. We patched the 
problem in 2.4.21 by stripping the weird prefix and called it good.


Fast forward to find that in *some* cases, SCRIPT_FILENAME could still 
contain weird values that didn't make any sense (in this case, a query 
string), because proxy canonicalization doesn't run correctly in 
per-directory rewrites. The conversation is in [2]. We stripped the 
query string and called it good.


Fast forward to find that in *some* cases involving mod_rewrite, PHP-FPM 
now refuses to operate correctly with mod_proxy_fcgi. That conversation 
is in [3]. There is no patch, and we can't call this good anymore...


== Problem ==

PHP-FPM does a lot of gymnastics to get around old CGI incompatibilities 
with mod_fastcgi, et al. It was using our accidental "proxy:fcgi//" 
prefix as a marker to say "this is a new Apache server with 
mod_proxy_fcgi; don't do some of the weird fixups". Without the marker, 
in specific cases PHP-FPM will assume it is talking to an older 
incompatible FCGI module and "fix" things incorrectly.


I looked to see if there was a way we could keep the current behavior 
and trick current PHP-FPM deployments into not enabling their "quirks 
mode" fixups. I don't see any, not without breaking other valid use 
cases. For now, I've filed a Showstopper against 2.4.x with the 
intention of reverting the change entirely for 2.4.26.


== Moving Forward ==

I think the best way to make both us and our users happy is to talk to 
the PHP devs and fix both sides at once. To fix our side, I think we 
have to do two things:


1) Fix the CGI 1.1 incompatibilities in all of our first-party FCGI 
modules so backends don't have to perform any fixup.


Those quirks have their own (in)famous bug:

https://bz.apache.org/bugzilla/show_bug.cgi?id=51517

in which the reporter melts down after three years of inaction.

The difficulty here is that there are a bunch of supported ways to 
invoke FCGI (Action, SetHandler, ProxyPass, RewriteRule), all of which 
seem to define CGI variables to different values in different situations.


2) Define what SCRIPT_FILENAME means.

SCRIPT_FILENAME isn't actually a CGI 1.1 standard variable. We appear to 
have defined it as "whatever r->filename contains", so we've effectively 
coupled our implementation details to external clients. PHP-FPM and 
fcgiwrap, for example, assume that SCRIPT_FILENAME should point to the 
script that should be executed to handle the request. We need to 
standardize it.


These fixes probably can't be enabled by default until we go through a 
compatibility version bump. But I've started chatting with people on the 
PHP side [4] and we can hopefully fix these sorts of things once and for 
all.


--Jacob

[1] https://bz.apache.org/bugzilla/show_bug.cgi?id=59618
[2] 
https://lists.apache.org/thread.html/87fd8d1dc208b7d74fadf7b3929e71cfd7279c5c5b9b2566386cb684@%3Cdev.httpd.apache.org%3E

[3] https://github.com/apache/httpd/commit/cab0bfbb#commitcomment-20393588
[4] https://github.com/php/php-src/pull/694#issuecomment-271963956

Re: Frequent wake-ups for mpm_event

[Stefan Priebe]

Hi Yann,

while testing V6 i'm experiencing segfaults.

exit signal Segmentation

server-error.log:
AH00052: child pid 14110 exit signal Segmentation fault (11)

currently i'm trying to grab a core dump.

Greets,
Stefan

Am 26.12.2016 um 21:18 schrieb Stefan Priebe - Profihost AG:

Am 23.12.2016 um 01:41 schrieb Yann Ylavic:

Hi Stefan,

On Tue, Dec 20, 2016 at 1:52 PM, Stefan Priebe - Profihost AG
 wrote:


Today i had another server giving no answers to any requests. apache
fullstatus did not respond.


Since v5 of the patch, I committed another related change in trunk,
namely: http://svn.apache.org/r1774538
It's about lingering keepalive connections on graceful restart which
may not cause a wakeup.
Does it help?


I'll try that but wanted to rebuild based on http 2.4.25. But your
mpm_event_listener_wakeup_bug57399_V5 patch does no longer apply to http
2.2.25. Can you rebase it?


gdb bt shows this for all httpd childs:


These backtraces are the ones of the main thread, probably not the culprit.
What does "thread apply all bt" say?


Will redo / save that output next time.

Thanks!

Greets,
Stefan


Regards,
Yann.

Re: HTTP/2 frame prioritization not honored

[Kyriakos Zarifis]

Hi Stefan,

Sorry for the delay, I just got back from traveling. I just tried your new
patch and indeed it gets rid of the 100ms delay: The server now serves the
high priority object only ~5-20ms (did a few runs) after it receives its
request, and only sending 5-6 lower-prio frames in between!

That's is a dramatic improvement compared to what I was observing in the
first experiments (~500ms delay), and I think it affects not only this
scenario that I was testing, but any scenario where objects of different
priorities are conflicting. To verify this, I also tested another simple
scenario in which I aggressively Server-Push several big objects when the
server gets the base HTML file. Without the patch, objects embedded in the
HTML (requests normally) are backed behind a large fraction of the pushed
payload and delayed considerably (500ms). With the patch this is avoided
(embedded objects server within a few ms after their request arrives,
preempting Pushed objects)

If you are interested, I have logs comparing the v1.8.8 performance to the
baseline, for both the scenarios ( 1: "prefetched" objects triggered at the
end of a page load delaying normal objects from the next navigation, and 2:
"server-pushed" objects conflicting with embedded objects on the current
page)

Would this patch eventually make it upstream? I'd be very interested in
some details on what was causing this and how you resolved it.


On Fri, Jan 13, 2017 at 8:43 AM, Stefan Eissing <
stefan.eiss...@greenbytes.de> wrote:

> Hi Kyriakos,
>
> maybe you can give https://github.com/icing/mod_h2/releases/tag/v1.8.8 a
> try in your setup? I would be interested if it gets rid of the 100ms delay
> in response processing. Thanks!
>
> Cheers,
>
> Stefan
>
> > Am 04.01.2017 um 19:27 schrieb Kyriakos Zarifis :
> >
> > Hi Stefan,
> >
> > Yes, this is making a big, obvservable difference!
> >
> > Specifically, in all 3 repeats, the high priority stream is now served
> 100ms after it was received, writing ~100 frames (~1.6MB) of currently
> served, lower-priority stream.   (was: 500ms, 500frames(~7.5MB))
> >
> > In more detail, after the high-prio request is received, 20 more
> low-prio frames are served before the h2_task for it logs that it opens the
> output for the new stream. Then, another 80 low-frames are served before
> the high-prio reply is written. (relevant logs below)
> >
> > This already has an observable impact on the transition to the next page
> the moment I click on the link (goes from 1.5sec to less than 500ms), which
> I think is great because this tweak is relevant not just to this scenario,
> but to any higher level stream that begins while lower ones are served,
> even within a single page.
> >
> > I'm wondering if the change you made can be pushed harder to make the
> switch to the new stream even faster, e.g. avoiding even those 100 frames?
> >
> >
> > Thanks,
> > Kyriakos
> >
> >
> >
> > [Wed Jan 04 10:14:48.577687 2017] [http2:debug] [pid 24864]
> h2_stream.c(213): [client] AH03082: h2_stream(0-19): opened
> >
> > [Wed Jan 04 10:14:48.577758 2017] [http2:debug] [pid 24864]
> h2_session.c(452): [client] AH03066: h2_session(0): recv
> FRAME[HEADERS[length=39, hend=1, stream=19, eos=1]], frames=13/1486 (r/s)
> >
> > 20 x lower-prio frames:
> >
> > [Wed Jan 04 10:14:48.577864 2017] [http2:debug] [pid 24864]
> h2_session.c(685): [client] AH03068: h2_session(0): sent
> FRAME[DATA[length=16275, flags=0, stream=5, padlen=0]], frames=16/1486 (r/s)
> >
> > [Wed Jan 04 10:14:48.578775 2017] [http2:debug] [pid 24864]
> h2_task.c(106): [client] AH03348: h2_task(0-19): open output to GET
> 204.57.7.200 /preposition/nextnav.html
> >
> > 80 x lower-prio frames:
> > [Wed Jan 04 10:14:48.578790 2017] [http2:debug] [pid 24864]
> h2_session.c(685): [client] AH03068: h2_session(0): sent
> FRAME[DATA[length=16275, flags=0, stream=5, padlen=0]], frames=16/1504 (r/s)
> >
> > [Wed Jan 04 10:14:48.682168 2017] [http2:debug] [pid 24864]
> h2_session.c(685): [client] AH03068: h2_session(0): sent
> FRAME[HEADERS[length=87, hend=1, stream=19, eos=0]], frames=16/1587 (r/s)
> >
> >
> > [Wed Jan 04 10:14:48.682186 2017] [http2:debug] [pid 24864]
> h2_session.c(685): [client] AH03068: h2_session(0): sent
> FRAME[DATA[length=456, flags=1, stream=19, padlen=0]], frames=16/1588 (r/s)
> >
> >
> > On Wed, Jan 4, 2017 at 9:28 AM, Stefan Eissing <
> stefan.eiss...@greenbytes.de> wrote:
> > Hi Kyriakos,
> >
> > sorry for not replying earlier. I could find the issue you ran into,
> namely that mod_http2 is obsessed with the streams it already has and does
> not submit ready responses - until the existing streams are done or pause.
> >
> > I hope that the new release works much more nicely for you. You find it
> at https://github.com/icing/mod_h2/releases/tag/v1.8.7
> >
> > Thanks,
> >
> > Stefan
> >
> > > Am 02.01.2017 um 23:33 schrieb Kyriakos Zarifis  >:
> > >
> > > Thanks Stefan!
> > >
> > > I just tried the tweaked 

Re: JSON for mod_status

[Jim Jagielski]

It all depends on what Bill decides regarding mod_bmx and if
it is something we intent to backport to 2.4.x

Still not sure on how to *use* BMX, or how other modules
"hook in" (right now we have several modules hook into
mod_status so the "how" is well known and documented), so
I would require some sort of docs in addition to the actual
code, of course.

> On Jan 17, 2017, at 12:35 PM, Luca Toscano  wrote:
> 
> 
> 
> 2016-11-30 18:54 GMT+01:00 Jim Jagielski :
> I'm thinking about adding JSON support to mod_status...
> the "plain" version output really stinks and lacks parity
> w/ the info we provide via HTML, and it would be nice
> to produce a really easily parseable format.
> 
> Thoughts...?
> 
> I know it was extensively discussed, but do we have an agreement about a plan 
> to add this feature?  :)
> 
> Luca
> 
> 
> 

Re: Async write completion broken in trunk?

[Luca Toscano]

2016-09-18 10:57 GMT+02:00 Stefan Fritsch :

> Hi Graham,
>
> On Wed, 14 Sep 2016, Graham Leggett wrote:
>
> > On 06 Sep 2016, at 12:06 AM, Stefan Fritsch  wrote:
> >
> > > in trunk, when having a lot of slow long running transfers, most of
> them seem
> > > to hog a thread and only a few of them go into async write completion
> mode.
> > > Compare this to 2.4, where many transfers are doing async write
> completion and
> > > only a small number of threads are busy.
> > >
> > > Is this a known issue?
> >
> > I’ve seen this before, but couldn’t reproduce it reliably.
> >
> > If you can catch a long running transfer can you get a stacktrace from
> > it? One possible option is that a bucket is insisting on being consumed
> > in one go and thus refuses to leave the handler hook and enter write
> > completion, the second option is that the bucket ends up in write
> > completion but blocks, and we would need to know why.
> >
> > There are still a number of filters out there that insist on swallowing
> > whole bucket brigades in one go. Any of these filters will prevent write
> > completion from working. This will show up in the stacktrace.
>
> Stack trace of a few such threads is below. I don't see anything besides
> the core output filter there. Maybe you see more.
>
> I have also included the list of loaded modules. From extra/ I have
> httpd-userdir.conf proxy-html.conf httpd-info.conf httpd-mpm.conf
> included. Apart from that, it's a somewhat outdated but unmodified default
> config.
>
>
>
> Cheers,
> Stefan
>
>
> Thread 5 (Thread 0x7f70c9a69700 (LWP 3398)):
>
> #0  0x7f70daaea50d in poll () at ../sysdeps/unix/syscall-template.S:84
> No locals.
> #1  0x7f70db83c5d2 in apr_poll (aprset=0x7f70c9a68c20, num=1,
> nsds=0x7f70c9a68c0c, timeout=6) at poll/unix/poll.c:120
> i = 1
> num_to_poll = 1
> pollset = 0x7f70c9a68b60
> #2  0x004529b1 in send_brigade_blocking (c=0x7f70c825b348,
> bytes_written=0x7f70c8257a00, bb=0x7f70c8257658, s=0x7f70c825b0b0)
> at core_filters.c:633
> nsds = 1
> timeout = 6000
> pollset = {p = 0x7f70c8257028, desc_type = APR_POLL_SOCKET,
> reqevents = 4, rtnevents = 4, desc = {f = 0x7f70c825b0b0,
> s = 0x7f70c825b0b0}, client_data = 0x7f70c82576a8}
> rv = 
> #3  ap_core_output_filter (f=0x7f70c8257830, bb=0x7f70c8257658) at
> core_filters.c:449
> c = 0x7f70c825b348
> net = 0x7f70c82577d8
> ctx = 0x7f70c82579f0
> flush_upto = 0x7f70c825a6e8
> rv = 
> loglevel = 4
> #4  0x0043955f in ap_pass_brigade (bb=,
> next=0x7f70c8257830) at util_filter.c:610
> e = 
> #5  ap_filter_output_pending (c=0x7f70c825b348) at util_filter.c:985
> f = 0x7f70c8257830
> rindex = 0x7f70c82576a8
> data_in_output_filters = -1
> #6  0x00435ef0 in ap_run_output_pending (c=c@entry=0x7f70c825b348)
> at mpm_common.c:122
> pHook = 
> n = 0
> rv = -1
> #7  0x7f70ce5da6fc in process_socket (my_thread_num=3, my_child_num=1,
> cs=0x7f70c825b2b8, sock=0x7f70c825b0b0, p=0x7f70c825b028,
> thd=0x7f70dc57cee8) at event.c:1152
> not_complete_yet = 
> c = 0x7f70c825b348
>
> sbh = 0x7f70c825b2a0
> conn_id = 
> rc = 
> #8  worker_thread (thd=0x7f70dc57cee8, dummy=) at
> event.c:2261
> csd = 0x7f70c825b0b0
> cs = 0x0
> te = 0x0
> ptrans = 0x7f70c825b028
> ti = 
> process_slot = 1
> thread_slot = 3
> rv = 
> is_idle = 0
> #9  0x7f70db842488 in dummy_worker (opaque=0x7f70dc57cee8) at
> threadproc/unix/thread.c:142
> thread = 0x7f70dc57cee8
> #10 0x7f70dafb4464 in start_thread (arg=0x7f70c9a69700) at
> pthread_create.c:333
> __res = 
> pd = 0x7f70c9a69700
> now = 
> unwind_buf = {cancel_jmp_buf = {{jmp_buf = {140122396202752,
> 6480505367792406515, 0, 140122429770751, 0, 140122711343168,
> -6408919108241895437, -6408951309539651597},
> mask_was_saved = 0}}, priv = {pad = {0x0, 0x0, 0x0, 0x0}, data = {
>   prev = 0x0, cleanup = 0x0, canceltype = 0}}}
> not_first_call = 
> pagesize_m1 = 
> sp = 
> freesize = 
> __PRETTY_FUNCTION__ = "start_thread"
> #11 0x7f70daaf397f in clone () at ../sysdeps/unix/sysv/linux/
> x86_64/clone.S:105
> No locals.
>
> Thread 3 (Thread 0x7f70caa6b700 (LWP 3394)):
> #0  0x7f70daaea50d in poll () at ../sysdeps/unix/syscall-template.S:84
> No locals.
> #1  0x7f70db83c5d2 in apr_poll (aprset=0x7f70caa6ac20, num=1,
> nsds=0x7f70caa6ac0c, timeout=6) at poll/unix/poll.c:120
> i = 1
> num_to_poll = 1
> pollset = 0x7f70caa6ab60
> #2  0x004529b1 in send_brigade_blocking (c=0x7f70dc506348,
> bytes_written=0x7f70dc502a00, bb=0x7f70dc502658, s=0x7f70dc5060b0)
> at core_filters.c:633
> 

Re: JSON for mod_status

[Luca Toscano]

2016-11-30 18:54 GMT+01:00 Jim Jagielski :

> I'm thinking about adding JSON support to mod_status...
> the "plain" version output really stinks and lacks parity
> w/ the info we provide via HTML, and it would be nice
> to produce a really easily parseable format.
>
> Thoughts...?
>

I know it was extensively discussed, but do we have an agreement about a
plan to add this feature?  :)

Luca

Re: mod_fcgi: Excessive memory usage when large files are uploaded

[Jim Jagielski]

> On Jan 17, 2017, at 6:03 AM, Yann Ylavic  wrote:
> 
> On Tue, Jan 17, 2017 at 9:06 AM, Ivan Zahariev  wrote:
>> 
>> 1. Delete each bucket after sending it to the "ipc_handle". I've looked 
>> through
>> the call tree and the *output_brigade is last used by proc_write_ipc().
>> Therefore, it should be safe to empty it while being processed there.
>> 2. Take the same approach as mod_http2, which handles FILE buckets in a
>> different way. Instead of using apr_bucket_read(), they process FILE buckets
>> by apr_file_read() and manage the data buffer manually. This way the
>> original *output_brigade won't be modified and automatically split by
>> apr_bucket_read(). This requires more coding work.
> 
> I would go for 1., but keep in mind that you can't delete the bucket
> while it is still pointed to by the iovec...
> 

+1

Re: mod_fcgi: Excessive memory usage when large files are uploaded

[Ivan Zahariev]

  
  
Hi,
Here is the patch --
https://github.com/famzah/mod_fcgid/commit/84c7c2dbf2047745c6aea87a4bc6b4061c98ac8e
A few notes:

  I tested it with several files which had random data and
various lengths: 0b, 127b, 128b, 129b, 150MB, 1MB, 1b, 250MB,
350MB, 8191b, 8192b, 8193b. The uploaded size and SHA1 checksum
match.
  
  Also made sure that the special case "There are something
left" in the code was tested too, at the end when "nvec != 0".
  I can't test this on Windows but porting the patch in
"fcgid_proc_win.c" looks easy, since the same loop method is
used there too. Additionally, "fcgid_proc_win.c" doesn't group
write requests in 8 chunks and does everything one-by-one (one
read, then one write to the IPC socket).


Please review the patch. If it looks good, I'll test it on our
production machines and then submit it for mainstream merge.

Best regards.
--Ivan

On 17.1.2017 г. 13:03 ч., Yann Ylavic
  wrote:


  On Tue, Jan 17, 2017 at 9:06 AM, Ivan Zahariev  wrote:

  

1. Delete each bucket after sending it to the "ipc_handle". I've looked through
the call tree and the *output_brigade is last used by proc_write_ipc().
Therefore, it should be safe to empty it while being processed there.
2. Take the same approach as mod_http2, which handles FILE buckets in a
different way. Instead of using apr_bucket_read(), they process FILE buckets
by apr_file_read() and manage the data buffer manually. This way the
original *output_brigade won't be modified and automatically split by
apr_bucket_read(). This requires more coding work.

  
  
I would go for 1., but keep in mind that you can't delete the bucket
while it is still pointed to by the iovec...

An (optimal) alternative would be to use apr_socket_sendfile() (for
file buckets) like sendfile_nonblocking() does in httpd's core output
filter (see server/core_filters.c).
But 1. is wise still for non file buckets.

Regards,
Yann.



  

Re: clang-analyzer?

[Jim Jagielski]

> On Jan 9, 2017, at 4:48 AM, Graham Leggett  wrote:
> 
> On 08 Jan 2017, at 4:45 AM, Leif Hedstrom  wrote:
> 
>> I ran clang-analyzer against the HTTPD master branch, and it found 126 
>> issues. Many of these are benign, but I was curious if the community has any 
>> thoughts on this? With another project, I’ve found that keep static code 
>> analysis to zero issues can really help finding new, serious issues 
>> (basically, we put the tree in failed state if there’s a new static code 
>> analysis issue).
>> 
>> The issues are all over the source code, in core and mod_’s alike. It’d be 
>> pretty tedious to file individual tickets for each of them, so curious if 
>> there’s any interest in cleaning this up to start with a clean state? It’d 
>> then be easy to add clang-analyzer to the release process for example.
> 
> Adding clang-analyzer to a make target (not a default part of the build) 
> would be a good step, it would make it easy for anyone to run it if they had 
> it available.
> 

Sounds good to me.

Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Jim Jagielski]

> On Jan 16, 2017, at 6:57 PM, Daniel Ruggeri  wrote:
> 
> For the most part, yes except the portions that make the header presence
> optional (the HDR_MISSING case). Those were added as it came into the
> code base to handle a use case I was working on. I've added some
> comments inline since I won't have time to poke around myself for a
> while yet.
> 
> 
> For convenience, here's a link to the original code
> 
> https://github.com/roadrunner2/mod-proxy-protocol/blob/master/mod_proxy_protocol.c
> 

Would it make sense to have the "stable" version available
for backport, and keep in the WIP in trunk?

Re: FYI brotli

[Jim Jagielski]

Besides, we had no problems supporting OpenSSL 0.9.6 for
years :)

If/when brotli 1.0.0 is released, we simply add support
for that as well. No biggie.

> On Jan 17, 2017, at 8:27 AM, Jim Jagielski  wrote:
> 
> Actually, it works fine w/ Brotli 0.5.2 which is
> what I have installed.
> 
>> On Jan 16, 2017, at 3:28 PM, Evgeny Kotkov  
>> wrote:
>> 
>> Jim Jagielski  writes:
>> 
>>> Functional patch avail... working on doccos.
>>> 
>>>   http://home.apache.org/~jim/patches/brotli-2.4.patch
>> 
>> Hi Jim,
>> 
>> Thank you for the backport patch.
>> 
>> There is, however, a potential problem with backporting mod_brotli, since
>> it relies on the Brotli library 1.0.0, which has not yet been released.
>> In other words, if the upstream changes the API or the library layout
>> or their pkg-config files after mod_brotli is shipped with httpd 2.4.x, it's
>> either going to stop building or working.
>> 
>> The open ticket about the new release is here:
>> 
>> https://github.com/google/brotli/issues/483
>> 
>> My impression on this is that mod_brotli is only safe to backport after the
>> Brotli authors publish the 1.0.0 version of their library.  But perhaps I am
>> missing something?
>> 
>> (Apart from this, I think that Brotli did change the layout of their 
>> pkg-config
>> files in [https://github.com/google/brotli/commit/fe9f9a91], and it requires
>> an update in the filters/config.m4 file; I'll do that.)
>> 
>> 
>> Regards,
>> Evgeny Kotkov
> 

Re: FYI brotli

[Jim Jagielski]

Actually, it works fine w/ Brotli 0.5.2 which is
what I have installed.

> On Jan 16, 2017, at 3:28 PM, Evgeny Kotkov  
> wrote:
> 
> Jim Jagielski  writes:
> 
>> Functional patch avail... working on doccos.
>> 
>>http://home.apache.org/~jim/patches/brotli-2.4.patch
> 
> Hi Jim,
> 
> Thank you for the backport patch.
> 
> There is, however, a potential problem with backporting mod_brotli, since
> it relies on the Brotli library 1.0.0, which has not yet been released.
> In other words, if the upstream changes the API or the library layout
> or their pkg-config files after mod_brotli is shipped with httpd 2.4.x, it's
> either going to stop building or working.
> 
> The open ticket about the new release is here:
> 
>  https://github.com/google/brotli/issues/483
> 
> My impression on this is that mod_brotli is only safe to backport after the
> Brotli authors publish the 1.0.0 version of their library.  But perhaps I am
> missing something?
> 
> (Apart from this, I think that Brotli did change the layout of their 
> pkg-config
> files in [https://github.com/google/brotli/commit/fe9f9a91], and it requires
> an update in the filters/config.m4 file; I'll do that.)
> 
> 
> Regards,
> Evgeny Kotkov

Re: FYI brotli

[Hanno Böck]

On Mon, 16 Jan 2017 18:06:40 -0600
William A Rowe Jr  wrote:

> If so, maybe we teach both to step out of the way when SSL encryption
> filters are in place?

This would make no sense. Brotli is only supported over HTTPS by
browsers.

Compression-based attacks are a tricky problem, however someone has yet
to show that they are abused in practice. But preventing deployment of a
new compression algorithm doesn't help. You'd have to disable
compression altogether to avoid them.

-- 
Hanno Böck
https://hboeck.de/

mail/jabber: ha...@hboeck.de
GPG: FE73757FA60E4E21B937579FA5880072BBB51E42

Re: mod_fcgi: Excessive memory usage when large files are uploaded

[Yann Ylavic]

On Tue, Jan 17, 2017 at 9:06 AM, Ivan Zahariev  wrote:
>
> 1. Delete each bucket after sending it to the "ipc_handle". I've looked 
> through
> the call tree and the *output_brigade is last used by proc_write_ipc().
> Therefore, it should be safe to empty it while being processed there.
> 2. Take the same approach as mod_http2, which handles FILE buckets in a
> different way. Instead of using apr_bucket_read(), they process FILE buckets
> by apr_file_read() and manage the data buffer manually. This way the
> original *output_brigade won't be modified and automatically split by
> apr_bucket_read(). This requires more coding work.

I would go for 1., but keep in mind that you can't delete the bucket
while it is still pointed to by the iovec...

An (optimal) alternative would be to use apr_socket_sendfile() (for
file buckets) like sendfile_nonblocking() does in httpd's core output
filter (see server/core_filters.c).
But 1. is wise still for non file buckets.

Regards,
Yann.

AW: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c

[Plüm , Rüdiger , Vodafone Group]

> -Ursprüngliche Nachricht-
> Von: Daniel Ruggeri [mailto:drugg...@primary.net]
> Gesendet: Dienstag, 17. Januar 2017 00:57
> An: dev@httpd.apache.org
> Betreff: Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-
> message-tags/next-number docs/manual/mod/mod_remoteip.xml
> modules/metadata/mod_remoteip.c
> 
> For the most part, yes except the portions that make the header presence
> optional (the HDR_MISSING case). Those were added as it came into the
> code base to handle a use case I was working on. I've added some
> comments inline since I won't have time to poke around myself for a
> while yet.
> 
> 
> For convenience, here's a link to the original code
> 
> https://github.com/roadrunner2/mod-proxy-
> protocol/blob/master/mod_proxy_protocol.c
> 
> 
> On 1/16/2017 10:14 AM, Jim Jagielski wrote:
> > Let me take a look... afaict, this is a copy of what
> > was donated, which has been working for numerous people.
> > But that doesn't mean it can't have bugs ;)
> >
> >> On Jan 16, 2017, at 7:20 AM, Ruediger Pluem 
> wrote:
> >>
> >> Anyone?
> 
> Apologies for missing the original reply
> 
> >> Regards
> >>
> >> Rüdiger
> >>
> >> On 01/10/2017 12:39 PM, Ruediger Pluem wrote:
> >>>
> >>> On 12/30/2016 03:20 PM, drugg...@apache.org wrote:
>  Author: druggeri
>  Date: Fri Dec 30 14:20:48 2016
>  New Revision: 1776575
> 
>  URL: http://svn.apache.org/viewvc?rev=1776575=rev
>  Log:
>  Merge new PROXY protocol code into mod_remoteip
> 
>  Modified:
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
> httpd/httpd/trunk/modules/metadata/mod_remoteip.c
> 
> 
> 
> ==
>  --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
>  +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30
> 14:20:48 2016
>  + */
>  +static int remoteip_hook_pre_connection(conn_rec *c, void *csd)
>  +{
>  +remoteip_config_t *conf;
>  +remoteip_conn_config_t *conn_conf;
>  +int optional;
>  +
>  +conf = ap_get_module_config(ap_server_conf->module_config,
>  +_module);
>  +
>  +/* Used twice - do the check only once */
>  +optional = remoteip_addr_in_list(conf-
> >proxy_protocol_optional, c->local_addr);
>  +
>  +/* check if we're enabled for this connection */
>  +if ((!remoteip_addr_in_list(conf->proxy_protocol_enabled, c-
> >local_addr)
>  +  && !optional )
>  +|| remoteip_addr_in_list(conf->proxy_protocol_disabled, c-
> >local_addr)) {
>  +return DECLINED;
>  +}
>  +
>  +/* mod_proxy creates outgoing connections - we don't want
> those */
>  +if (!remoteip_is_server_port(c->local_addr->port)) {
>  +return DECLINED;
>  +}
> >>> Why is the c->local_addr->port set to a port we are listening on in
> case of proxy connections?
> 
> This is from the original code, but wouldn't this be the local port on
> the outbound connection (some high number)? The remoteip_is_server_port
> should therefore fail this check.

My bad I missed the ! in the condition. So this should work.

>  +
>  +switch (psts) {
>  +case HDR_MISSING:
>  +if (conn_conf->proxy_protocol_optional) {
>  +/* Same as DONE, but don't delete the
> bucket. Rather, put it
>  +   back into the brigade and move the
> request along the stack */
>  +ctx->done = 1;
>  +APR_BRIGADE_INSERT_HEAD(bb_out, b);
> >>> See below. We need to restore all buckets. What if the original read
> was speculative?
> >>>
>  +return ap_pass_brigade(f->next, ctx->bb);
> >>> Why using ap_pass_brigade on f->next? We are an input filter and f-
> >next is our upstream input filter.
> 
> This is probably my own misunderstanding... what would be the preferred
> way to move this down the stack, then?

What do you want to do here? Return what you have in ctx->bb to the caller that 
pulls
from you? You pull from input filter while you push to output filters.

> 
> >>>
>  +}
>  +else {
>  +ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f-
> >c, APLOGNO(03510)
>  + "RemoteIPProxyProtocol: no
> valid PROXY header found");
>  +/* fall through to error case */
>  +}
>  +case HDR_ERROR:
>  +f->c->aborted = 1;
>  +apr_bucket_delete(b);
>  +apr_brigade_destroy(ctx->bb);
>  +return 

mod_fcgi: Excessive memory usage when large files are uploaded

[Ivan Zahariev]

Hello,

If a large amount of data is POST'ed to a process running mod_fcgid, the 
Apache child uses an excessive amount of memory when processing it.


The client request is properly received and the following statement from 
the documentation is true: "Once the amount of request body read from 
the client exceeds FcgidMaxRequestInMem bytes, the remainder of the 
request body will be stored in a temporary file."


The problem occurs when the temporary file is being sent to the FastCGI 
handling process via its IPC socket. Here is the corresponding function 
which sends the prepared "output_brigade" to the IPC socket: 
https://gist.github.com/famzah/125a91971fba3450dd4926fe13e0ede6


The documentation of apr_bucket_read() clearly states that "if buckets 
are read in a loop, and aren't deleted after being processed, the 
potentially large bucket will slowly be converted into RAM resident heap 
buckets. If the file is larger than available RAM, an out of memory 
condition could be caused."


I need your guidance, in order to fix this properly. I've researched a 
bit and see the following possible options to fix this:


1. Delete each bucket after sending it to the "ipc_handle". I've looked
   through the call tree and the *output_brigade is last used by
   proc_write_ipc(). Therefore, it should be safe to empty it while
   being processed there.
2. Take the same approach as mod_http2, which handles FILE buckets in a
   different way. Instead of using apr_bucket_read(), they process FILE
   buckets by apr_file_read() and manage the data buffer manually. This
   way the original *output_brigade won't be modified and automatically
   split by apr_bucket_read(). This requires more coding work.


Best regards.
--Ivan