Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Apache 2.4.10+ SetHandler proxy:fcgi:// incompatibilities #694

Closed
wants to merge 1 commit into from

Conversation

8 participants
@dzuelke
Copy link
Contributor

commented Jun 12, 2014

Apache 2.4.10+ will allow the following:

<FilesMatch \.php$>
SetHandler proxy:fcgi://localhost:9000
</FilesMatch>

This is much easier than using ProxyPassMatch (which prevents rewriting and other stuff) and rewrites (which are a bag of hurt because when combined with user-land .htaccess rewrites, there's always rewrite loops, prefix breakage etc (I've tried, for weeks).

It's basically the future of using Apache (via mod_proxy_fcgi) together with PHP-FPM. It's also available for older versions as a standalone module, very easy to install: https://gist.github.com/progandy/6ed4eeea60f6277c3e39

However, the two bits of code this commit deletes interfere with that. They both cover CGI-only mode and were copied from that SAPI into the FPM source. See e.g. https://bugs.php.net/bug.php?id=47042

The first deleted part mangled SCRIPT_NAME if something like

RewriteCond %{REQUEST_FILENAME} !-f
RewriteRule (.*) index.php/$1 [L]

is used (i.e. rewriting to PATH_INFO. The second part drops PATH_INFO if there was a REDIRECT_URL (with CGI mode, SCRIPT_FILENAME in Apache is the path to the PHP binary, and PATH_INFO contains the name of the script to run).

Clearly, neither applies in the case of FastCGI, so both are safe to delete.

Fix SetHandler proxy:fcgi:// incompatibilities
Apache 2.4.10+ will allow the following:

```
<FilesMatch \.php$>
SetHandler proxy:fcgi://localhost:9000
</FilesMatch>
```

This is much easier than using `ProxyPassMatch` (which prevents rewriting and other stuff) and rewrites (which are a bag of hurt because when combined with user-land `.htaccess` rewrites, there's always rewrite loops, prefix breakage etc (I've tried, for weeks).

It's basically the future of using Apache (via `mod_proxy_fcgi`) together with PHP-FPM. It's also available for older versions as a standalone module, very easy to install: https://gist.github.com/progandy/6ed4eeea60f6277c3e39

However, the two bits of code this commit deletes interfere with that. They both cover CGI-only mode and were copied from that SAPI into the FPM source. See e.g. https://bugs.php.net/bug.php?id=47042

The first deleted part mangled `SCRIPT_NAME` if something like

```
RewriteCond %{REQUEST_FILENAME} !-f
RewriteRule (.*) index.php/$1 [L]
```

is used (i.e. rewriting to `PATH_INFO`. The second part drops `PATH_INFO` if there was a `REDIRECT_URL` (with CGI mode, `SCRIPT_FILENAME` in Apache is the path to the PHP binary, and `PATH_INFO` contains the name of the script to run).

Clearly, neither applies in the case of FPM, so both are safe to delete.
@php-pulls

This comment has been minimized.

Copy link

commented Jun 29, 2014

Comment on behalf of tyrael at php.net:

I've merged it into PHP-5.6

@php-pulls php-pulls closed this Jun 29, 2014

@dzuelke

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2014

Thanks for the merge @Tyrael but this addresses a different issue than bug 65641. That one is still unaddressed and should be re-opened.

The NEWS entry for this PR should be something like "Support Apache 2.4.10+ SetHandler method for mod_proxy_fcgi and PHP-FPM".

@Tyrael Tyrael reopened this Jun 30, 2014

@php-pulls

This comment has been minimized.

Copy link

commented Jun 30, 2014

Comment on behalf of tyrael at php.net:

closing again, had to open so I can link the PR from the newly created https://bugs.php.net/bug.php?id=67541

@php-pulls php-pulls closed this Jun 30, 2014

@Tyrael

This comment has been minimized.

Copy link
Member

commented Jun 30, 2014

I've created a separate bug for this, linked the pr from the bug and updated the NEWS entry.

@pbowyer

This comment has been minimized.

Copy link

commented Jul 10, 2014

I can confirm this patch works. I'm using Apache 2.4.9 from ppa:ondrej/apache2 w mod_proxy_handler manually compiled.

So others can verify my findings I've posted my test script at https://gist.github.com/pbowyer/bba54cbf01cf6b250c71.

Results

Under PHP 5.6.0RC2:

PHP Version: 5.6.0RC2
ORIG_SCRIPT_NAME: 
SCRIPT_NAME: /index.php
ORIG_SCRIPT_FILENAME: 
SCRIPT_FILENAME: /var/www/example.com/htdocs/index.php
PATH_INFO: /sample/test
ORIG_PATH_INFO: 

Under all previous versions of PHP (tested 5.5.10 and 5.3.28):

File not found.

I would love to see this backported as far as PHP 5.3. If not I'm going to have to manually patch it.

dzuelke added a commit to dzuelke/php-src that referenced this pull request Aug 9, 2014

Tyrael added a commit that referenced this pull request Aug 12, 2014

Merge branch 'pr-765' into PHP-5.6
* pr-765:
  NEWS entry for e6d93a1 / d73d44c
  restore FPM compatibility with mod_fastcgi broken since #694 / 67541, fixes bug 67606
  Revert "Merge branch 'pull-request/694' into PHP-5.6"

Tyrael added a commit that referenced this pull request Aug 12, 2014

Merge branch 'PHP-5.6'
* PHP-5.6:
  NEWS entry for e6d93a1 / d73d44c
  restore FPM compatibility with mod_fastcgi broken since #694 / 67541, fixes bug 67606
  Revert "Merge branch 'pull-request/694' into PHP-5.6"

Tyrael added a commit that referenced this pull request Aug 14, 2014

Merge branch 'PHP-5.6' into PHP-5.6.0
* PHP-5.6: (60 commits)
  new NEWS block for the next release
  Updated NEWS for #66091
  Updated NEWS for #66091
  Fixed #66091
  updated NEWS
  updated NEWS
  updated NEWS
  backported the fix for bug #41577
  fix the failing date tests introduced with the latest timezonedb update Derick confirmed on irc that the new/current behavior is the correct and that the tests should be updated to reflect it
  Fixed bug #67813 (CachingIterator::__construct InvalidArgumentException wrong message)
  NEWS entry for e6d93a1 / d73d44c
  restore FPM compatibility with mod_fastcgi broken since #694 / 67541, fixes bug 67606
  Revert "Merge branch 'pull-request/694' into PHP-5.6"
  Add __debugInfo() to UPGRADING.
  fix TS build
  Update NEWS
  Update NEWS
  Update NEWS
  Bug #41631: Observe socket read timeouts in SSL streams
  wrap int8_t and int16_t with #ifdef to avoid possible clashes
  ...

Conflicts:
	NEWS
@dzuelke

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2014

Revised fix in #765

php-pulls pushed a commit that referenced this pull request Aug 14, 2014

Merge branch 'master' into phpng
* master: (51 commits)
  Update Git rules
  Back to -dev (with EOL notice in NEWS)
  new NEWS block for the next release
  It's 2014 already, fix copyright year where user visible
  PHP 5.3.29
  Some changes were lost in the merge commit of #66091
  Updated NEWS for #66091
  Fixed #66091
  Updated NEWS for #66091
  Updated NEWS for #66091
  Fixed #66091
  updated NEWS
  updated NEWS
  updated NEWS
  backported the fix for bug #41577
  NEWS entry for e6d93a1 / d73d44c
  restore FPM compatibility with mod_fastcgi broken since #694 / 67541, fixes bug 67606
  Revert "Merge branch 'pull-request/694' into PHP-5.6"
  PHP 5.3.29RC1
  Fix missing type checks in various functions
  ...

Conflicts:
	ext/date/php_date.c
	ext/standard/math.c

php-pulls pushed a commit that referenced this pull request Aug 29, 2014

php-pulls pushed a commit that referenced this pull request Aug 29, 2014

Merge branch 'PHP-5.4' into PHP-5.5
* PHP-5.4:
  fix NEWS for fcgi fix merge
  restore FPM compatibility with mod_fastcgi broken since #694 / 67541, fixes bug 67606

php-pulls pushed a commit that referenced this pull request Aug 29, 2014

Merge branch 'PHP-5.5' into PHP-5.6
* PHP-5.5:
  fix NEWS for fcgi fix merge
  restore FPM compatibility with mod_fastcgi broken since #694 / 67541, fixes bug 67606

php-pulls pushed a commit that referenced this pull request Aug 29, 2014

Merge branch 'PHP-5.6'
* PHP-5.6:
  fix NEWS for fcgi fix merge
  restore FPM compatibility with mod_fastcgi broken since #694 / 67541, fixes bug 67606
@smtalk

This comment has been minimized.

Copy link

commented Sep 10, 2014

It still doesn't work well according to Apache documentation. It adds an additional trailing slash (/) at the beginning of SCRIPT_FILENAME if SetHandler ends with a trailing slash like in http://httpd.apache.org/docs/current/mod/mod_proxy.html#handler:
SetHandler "proxy:unix:/path/to/app.sock|fcgi://localhost/"

The following one has no problems, of course:
SetHandler "proxy:unix:/path/to/app.sock|fcgi://localhost"

So, currently we still have the following problem: https://bugs.php.net/bug.php?id=67998

@dzuelke

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2014

Those docs are wrong. Just omit the trailing slash. Either way, that is a separate issue from what this pull request addresses, @smtalk

@fkooman

This comment has been minimized.

Copy link

commented Dec 14, 2016

On:

  • httpd-2.4.23-5.fc25.x86_64
  • php-fpm-7.0.14-1.fc25.x86_64

I'm still running into some issues with mod_proxy_fcgi, sockets, SetHandler and mod_rewrite:

<Directory "/var/www/html/fkooman">
    RewriteEngine on
    RewriteBase /fkooman
    RewriteCond %{REQUEST_FILENAME} !-f
    RewriteCond %{REQUEST_FILENAME} !-d
    RewriteRule ^(.*)$ index.php/$1 [L,QSA]
    <Files "index.php">
        SetHandler "proxy:unix:/run/php-fpm/www.sock|fcgi://localhost"
    </Files>
</Directory>

If I request http://localhost/fkooman/index.php/foo/bar PATH_INFO is set to /foo/bar as expected, and in the php-fpm access log I see this: - - 14/Dec/2016:23:48:31 +0100 "GET /fkooman/index.php" 200 which seems fine.

But if I don't specify index.php, i.e. go to http://localhost/fkooman/foo/bar the rewrite does not seem to work properly, or something else is weird, and in the php-fpm access log I get: - - 14/Dec/2016:23:55:03 +0100 "GET /fkooman/foo/bar" 404. it seems the index.php is dropped from the request URL, but obviously it is matched by the Apache config, otherwise php-fpm would not see it...

I also tested with mod_php, and then the rewriting works fine and whether or not I add index.php to the URL or not makes no difference. Am I missing something? Is this an Apache issue?

@jchampio

This comment has been minimized.

Copy link

commented Jan 11, 2017

@fkooman This is probably not the right place for extended discussion, but the patch we made in 2.4.21 to fix other FCGI backends with mod_proxy_fcgi broke the detection code @dzuelke was messing with in this series of patches.

See apache/httpd@cab0bfb#commitcomment-20393588 and https://bz.apache.org/bugzilla/show_bug.cgi?id=59618 . We've written ourselves into a mutually dependent corner here and I'm trying to see if there's a way back out.

@dzuelke

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2017

What's the plan forward then, @jchampio... will this be reverted for 2.4.26?

@jchampio

This comment has been minimized.

Copy link

commented Jan 16, 2017

@dzuelke Yes. 2.4.26 won't release without a reversion unless my showstopper gets vetoed, and trunk contains a patch from @covener to hide these changes behind a (non-default) configuration directive.

As for my plan forward (which is not guaranteed to be the plan forward):

  • I want to fix our FastCGI modules to conform to the CGI 1.1 specification, per this infamous long-standing bug filed by one of the PHP-FPM devs. This is likely to be a long, twisty road and will not (cannot) take effect by default until we get our next version bump. There's an excellent chance that it will break the third-party-and-unmaintained mod_fastcgi, which for some reason is still very popular.

  • That's still not good enough, because SCRIPT_FILENAME is not defined in the 1.1 spec. We all need to agree on what SCRIPT_FILENAME means. PHP appears to have defined it as "the filesystem-translated version of SCRIPT_NAME" (in other words, SCRIPT_FILENAME is to SCRIPT_NAME as PATH_TRANSLATED is to PATH_INFO). As far as I can tell, when Apache introduced this non-standard variable, it was defined to be "whatever is in our internal r->filename struct member". Since this is an internal implementation detail, it gets set to all manner of things that are not useful for the outside world. This impedance mismatch seems to have caused a lot of pain.

Just those two alone will require a pretty good conversation. Is there a better place than your mailing list to have it?

@dzuelke

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2017

I asked myself the same question. I'll surface this on the PHP internals list to see if people have any preference.

@dzuelke

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2017

Btw @jchampio, this PR here broke mod_fastcgi back in the day, which is why the detection logic is now so complicated; see #765

@jimjag

This comment has been minimized.

Copy link

commented Jan 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.