Gustaf,
Here is the blowhard answer (the short answers are below):
I agree that there must be some additional code paths which haven't been
checked. I'll try to go through what I discovered based on your
questions, but if I miss something ask again.
One nice thing about the AOLserver code is that usually all of the logic
for a particular level of processing is contained in a single function.
For a running a request, we have ConnRun in queue.c.
The basic logic is: run preauth filters, check the return code, if
NS_OK, continue to authorization, if NS_OK, continue to postauth
filters, if NS_OK, continue to the registered proc, if NS_OK, continue
to trace filters, etc.
All filters must return either NS_OK, NS_ERROR, NS_FILTER_BREAK,
NS_FILTER_RETURN, otherwise the filter running function returns
NS_ERROR. (Actually not sure if I've ever seen a filter return NS_ERROR,
or what the point would be in doing so.)
The authorization proc returns one of the authorization return codes,
but these are handled in ConnRun. It would be an error for a filter to
return one of these codes.
The newest code in ConnRun is different from the original code which
used a "goto:" if any of the request stages returned NS_ERROR. The new
code, before my proposed patch allows trace filters to run if preauth
filters return an NS_ERROR.
There was some discussion that trace filters were always skipped
(originally) because code may not have been initialized, but who knows,
it may have also been a simple way to signal that the request was done
and no additional code should run. (This is also the intended effect of
ns_adp_abort and ns_tcl_abort, but they only operate within a particular
stage.)
But this argument, if it ever existed, is no invalid. We now have proxy
requests, internal redirects, etc. These very useful features make it
impossible to rely on code initialization based upon a particular url
pattern. By the time the trace filter runs, the url may have changed.
Also, if a new return code (500, 404) is set, a proxy runs which might
access a configured procedure or file.
So my best guess is that when this latest code was written, some of
these observations may have been in play, but the change was only
applied to preauth filters.
My changes are based upon the code comment which explains what is going
on with preauth errors:
status = NsRunFilters(conn, NS_FILTER_PRE_AUTH);
if (status == NS_OK) {
...
} else if (status != NS_FILTER_RETURN) {
/* if not ok or filter_return, then the pre-auth filter coughed
* an error. We are not going to proceed, but also we
* can't count on the filter to have sent a response
* back to the client. So, send an error response.
*/
Ns_ConnReturnInternalError(conn);
status = NS_FILTER_RETURN; /* to allow tracing to happen */
}
Of course we can never count on an error being sent back if in the cases
where postauth filters or the registered proc return an internal error,
so the same reasoning should apply in those cases as well.
Note that Ns_ConnReturnInternalError will just generate an error.log
error message if the conn is closed.
The changes I made to the other two files just allow the code in queue.c
to work as expected. Before the bug patch, non-errors were seen as
errors in ConnRun, so trace filters were skipped, including logging of
200 responses.
What happens when code ends with "break", "continue"? This would be a
bug in the Tcl code, it is a Tcl error to call break or continue outside
of a loop, where the loop is bounded by a procedure body. Any procedure
which ends with [continue] or [break] will end up returning a Tcl
error.
In fact, the way I discovered the solution to this issue was due to
problems with using ns_return inside a procedure or file that was
followed by additional code which I didn't want to run. In a tcl file,
you have to use ns_tcl_abort to correctly back out of deeply nested
code. The command generate a Tcl error until it reaches the top-level
where it is converted to NS_OK/TCL_OK.
So a procedure which ends in [continue] or [break] will be both a Tcl
error and an error in using the correct abort mechanism
You said:
> - when either the POST_AUTH filter or the request returns != NS_OK
> the server returns a 500 error
> - if there was an earlier "ns_return XXX", it is ignored.
>
This isn't exactly true. The server always attempts to run
Ns_ConnReturnInternalError. The code in ConnRun doesn't know anything
about any earlier ns_returnXXX. If you use ns_returnXXX, it should be
followed by ns_adp_abort or ns_tcl_abort. However, it is possible that
you might want to run code after and ns_returnXXX, but before abort. My
opinion is that you should probably wrap this in a [catch] so that any
errors are caught and handled correctly in the local code. But it might
not be absolutely necessary. The Ns_ConnReturnInternalError will notice
that the conn is closed and just log an error.
I have not checked if the access.log entry would change to a 500
response in this case. That would be somewhat misleading, but it might
be helpful in finding errors related to specific page requests.
Finish blowhard answer....
Sorry for the long winded response, I'll put short answers below:
On Sat, 2009-04-11 at 03:53 +0200, Gustaf Neumann wrote:
> Tom Jackson schrieb:
> > Okay, great! The changes should probably be divided into two patches:
>
> good idea.
>
> i was not able to follow the full discussion and most likely one needs
> more thoughts and largers test cases to consider. Do i understand correctly:
> - when either the POST_AUTH filter or the request returns != NS_OK
> the server returns a 500 error
> - if there was an earlier "ns_return XXX", it is ignored.
ConnRun doesn't know anything about earlier run code, just the result
code was not NS_OK or NS_FILTER_RETURN.
> NsRunFilters retuns either NS_OK or NS_FILTER_BREAK, NS_FILTER_TRACE or
> NS_FILTER_RETURN. Don't we have to deal with the other non-ok cases?
>
No, filters can only return NS_OK, NS_FILTER_BREAK or NS_FILTER_RETURN.
> More filters and the traces are run, when the status is NS_OK
> or NS_FILTER_RETURN. If i look at the code, i wonder,
> what happens with filters and traces for status codes like
> NS_FORBIDDEN, NS_UNAUTHORIZED, or NS_SHUTDOWN?
>
An error occurs, filters must return NS_OK, NS_FILTER_BREAK or
NS_FILTER_RETURN.
> Have you checked what happens, when a Ns_ConnRunRequest
> ended in Tcl with a "return", "break" or "continue"?
[return] is okay, [break] and [continue] will be errors if code reaches
the end of a proc or the Tcl top level outside of a loop (usually this
happens pretty quick and is a Tcl use bug). Just notice here that
[return] has a return code of TCL_OK. Also notice that Ns_ConnRunRequest
can only return NS_OK or NS_ERROR, no other options are needed here.
tom jackson
--
AOLserver - http://www.aolserver.com/
To Remove yourself from this list, simply send an email to
<lists...@listserv.aol.com> with the
body of "SIGNOFF AOLSERVER" in the email message. You can leave the Subject:
field of your email blank.
