httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
Subject Re: svn commit: r1639717 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/aaa/mod_authnz_fcgi.c
Date Fri, 14 Nov 2014 23:40:25 GMT
On Fri, Nov 14, 2014 at 6:00 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:

> On Fri, Nov 14, 2014 at 11:25 PM, Jeff Trawick <trawick@gmail.com> wrote:
> > On Fri, Nov 14, 2014 at 1:18 PM, <ylavic@apache.org> wrote:
> []
> >> -static int handle_headers(request_rec *r,
> >> -                          int *state,
> >> -                          char *readbuf)
> >> +static int handle_headers(request_rec *r, int *state,
> >> +                          char *readbuf, apr_size_t readlen)
> >>  {
> >>      const char *itr = readbuf;
> >>
> >> -    while (*itr) {
> >> +    while (readlen) {
> >>          if (*itr == '\r') {
> >>              switch (*state) {
> >>                  case HDR_STATE_GOT_CRLF:
> >> @@ -443,13 +442,17 @@ static int handle_headers(request_rec *r
> >>                       break;
> >>              }
> >>          }
> >> -        else {
> >> +        else if (*itr == '\t' || !apr_iscntrl(*itr)) {
> >>              *state = HDR_STATE_READING_HEADERS;
> >>          }
> >> +        else {
> >> +            return -1;
> >> +        }
> >
> []
> > I guess I don't understand the importance of this code which returns an
> > error on non-tab control characters.  Why isn't the character checking
> the
> > same as in ap_scan_script_header_err_brigade_ex() since that is what will
> > finally parse the buffer?
>
> Well, the previous code was stopping on '\0' (hopefully a '\n' was
> reached before), and I wanted to preserve this behaviour.
> While at it, I chose to also stop on any control char since they are
> not valid in HTTP headers (but '\t').
> getsfunc_BRIGADE() does not seem to care about those, and is not
> supposed to return anything but -1 (timeout) as error...
>
> Isn't handle_headers() handling HTTP headers?
> If not I'm afraid I overdid that fix.
>

handle_headers() is processing these headers just enough to tell when the
complete set of headers is in the brigade, at which point it will be
processed by ap_scan_script_header_err_brigade_ex().  I don't think
catching errors that ap_scan_* don't catch is a serious problem, but it
might cause confusion in some extremely odd cases such as programmers
comparing the code :) or having a "bad" script work with a module that only
uses ap_scan_* but fail with one of these).

Is the header syntax actually supported by ap_scan_script_header* HTTP or
HTTP-like?  Here's an easy way out of that:  It doesn't support folded
lines AFAICT, so it is HTTP-like ;)  But seriously, it seems very plausible
that the CGI spec is intended to describe script headers as following the
HTTP header syntax.  (It doesn't bother describing LWS.)

Summary: I don't think it is a big deal in practice but I think it can be
confusing that the code that just needs to determine if the end has been
found can discover errors that otherwise would be unnoticed.

-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Mime
View raw message