You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Stefan Fritsch <sf...@sfritsch.de> on 2012/01/29 20:53:09 UTC
[PATCH] trunk/2.4 core output filter is broken
On Wednesday 25 January 2012, Jim Jagielski wrote:
> Looking over the code, impl as a hook seems more "isolated",
> rather than the current impl which is intrusive (which is
> part of what we're trying to avoid, aren't we?)
OK, patch is attached. This needs review/testing for Windows.
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Jim Jagielski <ji...@jaguNET.com>.
FanTAStic!!!
On Feb 4, 2012, at 5:11 AM, Stefan Fritsch wrote:
> On Saturday 04 February 2012, Gregg Smith wrote:
>
>> This fixes the crash, thanks.
>
> Thanks. Committed to trunk and 2.4
>
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Saturday 04 February 2012, Gregg Smith wrote:
> This fixes the crash, thanks.
Thanks. Committed to trunk and 2.4
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Gregg Smith <gl...@gknw.net>.
On 2/3/2012 10:43 AM, Stefan Fritsch wrote:
> On Thursday 02 February 2012, William A. Rowe Jr. wrote:
>> On 2/2/2012 8:36 AM, Jim Jagielski wrote:
>>> bb == NULL ??
>> Looking at his attached screen scrape; no. Which leaves with
>> something like e == NULL or a broken bb.
> The former :-(
>
> Gregg, please add this or try the attached patch which is against
> current trunk and includes all fixes so far. Thanks in advance.
>
> --- a/server/mpm/winnt/child.c
> +++ b/server/mpm/winnt/child.c
> @@ -743,11 +743,10 @@ apr_status_t
> winnt_insert_network_bucket(conn_rec *c,
> apr_bucket *e;
> winnt_conn_ctx_t *context = ap_get_module_config(c->conn_config,
> &mpm_winnt_module);
> - if (context == NULL)
> + if (context == NULL || (e = context->overlapped.Pointer) == NULL)
> return DECLINED;
>
> /* seed the brigade with AcceptEx read heap bucket */
> - e = context->overlapped.Pointer;
> APR_BRIGADE_INSERT_HEAD(bb, e);
> /* also seed the brigade with the client socket. */
> e = apr_bucket_socket_create(socket, c->bucket_alloc);
Stefan,
This fixes the crash, thanks.
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Thursday 02 February 2012, William A. Rowe Jr. wrote:
> On 2/2/2012 8:36 AM, Jim Jagielski wrote:
> > bb == NULL ??
>
> Looking at his attached screen scrape; no. Which leaves with
> something like e == NULL or a broken bb.
The former :-(
Gregg, please add this or try the attached patch which is against
current trunk and includes all fixes so far. Thanks in advance.
--- a/server/mpm/winnt/child.c
+++ b/server/mpm/winnt/child.c
@@ -743,11 +743,10 @@ apr_status_t
winnt_insert_network_bucket(conn_rec *c,
apr_bucket *e;
winnt_conn_ctx_t *context = ap_get_module_config(c->conn_config,
&mpm_winnt_module);
- if (context == NULL)
+ if (context == NULL || (e = context->overlapped.Pointer) == NULL)
return DECLINED;
/* seed the brigade with AcceptEx read heap bucket */
- e = context->overlapped.Pointer;
APR_BRIGADE_INSERT_HEAD(bb, e);
/* also seed the brigade with the client socket. */
e = apr_bucket_socket_create(socket, c->bucket_alloc);
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 2/2/2012 8:36 AM, Jim Jagielski wrote:
> bb == NULL ??
Looking at his attached screen scrape; no. Which leaves with something
like e == NULL or a broken bb.
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Jim Jagielski <ji...@jaguNET.com>.
bb == NULL ??
On Feb 2, 2012, at 3:40 AM, Gregg Smith wrote:
> Stefan,
>
> On 2/1/2012 12:44 PM, Stefan Fritsch wrote:
>> Try removing the "static":
>>
>> --- a/server/mpm/winnt/child.c
>> +++ b/server/mpm/winnt/child.c
>> @@ -736,9 +736,9 @@ static winnt_conn_ctx_t
>> *winnt_get_connection(winnt_conn_ctx_t *context)
>> return context;
>> }
>>
>> -static apr_status_t winnt_insert_network_bucket(conn_rec *c,
>> - apr_bucket_brigade
>> *bb,
>> - apr_socket_t *socket)
>> +apr_status_t winnt_insert_network_bucket(conn_rec *c,
>> + apr_bucket_brigade *bb,
>> + apr_socket_t *socket)
>> {
>> apr_bucket *e;
>> winnt_conn_ctx_t *context = ap_get_module_config(c->conn_config
>
> That builds now, but one step forward, two steps back.
>
> libhttpd crashes on any request with
>
> AcceptFilter http none
> or
> AcceptFilter http connect
>
> The default AcceptFilter http data works fine. I did not try https.
>
> Regards,
>
> Gregg
>
> <acfilter_crash.png>
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Gregg Smith <gl...@gknw.net>.
Stefan,
On 2/1/2012 12:44 PM, Stefan Fritsch wrote:
> Try removing the "static":
>
> --- a/server/mpm/winnt/child.c
> +++ b/server/mpm/winnt/child.c
> @@ -736,9 +736,9 @@ static winnt_conn_ctx_t
> *winnt_get_connection(winnt_conn_ctx_t *context)
> return context;
> }
>
> -static apr_status_t winnt_insert_network_bucket(conn_rec *c,
> - apr_bucket_brigade
> *bb,
> - apr_socket_t *socket)
> +apr_status_t winnt_insert_network_bucket(conn_rec *c,
> + apr_bucket_brigade *bb,
> + apr_socket_t *socket)
> {
> apr_bucket *e;
> winnt_conn_ctx_t *context = ap_get_module_config(c->conn_config
That builds now, but one step forward, two steps back.
libhttpd crashes on any request with
AcceptFilter http none
or
AcceptFilter http connect
The default AcceptFilter http data works fine. I did not try https.
Regards,
Gregg
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Wednesday 01 February 2012, Gregg Smith wrote:
> Getting closer
>
> Linking...
> Creating library .\Release/libhttpd.lib and object
> .\Release/libhttpd.exp
> mpm_winnt.obj : error LNK2019: unresolved external symbol
> _winnt_insert_network_bucket referenced in function _winnt_hooks
>
> I don't quite get this error though.
Having a Windows compiler myself would really help :-/
Try removing the "static":
--- a/server/mpm/winnt/child.c
+++ b/server/mpm/winnt/child.c
@@ -736,9 +736,9 @@ static winnt_conn_ctx_t
*winnt_get_connection(winnt_conn_ctx_t *context)
return context;
}
-static apr_status_t winnt_insert_network_bucket(conn_rec *c,
- apr_bucket_brigade
*bb,
- apr_socket_t *socket)
+apr_status_t winnt_insert_network_bucket(conn_rec *c,
+ apr_bucket_brigade *bb,
+ apr_socket_t *socket)
{
apr_bucket *e;
winnt_conn_ctx_t *context = ap_get_module_config(c->conn_config,
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Gregg Smith <gl...@gknw.net>.
On 1/31/2012 1:30 PM, Stefan Fritsch wrote:
> On Tuesday 31 January 2012, Gregg Smith wrote:
>> On 1/29/2012 11:53 AM, Stefan Fritsch wrote:
>>> On Wednesday 25 January 2012, Jim Jagielski wrote:
>>>> Looking over the code, impl as a hook seems more "isolated",
>>>> rather than the current impl which is intrusive (which is
>>>> part of what we're trying to avoid, aren't we?)
>>> OK, patch is attached. This needs review/testing for Windows.
>> Patched trunk at r1237447, saw same after patching 2.4-HEAD so I
>> tried trunk.
>>
>> Regards,
>> Gregg
>>
>> Compiling...
>> service.c
>> .\server\mpm\winnt\service.c(38) : error C2370: 'mpm_winnt_module'
>> : redefinition; different storage class
>>
>> c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70)
>>
>> : see declaration of 'mpm_winnt_module'
> Oops. Please try this in addition. Thanks for the testing.
>
> --- a/server/mpm/winnt/mpm_winnt.h
> +++ b/server/mpm/winnt/mpm_winnt.h
> @@ -67,7 +67,7 @@ void mpm_nt_eventlog_stderr_flush(void);
>
> /* From mpm_winnt.c: */
>
> -extern module mpm_winnt_module;
> +extern module AP_MODULE_DECLARE_DATA mpm_winnt_module;
> extern int ap_threads_per_child;
>
> extern DWORD my_pid;
>
Getting closer
Linking...
Creating library .\Release/libhttpd.lib and object
.\Release/libhttpd.exp
mpm_winnt.obj : error LNK2019: unresolved external symbol
_winnt_insert_network_bucket referenced in function _winnt_hooks
I don't quite get this error though.
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Rainer Jung <ra...@kippdata.de>.
On 31.01.2012 22:30, Stefan Fritsch wrote:
> On Tuesday 31 January 2012, Gregg Smith wrote:
>> On 1/29/2012 11:53 AM, Stefan Fritsch wrote:
>>> On Wednesday 25 January 2012, Jim Jagielski wrote:
>>>> Looking over the code, impl as a hook seems more "isolated",
>>>> rather than the current impl which is intrusive (which is
>>>> part of what we're trying to avoid, aren't we?)
>>>
>>> OK, patch is attached. This needs review/testing for Windows.
>>
>> Patched trunk at r1237447, saw same after patching 2.4-HEAD so I
>> tried trunk.
>>
>> Regards,
>> Gregg
>>
>> Compiling...
>> service.c
>> .\server\mpm\winnt\service.c(38) : error C2370: 'mpm_winnt_module'
>> : redefinition; different storage class
>>
>> c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70)
>>
>> : see declaration of 'mpm_winnt_module'
>
> Oops. Please try this in addition. Thanks for the testing.
No info about Windows, but: I applied the patches on trunk and ran the
test suite against a reallyall build on Solaris 10. No test failures.
Regards,
Rainer
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Tuesday 31 January 2012, Gregg Smith wrote:
> On 1/29/2012 11:53 AM, Stefan Fritsch wrote:
> > On Wednesday 25 January 2012, Jim Jagielski wrote:
> >> Looking over the code, impl as a hook seems more "isolated",
> >> rather than the current impl which is intrusive (which is
> >> part of what we're trying to avoid, aren't we?)
> >
> > OK, patch is attached. This needs review/testing for Windows.
>
> Patched trunk at r1237447, saw same after patching 2.4-HEAD so I
> tried trunk.
>
> Regards,
> Gregg
>
> Compiling...
> service.c
> .\server\mpm\winnt\service.c(38) : error C2370: 'mpm_winnt_module'
> : redefinition; different storage class
>
> c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70)
>
> : see declaration of 'mpm_winnt_module'
Oops. Please try this in addition. Thanks for the testing.
--- a/server/mpm/winnt/mpm_winnt.h
+++ b/server/mpm/winnt/mpm_winnt.h
@@ -67,7 +67,7 @@ void mpm_nt_eventlog_stderr_flush(void);
/* From mpm_winnt.c: */
-extern module mpm_winnt_module;
+extern module AP_MODULE_DECLARE_DATA mpm_winnt_module;
extern int ap_threads_per_child;
extern DWORD my_pid;
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Thursday 02 February 2012, Joe Orton wrote:
> The combination of APR_SUCCESS and DECLINED is unusual; an int
> return value with OK/DECLINED?
Input and output filters should return an apr_status_t. So, if the
hook does not return an apr_status_t, core_input_filter() would have
to invent some apr_status_t value, which is bad. And I think the
general principle that return code 'int' means HTTP_* error code
should be kept.
But I agree that DECLINED == -1 is not a good idea, because it could
in theory collide with another APR_E* code. Should we define an
AP_STATUS_DECLINED or something in the APR_OS_START_USERERR range?
Or simply change the definition of DECLINED to be in the
APR_OS_START_USERERR range? APR_OS_START_USERERR is 120000, so there
should be no danger of a collision with HTTP_*. It would require a
major MMN bump, but making core_output_filter_ctx_t and core_ctx_t
private is an API change, anyway.
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Joe Orton <jo...@redhat.com>.
On Sun, Jan 29, 2012 at 08:53:09PM +0100, Stefan Fritsch wrote:
> + * Insert the network bucket into the core input filter's input brigade.
> + * This hook is intended for MPMs or protocol modules that need to do special
> + * socket setup.
> + * @param c The connection
> + * @param bb The brigade to insert the bucket into
> + * @param socket The socket to put into a bucket
> + * @return DECLINED if the current function does not handle this connection,
> + * APR_SUCCESS or an error otherwise.
> + */
> +AP_DECLARE_HOOK(apr_status_t, insert_network_bucket,
> + (conn_rec *c, apr_bucket_brigade *bb, apr_socket_t *socket))
The combination of APR_SUCCESS and DECLINED is unusual; an int return
value with OK/DECLINED? Looks great otherwise - thanks Stefan!
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Daniel Ruggeri <DR...@primary.net>.
On 1/31/2012 12:01 AM, Rainer Jung wrote:
> What's you runtime env then (Win version and 32 or 64 Bits)?
>
> Rainer
I have at my immediate disposal Server 2003 32-bit as well as Win7 and
Server 2008 64-bit. I may be able to scrounge up some more exotic
configurations if needed. My earlier tests to duplicate the problem were
done on the Win7 64 box.
--
Daniel Ruggeri
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Rainer Jung <ra...@kippdata.de>.
On 31.01.2012 00:36, Daniel Ruggeri wrote:
> On 1/30/2012 7:51 AM, Jim Jagielski wrote:
>> Anyone with Windows willing to sign up to review/test?
>
> I don't have a build environment to create something based on the diff,
> but if someone can create a build , I'll happily do the testing.
What's you runtime env then (Win version and 32 or 64 Bits)?
Rainer
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Daniel Ruggeri <DR...@primary.net>.
On 1/30/2012 7:51 AM, Jim Jagielski wrote:
> Anyone with Windows willing to sign up to review/test?
I don't have a build environment to create something based on the diff,
but if someone can create a build , I'll happily do the testing.
--
Daniel Ruggeri
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Jim Jagielski <ji...@jaguNET.com>.
Anyone with Windows willing to sign up to review/test?
On Jan 29, 2012, at 2:53 PM, Stefan Fritsch wrote:
> On Wednesday 25 January 2012, Jim Jagielski wrote:
>> Looking over the code, impl as a hook seems more "isolated",
>> rather than the current impl which is intrusive (which is
>> part of what we're trying to avoid, aren't we?)
>
> OK, patch is attached. This needs review/testing for Windows.
> <insert_network_bucket.diff>
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Gregg Smith <gl...@gknw.net>.
On 1/29/2012 11:53 AM, Stefan Fritsch wrote:
> On Wednesday 25 January 2012, Jim Jagielski wrote:
>> Looking over the code, impl as a hook seems more "isolated",
>> rather than the current impl which is intrusive (which is
>> part of what we're trying to avoid, aren't we?)
> OK, patch is attached. This needs review/testing for Windows.
Patched trunk at r1237447, saw same after patching 2.4-HEAD so I tried
trunk.
Regards,
Gregg
Compiling...
service.c
.\server\mpm\winnt\service.c(38) : error C2370: 'mpm_winnt_module' :
redefinition; different storage class
c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70)
: see declaration of 'mpm_winnt_module'
scoreboard.c
provider.c
nt_eventlog.c
mpm_winnt.c
.\server\mpm\winnt\mpm_winnt.c(1765) : error C2370: 'mpm_winnt_module' :
redefinition; different storage class
c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70)
: see declaration of 'mpm_winnt_module'
.\server\mpm\winnt\mpm_winnt.c(1765) : error C2370: 'mpm_winnt_module' :
redefinition; different storage class
c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70)
: see declaration of 'mpm_winnt_module'
mpm_common.c
listen.c
child.c
.\server\mpm\winnt\child.c(69) : error C2370: 'mpm_winnt_module' :
redefinition; different storage class
c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70)
: see declaration of 'mpm_winnt_module'
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Rainer Jung <ra...@kippdata.de>.
Hi Stefan,
On 29.01.2012 20:53, Stefan Fritsch wrote:
> On Wednesday 25 January 2012, Jim Jagielski wrote:
>> Looking over the code, impl as a hook seems more "isolated",
>> rather than the current impl which is intrusive (which is
>> part of what we're trying to avoid, aren't we?)
>
> OK, patch is attached. This needs review/testing for Windows.
as far as I understand, this patch is for trunk.
Testing against 2.4.x would in addition need to first apply the
following four patches (all the trunk commits apply clean to 2.4.x):
------------------------------------------------------------------------
r1236122 | rpluem | 2012-01-26 11:03:36 +0100 (Thu, 26 Jan 2012) | 1 line
* Don't typedef twice (in .c and .h file).
------------------------------------------------------------------------
r1233882 | jorton | 2012-01-20 13:41:18 +0100 (Fri, 20 Jan 2012) | 4 lines
* server/core_filters.c (ap_core_input_filter): Only treat EAGAIN as
success if a non-blocking read was requested; for a blocking read,
it is an error condition.
------------------------------------------------------------------------
r1234899 | jorton | 2012-01-23 17:57:07 +0100 (Mon, 23 Jan 2012) | 4 lines
* server/core_filters.c (send_brigade_nonblocking): Use a non-blocking
bucket read, allowing any pending data to be flushed before trying a
(potentially slow) blocking read.
------------------------------------------------------------------------
r1235019 | sf | 2012-01-23 22:58:42 +0100 (Mon, 23 Jan 2012) | 6 lines
Make the core input/output filter contexts private and provide accessor APIs
for mpm_winnt and mod_ftp.
This allows to add members to the context structs without breaking binary
compatibility.
------------------------------------------------------------------------
Is that your understanding as well? Or are some of those unwanted for 2.4.x?
Regards,
Rainer
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Mon, 30 Jan 2012, Jim Jagielski wrote:
> On Jan 29, 2012, at 2:53 PM, Stefan Fritsch wrote:
>
>> On Wednesday 25 January 2012, Jim Jagielski wrote:
>>> Looking over the code, impl as a hook seems more "isolated",
>>> rather than the current impl which is intrusive (which is
>>> part of what we're trying to avoid, aren't we?)
>>
>> OK, patch is attached. This needs review/testing for Windows.
>> <insert_network_bucket.diff>
>
> Just a quick scan but:
>
> +AP_IMPLEMENT_HOOK_RUN_FIRST(apr_status_t, insert_network_bucket,
> + (conn_rec *c, apr_bucket_brigade *bb,
> + apr_socket_t *socket),
> + (c, bb, socket), DECLINED)
> +
> ...
> + ap_hook_insert_network_bucket(core_insert_network_bucket, NULL, NULL,
> + APR_HOOK_REALLY_LAST);
>
> looks wonky...
I fail to see what is wonky here. Can you be more specific?
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jan 29, 2012, at 2:53 PM, Stefan Fritsch wrote:
> On Wednesday 25 January 2012, Jim Jagielski wrote:
>> Looking over the code, impl as a hook seems more "isolated",
>> rather than the current impl which is intrusive (which is
>> part of what we're trying to avoid, aren't we?)
>
> OK, patch is attached. This needs review/testing for Windows.
> <insert_network_bucket.diff>
Just a quick scan but:
+AP_IMPLEMENT_HOOK_RUN_FIRST(apr_status_t, insert_network_bucket,
+ (conn_rec *c, apr_bucket_brigade *bb,
+ apr_socket_t *socket),
+ (c, bb, socket), DECLINED)
+
...
+ ap_hook_insert_network_bucket(core_insert_network_bucket, NULL, NULL,
+ APR_HOOK_REALLY_LAST);
looks wonky...
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Gregg Smith <gl...@gknw.net>.
On 1/30/2012 11:14 PM, Gregg Smith wrote:
> On 1/29/2012 11:53 AM, Stefan Fritsch wrote:
>> On Wednesday 25 January 2012, Jim Jagielski wrote:
>>> Looking over the code, impl as a hook seems more "isolated",
>>> rather than the current impl which is intrusive (which is
>>> part of what we're trying to avoid, aren't we?)
>> OK, patch is attached. This needs review/testing for Windows.
> I'm assuming this is part of what is needed to get 2.4 going as well
> as Joe's big core overhaul in r1235019?
>
s/Joe's/Stefan's/
> I'm willing yet would prefer patching 2.4 for testing than patch &
> build trunk.
>
>
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Gregg Smith <gl...@gknw.net>.
On 1/29/2012 11:53 AM, Stefan Fritsch wrote:
> On Wednesday 25 January 2012, Jim Jagielski wrote:
>> Looking over the code, impl as a hook seems more "isolated",
>> rather than the current impl which is intrusive (which is
>> part of what we're trying to avoid, aren't we?)
> OK, patch is attached. This needs review/testing for Windows.
I'm assuming this is part of what is needed to get 2.4 going as well as
Joe's big core overhaul in r1235019?
I'm willing yet would prefer patching 2.4 for testing than patch & build
trunk.