httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From William A Rowe Jr <wr...@rowe-clan.net>
Subject Re: svn commit: r1772678 [2/2] - in /httpd/httpd/branches/2.4.x: ./ docs/manual/mod/ include/ modules/http/ server/
Date Wed, 07 Dec 2016 22:50:50 GMT
Patch incoming. More below...

On Wed, Dec 7, 2016 at 4:01 PM, William A Rowe Jr <wrowe@rowe-clan.net>
wrote:

> On Wed, Dec 7, 2016 at 2:11 PM, Ruediger Pluem <rpluem@apache.org> wrote:
>
>> Sorry for chiming in so late :-(.
>
>
> Again, no worries...
>
> On 12/05/2016 03:34 PM, jim@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);

Mime
View raw message