Patch incoming. More below... On Wed, Dec 7, 2016 at 4:01 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Wed, Dec 7, 2016 at 2:11 PM, Ruediger Pluem <rpl...@apache.org> wrote: > >> Sorry for chiming in so late :-(. > > > Again, no worries... > > On 12/05/2016 03:34 PM, j...@apache.org wrote: >> > Modified: httpd/httpd/branches/2.4.x/server/protocol.c >> > URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/serv >> er/protocol.c?rev=1772678&r1=1772677&r2=1772678&view=diff >> > ============================================================ >> ================== >> > --- httpd/httpd/branches/2.4.x/server/protocol.c (original) >> > +++ httpd/httpd/branches/2.4.x/server/protocol.c Mon Dec 5 14:34:29 >> 2016 >> >> > + else if (r->protocol[0]) { >> > + r->assbackwards = 0; >> > + r->proto_num = HTTP_VERSION(1,0); >> > + /* Defer setting the r->protocol string till error msg is >> composed */ >> > + if (strict && deferred_error == rrl_none) >> > + deferred_error = rrl_badprotocol; >> > Rethinking this, anything other than /HTTP\/n.n/i is truly garbage. More likely than not, there was a raw SP in the URL. This must not be evaluated based on 'strict'ness. > > + else >> > + r->protocol = "HTTP/1.0"; >> >> Hm. r->protocol is not const char*. Shouldn't we use >> >> apr_pstrdup(r->pool, "HTTP/1.0"); >> > It was easier to simplify this down to an exception in rrl_failed by setting the protocol to 0.9 for the moment. > > + } >> > > + else { >> > + r->assbackwards = 1; >> > + r->protocol = "HTTP/0.9"; >> > + r->proto_num = HTTP_VERSION(0, 9); >> > + } >> > > Does it make more sense to to apply the apr_pstrdup() against > r->protocol just here, after all but one assignment is made? > > The same question occurs here with 0.9, and down at rrl_failed: > below. We can apr_pstrdup each of these three, letting the caller > manipulate the original we scanned in the successful case, or > make a copy in all cases right here, once again below for 1.0. > Also note a few cases of 0-length strings. Can someone alter our empty NULL terminator and expect any good to come of that? ... r->protocol = uri = ""; ... r->protocol = ""; But these will be wiped out shortly thereafter. > I had expected to see some warning... and mentioned it in the > initial commit log, but my maintainer-mode build did not trip on this. > Hmmm... using a constant is not unprecedented... For example... protocol.c (an unedited bit of it)... AP_DECLARE(void) ap_set_sub_req_protocol(request_rec *rnew, const request_rec *r) { rnew->the_request = r->the_request; /* Keep original request-line */ rnew->assbackwards = 1; /* Don't send headers from this. */ rnew->no_local_copy = 1; /* Don't try to send HTTP_NOT_MODIFIED for a * fragment. */ rnew->method = "GET"; rnew->method_number = M_GET; rnew->protocol = "INCLUDED"; mod_proxy_hcheck.c ... /* * Create a dummy request rec, simply so we can use ap_expr. * Use our short-lived poll for bucket_alloc */ static request_rec *create_request_rec(apr_pool_t *p1, conn_rec *conn, const char *method) { [...] r->protocol = "HTTP/1.0"; r->proto_num = HTTP_VERSION(1, 0);
