On 2/6/2017 2:51 PM, Ruediger Pluem wrote:
>
> On 02/04/2017 09:14 PM, drugg...@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
Index: modules/metadata/mod_remoteip.c
===================================================================
--- modules/metadata/mod_remoteip.c (revision 1781701)
+++ modules/metadata/mod_remoteip.c (working copy)
@@ -1031,6 +1031,8 @@
remoteip_parse_status_t psts = HDR_NEED_MORE;
const char *ptr;
apr_size_t len;
+ apr_size_t this_read = 0; /* Track bytes read in each brigade */
+ apr_size_t prev_read = 0;
if (f->c->aborted) {
return APR_ECONNABORTED;
@@ -1077,9 +1079,23 @@
return block == APR_NONBLOCK_READ ? APR_SUCCESS : APR_EOF;
}
+ if (ctx->peeking) {
+ ctx->rcvd = 0;
+ ctx->need = MIN_HDR_LEN;
+ }
+
while (!ctx->done && !APR_BRIGADE_EMPTY(ctx->bb)) {
b = APR_BRIGADE_FIRST(ctx->bb);
+ if (ctx->peeking && APR_BUCKET_IS_EOS(b)) {
+ /* Shortcut - we know no header was found yet and an
+ EOS indicates we never will */
+ apr_brigade_destroy(ctx->bb);
+ ctx->bb = NULL;
+ ctx->done = 1;
+ return APR_SUCCESS;
+ }
+
ret = apr_bucket_read(b, &ptr, &len, block);
if (APR_STATUS_IS_EAGAIN(ret) && block == APR_NONBLOCK_READ) {
return APR_SUCCESS;
@@ -1091,6 +1107,10 @@
memcpy(ctx->header + ctx->rcvd, ptr, len);
ctx->rcvd += len;
+ if (ctx->peeking && block == APR_NONBLOCK_READ) {
+ this_read += len;
+ }
+
apr_bucket_delete(b);
psts = HDR_NEED_MORE;
@@ -1103,12 +1123,11 @@
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);
-
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.");
+ apr_brigade_destroy(ctx->bb);
ctx->bb = NULL;
ctx->done = 1;
return ap_get_brigade(f->next, bb_out, mode,
block, readbytes);
@@ -1118,10 +1137,10 @@
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;
+ apr_brigade_cleanup(ctx->bb);
ap_get_brigade(f->next, ctx->bb, ctx->mode, block,
ctx->need - ctx->rcvd);
@@ -1184,6 +1203,18 @@
break;
}
}
+
+ /* In SPECULATIVE mode, upstream will return all data on each brigade
get - even data
+ we've seen. For non blocking read, make sure we got new data or
return early when
+ we haven't */
+ if (ctx->peeking && block == APR_NONBLOCK_READ) {
+ if (this_read == prev_read) {
+ return APR_SUCCESS;
+ }
+ else {
+ prev_read = this_read;
+ }
+ }
}
/* we only get here when done == 1 */
