Summary: Broken FastCGI with httpd

classic Classic list List threaded Threaded
59 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Summary: Broken FastCGI with httpd

Jacob Champion-2
(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
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>:
>
> (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

Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

Jim Jagielski
It seems to me that SCRIPT_FILENAME is the key w/ PHP-FPM, but
I could be wrong.

Also, the fact that different methods of invoking FCGI result
in different vars, at 1st blush, doesn't seem "incorrect"
assuming that each difference makes some sense, in a way.

Finally, I think that instead of trying to address these in
the various proxy modules/submodules, etc, we should do so
in util_script.c instead. One central place where we "canonically"
set these params, and handle the different ways r->filename may
be mangled ;)
Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

David Zuelke
In reply to this post by Jacob Champion-2
> On 17.01.2017, at 23:16, Jacob Champion <[hidden email]> wrote:
>
> (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.)

Thanks for picking this up!


> 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.

There's one more caveat around SCRIPT_FILENAME, I think: it might not be the same for httpd and the FCGI backend if they're running on separate machines!

David

Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

Eric Covener
On Wed, Jan 18, 2017 at 9:47 AM, David Zuelke <[hidden email]> wrote:
> There's one more caveat around SCRIPT_FILENAME, I think: it might not be the same for httpd and the FCGI backend if they're running on separate machines!

I think this goes to the notion of overriding these variables w/
config rather than trying to coax the server to try to construct them
correctly by rewriting.

Unfortunately we might need our own directives for the overrides here
to get them to run after the normal vars are calculated and only when
we're in the proxy_fcgi  handler.

--
Eric Covener
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

Jim Jagielski
In reply to this post by David Zuelke
Just some additional info (the perl script described might be useful,
esp if we fold it into the test framework):

    https://bugs.php.net/bug.php?id=54152


> On Jan 18, 2017, at 9:47 AM, David Zuelke <[hidden email]> wrote:
>
>> On 17.01.2017, at 23:16, Jacob Champion <[hidden email]> wrote:
>>
>> (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.)
>
> Thanks for picking this up!
>
>
>> 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.
>
> There's one more caveat around SCRIPT_FILENAME, I think: it might not be the same for httpd and the FCGI backend if they're running on separate machines!
>
> David
>

Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

Jacob Champion-2
In reply to this post by Jim Jagielski
On 01/18/2017 06:43 AM, Jim Jagielski wrote:
> Also, the fact that different methods of invoking FCGI result
> in different vars, at 1st blush, doesn't seem "incorrect"
> assuming that each difference makes some sense, in a way.

They don't make sense. For example, mod_proxy_fcgi can be set up in a
way that mirrors the mod_fastcgi preferred configuration, using an Action:

   AddType application/x-php7-fpm .php
   Action application/x-php7-fpm /php7-fpm virtual
   <Location /php7-fpm>
     SetHandler proxy:fcgi://localhost:9000
   </Location>

This results in the following variables when I access the URL
'/hello.php/path/info?query':

   SCRIPT_FILENAME: /var/www/php7-fpm
   SCRIPT_NAME:     /php7-fpm
   PATH_INFO:       /hello.php/path/info
   PATH_TRANSLATED: /var/www/hello.php/path/info
   QUERY_STRING:    query

We only get QUERY_STRING right. (Well, I won't say SCRIPT_FILENAME is
"wrong", since there is no standard definition, but it's not helpful.)
SCRIPT_NAME is supposed to point to a possible script-path (see RFC 3875
for definitions), '/hello.php' in this instance. PATH_INFO and
PATH_TRANSLATED should not include 'hello.php' in their values.

It's not just Action. PR 51517 contains examples using ProxyPassMatch.

--Jacob
Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

Jacob Champion-2
In reply to this post by Eric Covener
On 01/18/2017 06:56 AM, Eric Covener wrote:
> Unfortunately we might need our own directives for the overrides here
> to get them to run after the normal vars are calculated and only when
> we're in the proxy_fcgi  handler.

See https://bz.apache.org/bugzilla/show_bug.cgi?id=28903 for an older
conversation about this.

--Jacob
Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

Jacob Champion-2
In reply to this post by Stefan Eissing
On 01/17/2017 11:46 PM, Stefan Eissing wrote:
> Would it make sense to narrow down the setups to a few blessed ones that become part of our tests?

I think so. Problem is that I don't, myself, know what is actually in
use out there. I keep learning about new ways to invoke CGI every month,
it seems.

If anyone has a test matrix they're using for themselves, or even just a
good list of "ways to configure (Fast)CGI", that'd be really useful.

--Jacob
Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

Jacob Champion-2
In reply to this post by David Zuelke
On 01/18/2017 06:47 AM, David Zuelke wrote:
> Thanks for picking this up!

And thank you for driving this on the PHP side!

--Jacob

Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

Jim Jagielski
In reply to this post by Jacob Champion-2
The thing is that when we are proxying back, some of those
values can't make sense, since, for example, there is no
"real" path on the httpd server that maps to the actual
request file....

I think the original idea was to simply send the full URL to
the FCGI server, to let it parse things out for itself...
After all, it's easier for the FCGI server to know the SCRIPT_NAME
than httpd to "guess"... This was, iirc, the initial
concept related to how to leverage SCRIPT_FILENAME on
the PHP side.

What we need is some specific understanding between, in this
case, PHP-FPM and httpd on what it wants/expects/needs regarding
these env-vars. Yeah, SCRIPT_FILENAME seems core to this, I
think.

> On Jan 18, 2017, at 2:01 PM, Jacob Champion <[hidden email]> wrote:
>
> On 01/18/2017 06:43 AM, Jim Jagielski wrote:
>> Also, the fact that different methods of invoking FCGI result
>> in different vars, at 1st blush, doesn't seem "incorrect"
>> assuming that each difference makes some sense, in a way.
>
> They don't make sense. For example, mod_proxy_fcgi can be set up in a way that mirrors the mod_fastcgi preferred configuration, using an Action:
>
>  AddType application/x-php7-fpm .php
>  Action application/x-php7-fpm /php7-fpm virtual
>  <Location /php7-fpm>
>    SetHandler proxy:fcgi://localhost:9000
>  </Location>
>
> This results in the following variables when I access the URL '/hello.php/path/info?query':
>
>  SCRIPT_FILENAME: /var/www/php7-fpm
>  SCRIPT_NAME:     /php7-fpm
>  PATH_INFO:       /hello.php/path/info
>  PATH_TRANSLATED: /var/www/hello.php/path/info
>  QUERY_STRING:    query
>
> We only get QUERY_STRING right. (Well, I won't say SCRIPT_FILENAME is "wrong", since there is no standard definition, but it's not helpful.) SCRIPT_NAME is supposed to point to a possible script-path (see RFC 3875 for definitions), '/hello.php' in this instance. PATH_INFO and PATH_TRANSLATED should not include 'hello.php' in their values.

That is because we have no idea what SCRIPT_NAME is... With the "guess" that
it is /php7-fpm, then PATH_INFO kind of makes sense, since it is
the portion of the URI following the script. And considering
that we use

   Action application/x-php7-fpm /php7-fpm virtual

the 2nd parameter is specifically noted as *being the cgi-script* and
so of course httpd assumes that /php7-fpm is SCRIPT_NAME, because
we explicitly called it that :)


>
> It's not just Action. PR 51517 contains examples using ProxyPassMatch.
>
> --Jacob

Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

Jim Jagielski
Would it make sense, mostly from a PHP point-of-view, to
send something like SCRIPT_FILENAME_RAW (or even
APACHE_SCRIPT_FILENAME)... Or does that simply
complicate an already fuzzy and nebulous situation?

Is [1] being used as the canonical place for this
discussion w/ the PHP Group?

1. https://github.com/php/php-src/pull/694#issuecomment-271963956
Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

Jacob Champion-2
In reply to this post by Jim Jagielski
On 01/18/2017 01:00 PM, Jim Jagielski wrote:
> After all, it's easier for the FCGI server to know the SCRIPT_NAME
> than httpd to "guess"...

I think the recent breakage calls this assumption into question. The
server admin knows exactly where the scripts are in the use cases we're
currently discussing; why should the backend have to "know" anything in
those situations?

It's worth noting that letting PHP try to split apart SCRIPT_FILENAME on
its own has led to security issues in the past [1,2]. They were
mitigated by other means on the PHP side, I believe, but we can avoid
recurrences in future FCGI implementations.

>> On Jan 18, 2017, at 2:01 PM, Jacob Champion <[hidden email]> wrote:
>> We only get QUERY_STRING right. (Well, I won't say SCRIPT_FILENAME is "wrong", since there is no standard definition, but it's not helpful.) SCRIPT_NAME is supposed to point to a possible script-path (see RFC 3875 for definitions), '/hello.php' in this instance. PATH_INFO and PATH_TRANSLATED should not include 'hello.php' in their values.
>
> That is because we have no idea what SCRIPT_NAME is...

One way to approach unknowns is to have httpd and/or the backend guess
at things; another way (that we can't start doing until a version-bump
release) is to refuse to send back a FastCGI request unless we know
exactly what these variables should be. That lets the user fix the issue
and gain a better understanding of what is going on, instead of relying
on us and the backend to perform magic together during the correct phase
of the moon.

> With the "guess" that
> it is /php7-fpm, then PATH_INFO kind of makes sense, since it is
> the portion of the URI following the script. And considering
> that we use
>
>    Action application/x-php7-fpm /php7-fpm virtual
>
> the 2nd parameter is specifically noted as *being the cgi-script* and
> so of course httpd assumes that /php7-fpm is SCRIPT_NAME, because
> we explicitly called it that :)

