You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ryan Bloom <rb...@covalent.net> on 2002/03/22 21:29:41 UTC
[PATCH] invalid HTTP status codes in access log
Ryan Morgan found a bug earlier today that he was finding 104 errors in
his access_log. We then looked through the code, and tracked the
problem down. The problem can be tracked to the return code from
filters. Essentially, filters were supposed to return HTTP status
codes, so that the server could do the right thing, and any log messages
should be logged from within the server. Unfortunately, I don't believe
most of our filters do that. This leads to us logging errno values
instead of HTTP status codes.
This patch fixes the core_output_filter. There are two error cases, I
am 100% convinced that the first one should be a 500 error (if a bucket
is unreadable), but the second has me stumped. If the user aborts the
connection we shouldn't log 200, but I don't know what is correct. Ryan
and I decided to use 206 for now. Can I get some opinions on the
correct value?
Ryan
? server/gen_test_char.vcproj
Index: server/core.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/core.c,v
retrieving revision 1.166
diff -u -d -b -w -u -r1.166 core.c
--- server/core.c 20 Mar 2002 22:19:10 -0000 1.166
+++ server/core.c 22 Mar 2002 20:25:56 -0000
@@ -3744,7 +3744,7 @@
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ERR, rv,
c->base_server,
"core_output_filter: Error reading
from bucket.");
- return rv;
+ return HTTP_INTERNAL_SERVER_ERROR;
}
apr_brigade_write(ctx->b, NULL, NULL, str, n);
@@ -3829,7 +3829,7 @@
c->aborted = 1;
}
- return rv;
+ return HTTP_PARTIAL_CONTENT;
}
b = more;
----------------------------------------------
Ryan Bloom rbb@covalent.net
645 Howard St. rbb@apache.org
San Francisco, CA
RE: [PATCH] invalid HTTP status codes in access log
Posted by Ryan Bloom <rb...@ntrnet.net>.
On Fri, 22 Mar 2002, William A. Rowe, Jr. wrote:
> At 05:19 PM 3/22/2002, Ryan wrote:
>
> >You don't know that something really broke just because the filter had a
> >problem.
>
> Ok, I'm confused. If the filter has a problem, but it doesn't hurt anything,
> why on earth would it report the error?
see below, I already gave an example.
> >I have written Apache filters (for 1.3) that decided
> >authentication based on content in the request/response files. All you
> >know when the filter returns is what it tells you. In this case, all it
> >can tell you is the HTTP response code. The filter should do the right
> >thing with the error code, because the core can't know what an error
> >code from a filter means.
>
> It seems to me [I might be really confused] that the filter stack
> is often too late to make these determinations. If we send
> a 200 header response, and the filter stack goes belly-up
> or decides it doesn't like what its parsing, what can we do at
> that point.
If the data is originating on your computer, you can guarantee that the
tag you are looking for is within the first 100 bytes. That allows you to
easily make the decision before the first chunk of data is sent to the
client, without holding up the response from the user's point of view.
Once we have found the tag in the file, we can return HTTP_UNAUTHORIZED
from the filter, and everything will flow correctly to ap_die.
> Even if you did try morphing the response code in the filter
> stack, what would happen with custom error responses, etc?
> Seems like things aren't recoverable at that point.
Everything goes through ap_die, so all custom responses are handled
correctly.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
550 Jean St
Oakland CA 94610
-------------------------------------------------------------------------------
RE: [PATCH] invalid HTTP status codes in access log
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 05:19 PM 3/22/2002, Ryan wrote:
>You don't know that something really broke just because the filter had a
>problem.
Ok, I'm confused. If the filter has a problem, but it doesn't hurt anything,
why on earth would it report the error?
>I have written Apache filters (for 1.3) that decided
>authentication based on content in the request/response files. All you
>know when the filter returns is what it tells you. In this case, all it
>can tell you is the HTTP response code. The filter should do the right
>thing with the error code, because the core can't know what an error
>code from a filter means.
It seems to me [I might be really confused] that the filter stack
is often too late to make these determinations. If we send
a 200 header response, and the filter stack goes belly-up
or decides it doesn't like what its parsing, what can we do at
that point.
Even if you did try morphing the response code in the filter
stack, what would happen with custom error responses, etc?
Seems like things aren't recoverable at that point.
Bill
Re: [PATCH] invalid HTTP status codes in access log
Posted by Greg Stein <gs...@lyra.org>.
On Fri, Mar 22, 2002 at 03:19:00PM -0800, Ryan Bloom wrote:
> > At 05:01 PM 3/22/2002, you wrote:
> > > > > Filters shouldn't return apr_status_t's, because there is nothing
> > > > > that the core can do with a status code.
> >
> > True, they can do nothing with the apr_status_t code, any more than they
> > can do anything with the HTTP_SOME_CODE or any other magic number.
Untrue. apr_status_t has an application-code concept. You can define
application-specific status codes that signify particular behavior to other
app filters or the application that is writing to the filter stack.
In general, though, you are right: most callers will just see non-zero and
throw it further up the stack.
> > But an apr_status_t is far more descriptive of what went wrong in the
> > filter
> > stack, and it doesn't bind filters as tightly to HTTP server applications.
Absolutely on all accounts.
I believe that I raised the issue of conflicting status/http-codes in the
filter stuff well over a year ago. Some mumbles and hoohaas, and it was
forgotten.
> > I know there was a great deal of interest in using them in a client, and
> > there will certainly be other network applications over time.
Yup. The filter functions are defined in util_filter.h to return an
apr_status_t. All filter functions are, in turn, declared that way. That
means that returning an HTTP_FOO is totally wrong.
The correct fix for logging was not to make the filters return HTTP_FOO
values, but that whoever took the return value from a filter function and
passed it as an http status code was broken.
> > Binding the result to HTTP code seems silly, Cliff is right --- if there
> > is any
> > non-APR_SUCCESS result, the core should 500, something really broke.
Yes.
> You don't know that something really broke just because the filter had a
> problem.
Either the filter had a problem, or it didn't. Simple as that. It should not
throw an error, if it didn't really have one.
> I have written Apache filters (for 1.3) that decided
> authentication based on content in the request/response files. All you
> know when the filter returns is what it tells you. In this case, all it
> can tell you is the HTTP response code. The filter should do the right
> thing with the error code, because the core can't know what an error
> code from a filter means.
In this case, the filter is going to have to buffer up however much of the
response is necessary until it can determine the response code. At that
point, it would set the right r-> value, and then let the content through,
or replace it with error content.
In either case, it probably is not returning an error up the filter stack.
If it *did* do that, then the filter is signifying that an *error* occurred,
and the application will probably stop all processing.
You could also say that it returns APR_OS_START_USEERR+1 to signify the
authentication problem. The app then maps that to HTTP_UNAUTHORIZED.
But to tie the filter stack to HTTP_ error codes is wrong.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
RE: [PATCH] invalid HTTP status codes in access log
Posted by Cliff Woolley <jw...@virginia.edu>.
On Fri, 22 Mar 2002, William A. Rowe, Jr. wrote:
> But an apr_status_t is far more descriptive of what went wrong in the filter
> stack, and it doesn't bind filters as tightly to HTTP server applications.
> I know there was a great deal of interest in using them in a client, and
> there will certainly be other network applications over time.
True...
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
RE: [PATCH] invalid HTTP status codes in access log
Posted by Ryan Bloom <rb...@covalent.net>.
> At 05:01 PM 3/22/2002, you wrote:
> > > > Filters shouldn't return apr_status_t's, because there is
nothing
> > > > that the core can do with a status code.
>
> True, they can do nothing with the apr_status_t code, any more than
they
> can do anything with the HTTP_SOME_CODE or any other magic number.
> But an apr_status_t is far more descriptive of what went wrong in the
> filter
> stack, and it doesn't bind filters as tightly to HTTP server
applications.
> I know there was a great deal of interest in using them in a client,
and
> there will certainly be other network applications over time.
>
> Binding the result to HTTP code seems silly, Cliff is right --- if
there
> is
> any
> non-APR_SUCCESS result, the core should 500, something really broke.
You don't know that something really broke just because the filter had a
problem. I have written Apache filters (for 1.3) that decided
authentication based on content in the request/response files. All you
know when the filter returns is what it tells you. In this case, all it
can tell you is the HTTP response code. The filter should do the right
thing with the error code, because the core can't know what an error
code from a filter means.
Ryan
RE: [PATCH] invalid HTTP status codes in access log
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 05:01 PM 3/22/2002, you wrote:
> > > Filters shouldn't return apr_status_t's, because there is nothing
> > > that the core can do with a status code.
True, they can do nothing with the apr_status_t code, any more than they
can do anything with the HTTP_SOME_CODE or any other magic number.
But an apr_status_t is far more descriptive of what went wrong in the filter
stack, and it doesn't bind filters as tightly to HTTP server applications.
I know there was a great deal of interest in using them in a client, and
there will certainly be other network applications over time.
Binding the result to HTTP code seems silly, Cliff is right --- if there is
any
non-APR_SUCCESS result, the core should 500, something really broke.
Bill
RE: [PATCH] invalid HTTP status codes in access log
Posted by Ryan Bloom <rb...@covalent.net>.
> > Filters shouldn't return apr_status_t's, because there is nothing
that
> > the core can do with a status code.
>
> Okay then... core_output_filter should not be returning APR_SUCCESS,
and
> its return type should not be apr_status_t. I had two options, and
picked
> the other. Both are fine. :)
I agree, filters should be returning int, and they should return HTTP_OK
or HTTP_RANDOM_STATUS_CODE. Now, we just have to make the change. :-)
Ryan
RE: [PATCH] invalid HTTP status codes in access log
Posted by Cliff Woolley <jw...@virginia.edu>.
On Fri, 22 Mar 2002, Ryan Bloom wrote:
> Filters shouldn't return apr_status_t's, because there is nothing that
> the core can do with a status code.
Okay then... core_output_filter should not be returning APR_SUCCESS, and
its return type should not be apr_status_t. I had two options, and picked
the other. Both are fine. :)
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
RE: [PATCH] invalid HTTP status codes in access log
Posted by Ryan Bloom <rb...@covalent.net>.
> > I am more than happy with that argument. I will change the second
> > return to APR_SUCCESS, and commit.
>
> One thing I'm confused about... core_output_filter's return type is
> apr_status_t. HTTP_INTERNAL_SERVER_ERROR is not an apr_status_t. It
> seems to me that core_output_filter should keep returning rv and the
> caller should look for a filter return code != APR_SUCCESS and
translate
> that into HTTP_INTERNAL_SERVER_ERROR at that time.
Filters shouldn't return apr_status_t's, because there is nothing that
the core can do with a status code. The filter must be responsible for
converting the error code to an HTTP status code. The reason that the
filters must return the HTTP status code, is that handlers return the
status code, and 99% of the time, handlers call ap_pass_brigade, and
return the result.
Ryan
RE: [PATCH] invalid HTTP status codes in access log
Posted by Cliff Woolley <jw...@virginia.edu>.
On Fri, 22 Mar 2002, Ryan Bloom wrote:
> I am more than happy with that argument. I will change the second
> return to APR_SUCCESS, and commit.
One thing I'm confused about... core_output_filter's return type is
apr_status_t. HTTP_INTERNAL_SERVER_ERROR is not an apr_status_t. It
seems to me that core_output_filter should keep returning rv and the
caller should look for a filter return code != APR_SUCCESS and translate
that into HTTP_INTERNAL_SERVER_ERROR at that time.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
RE: [PATCH] invalid HTTP status codes in access log
Posted by Ryan Bloom <rb...@covalent.net>.
> > but the second has me stumped. If the user aborts the connection we
> > shouldn't log 200, but I don't know what is correct. Ryan and I
decided
> > to use 206 for now. Can I get some opinions on the correct value?
>
> IMO, the access log should *always* reflect the status code that was
(or
> would have been) sent to the client, regardless of whether the client
> aborted the transaction. Noting that the client aborted the
transaction
> is the error log's job.
I am more than happy with that argument. I will change the second
return to APR_SUCCESS, and commit.
Ryan
Re: [PATCH] invalid HTTP status codes in access log
Posted by Cliff Woolley <jw...@virginia.edu>.
On Fri, 22 Mar 2002, Ryan Bloom wrote:
> but the second has me stumped. If the user aborts the connection we
> shouldn't log 200, but I don't know what is correct. Ryan and I decided
> to use 206 for now. Can I get some opinions on the correct value?
IMO, the access log should *always* reflect the status code that was (or
would have been) sent to the client, regardless of whether the client
aborted the transaction. Noting that the client aborted the transaction
is the error log's job.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA