You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jim Jagielski <ji...@jaguNET.com> on 2017/02/08 15:45:30 UTC
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Which ones? You mean the special case one?
Would it be better if we did that *before* calling
the fix_cgivars(). What is there makes the following
config pair work as-is:
AddType application/x-php7-fpm .php
Action application/x-php7-fpm /fpm virtual
<Location /fpm>
SetHandler proxy:fcgi://localhost:9001
</Location>
--
<FilesMatch \.php$>
SetHandler "proxy:fcgi://localhost:9001
</FilesMatch>
By allowing fix_cgivars() after that, then we could work around
those rare cases when even that fixup isn't correct for the users
environ.
> On Feb 8, 2017, at 10:28 AM, covener@apache.org wrote:
>
> Author: covener
> Date: Wed Feb 8 15:28:56 2017
> New Revision: 1782209
>
> URL: http://svn.apache.org/viewvc?rev=1782209&view=rev
> Log:
> danger?
>
> Modified:
> httpd/httpd/branches/2.4.x/STATUS
>
> Modified: httpd/httpd/branches/2.4.x/STATUS
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/STATUS?rev=1782209&r1=1782208&r2=1782209&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.4.x/STATUS (original)
> +++ httpd/httpd/branches/2.4.x/STATUS Wed Feb 8 15:28:56 2017
> @@ -223,6 +223,7 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
> http://svn.apache.org/r1782194
> 2.4.x patch: http://home.apache.org/~jim/patches/mod_proxy_fcgi.patch
> +1: jim
> + covener: -0.9: new fixups in here seem risky, do we really want these?
>
>
> PATCHES/ISSUES THAT ARE BEING WORKED
>
>
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Feb 8, 2017, at 10:52 AM, Eric Covener <co...@gmail.com> wrote:
>
> On Wed, Feb 8, 2017 at 10:45 AM, Jim Jagielski <ji...@jagunet.com> wrote:
>> Which ones? You mean the special case one?
>>
>> Would it be better if we did that *before* calling
>> the fix_cgivars(). What is there makes the following
>> config pair work as-is:
>>
>> AddType application/x-php7-fpm .php
>> Action application/x-php7-fpm /fpm virtual
>> <Location /fpm>
>> SetHandler proxy:fcgi://localhost:9001
>> </Location>
>>
>> --
>>
>> <FilesMatch \.php$>
>> SetHandler "proxy:fcgi://localhost:9001
>> </FilesMatch>
>
>
> My concern is that nobody ever reported this config as broken and
> there's no telling what any on-by-default change here can break.
But this is covered isn't it, but if one uses ProxyFCGIBackendType.
It's a noop if desired for that particular use-case.
How about if we move this segment to above fix_cgivars() and
give a head's up to PHP that 2.4-HEAD includes this...
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 06/20/2017 06:39 PM, Jan Ehrhardt wrote:
> BTW: the developers at Directadmin are aware of this bug.
> https://forum.directadmin.com/showthread.php?t=54952&page=2&p=281618#post281618
> I ran into it while updating my Centos 6 servers.
Thanks for the heads up. CC's are coming in on the related bugs in
Bugzilla as well.
Anyone opposed to a users@ announcement on the issue?
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jan Ehrhardt <ph...@ehrhardt.nl>.
Jacob Champion in gmane.comp.apache.devel (Tue, 20 Jun 2017 09:07:44
-0700):
>On 02/08/2017 07:56 PM, Eric Covener wrote:
>> Assuming there's some alternate path that actually does change
>> SCRIPT_NAME by default, we a) don't have any complaint about
>> SCRIPT_NAME and b) have the SetEnv thing. If we want more options,
>> maybe we can stash this older SCRIPT_NAME into a new variable and show
>> how to copy it over SCRIPT_NAME?
>
>...and restarting this conversation, since the new behavior seems to
>have a bug:
>
>https://bz.apache.org/bugzilla/show_bug.cgi?id=61202
BTW: the developers at Directadmin are aware of this bug.
https://forum.directadmin.com/showthread.php?t=54952&page=2&p=281618#post281618
I ran into it while updating my Centos 6 servers.
--
Jan
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 06/20/2017 11:48 AM, Jim Jagielski wrote:
> Ahh... the tests from the orig bug report
There have been several reports. I'm hoping to get the test case you
were using to test the new `if (fpm ...` logic in mod_proxy_fcgi. Even
if it was just a manual test; I just want to get it recorded in the suite.
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Jun 28, 2017, at 6:46 AM, Eric Covener <co...@gmail.com> wrote:
>
> On Wed, Jun 28, 2017 at 5:50 AM, Jim Jagielski <ji...@jagunet.com> wrote:
>> Is there someplace a set of examples on This Is What PHP and PHP-FPM
>> Expect These Values To Be?
>>
>> Like a whole slew of:
>>
>> For request: /blag/futo/gtyj.php?qur=kjr
>> SCRIPT_NAME: Should be /gtyj.php
>> PATH_INFO Should be /blag/futo
>> PATH_TRANSLATED Should be /Home/Charlie/public_html/site/blag/futo/
>>
>> or whatever?
>>
>> I think until we have something like this, we will be constantly
>> playing fetch-me-a-rock
>
> The problem is that FPM has very ugly code to second-guess most of
> these values based on other values and its own configuration. I just
> can't follow it. But we know it worked for quite a long time.
>
> We had not been fixing FPM compatibility for a very long time. We got
> into trouble because we tried to clean up some of the variables that
> FPM was already using as markers to ID mod_proxy_fcgi and cleaning up
> itself.
>
> The first of the modern problems was an attempt to make the variables
> consumable by non-FPM fcgi servers. That's the intent of having the
> ProxyFCGIBackendType (or whatever) so we can leave FPM input alone but
> do a better job on variables where they can be more directly verified
> without some overly complex layer second-guessing them.
FWIW, I'm looking thru
https://mail.python.org/pipermail/web-sig/2007-January/002475.html
Anyway, at least now we have in the test framework the ability to
test fcgi and php-fpm, if available, so we can at least ensure no
regressions or, if we like, finally "fix" things so that Apache and PHP
have similar expectations :)
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Eric Covener <co...@gmail.com>.
On Wed, Jun 28, 2017 at 5:50 AM, Jim Jagielski <ji...@jagunet.com> wrote:
> Is there someplace a set of examples on This Is What PHP and PHP-FPM
> Expect These Values To Be?
>
> Like a whole slew of:
>
> For request: /blag/futo/gtyj.php?qur=kjr
> SCRIPT_NAME: Should be /gtyj.php
> PATH_INFO Should be /blag/futo
> PATH_TRANSLATED Should be /Home/Charlie/public_html/site/blag/futo/
>
> or whatever?
>
> I think until we have something like this, we will be constantly
> playing fetch-me-a-rock
The problem is that FPM has very ugly code to second-guess most of
these values based on other values and its own configuration. I just
can't follow it. But we know it worked for quite a long time.
We had not been fixing FPM compatibility for a very long time. We got
into trouble because we tried to clean up some of the variables that
FPM was already using as markers to ID mod_proxy_fcgi and cleaning up
itself.
The first of the modern problems was an attempt to make the variables
consumable by non-FPM fcgi servers. That's the intent of having the
ProxyFCGIBackendType (or whatever) so we can leave FPM input alone but
do a better job on variables where they can be more directly verified
without some overly complex layer second-guessing them.
--
Eric Covener
covener@gmail.com
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
Is there someplace a set of examples on This Is What PHP and PHP-FPM
Expect These Values To Be?
Like a whole slew of:
For request: /blag/futo/gtyj.php?qur=kjr
SCRIPT_NAME: Should be /gtyj.php
PATH_INFO Should be /blag/futo
PATH_TRANSLATED Should be /Home/Charlie/public_html/site/blag/futo/
or whatever?
I think until we have something like this, we will be constantly
playing fetch-me-a-rock
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 06/27/2017 04:59 PM, Eric Covener wrote:
> I would just as well pull this block out entirely rather than taking
> the "fpm||" half of the test out. It seems like if you go out of your
> way to run a script with PATH_INFO set as some parameter that we
> shouldn't negate that. And like the non path_info case, nobody has
> ever asked us for this.
Checked in as r1800306 and proposed for backport.
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Eric Covener <co...@gmail.com>.
On Wed, Jun 21, 2017 at 2:00 PM, Jacob Champion <ch...@gmail.com> wrote:
> On 06/20/2017 02:19 PM, Jacob Champion wrote:
>>
>> Here's why I'm asking: if I were to propose the attached patch for
>> backport, what is the test case that *should* fail but doesn't?
>> (proxy_fcgi.t passes, no problem.) And once we get that test case, can we
>> show that it actually addresses a valid PHP-FPM use case, or is it a feature
>> without a user?
>
>
> Any thoughts on this?
I still cannot make any sense of what FPM is doing with our input.
I don't think either of the SCRIPT_NAME transformations make sense
just by the virtue of talking to FPM, primarily because it is neither
historically necessary or demonstrated as a problem.
if (fpm || apr_table_get(r->notes, "virtual_script")) {
/*
* Adjust SCRIPT_NAME, PATH_INFO and PATH_TRANSLATED for PHP-FPM
* TODO: Right now, PATH_INFO and PATH_TRANSLATED look OK...
*/
const char *pend;
const char *script_name = apr_table_get(r->subprocess_env,
"SCRIPT_NAME");
pend = script_name + strlen(script_name);
if (r->path_info && *r->path_info) {
pend = script_name + ap_find_path_info(script_name,
r->path_info) - 1;
}
while (pend != script_name && *pend != '/') {
pend--;
}
apr_table_setn(r->subprocess_env, "SCRIPT_NAME", pend);
ap_log_rerror(APLOG_MARK, APLOG_TRACE4, 0, r,
"fpm:virtual_script: Modified SCRIPT_NAME to: %s",
pend);
}
I would just as well pull this block out entirely rather than taking
the "fpm||" half of the test out. It seems like if you go out of your
way to run a script with PATH_INFO set as some parameter that we
shouldn't negate that. And like the non path_info case, nobody has
ever asked us for this.
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 06/20/2017 02:19 PM, Jacob Champion wrote:
> Here's why I'm asking: if I were to propose the attached patch for
> backport, what is the test case that *should* fail but doesn't?
> (proxy_fcgi.t passes, no problem.) And once we get that test case, can
> we show that it actually addresses a valid PHP-FPM use case, or is it a
> feature without a user?
Any thoughts on this?
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Perkins <ja...@cpanel.net>.
This patch has fixed the issues for us. Do you feel this is a production worthy patch, or will there be more movement on it?
—
Jacob Perkins
Product Owner
cPanel Inc.
jacob.perkins@cpanel.net <ma...@cpanel.net>
Office: 713-529-0800 x 4046
Cell: 713-560-8655
> On Jun 20, 2017, at 4:19 PM, Jacob Champion <ch...@gmail.com> wrote:
>
> On 06/20/2017 01:35 PM, Jacob Champion wrote:
>> What requests and server config did you use with this test setup?
>
> Here's why I'm asking: if I were to propose the attached patch for backport, what is the test case that *should* fail but doesn't? (proxy_fcgi.t passes, no problem.) And once we get that test case, can we show that it actually addresses a valid PHP-FPM use case, or is it a feature without a user?
>
> --Jacob
> <remove-fpm.patch>
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 06/20/2017 01:35 PM, Jacob Champion wrote:
> What requests and server config did you use with this test setup?
Here's why I'm asking: if I were to propose the attached patch for
backport, what is the test case that *should* fail but doesn't?
(proxy_fcgi.t passes, no problem.) And once we get that test case, can
we show that it actually addresses a valid PHP-FPM use case, or is it a
feature without a user?
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 06/23/2017 09:42 AM, Jim Jagielski wrote:
> Over this weekend I may try to extend the current fcgi testing
> to include php-fpm... we should not, imo, fold in any patches
> until we can test each applicable use case and avoid regressions.
I've also added one of the known 2.4.26 regressions to the existing
tests. I'll be going through today and adding cases for the GENERIC
backend as well.
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
Over this weekend I may try to extend the current fcgi testing
to include php-fpm... we should not, imo, fold in any patches
until we can test each applicable use case and avoid regressions.
Hacking on the test will keep my mind off of things...
> On Jun 23, 2017, at 7:52 AM, Jim Jagielski <ji...@jaguNET.com> wrote:
>
> AddType application/x-php7-fpm .php
> Action application/x-php7-fpm /fpm virtual
> <Location /fpm>
> SetHandler proxy:fcgi://localhost:9001
> </Location>
>
> <FilesMatch \.php$>
> SetHandler "proxy:fcgi://localhost:9001
> </FilesMatch>
>
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
AddType application/x-php7-fpm .php
Action application/x-php7-fpm /fpm virtual
<Location /fpm>
SetHandler proxy:fcgi://localhost:9001
</Location>
<FilesMatch \.php$>
SetHandler "proxy:fcgi://localhost:9001
</FilesMatch>
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 06/20/2017 12:23 PM, Jim Jagielski wrote:
> % cat fcgi.pl
> #!/usr/bin/env perl
> <snip>
What requests and server config did you use with this test setup?
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
% cat fcgi.pl
#!/usr/bin/env perl
use FCGI;
use Socket;
use FCGI::ProcManager;
use Data::Dumper;
$num_args = $#ARGV + 1;
if ($num_args != 1) {
print "\nUsage: fcgi.pl <socket>\n";
exit 1;
}
$proc_manager = FCGI::ProcManager->new( {n_processes => 1} );
$socket = FCGI::OpenSocket( $ARGV[0], 10 );
$request = FCGI::Request( \*STDIN, \*STDOUT, \*STDERR, \%req_params,
$socket, &FCGI::FAIL_ACCEPT_ON_INTR );
$proc_manager->pm_manage();
if ($request) {
while ( $request->Accept() >= 0 ) {
$proc_manager->pm_pre_dispatch();
print("Content-type: text/plain\r\n\r\n");
print Dumper(\%req_params);
}
}
FCGI::CloseSocket($socket);
% cat fpm.sh
php-fpm70 -p /usr/local2/php-fpm
% ls /usr/local2/php-fpm
docs/ etc/ fcgi.pl* fpm/ fpm.sh* log/ pools/ run/ var/ www/
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
Ahh... the tests from the orig bug report
> On Jun 20, 2017, at 2:39 PM, Jacob Champion <ch...@gmail.com> wrote:
>
> SCRIPT_NAME
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 06/20/2017 10:55 AM, Jim Jagielski wrote:
> Last I checked, it's in the test framework...
Quick grep for "SCRIPT_NAME" in the tests doesn't turn up anything; can
you point me to it?
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
Last I checked, it's in the test framework...
> On Jun 20, 2017, at 1:42 PM, Jacob Champion <ch...@gmail.com> wrote:
>
> On 06/20/2017 09:47 AM, Jacob Champion wrote:
>> I think. Still trying to context switch back three months.
>
> Jim, do you have a good test case for the current FPM logic so we don't break that with a fix?
>
> --Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 06/20/2017 09:47 AM, Jacob Champion wrote:
> I think. Still trying to context switch back three months.
Jim, do you have a good test case for the current FPM logic so we don't
break that with a fix?
--Jacob
RE: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by "Moradhassel, Kavian" <km...@ciena.com>.
Thanks! That’s exactly the kind of ballpark I was hoping to hear. ☺
From: William A Rowe Jr [mailto:wrowe@rowe-clan.net]
Sent: Tuesday, June 27, 2017 1:15 PM
To: httpd <de...@httpd.apache.org>
Subject: RE: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
On Jun 27, 2017 12:08 PM, "Moradhassel, Kavian" <km...@ciena.com>> wrote:
Did this discussion result in a decision to provide a fix for the bug in 2.4.26 and plan for a 2.4.27 soon? I'm wondering if I should be waiting for a 2.4.27 in the next handful of weeks, or if I should just accept that 2.4.26 has a bug that we need to work around...
We don't generally try to predict our release schedule, when it's baked we publish a release.
But Jim and Jacob are furiously updating the test framework as you were asking your question, so I'm guessing the group will be working to release this fix just as soon as it is agreed upon, within a week or few.
RE: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Jun 27, 2017 12:08 PM, "Moradhassel, Kavian" <km...@ciena.com> wrote:
Did this discussion result in a decision to provide a fix for the bug in
2.4.26 and plan for a 2.4.27 soon? I'm wondering if I should be waiting
for a 2.4.27 in the next handful of weeks, or if I should just accept that
2.4.26 has a bug that we need to work around...
We don't generally try to predict our release schedule, when it's baked we
publish a release.
But Jim and Jacob are furiously updating the test framework as you were
asking your question, so I'm guessing the group will be working to release
this fix just as soon as it is agreed upon, within a week or few.
RE: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by "Moradhassel, Kavian" <km...@ciena.com>.
Did this discussion result in a decision to provide a fix for the bug in 2.4.26 and plan for a 2.4.27 soon? I'm wondering if I should be waiting for a 2.4.27 in the next handful of weeks, or if I should just accept that 2.4.26 has a bug that we need to work around...
Thanks!
-----Original Message-----
From: William A Rowe Jr [mailto:wrowe@rowe-clan.net]
Sent: Tuesday, June 20, 2017 1:00 PM
To: httpd <de...@httpd.apache.org>
Subject: Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
On Tue, Jun 20, 2017 at 11:17 AM, Jacob Champion <ch...@gmail.com> wrote:
> On 06/20/2017 09:16 AM, William A Rowe Jr wrote:
>>
>> It's released into the wild, what is done is done.
>
>
> Of course. But having it in the wild for three days is different than having
> it in the wild for six months.
Unless you are classifying this a security fix, it may prove to be
a less popular update given what we announced as fixed in 2.4.26.
The same isn't expected for 2.4.27, since some users prioritize
security updates and let others sit for a while, or undergo added
scrutiny for non-critical updates.
In any case, it will be in the wild for >3 days. It's at five days and
counting, if you tag and roll today. Vote is 72 hrs long, and we
always give our mirrors a day to catch up with us.
You must presume it is in the wild, and shortening the exposure
by a matter of days isn't significant. It just needs the appropriate
clear note in CHANGES with 2.4.27 so that users are prepared
for the change (or reversion).
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 06/20/2017 10:55 AM, Jim Jagielski wrote:
> It's already in the wild.
We do not guarantee bug compatibility. That's our judgment call based on
adoption, expected user disruption, time in the wild, etc.
The purpose of my Showstopper was to revert to known-good behavior. That
didn't happen, not least because I didn't catch this bug in the
backport, I wanted to keep things moving, and I +1'd it despite the
presence of the rider patch that I tried unsuccessfully to keep out [1].
So that's on me (and that's why I'm grinding this particular axe). I
want to fix the mistake I started last year, and not punish our users
with yet another unusable default for the third release in a row.
--Jacob
[1]
https://lists.apache.org/thread.html/539645ba41021f85a87ad397861cb8d09fb057f7fb7e9ed768781e85@%3Cdev.httpd.apache.org%3E
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Jun 20, 2017, at 1:18 PM, Jacob Champion <ch...@gmail.com> wrote:
>
> On 06/20/2017 10:12 AM, Jim Jagielski wrote:
>> All this is, IMO, moot until we have a *patch*.
>
> Agreed. See my other fork of this thread for my questions on that.
>
>> Right now there
>> is a work-around, which, again IMO, reduces the "need" to release
>> something "now"... the only question is whether the patch changes
>> the behavior of the 2 current settings or whether it adds a 3rd
>> option (the latter is my recommendation).
>
> The faster we can patch, the less there is a need to introduce yet another "ProxyFcgiBackendType FPM-But-Better-And-Without-Bugs-This-Time" option.
>
It's already in the wild.
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 06/20/2017 10:12 AM, Jim Jagielski wrote:
> All this is, IMO, moot until we have a *patch*.
Agreed. See my other fork of this thread for my questions on that.
> Right now there
> is a work-around, which, again IMO, reduces the "need" to release
> something "now"... the only question is whether the patch changes
> the behavior of the 2 current settings or whether it adds a 3rd
> option (the latter is my recommendation).
The faster we can patch, the less there is a need to introduce yet
another "ProxyFcgiBackendType FPM-But-Better-And-Without-Bugs-This-Time"
option.
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Jun 20, 2017 at 1:32 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Tue, Jun 20, 2017 at 12:12 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>>
>>> On Jun 20, 2017, at 1:03 PM, Jacob Champion <ch...@gmail.com> wrote:
>>>
>>> On 06/20/2017 10:00 AM, William A Rowe Jr wrote:
>>>> You must presume it is in the wild, and shortening the exposure
>>>> by a matter of days isn't significant.
>>>
>>> My point is that we should fix it ASAP. Days vs. more days may not be significant, but days vs. months is definitely significant when it comes to bug-compatibility and user expectations.
>>
>> All this is, IMO, moot until we have a *patch*. Right now there
>> is a work-around, which, again IMO, reduces the "need" to release
>> something "now"... the only question is whether the patch changes
>> the behavior of the 2 current settings or whether it adds a 3rd
>> option (the latter is my recommendation).
>
> To your point and Jacob's concern, whatever we settle on, we can
> surely add that upon adoption to
... the http://www.apache.org/dist/httpd/patches/apply_to_2.4.26/ tree
and provide the appropriate pointer on users@ and mention on the
various bugzilla tickets that arise.
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Jun 20, 2017 at 12:12 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>
>> On Jun 20, 2017, at 1:03 PM, Jacob Champion <ch...@gmail.com> wrote:
>>
>> On 06/20/2017 10:00 AM, William A Rowe Jr wrote:
>>> You must presume it is in the wild, and shortening the exposure
>>> by a matter of days isn't significant.
>>
>> My point is that we should fix it ASAP. Days vs. more days may not be significant, but days vs. months is definitely significant when it comes to bug-compatibility and user expectations.
>
> All this is, IMO, moot until we have a *patch*. Right now there
> is a work-around, which, again IMO, reduces the "need" to release
> something "now"... the only question is whether the patch changes
> the behavior of the 2 current settings or whether it adds a 3rd
> option (the latter is my recommendation).
To your point and Jacob's concern, whatever we settle on, we can
surely add that upon adoption to
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Perkins <ja...@cpanel.net>.
As a general FYI: cPanel can’t release 2.4.26 until this is patched / fixed. We were bitten *hard* by the SCRIPT_URI breakage a while back, and this is going to put a blocker on our release.
—
Jacob Perkins
Product Owner
cPanel Inc.
jacob.perkins@cpanel.net <ma...@cpanel.net>
Office: 713-529-0800 x 4046
Cell: 713-560-8655
> On Jun 20, 2017, at 12:12 PM, Jim Jagielski <ji...@jaguNET.com> wrote:
>
>
>> On Jun 20, 2017, at 1:03 PM, Jacob Champion <ch...@gmail.com> wrote:
>>
>> On 06/20/2017 10:00 AM, William A Rowe Jr wrote:
>>> You must presume it is in the wild, and shortening the exposure
>>> by a matter of days isn't significant.
>>
>> My point is that we should fix it ASAP. Days vs. more days may not be significant, but days vs. months is definitely significant when it comes to bug-compatibility and user expectations.
>
> All this is, IMO, moot until we have a *patch*. Right now there
> is a work-around, which, again IMO, reduces the "need" to release
> something "now"... the only question is whether the patch changes
> the behavior of the 2 current settings or whether it adds a 3rd
> option (the latter is my recommendation).
>
> And we should update ./t/modules/proxy_fcgi.t and maybe create
> a test which runs if PHP is available.
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Jun 20, 2017, at 1:03 PM, Jacob Champion <ch...@gmail.com> wrote:
>
> On 06/20/2017 10:00 AM, William A Rowe Jr wrote:
>> You must presume it is in the wild, and shortening the exposure
>> by a matter of days isn't significant.
>
> My point is that we should fix it ASAP. Days vs. more days may not be significant, but days vs. months is definitely significant when it comes to bug-compatibility and user expectations.
All this is, IMO, moot until we have a *patch*. Right now there
is a work-around, which, again IMO, reduces the "need" to release
something "now"... the only question is whether the patch changes
the behavior of the 2 current settings or whether it adds a 3rd
option (the latter is my recommendation).
And we should update ./t/modules/proxy_fcgi.t and maybe create
a test which runs if PHP is available.
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 06/20/2017 10:00 AM, William A Rowe Jr wrote:
> You must presume it is in the wild, and shortening the exposure
> by a matter of days isn't significant.
My point is that we should fix it ASAP. Days vs. more days may not be
significant, but days vs. months is definitely significant when it comes
to bug-compatibility and user expectations.
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Jun 20, 2017 at 11:17 AM, Jacob Champion <ch...@gmail.com> wrote:
> On 06/20/2017 09:16 AM, William A Rowe Jr wrote:
>>
>> It's released into the wild, what is done is done.
>
>
> Of course. But having it in the wild for three days is different than having
> it in the wild for six months.
Unless you are classifying this a security fix, it may prove to be
a less popular update given what we announced as fixed in 2.4.26.
The same isn't expected for 2.4.27, since some users prioritize
security updates and let others sit for a while, or undergo added
scrutiny for non-critical updates.
In any case, it will be in the wild for >3 days. It's at five days and
counting, if you tag and roll today. Vote is 72 hrs long, and we
always give our mirrors a day to catch up with us.
You must presume it is in the wild, and shortening the exposure
by a matter of days isn't significant. It just needs the appropriate
clear note in CHANGES with 2.4.27 so that users are prepared
for the change (or reversion).
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 06/20/2017 09:16 AM, William A Rowe Jr wrote:
> It's released into the wild, what is done is done.
Of course. But having it in the wild for three days is different than
having it in the wild for six months.
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Jun 20, 2017 at 11:15 AM, Jacob Champion <ch...@gmail.com> wrote:
> On 06/20/2017 09:14 AM, William A Rowe Jr wrote:
>>
>> Would encourage us to wait at least a couple more days for
>> other, unrelated regression reports to filter in while fixing this
>> defect. But there is nothing stopping a 2.4.27 in rapid
>> succession, we simply don't retroactively "retract" releases.
>
>
> Right, re-issuing 2.4.26 isn't what I meant. I just want to patch this up as
> quickly as possible so people don't standardize on the new behavior.
It's released into the wild, what is done is done.
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 06/20/2017 09:14 AM, William A Rowe Jr wrote:
> Would encourage us to wait at least a couple more days for
> other, unrelated regression reports to filter in while fixing this
> defect. But there is nothing stopping a 2.4.27 in rapid
> succession, we simply don't retroactively "retract" releases.
Right, re-issuing 2.4.26 isn't what I meant. I just want to patch this
up as quickly as possible so people don't standardize on the new behavior.
--Jaccob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Jun 20, 2017 at 11:07 AM, Jacob Champion <ch...@gmail.com> wrote:
> On 02/08/2017 07:56 PM, Eric Covener wrote:
>>
>> Assuming there's some alternate path that actually does change
>> SCRIPT_NAME by default, we a) don't have any complaint about
>> SCRIPT_NAME and b) have the SetEnv thing. If we want more options,
>> maybe we can stash this older SCRIPT_NAME into a new variable and show
>> how to copy it over SCRIPT_NAME?
>
>
> ...and restarting this conversation, since the new behavior seems to have a
> bug:
>
> https://bz.apache.org/bugzilla/show_bug.cgi?id=61202
>
> Can we do a quick fixup and reroll before it's too late?
Define reroll? too late?
I know what you mean by fixup, hopefully not so quick as to
overlook other tangential issues. But we never 're-roll', we
discard the candidate and roll the next version (this assures
that whomever has httpd-2.4.26.tar.gz in their hands has the
one and only one package of that name that was ever created.)
And 2.4.26 is released, announcement is away.
Would encourage us to wait at least a couple more days for
other, unrelated regression reports to filter in while fixing this
defect. But there is nothing stopping a 2.4.27 in rapid
succession, we simply don't retroactively "retract" releases.
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 06/20/2017 09:07 AM, Jacob Champion wrote:
> Can we do a quick fixup and reroll before it's too late?
And... what *is* the fixup?
It looks like the logic here -- where we start at the end of SCRIPT_NAME
(or the beginning of PATH_INFO) and work backwards -- is designed to
stop at the first directory separator no matter what, which I completely
missed in my review. It needs to stop wherever the "URL" part of the
path ends and the "filesystem" part of the path begins.
I think. Still trying to context switch back three months.
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 02/08/2017 07:56 PM, Eric Covener wrote:
> Assuming there's some alternate path that actually does change
> SCRIPT_NAME by default, we a) don't have any complaint about
> SCRIPT_NAME and b) have the SetEnv thing. If we want more options,
> maybe we can stash this older SCRIPT_NAME into a new variable and show
> how to copy it over SCRIPT_NAME?
...and restarting this conversation, since the new behavior seems to
have a bug:
https://bz.apache.org/bugzilla/show_bug.cgi?id=61202
Can we do a quick fixup and reroll before it's too late?
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Eric Covener <co...@gmail.com>.
On Wed, Feb 8, 2017 at 3:31 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> I have. With PHP-FPM as well as that Perl script FPM. All works
> as expected.
In my env, the change doesn't result in any change to SCRIPT_NAME. I
only test w/ the perl script and use *.phx vs *.php to see both paths.
AcceptPathInfo ON
AddType application/x-php7-fpm .php
Action application/x-php7-fpm /fpm virtual
<Location /fpm>
SetHandler "proxy:fcgi://localhost:12345"
</Location>
<FilesMatch \.phx$>
SetHandler "proxy:fcgi://127.0.0.1:12345"
</FilesMatch>
If I hit /info.phx/path.info, SCRIPT_NAME is /info.phx and stays that way.
If I hit /info.php/path/info, SCRIPT_NAME starts /fpm and stays that
way (if using FPM, it would see REDIRECT_* and figure out our real
paths)
(added orig string to trace locally):
[Thu Feb 09 03:47:01.331270 2017] [proxy_fcgi:trace4] [pid 26390:tid
139707007817472] mod_proxy_fcgi.c(392): [client 127.0.0.1:43100]
fpm:virtual_script: Modified SCRIPT_NAME from /info.phx to: /info.phx
[Thu Feb 09 03:49:54.050047 2017] [proxy_fcgi:trace4] [pid 26390:tid
139706991032064] mod_proxy_fcgi.c(392): [client 127.0.0.1:43168]
fpm:virtual_script: Modified SCRIPT_NAME from /fpm to: /fpm
Assuming there's some alternate path that actually does change
SCRIPT_NAME by default, we a) don't have any complaint about
SCRIPT_NAME and b) have the SetEnv thing. If we want more options,
maybe we can stash this older SCRIPT_NAME into a new variable and show
how to copy it over SCRIPT_NAME?
--
Eric Covener
covener@gmail.com
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Luca Toscano <to...@gmail.com>.
2017-02-08 22:37 GMT+01:00 Jacob Champion <ch...@gmail.com>:
> On 02/08/2017 01:32 PM, Jim Jagielski wrote:
>
>> I am confused then... what else are you proposing? Eric's envar fix allows
>> for people to basically adjust at their whim. What else is needed??
>>
>
> In my view, nothing else is needed. We revert my changes (and the followup
> query string modification) in 2.4.x, and backport Eric's envvar directive
> by itself to make up for it.
>
> We can sit on other SCRIPT_FILENAME changes, and the Backend directive, in
> trunk until PHP wants to sit down with us to decide what SCRIPT_FILENAME
> actually means to FPM.
>
>
FWIW my 2 cents: Jim's view and Jacob/Eric's are all valuable, but since
there is a lot of instability for mod_proxy_fcgi I'd like more to proceed
with the minimal set of changes for the next 2.4 release. We revert to a
known "good" state, we introduce the new ProxyFCGISetEnvIf directive and
listen to what our users think about it. In the meantime, we work with PHP
on Jim's solution and we think about a second backport later on if needed.
Everybody is happy :)
Luca
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 02/08/2017 01:32 PM, Jim Jagielski wrote:
> I am confused then... what else are you proposing? Eric's envar fix allows
> for people to basically adjust at their whim. What else is needed??
In my view, nothing else is needed. We revert my changes (and the
followup query string modification) in 2.4.x, and backport Eric's envvar
directive by itself to make up for it.
We can sit on other SCRIPT_FILENAME changes, and the Backend directive,
in trunk until PHP wants to sit down with us to decide what
SCRIPT_FILENAME actually means to FPM.
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Feb 8, 2017, at 4:09 PM, Jacob Champion <ch...@gmail.com> wrote:
>
> On 02/08/2017 12:31 PM, Jim Jagielski wrote:
>> Have you even TRIED it?
>
> Yes, with the latest trunk, a config of
>
> <FilesMatch "\.php$">
> SetHandler "proxy:fcgi://localhost:10102/"
> </FilesMatch>
>
> leads to a SCRIPT_FILENAME of 'proxy:fcgi://localhost:10102//tmp/apache-svn-trunk/htdocs/hello.php'.
>
>> I have. With PHP-FPM as well as that Perl script FPM. All works
>> as expected.
>
> I understand that it works for the use cases you have tested. My change worked for the use cases I tested, and Eric's follow-up change worked for the use cases he tested, but we keep finding new regressions. This is a high-risk code path, and the proposed backport makes it more complex instead of less IMO.
>
> There's a good chance that the core of your addition will be part of the eventual solution. My point is that without fixing the other stuff at the same time, we've just added yet another type of almost-correct behavior that we and the FCGI implementations have to code around.
>
I am confused then... what else are you proposing? Eric's envar fix allows
for people to basically adjust at their whim. What else is needed??
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 02/08/2017 12:31 PM, Jim Jagielski wrote:
> Have you even TRIED it?
Yes, with the latest trunk, a config of
<FilesMatch "\.php$">
SetHandler "proxy:fcgi://localhost:10102/"
</FilesMatch>
leads to a SCRIPT_FILENAME of
'proxy:fcgi://localhost:10102//tmp/apache-svn-trunk/htdocs/hello.php'.
> I have. With PHP-FPM as well as that Perl script FPM. All works
> as expected.
I understand that it works for the use cases you have tested. My change
worked for the use cases I tested, and Eric's follow-up change worked
for the use cases he tested, but we keep finding new regressions. This
is a high-risk code path, and the proposed backport makes it more
complex instead of less IMO.
There's a good chance that the core of your addition will be part of the
eventual solution. My point is that without fixing the other stuff at
the same time, we've just added yet another type of almost-correct
behavior that we and the FCGI implementations have to code around.
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
Have you even TRIED it?
I have. With PHP-FPM as well as that Perl script FPM. All works
as expected.
> On Feb 8, 2017, at 3:27 PM, Jacob Champion <ch...@gmail.com> wrote:
>
> On 02/08/2017 12:10 PM, Jim Jagielski wrote:
>> Doesn't the below make it work without changes.
>>
>> #define FCGI_MAY_BE_FPM(dconf) \
>> (dconf && \
>> ((dconf->backend_type == BACKEND_DEFAULT_UNKNOWN) || \
>> (dconf->backend_type == BACKEND_FPM)))
>
> No. (To "make it work" we have to cover a gigantic number of use cases, not just PHP-FPM, which is probably why we're talking past each other.)
>
> With your proposed backport, any users who were able to start using FastCGI backends that required an "actual" SCRIPT_FILENAME, in the 2.4.21 or later timeframe, will still have to modify their configs to specify a generic FastCGI backend so that the "proxy:fcgi" stripping can kick in. Existing PHP-FPM users don't change their config, sure, but they'll see yet another behavior change because of the new fixup, which is just too risky IMO.
>
> Compare that to a complete reversion to the previous behavior, plus the addition of the new FCGI SetEnv directive. The users requiring an "actual" SCRIPT_FILENAME will be able to set it manually, which is no worse than having to set the new Backend directive (better, actually, because SCRIPT_FILENAME has no globally accepted meaning). Existing PHP-FPM users are guaranteed the *exact* behavior they saw before 2.4.21, making it less risky to upgrade.
>
> --Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 02/08/2017 12:10 PM, Jim Jagielski wrote:
> Doesn't the below make it work without changes.
>
> #define FCGI_MAY_BE_FPM(dconf) \
> (dconf && \
> ((dconf->backend_type == BACKEND_DEFAULT_UNKNOWN) || \
> (dconf->backend_type == BACKEND_FPM)))
No. (To "make it work" we have to cover a gigantic number of use cases,
not just PHP-FPM, which is probably why we're talking past each other.)
With your proposed backport, any users who were able to start using
FastCGI backends that required an "actual" SCRIPT_FILENAME, in the
2.4.21 or later timeframe, will still have to modify their configs to
specify a generic FastCGI backend so that the "proxy:fcgi" stripping can
kick in. Existing PHP-FPM users don't change their config, sure, but
they'll see yet another behavior change because of the new fixup, which
is just too risky IMO.
Compare that to a complete reversion to the previous behavior, plus the
addition of the new FCGI SetEnv directive. The users requiring an
"actual" SCRIPT_FILENAME will be able to set it manually, which is no
worse than having to set the new Backend directive (better, actually,
because SCRIPT_FILENAME has no globally accepted meaning). Existing
PHP-FPM users are guaranteed the *exact* behavior they saw before
2.4.21, making it less risky to upgrade.
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Feb 8, 2017, at 2:49 PM, Jacob Champion <ch...@gmail.com> wrote:
>
> On 02/08/2017 11:32 AM, Jim Jagielski wrote:
>> It does it automatically requiring no config-file changes
>> to the end user.
>
> Anyone picking up the SCRIPT_FILENAME change (which was my change that started this whole mess) still has to change their backend type manually, so I'm not convinced.
>
??
Doesn't the below make it work without changes.
#define FCGI_MAY_BE_FPM(dconf) \
(dconf && \
((dconf->backend_type == BACKEND_DEFAULT_UNKNOWN) || \
(dconf->backend_type == BACKEND_FPM)))
Unless you mean that those who "fixed" their configs due to 2.4.24/25
will need to do so again?
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 02/08/2017 11:32 AM, Jim Jagielski wrote:
> It does it automatically requiring no config-file changes
> to the end user.
Anyone picking up the SCRIPT_FILENAME change (which was my change that
started this whole mess) still has to change their backend type
manually, so I'm not convinced.
But reverting to previous behavior *is* an automatic fix, since we know
PHP-FPM already performs its fixups. Then people can manually edit
SCRIPT_FILENAME if needed until we get our act together alongside PHP.
We don't even have to ship the Backend directive until we're sure that's
the direction we want to go.
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Feb 8, 2017, at 1:08 PM, Jacob Champion <ch...@gmail.com> wrote:
>
> On 02/08/2017 07:52 AM, Eric Covener wrote:
>> My concern is that nobody ever reported this config as broken and
>> there's no telling what any on-by-default change here can break.
>
> +1.
>
> Maybe a different way to put it: what does this approach solve that a revert-to-previous-behavior + FCGI-SetEnv doesn't solve already?
>
It does it automatically requiring no config-file changes
to the end user.
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jacob Champion <ch...@gmail.com>.
On 02/08/2017 07:52 AM, Eric Covener wrote:
> My concern is that nobody ever reported this config as broken and
> there's no telling what any on-by-default change here can break.
+1.
Maybe a different way to put it: what does this approach solve that a
revert-to-previous-behavior + FCGI-SetEnv doesn't solve already?
--Jacob
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
Just to be clear, I simply *tested* against that config... the patch
is not designed to do anything with it. It was basically to ensure
that it introduced no regressions.
Re: svn commit: r1782209 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Eric Covener <co...@gmail.com>.
On Wed, Feb 8, 2017 at 10:45 AM, Jim Jagielski <ji...@jagunet.com> wrote:
> Which ones? You mean the special case one?
>
> Would it be better if we did that *before* calling
> the fix_cgivars(). What is there makes the following
> config pair work as-is:
>
> AddType application/x-php7-fpm .php
> Action application/x-php7-fpm /fpm virtual
> <Location /fpm>
> SetHandler proxy:fcgi://localhost:9001
> </Location>
>
> --
>
> <FilesMatch \.php$>
> SetHandler "proxy:fcgi://localhost:9001
> </FilesMatch>
My concern is that nobody ever reported this config as broken and
there's no telling what any on-by-default change here can break.