Fine by me. So then, in my opinion, it seems like one part of moving
forward should be to deprecate the use of Action to invoke script
multiplexers like PHP-FPM, and document accordingly. I.e. if you're not
redirecting to *the script*, as in standard CGI, no Action for you.

(That explicitly deprecates the mod_fastcgi model for use with PHP-FPM,
which would also be fine by me. I don't like the way it couples the
public URI-space to an implementation detail.)

Are there similar arguments for the variable values discussed in PR
51517? It used ProxyPassMatch, not Action, for its implementation.

> Would it make sense, mostly from a PHP point-of-view, to send
> something like SCRIPT_FILENAME_RAW (or even
> APACHE_SCRIPT_FILENAME)... Or does that simply complicate an already
> fuzzy and nebulous situation?

I think it probably makes things worse. It's just one more non-standard
variable for us to maintain and backends to manipulate.

> Is [1] being used as the canonical place for this discussion w/ the
> PHP Group?
>
> 1. https://github.com/php/php-src/pull/694#issuecomment-271963956

I'm not sure if there is a canonical place yet...

     http://news.php.net/php.internals/97810

is another possibility, if the conversation is picked up.

--Jacob

[1] https://forum.nginx.org/read.php?2,88845,88845#msg-88845
[2] https://bugs.php.net/bug.php?id=55181
     (can't find a CVE right now, sorry)

Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

David Zuelke
On 19.01.2017, at 19:00, Jacob Champion <[hidden email]> wrote:

>
> On 01/18/2017 01:00 PM, Jim Jagielski wrote:
>> After all, it's easier for the FCGI server to know the SCRIPT_NAME
>> than httpd to "guess"...
>
> I think the recent breakage calls this assumption into question. The server admin knows exactly where the scripts are in the use cases we're currently discussing; why should the backend have to "know" anything in those situations?
>
> It's worth noting that letting PHP try to split apart SCRIPT_FILENAME on its own has led to security issues in the past [1,2]. They were mitigated by other means on the PHP side, I believe, but we can avoid recurrences in future FCGI implementations.
>
>>> On Jan 18, 2017, at 2:01 PM, Jacob Champion <[hidden email]> wrote:
>>> We only get QUERY_STRING right. (Well, I won't say SCRIPT_FILENAME is "wrong", since there is no standard definition, but it's not helpful.) SCRIPT_NAME is supposed to point to a possible script-path (see RFC 3875 for definitions), '/hello.php' in this instance. PATH_INFO and PATH_TRANSLATED should not include 'hello.php' in their values.
>>
>> That is because we have no idea what SCRIPT_NAME is...
>
> One way to approach unknowns is to have httpd and/or the backend guess at things; another way (that we can't start doing until a version-bump release) is to refuse to send back a FastCGI request unless we know exactly what these variables should be. That lets the user fix the issue and gain a better understanding of what is going on, instead of relying on us and the backend to perform magic together during the correct phase of the moon.
>
>> With the "guess" that
>> it is /php7-fpm, then PATH_INFO kind of makes sense, since it is
>> the portion of the URI following the script. And considering
>> that we use
>>
>>   Action application/x-php7-fpm /php7-fpm virtual
>>
>> the 2nd parameter is specifically noted as *being the cgi-script* and
>> so of course httpd assumes that /php7-fpm is SCRIPT_NAME, because
>> we explicitly called it that :)
>
> Fine by me. So then, in my opinion, it seems like one part of moving forward should be to deprecate the use of Action to invoke script multiplexers like PHP-FPM, and document accordingly. I.e. if you're not redirecting to *the script*, as in standard CGI, no Action for you.
>
> (That explicitly deprecates the mod_fastcgi model for use with PHP-FPM, which would also be fine by me. I don't like the way it couples the public URI-space to an implementation detail.)
>
> Are there similar arguments for the variable values discussed in PR 51517? It used ProxyPassMatch, not Action, for its implementation.

ProxyPassMatch has its own set of problems in combination with rewrites in e.g. .htaccess. The only method that I've found works reliably with any .htaccess rule that also works with e.g. mod_php is through SetHandler in 2.4.10+.


>> Would it make sense, mostly from a PHP point-of-view, to send
>> something like SCRIPT_FILENAME_RAW (or even
>> APACHE_SCRIPT_FILENAME)... Or does that simply complicate an already
>> fuzzy and nebulous situation?
>
> I think it probably makes things worse. It's just one more non-standard variable for us to maintain and backends to manipulate.

Agreed


>> Is [1] being used as the canonical place for this discussion w/ the
>> PHP Group?
>>
>> 1. https://github.com/php/php-src/pull/694#issuecomment-271963956
>
> I'm not sure if there is a canonical place yet...
>
>    http://news.php.net/php.internals/97810
>
> is another possibility, if the conversation is picked up.

Yeah, not much interest yet, sorry :( Jim, I think you have a php.net account; are you also on that mailing list and can chime in?


> --Jacob
>
> [1] https://forum.nginx.org/read.php?2,88845,88845#msg-88845
> [2] https://bugs.php.net/bug.php?id=55181
>    (can't find a CVE right now, sorry)
>

Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

Jim Jagielski
In reply to this post by Jacob Champion-2

> On Jan 19, 2017, at 1:00 PM, Jacob Champion <[hidden email]> wrote:
>
>> Would it make sense, mostly from a PHP point-of-view, to send
>> something like SCRIPT_FILENAME_RAW (or even
>> APACHE_SCRIPT_FILENAME)... Or does that simply complicate an already
>> fuzzy and nebulous situation?
>
> I think it probably makes things worse. It's just one more non-standard variable for us to maintain and backends to manipulate.
>

The idea is that they would get the "raw" URL... the whole thing.
Complete and unfiltered.

Yeah, maybe not :)
Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

Jim Jagielski
In reply to this post by Jacob Champion-2
Let me mull this over... basically, we want/need
to be able to read into the AddType directive the
MIME type and the suffix and then look for the 1st occurance
of a "file" with that suffix in the URL path and *assume*
that is the actual SCRIPT_NAME. Being aware that this
is virtual via the Action directive may actually make
that *easier*.

It's been awhile since I look at those code paths, and
not sure where they all roam and wind, but let me dig
in a little.
Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

Jim Jagielski
In reply to this post by David Zuelke

> On Jan 19, 2017, at 1:08 PM, David Zuelke <[hidden email]> wrote:
>
> Yeah, not much interest yet, sorry :( Jim, I think you have a php.net account; are you also on that mailing list and can chime in?
>

It's been awhile... certainly before the Github switch.

Don't *think* I'm on the php-internals list, but will double check
and join if not
Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

Jim Jagielski
OK, I was thinking something like this, which tries to "compartmentalize"
it to where we actually create the CGI vars...

Anyone able to test?

diff --git a/modules/mappers/mod_actions.c b/modules/mappers/mod_actions.c
index 2a67a2742..efe22f814 100644
--- a/modules/mappers/mod_actions.c
+++ b/modules/mappers/mod_actions.c
@@ -186,7 +186,8 @@ static int action_handler(request_rec *r)
         ap_field_noparam(r->pool, r->content_type);
 
     if (action && (t = apr_table_get(conf->action_types, action))) {
-        if (*t++ == '0' && r->finfo.filetype == APR_NOFILE) {
+        int virtual = (*t++ == '0' ? 0 : 1);
+        if (!virtual && r->finfo.filetype == APR_NOFILE) {
             ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00652)
                           "File does not exist: %s", r->filename);
             return HTTP_NOT_FOUND;
@@ -197,6 +198,9 @@ static int action_handler(request_rec *r)
          * (will be REDIRECT_HANDLER there)
          */
         apr_table_setn(r->subprocess_env, "HANDLER", action);
+        if (virtual) {
+            apr_table_setn(r->subprocess_env, "virtual_script", "1");
+        }
     }
 
     if (script == NULL)
diff --git a/modules/proxy/mod_proxy_fcgi.c b/modules/proxy/mod_proxy_fcgi.c
index 6f7bda009..ff7f58b90 100644
--- a/modules/proxy/mod_proxy_fcgi.c
+++ b/modules/proxy/mod_proxy_fcgi.c
@@ -141,9 +141,12 @@ static int proxy_fcgi_canon(request_rec *r, char *url)
             if (!strcasecmp(pathinfo_type, "unescape")) {
                 ap_unescape_url_keep2f(r->path_info, 0);
             }
+        }
+        if (r->path_info && *r->path_info) {
             ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01061)
-                    "set r->path_info to %s", r->path_info);
+                          "set r->path_info to %s", r->path_info);
         }
