You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Doug MacEachern <do...@covalent.net> on 2002/04/02 18:08:44 UTC
fix t/ssl/http.t
the test started failing at some point due to filter changes. i think i
heard it is not longer possible for a filter to remove itself? in any
case, mod_ssl already checks in the output filter already passes if its
ssl pointer is NULL (normally due to error). the input filter should
probably do the same too. and with that it is simple to disable the ssl
filters in the case of 'HTTP spoken on HTTPS port'
if there's a better way, that'd be great, but the patch below passes all
httpd-tests for me.
Index: modules/ssl/ssl_engine_io.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/ssl/ssl_engine_io.c,v
retrieving revision 1.70
diff -u -r1.70 ssl_engine_io.c
--- modules/ssl/ssl_engine_io.c 30 Mar 2002 05:16:55 -0000 1.70
+++ modules/ssl/ssl_engine_io.c 2 Apr 2002 16:05:32 -0000
@@ -743,6 +743,13 @@
sizeof(HTTP_ON_HTTPS_PORT) - 1, \
alloc)
+static void ssl_io_filter_disable(ap_filter_t *f)
+{
+ ssl_io_input_ctx_t *ctx = f->ctx;
+ ctx->inbio.ssl = NULL;
+ ctx->frec->pssl = NULL;
+}
+
static apr_status_t ssl_io_filter_error(ap_filter_t *f,
apr_bucket_brigade *bb,
apr_status_t status)
@@ -758,6 +765,7 @@
/* fake the request line */
bucket = HTTP_ON_HTTPS_PORT_BUCKET(f->c->bucket_alloc);
+ ssl_io_filter_disable(f);
break;
default:
@@ -780,6 +788,10 @@
apr_size_t len = sizeof(ctx->buffer);
int is_init = (mode == AP_MODE_INIT);
+
+ if (!ctx->inbio.ssl) {
+ return ap_get_brigade(f->next, bb, mode, block, readbytes);
+ }
/* XXX: we don't currently support anything other than these modes. */
if (mode != AP_MODE_READBYTES && mode != AP_MODE_GETLINE &&
Re: fix t/ssl/http.t
Posted by Doug MacEachern <do...@covalent.net>.
On Wed, 3 Apr 2002, Cliff Woolley wrote:
> Only one other thing I'm concerned about with it: It's only correct if
> we're in AP_MODE_GETLINE at the time of the error. Which we are in this
> case, but will it always be that way?
i think so, assuming AP_MODE_GETLINE always happens first. on the first
call SSL_R_HTTP_REQUEST is detected and ssl filters are disabled. so any
get_brigade calls after that with same or different mode will happen with
ssl filters disabled.
Re: fix t/ssl/http.t
Posted by Cliff Woolley <jw...@virginia.edu>.
On Wed, 3 Apr 2002, Doug MacEachern wrote:
> > The correct solution is this:
> cool, +1
Only one other thing I'm concerned about with it: It's only correct if
we're in AP_MODE_GETLINE at the time of the error. Which we are in this
case, but will it always be that way?
Breakpoint 2, ssl_io_filter_Input (f=0x83b8728, bb=0x83b9f88,
mode=AP_MODE_GETLINE, block=APR_BLOCK_READ, readbytes=0)
at ssl_engine_io.c:813
813 return ssl_io_filter_error(f, bb, status);
But any of AP_MODE_GETLINE, AP_MODE_READBYTES, or AP_MODE_SPECULATIVE
could theoretically make it through to that point. Do we need to account
for that and send the LF's in those other cases? And if so, should it be
"\n\n" or "\r\n\r\n"? I need to trace through some more code.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: fix t/ssl/http.t
Posted by Doug MacEachern <do...@covalent.net>.
On Wed, 3 Apr 2002, Cliff Woolley wrote:
> On Tue, 2 Apr 2002, Doug MacEachern wrote:
>
> > apr_bucket_immortal_create(HTTP_ON_HTTPS_PORT, \
> > - sizeof(HTTP_ON_HTTPS_PORT) - 1, \
> > + sizeof(HTTP_ON_HTTPS_PORT), \
>
> Mmmm... no. I don't know how that makes it work its way through, but it's
> not right. That null character should never be allowed in.
like i said, "this is strange", never suggested the patch was correct.
> The correct solution is this:
cool, +1
Re: fix t/ssl/http.t
Posted by Cliff Woolley <jw...@virginia.edu>.
On Tue, 2 Apr 2002, Doug MacEachern wrote:
> apr_bucket_immortal_create(HTTP_ON_HTTPS_PORT, \
> - sizeof(HTTP_ON_HTTPS_PORT) - 1, \
> + sizeof(HTTP_ON_HTTPS_PORT), \
Mmmm... no. I don't know how that makes it work its way through, but it's
not right. That null character should never be allowed in.
The correct solution is this:
Index: ssl_engine_io.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/ssl/ssl_engine_io.c,v
retrieving revision 1.71
diff -u -d -r1.71 ssl_engine_io.c
--- ssl_engine_io.c 2 Apr 2002 17:30:08 -0000 1.71
+++ ssl_engine_io.c 3 Apr 2002 07:33:40 -0000
@@ -736,7 +736,7 @@
}
#define HTTP_ON_HTTPS_PORT \
- "GET /mod_ssl:error:HTTP-request HTTP/1.0\r\n\r\n"
+ "GET /mod_ssl:error:HTTP-request HTTP/1.0"
#define HTTP_ON_HTTPS_PORT_BUCKET(alloc) \
apr_bucket_immortal_create(HTTP_ON_HTTPS_PORT, \
Because we're expected to be returning a single line at this point. If
you leave the CRLF's on, you get weird things like this in
http_protocol.c:
apr_pstrmemdup (a=0x8240960, s=0x8241238 "HTTP/1.0\r\n", n=10)
at apr_strings.c:110
110 memcpy(res, s, n);
(gdb)
111 res[n] = '\0';
(gdb)
112 return res;
(gdb)
0x40030bd2 107 return NULL;
(gdb)
read_request_line (r=0x8240998) at protocol.c:703
703 if (len == 8
(gdb)
709 else if (2 == sscanf(r->protocol, "HTTP/%u.%u", &major,
&minor)
(gdb)
711 r->proto_num = HTTP_VERSION(major, minor);
(gdb)
715 return 1;
(gdb)
So it manages to survive through that, but then the next line, which is
\r\n *after* the \r\n was supposed to be stripped off, is an invalid
header.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: fix t/ssl/http.t
Posted by Doug MacEachern <do...@covalent.net>.
this is not quite fixed. currently does return 400 Bad Request, but
reports:
"Your browser sent a request that this server could not understand.
Request header field is missing colon separator."
with the patch below it properly reports:
"Your browser sent a request that this server could not understand.
Reason: You're speaking plain HTTP to an SSL-enabled server port.
Instead use the HTTPS scheme to access this URL, please.
Hint: https://localhost:8530/"
this is strange.
--- modules/ssl/ssl_engine_io.c 2 Apr 2002 17:30:08 -0000 1.71
+++ modules/ssl/ssl_engine_io.c 3 Apr 2002 04:19:23 -0000
@@ -740,7 +740,7 @@
#define HTTP_ON_HTTPS_PORT_BUCKET(alloc) \
apr_bucket_immortal_create(HTTP_ON_HTTPS_PORT, \
- sizeof(HTTP_ON_HTTPS_PORT) - 1, \
+ sizeof(HTTP_ON_HTTPS_PORT), \
alloc)
static void ssl_io_filter_disable(ap_filter_t *f)
RE: fix t/ssl/http.t
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 11:04 AM 4/2/2002, Ryan wrote:
> >
> > there is still a problem. ssl is removed from c->output_filters, but
>not
> > r->output_filters. mod_ssl does not have access to r->output_filters.
>
>Damn. I thought I had fixed that. You're right though, I couldn't fix
>that. The only solution looks to be always adding the request to the
>filter structure when the request is passed down the chain. That would
>suck though. :-(
My suggestion is more of a chain-filter for r->output_filters, that simply
invokes c->output_filters. This would allow us to drop all the confusion
of resetting the ->next filter members every time the first filter changes.
By using an ap_connection_filter() that simply dispatches over to the
c->output_filters chain, we break all the interdependencies and a lot
of opportunities for bugs to hide in that list maintenance code.
The same could be used to for proto_output_filters and other layers
that might someday be introduced. It would spare us from rehacking
all that fixup code every time the schema has a minor addition.
Bill
RE: fix t/ssl/http.t
Posted by Ryan Bloom <rb...@covalent.net>.
> > Nope, I fixed this. The problem was that we couldn't remove the
first
> > filter in any of the three lists, because the previous filter
structure
> > wouldn't be updated. The solution was to walk the filter list each
time
> > we tried to remove a filter. This allows us to find the correct
filter
> > entry and set the pointers appropriately.
>
> there is still a problem. ssl is removed from c->output_filters, but
not
> r->output_filters. mod_ssl does not have access to r->output_filters.
Damn. I thought I had fixed that. You're right though, I couldn't fix
that. The only solution looks to be always adding the request to the
filter structure when the request is passed down the chain. That would
suck though. :-(
Ryan
RE: fix t/ssl/http.t
Posted by Doug MacEachern <do...@covalent.net>.
On Tue, 2 Apr 2002, Ryan Bloom wrote:
> Nope, I fixed this. The problem was that we couldn't remove the first
> filter in any of the three lists, because the previous filter structure
> wouldn't be updated. The solution was to walk the filter list each time
> we tried to remove a filter. This allows us to find the correct filter
> entry and set the pointers appropriately.
there is still a problem. ssl is removed from c->output_filters, but not
r->output_filters. mod_ssl does not have access to r->output_filters.
RE: fix t/ssl/http.t
Posted by Ryan Bloom <rb...@covalent.net>.
> On Tue, 2 Apr 2002, Ryan Bloom wrote:
>
> > It is perfectly possible for a filter to remove itself. In fact,
the
> > byterange filter relies on that ability to work correctly. While I
> > would be interested to know what happened to make that case fail, if
the
> > patch below works, then +1.
>
> i was thinking of this comment from justin:
> Date: Thu, 7 Mar 2002 01:42:27 -0800
> From: Justin Erenkrantz <je...@ebuilt.com>
> To: dev@httpd.apache.org
> Subject: Re: httpd-test + cvs head
> Message-ID: <20...@ebuilt.com>
>
> ...
> However, mod_ssl is bogus. The ssl/http.t test is interesting
> since it causes mod_ssl to remove itself via
> ap_remove_output_filter() (mod_ssl.c:358). Since mod_ssl is a
> connection filter, our new strategy is that it can never be
> removed. Ooops. Since it doesn't have access to the request_rec,
> it can't destroy its predecessor's reference to itself. Perhaps
> this means we *do* need the ->prev.
> ...
>
> is this still true? (note: mod_ssl.c:358 is now 421)
Nope, I fixed this. The problem was that we couldn't remove the first
filter in any of the three lists, because the previous filter structure
wouldn't be updated. The solution was to walk the filter list each time
we tried to remove a filter. This allows us to find the correct filter
entry and set the pointers appropriately.
Ryan
RE: fix t/ssl/http.t
Posted by Doug MacEachern <do...@covalent.net>.
On Tue, 2 Apr 2002, Ryan Bloom wrote:
> It is perfectly possible for a filter to remove itself. In fact, the
> byterange filter relies on that ability to work correctly. While I
> would be interested to know what happened to make that case fail, if the
> patch below works, then +1.
i was thinking of this comment from justin:
Date: Thu, 7 Mar 2002 01:42:27 -0800
From: Justin Erenkrantz <je...@ebuilt.com>
To: dev@httpd.apache.org
Subject: Re: httpd-test + cvs head
Message-ID: <20...@ebuilt.com>
...
However, mod_ssl is bogus. The ssl/http.t test is interesting
since it causes mod_ssl to remove itself via
ap_remove_output_filter() (mod_ssl.c:358). Since mod_ssl is a
connection filter, our new strategy is that it can never be
removed. Ooops. Since it doesn't have access to the request_rec,
it can't destroy its predecessor's reference to itself. Perhaps
this means we *do* need the ->prev.
...
is this still true? (note: mod_ssl.c:358 is now 421)
RE: fix t/ssl/http.t
Posted by Ryan Bloom <rb...@covalent.net>.
> the test started failing at some point due to filter changes. i think
i
> heard it is not longer possible for a filter to remove itself? in any
It is perfectly possible for a filter to remove itself. In fact, the
byterange filter relies on that ability to work correctly. While I
would be interested to know what happened to make that case fail, if the
patch below works, then +1.
Ryan
> case, mod_ssl already checks in the output filter already passes if
its
> ssl pointer is NULL (normally due to error). the input filter should
> probably do the same too. and with that it is simple to disable the
ssl
> filters in the case of 'HTTP spoken on HTTPS port'
> if there's a better way, that'd be great, but the patch below passes
all
> httpd-tests for me.