Re: mod_proxy_fcgi, 304 responses and bogus error messages

[Luca Toscano]

2016-07-11 19:44 GMT+02:00 Jacob Champion :

> On 07/11/2016 08:53 AM, Luca Toscano wrote:
>
>> I am looking for some feedback about the patch proposed to figure out if
>> I am on the right track or not. Does it make sense to read all the data
>> returned by a FCGI backend even on error conditions to avoid subsequent
>> spurious reads or is there a smarter method?
>>
>
> (following up from IRC)
>
> Regarding your patch: I think that, rather than duplicate the jump back to
> recv_again, the error handling logic around the call to
> ap_scan_script_header_* should be improved. That function is documented to
> return binary success/failure (OK/INTERNAL_SERVER_ERROR), but in reality it
> can return other things too -- definitely NOT_MODIFIED (which is handled),
> perhaps PRECONDITION_FAILED (which is not handled), and maybe others?
>
> It looks like mod_proxy_fcgi used to take the binary approach ("anything
> that's not OK is an error; break out and run away") and when the exception
> was added for NOT_MODIFIED, that "error break" was left in. Perhaps we
> should just remove that break in the non-fatal-error cases (including,
> maybe, PRECONDITION_FAILED?).
>

I followed your suggestion and avoid the break in the HTTP_NOT_MODIFIED use
case, it works nicely! Updated the patch on the bugzilla PR. If anybody has
other suggestions please let us know..


> I don't know the correct answer to your "should we drain on error
> conditions too" question, since I don't know if the connection is torn down
> on errors. (If it's not torn down, then yeah, we should be draining the
> content and padding, but IMO tearing down the connection is probably the
> right thing to do.)
>

As we were discussing on IRC, the break were originally developed probably
for real error conditions and the HTTP_NOT_MODIFIED use case has been added
later on as a special use case. So I would be inclined to fix this
important use case without increasing the scope of the patch.


>
> And of course we need regression tests for all these fixes... does anyone
> have time to review
>
> https://bz.apache.org/bugzilla/attachment.cgi?id=33867
>
> as a possible FCGI regression test template?
>
>
+1 for a good test suite around FCGI handlers, I'll try to extend/review it
during the next days.

Thanks!

Luca

Re: mod_proxy_fcgi, 304 responses and bogus error messages

[Jacob Champion]

On 07/11/2016 08:53 AM, Luca Toscano wrote:

I am looking for some feedback about the patch proposed to figure out if
I am on the right track or not. Does it make sense to read all the data
returned by a FCGI backend even on error conditions to avoid subsequent
spurious reads or is there a smarter method?


(following up from IRC)

Regarding your patch: I think that, rather than duplicate the jump back 
to recv_again, the error handling logic around the call to 
ap_scan_script_header_* should be improved. That function is documented 
to return binary success/failure (OK/INTERNAL_SERVER_ERROR), but in 
reality it can return other things too -- definitely NOT_MODIFIED (which 
is handled), perhaps PRECONDITION_FAILED (which is not handled), and 
maybe others?


It looks like mod_proxy_fcgi used to take the binary approach ("anything 
that's not OK is an error; break out and run away") and when the 
exception was added for NOT_MODIFIED, that "error break" was left in. 
Perhaps we should just remove that break in the non-fatal-error cases 
(including, maybe, PRECONDITION_FAILED?).


I don't know the correct answer to your "should we drain on error 
conditions too" question, since I don't know if the connection is torn 
down on errors. (If it's not torn down, then yeah, we should be draining 
the content and padding, but IMO tearing down the connection is probably 
the right thing to do.)


And of course we need regression tests for all these fixes... does 
anyone have time to review


https://bz.apache.org/bugzilla/attachment.cgi?id=33867

as a possible FCGI regression test template?

--Jacob

mod_proxy_fcgi, 304 responses and bogus error messages

[Luca Toscano]

Hi Apache devs,

I collected some info about AH01075 ("Error dispatching request to") and
AH1068 ( "Got bogus version X expected 1") errors related to 304 responses
handled by mod_proxy_fcgi in
https://bz.apache.org/bugzilla/show_bug.cgi?id=59838

I am looking for some feedback about the patch proposed to figure out if I
am on the right track or not. Does it make sense to read all the data
returned by a FCGI backend even on error conditions to avoid subsequent
spurious reads or is there a smarter method?

Thanks in advance!

Regards,
Luca

Re: concurrent ssl context access

[William A Rowe Jr]

On Mon, Jul 11, 2016 at 10:38 AM, William A Rowe Jr 
wrote:

> On Mon, Jul 11, 2016 at 5:25 AM, Stefan Eissing <
> stefan.eiss...@greenbytes.de> wrote:
>
>> In https://bz.apache.org/bugzilla/show_bug.cgi?id=59840 the issue crept
>> up that StdEnvVars makes concurrent access to the SSL context when HTTP/2
>> is in place.
>>
>> The question is: how do we want to fix this?
>>
>> The general theme is that a module needs to perform some action on a
>> connection, sees that it is a slave connection and accesses the master
>> instead. Such a module needs to be aware that access to master may happen
>> in parallel with other request.
>>
>> Should we let each such module care for it on its own or do we want to
>> provide a way to access the master connection in a mutex protected way?
>>
>
> Rather than a mutex, which would become an issue because all master access
> to the context would require blocking, is there a way we can allow a
> module to
> perform its (hopefully swift) processing on the master thread? Send a
> metadata
> bucket to the master asking for a function to be invoked?
>

(Note that the ask of interrogating the ssl context for stdenvvars itself
by the
child request is a pretty sub-optimal solution. mod_ssl itself should
gather
and set this data aside once (and once again, upon renegotation) for our
concurrent, un-mutexed reference in the child request.)

Re: concurrent ssl context access

[William A Rowe Jr]

On Mon, Jul 11, 2016 at 5:25 AM, Stefan Eissing <
stefan.eiss...@greenbytes.de> wrote:

> In https://bz.apache.org/bugzilla/show_bug.cgi?id=59840 the issue crept
> up that StdEnvVars makes concurrent access to the SSL context when HTTP/2
> is in place.
>
> The question is: how do we want to fix this?
>
> The general theme is that a module needs to perform some action on a
> connection, sees that it is a slave connection and accesses the master
> instead. Such a module needs to be aware that access to master may happen
> in parallel with other request.
>
> Should we let each such module care for it on its own or do we want to
> provide a way to access the master connection in a mutex protected way?
>

Rather than a mutex, which would become an issue because all master access
to the context would require blocking, is there a way we can allow a module
to
perform its (hopefully swift) processing on the master thread? Send a
metadata
bucket to the master asking for a function to be invoked?

Re: svn commit: r1752099 - in /httpd/httpd/trunk: ./ build/rpm/ docs/log-message-tags/ docs/manual/ docs/manual/mod/ modules/filters/

[Ruediger Pluem]

On 07/10/2016 07:27 PM, minf...@apache.org wrote:
> Author: minfrin
> Date: Sun Jul 10 17:27:03 2016
> New Revision: 1752099
> 
> URL: http://svn.apache.org/viewvc?rev=1752099=rev
> Log:
> mod_crypto: Add the all purpose crypto filters with support for HLS.
> 
> Added:
> httpd/httpd/trunk/docs/manual/mod/mod_crypto.xml   (with props)
> httpd/httpd/trunk/docs/manual/mod/mod_crypto.xml.meta   (with props)
> httpd/httpd/trunk/modules/filters/NWGNUmod_crypto   (with props)
> httpd/httpd/trunk/modules/filters/mod_crypto.c   (with props)
> httpd/httpd/trunk/modules/filters/mod_crypto.dsp   (with props)
> httpd/httpd/trunk/modules/filters/mod_crypto.h   (with props)
> Modified:
> httpd/httpd/trunk/Apache-apr2.dsw
> httpd/httpd/trunk/Apache.dsw
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/build/rpm/httpd.spec.in
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/docs/manual/expr.xml
> httpd/httpd/trunk/docs/manual/mod/allmodules.xml
> httpd/httpd/trunk/modules/filters/NWGNUmakefile
> httpd/httpd/trunk/modules/filters/config.m4
> 

> Added: httpd/httpd/trunk/modules/filters/mod_crypto.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_crypto.c?rev=1752099=auto
> ==
> --- httpd/httpd/trunk/modules/filters/mod_crypto.c (added)
> +++ httpd/httpd/trunk/modules/filters/mod_crypto.c Sun Jul 10 17:27:03 2016

> +static void *merge_crypto_config(apr_pool_t * p, void *basev, void *addv)
> +{
> +crypto_conf *new = (crypto_conf *) apr_pcalloc(p, sizeof(crypto_conf));
> +crypto_conf *add = (crypto_conf *) addv;
> +crypto_conf *base = (crypto_conf *) basev;
> +
> +new->library = (add->library_set == 0) ? base->library : add->library;
> +new->params = (add->library_set == 0) ? base->params : add->params;
> +new->library_set = add->library_set || base->library_set;
> +
> +new->crypto = base->crypto;

Shouldn't this be:

new->crypto = add->crypto;

> +
> +return (void *) new;
> +}
> +
> +static void *create_crypto_dir_config(apr_pool_t * p, char *dummy)
> +{
> +crypto_dir_conf *new =
> +(crypto_dir_conf *) apr_pcalloc(p, sizeof(crypto_dir_conf));
> +
> +new->size_set = 0;  /* unset */

Is this needed? We do apr_pcalloc above.

> +new->size = DEFAULT_BUFFER_SIZE;/* default size */
> +new->cipher = DEFAULT_CIPHER;
> +new->cipher = DEFAULT_MODE;

Shouldn't this be:

new->mode = DEFAULT_MODE;


> +
> +return (void *) new;
> +}
> +

Regards

Rüdiger

concurrent ssl context access

[Stefan Eissing]

In https://bz.apache.org/bugzilla/show_bug.cgi?id=59840 the issue crept up that 
StdEnvVars makes concurrent access to the SSL context when HTTP/2 is in place. 

The question is: how do we want to fix this?

The general theme is that a module needs to perform some action on a 
connection, sees that it is a slave connection and accesses the master instead. 
Such a module needs to be aware that access to master may happen in parallel 
with other request.

Should we let each such module care for it on its own or do we want to provide 
a way to access the master connection in a mutex protected way?

-Stefan