+
     }
 
     return OK;
diff --git a/server/util_script.c b/server/util_script.c
index 0f12bacc2..0d6ec9313 100644
--- a/server/util_script.c
+++ b/server/util_script.c
@@ -381,6 +381,7 @@ AP_DECLARE(void) ap_add_cgi_vars(request_rec *r)
     core_dir_config *conf =
         (core_dir_config *)ap_get_core_module_config(r->per_dir_config);
     int request_uri_from_original = 1;
+    int path_info_exists = r->path_info && r->path_info[0];
     const char *request_uri_rule;
 
     apr_table_setn(e, "GATEWAY_INTERFACE", "CGI/1.1");
@@ -406,13 +407,43 @@ AP_DECLARE(void) ap_add_cgi_vars(request_rec *r)
 
     if (!strcmp(r->protocol, "INCLUDED")) {
         apr_table_setn(e, "SCRIPT_NAME", r->uri);
-        if (r->path_info && *r->path_info) {
+        if (path_info_exists) {
             apr_table_setn(e, "PATH_INFO", r->path_info);
         }
     }
-    else if (!r->path_info || !*r->path_info) {
+    else if (!path_info_exists) {
         apr_table_setn(e, "SCRIPT_NAME", r->uri);
     }
+    /*
+     * Deal with special cases where the scripts are virtual
+     * (external) and we have r->filename of the form:
+     *     proxy:fcgi://127.0.0.1:9000/path/to/test.php/foo/bar/
+     *     proxy:balancer://127.0.0.1:9000/path/to/test.php/foo1/bar2/
+     */
+
+    else if (apr_table_get(e, "virtual_script")) {
+        char *path;
+        char *script;
+        char *scan;
+        script = apr_pstrdup(r->pool, r->path_info);
+        scan = (char *)apr_table_get(e, "CONTENT_TYPE");
+        if (!scan) {
+            scan = ".";
+        }
+        path = ap_strstr(script, scan);
+        if (path) {
+            while (*path && *path != '/') {
+                path++;
+            }
+            if (*path) {
+                *path = '\0';
+                path++;
+            }
+        }
+        apr_table_setn(e, "SCRIPT_NAME", script);
+        apr_table_setn(e, "PATH_INFO", apr_pstrcat(r->pool, "/",
+                       ((path && *path) ? path : ""), NULL));
+    }
     else {
         int path_info_start = ap_find_path_info(r->uri, r->path_info);
 
@@ -422,7 +453,7 @@ AP_DECLARE(void) ap_add_cgi_vars(request_rec *r)
         apr_table_setn(e, "PATH_INFO", r->path_info);
     }
 
-    if (r->path_info && r->path_info[0]) {
+    if (path_info_exists) {
         /*
          * To get PATH_TRANSLATED, treat PATH_INFO as a URI path.
          * Need to re-escape it for this, since the entire URI was

Reply | Threaded
Open this post in threaded view
|

Re: Summary: Broken FastCGI with httpd

Jacob Champion-2
On 01/23/2017 09:05 AM, Jim Jagielski wrote:
> OK, I was thinking something like this, which tries to "compartmentalize"
> it to where we actually create the CGI vars...
>
> Anyone able to test?

This patch doesn't seem to change anything in my limited tests so far.
Comments inline:

> diff --git a/modules/mappers/mod_actions.c b/modules/mappers/mod_actions.c
> index 2a67a2742..efe22f814 100644
> --- a/modules/mappers/mod_actions.c
> +++ b/modules/mappers/mod_actions.c
> @@ -186,7 +186,8 @@ static int action_handler(request_rec *r)
>          ap_field_noparam(r->pool, r->content_type);
>
>      if (action && (t = apr_table_get(conf->action_types, action))) {
> -        if (*t++ == '0' && r->finfo.filetype == APR_NOFILE) {
> +        int virtual = (*t++ == '0' ? 0 : 1);
> +        if (!virtual && r->finfo.filetype == APR_NOFILE) {
>              ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00652)
>                            "File does not exist: %s", r->filename);
>              return HTTP_NOT_FOUND;
> @@ -197,6 +198,9 @@ static int action_handler(request_rec *r)
>           * (will be REDIRECT_HANDLER there)
>           */
>          apr_table_setn(r->subprocess_env, "HANDLER", action);
> +        if (virtual) {
> +            apr_table_setn(r->subprocess_env, "virtual_script", "1");
> +        }
>      }
>
>      if (script == NULL)
> diff --git a/modules/proxy/mod_proxy_fcgi.c b/modules/proxy/mod_proxy_fcgi.c
> index 6f7bda009..ff7f58b90 100644
> --- a/modules/proxy/mod_proxy_fcgi.c
> +++ b/modules/proxy/mod_proxy_fcgi.c
> @@ -141,9 +141,12 @@ static int proxy_fcgi_canon(request_rec *r, char *url)
>              if (!strcasecmp(pathinfo_type, "unescape")) {
>                  ap_unescape_url_keep2f(r->path_info, 0);
>              }
> +        }
> +        if (r->path_info && *r->path_info) {
>              ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01061)
> -                    "set r->path_info to %s", r->path_info);
> +                          "set r->path_info to %s", r->path_info);
>          }
> +
>      }
>
>      return OK;
> diff --git a/server/util_script.c b/server/util_script.c
> index 0f12bacc2..0d6ec9313 100644
> --- a/server/util_script.c
> +++ b/server/util_script.c
> @@ -381,6 +381,7 @@ AP_DECLARE(void) ap_add_cgi_vars(request_rec *r)
>      core_dir_config *conf =
>          (core_dir_config *)ap_get_core_module_config(r->per_dir_config);
>      int request_uri_from_original = 1;
> +    int path_info_exists = r->path_info && r->path_info[0];
>      const char *request_uri_rule;
>
>      apr_table_setn(e, "GATEWAY_INTERFACE", "CGI/1.1");
> @@ -406,13 +407,43 @@ AP_DECLARE(void) ap_add_cgi_vars(request_rec *r)
>
>      if (!strcmp(r->protocol, "INCLUDED")) {
>          apr_table_setn(e, "SCRIPT_NAME", r->uri);
> -        if (r->path_info && *r->path_info) {
> +        if (path_info_exists) {
>              apr_table_setn(e, "PATH_INFO", r->path_info);
>          }
>      }
> -    else if (!r->path_info || !*r->path_info) {
> +    else if (!path_info_exists) {
>          apr_table_setn(e, "SCRIPT_NAME", r->uri);
>      }
> +    /*
> +     * Deal with special cases where the scripts are virtual
> +     * (external) and we have r->filename of the form:
> +     *     proxy:fcgi://127.0.0.1:9000/path/to/test.php/foo/bar/
> +     *     proxy:balancer://127.0.0.1:9000/path/to/test.php/foo1/bar2/
> +     */
> +
> +    else if (apr_table_get(e, "virtual_script")) {

By the time we get here for my Action tests, "virtual_script" has
already been renamed to "REDIRECT_virtual_script".

> +        char *path;
> +        char *script;
> +        char *scan;
> +        script = apr_pstrdup(r->pool, r->path_info);
> +        scan = (char *)apr_table_get(e, "CONTENT_TYPE");

What situations lead to CONTENT_TYPE being set to a PATH_INFO delimiter?
I'm not sure what this is supposed to do.

> +        if (!scan) {
> +            scan = ".";
> +        }
> +        path = ap_strstr(script, scan);
> +        if (path) {
> +            while (*path && *path != '/') {
> +                path++;
> +            }
> +            if (*path) {
> +                *path = '\0';
> +                path++;
> +            }
> +        }
> +        apr_table_setn(e, "SCRIPT_NAME", script);
> +        apr_table_setn(e, "PATH_INFO", apr_pstrcat(r->pool, "/",
> +                       ((path && *path) ? path : ""), NULL));

This appears to give PATH_INFO a leading slash even if there was no
slash after the script in the URI.

> +    }
>      else {
>          int path_info_start = ap_find_path_info(r->uri, r->path_info);
>
> @@ -422,7 +453,7 @@ AP_DECLARE(void) ap_add_cgi_vars(request_rec *r)
>          apr_table_setn(e, "PATH_INFO", r->path_info);
>      }
>
> -    if (r->path_info && r->path_info[0]) {
> +    if (path_info_exists) {
>          /*
>           * To get PATH_TRANSLATED, treat PATH_INFO as a URI path.
>           * Need to re-escape it for this, since the entire URI was
>

Reply | Threaded
Open this post in threaded view
|

(no subject)

Jim Jagielski-2
Yeah, this was fast and dirty and really a concept...

I'm in Arlington VA for the next coupla of days and won't be able to get another look until later.

Basically, the idea was to trigger when FCGI is in place... Maybe notes is better than subprocess_env for that special knowledge.
123