On Fri, Nov 14, 2014 at 6:00 PM, Yann Ylavic <[email protected]> wrote:
> On Fri, Nov 14, 2014 at 11:25 PM, Jeff Trawick <[email protected]> wrote: > > On Fri, Nov 14, 2014 at 1:18 PM, <[email protected]> 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/
