httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Ruggeri <DRugg...@primary.net>
Subject Re: svn commit: r1781701 - in /httpd/httpd/trunk: docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c
Date Sat, 18 Feb 2017 22:07:02 GMT
On 2/6/2017 2:51 PM, Ruediger Pluem wrote:
>
> On 02/04/2017 09:14 PM, druggeri@apache.org wrote:
>> Author: druggeri
>> Date: Sat Feb  4 20:14:18 2017
>> New Revision: 1781701
>>
>> URL: http://svn.apache.org/viewvc?rev=1781701&view=rev
>> Log:
>> Change tactic for PROXY processing in Optional case
>>
>> Modified:
>>     httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml
>>     httpd/httpd/trunk/modules/metadata/mod_remoteip.c
>>
>> Modified: httpd/httpd/trunk/modules/metadata/mod_remoteip.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/metadata/mod_remoteip.c?rev=1781701&r1=1781700&r2=1781701&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
>> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Sat Feb  4 20:14:18 2017
>> @@ -1085,28 +1091,53 @@ static apr_status_t remoteip_input_filte
>>              memcpy(ctx->header + ctx->rcvd, ptr, len);
>>              ctx->rcvd += len;
>>  
>> -            /* Put this bucket into our temporary storage brigade because
>> -               we may permit a request without a header. In that case, the
>> -               data in the temporary storage brigade just gets copied
>> -               to bb_out */
>> -            APR_BUCKET_REMOVE(b);
>> -            apr_bucket_setaside(b, f->c->pool);
>> -            APR_BRIGADE_INSERT_TAIL(ctx->store_bb, b);
>> +            apr_bucket_delete(b);
>>              psts = HDR_NEED_MORE;
>>  
>>              if (ctx->version == 0) {
>>                  /* reading initial chunk */
>>                  if (ctx->rcvd >= MIN_HDR_LEN) {
>>                      ctx->version = remoteip_determine_version(f->c, ctx->header);
>> -                    if (ctx->version < 0) {
>> -                        psts = HDR_MISSING;
>> -                    }
>> -                    else if (ctx->version == 1) {
>> -                        ctx->mode = AP_MODE_GETLINE;
>> -                        ctx->need = sizeof(proxy_v1);
>> +
>> +                    /* We've read enough to know that a header is present. In peek
mode
>> +                       we purge the bb and can decide to step aside or switch to
>> +                       non-speculative read to consume the data */
>> +                    if (ctx->peeking) {
>> +                        apr_brigade_destroy(ctx->bb);
> apr_brigade_cleanup is better instead of destroy and create afterwards.

Agreed - updated


>
>> +
>> +                        if (ctx->version < 0) {
>> +                            ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, f->c, APLOGNO(03512)
>> +                                          "RemoteIPProxyProtocol: PROXY header is
missing from "
>> +                                          "request. Stepping aside.");
>> +                            ctx->bb = NULL;
>> +                            ctx->done = 1;
>> +                            return ap_get_brigade(f->next, bb_out, mode, block,
readbytes);
>> +                        }
>> +                        else {
>> +                            /* Rest ctx to initial values */
>> +                            ctx->rcvd = 0;
>> +                            ctx->need = MIN_HDR_LEN;
>> +                            ctx->version = 0;
>> +                            ctx->bb = apr_brigade_create(f->c->pool, f->c->bucket_alloc);
>> +                            ctx->done = 0;
>> +                            ctx->mode = AP_MODE_READBYTES;
>> +                            ctx->peeking = 0;
>> +
>> +                            ap_get_brigade(f->next, ctx->bb, ctx->mode,
block,
>> +                                           ctx->need - ctx->rcvd);
>> +                        }
>>                      }
>> -                    else if (ctx->version == 2) {
>> -                        ctx->need = MIN_V2_HDR_LEN;
>> +                    else {
>> +                        if (ctx->version < 0) {
>> +                            psts = HDR_MISSING;
>> +                        }
>> +                        else if (ctx->version == 1) {
>> +                            ctx->mode = AP_MODE_GETLINE;
>> +                            ctx->need = sizeof(proxy_v1);
>> +                        }
>> +                        else if (ctx->version == 2) {
>> +                            ctx->need = MIN_V2_HDR_LEN;
>> +                        }
>>                      }
>>                  }
>>              }
>
> Other comments:
>
> If you are reading in SPECULATIVE mode and renter the filter (e.g. MIN_HDR_LEN bytes
were not available and you were
> reading non blocking) or if you just do a second ap_get_brigade in the outer loop, the
returned brigade will contain
> all the data you had already seen plus potential new data. So you don't need to tuck
it away in ctx->header.

I think it's still necessary to copy each bucket's data to ctx->header
unless there's a guarantee that the bytes in the first bucket will be
followed in memory by the bytes in the next bucket and so on. Otherwise,
the function that examines the header as a char array may read beyond
the length of a single bucket into unsafe memory. By stashing whatever
we get into ctx->header, we make a nice and neat array for the function
to examine.


> You always
> need to examine the whole returned brigade for the header and the brigade should become
longer with a second
> ap_get_brigade.

Seems like resetting rcvd and need in ctx after the outer loop's
ap_get_brigade call would satisfy both cases mentioned above since the
filter would then just fill ctx->header from 0 index and continue asking
for a full header's worth of data.


> If not and you are in non blocking mode no new data was available and you should leave.

I've implemented a counter for data read from the buckets in this
brigade which is compared with the previous amount of data read when in
SPECULATIVE and NONBLOCK. See attached suggested patch. I'm returning
APR_SUCCESS, but it seems like APR_EAGAIN or APR_WOULDBLOCK could be
viable options, but that may be overkill.


> Also take care to handle meta buckets correctly. You could probably hit an EOS bucket
which tells you that no more
> data will be delivered.

Indeed - seems EOS would be the only bucket to look out for. I've added
that in the attached patch.


>
> Regards
>
> RĂ¼diger

Thank you for another review and comments. Let me know if the attached
diff lines up with what you had in mind for the suggestions.

-- 
Daniel Ruggeri


Mime
View raw message