You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by yl...@apache.org on 2018/01/03 13:37:50 UTC
svn commit: r1819969 - /httpd/httpd/trunk/modules/filters/mod_proxy_html.c
Author: ylavic
Date: Wed Jan 3 13:37:50 2018
New Revision: 1819969
URL: http://svn.apache.org/viewvc?rev=1819969&view=rev
Log:
mod_proxy_html: follow up to r1599012.
To determine whether or not HTML data are lower than 4 bytes, use a retain
buffer rather than assuming that all should be contained in a single bucket
with the next one being EOS (if any).
Modified:
httpd/httpd/trunk/modules/filters/mod_proxy_html.c
Modified: httpd/httpd/trunk/modules/filters/mod_proxy_html.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_proxy_html.c?rev=1819969&r1=1819968&r2=1819969&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/filters/mod_proxy_html.c (original)
+++ httpd/httpd/trunk/modules/filters/mod_proxy_html.c Wed Jan 3 13:37:50 2018
@@ -109,6 +109,9 @@ typedef struct {
const char *encoding;
urlmap *map;
const char *etag;
+ char rbuf[4];
+ apr_size_t rlen;
+ apr_size_t rmin;
} saxctxt;
@@ -873,6 +876,17 @@ static saxctxt *check_filter_init (ap_fi
return f->ctx;
}
+static void prepend_rbuf(saxctxt *ctxt, apr_bucket_brigade *bb)
+{
+ if (ctxt->rlen) {
+ apr_bucket *b = apr_bucket_transient_create(ctxt->rbuf,
+ ctxt->rlen,
+ bb->bucket_alloc);
+ APR_BRIGADE_INSERT_HEAD(bb, b);
+ ctxt->rlen = 0;
+ }
+}
+
static apr_status_t proxy_html_filter(ap_filter_t *f, apr_bucket_brigade *bb)
{
apr_bucket* b;
@@ -894,11 +908,15 @@ static apr_status_t proxy_html_filter(ap
if (APR_BUCKET_IS_METADATA(b)) {
if (APR_BUCKET_IS_EOS(b)) {
if (ctxt->parser != NULL) {
- consume_buffer(ctxt, buf, 0, 1);
+ consume_buffer(ctxt, "", 0, 1);
+ }
+ else {
+ prepend_rbuf(ctxt, ctxt->bb);
}
APR_BRIGADE_INSERT_TAIL(ctxt->bb,
- apr_bucket_eos_create(ctxt->bb->bucket_alloc));
+ apr_bucket_eos_create(ctxt->bb->bucket_alloc));
ap_pass_brigade(ctxt->f->next, ctxt->bb);
+ apr_brigade_cleanup(ctxt->bb);
}
else if (APR_BUCKET_IS_FLUSH(b)) {
/* pass on flush, except at start where it would cause
@@ -918,9 +936,18 @@ static apr_status_t proxy_html_filter(ap
* HTML rewriting. The URL schema (i.e. 'http') needs four bytes alone.
* And the HTML parser needs at least four bytes to initialise correctly.
*/
- if ((bytes < 4) && APR_BUCKET_IS_EOS(APR_BUCKET_NEXT(b))) {
- ap_remove_output_filter(f);
- return ap_pass_brigade(f->next, bb);
+ ctxt->rmin += bytes;
+ if (ctxt->rmin < sizeof(ctxt->rbuf)) {
+ memcpy(ctxt->rbuf + ctxt->rlen, buf, bytes);
+ ctxt->rlen += bytes;
+ continue;
+ }
+ if (ctxt->rlen && ctxt->rlen < sizeof(ctxt->rbuf)) {
+ apr_size_t rem = sizeof(ctxt->rbuf) - ctxt->rlen;
+ memcpy(ctxt->rbuf + ctxt->rlen, buf, rem);
+ ctxt->rlen += rem;
+ buf += rem;
+ bytes -= rem;
}
if (!xml2enc_charset ||
@@ -949,15 +976,25 @@ static apr_status_t proxy_html_filter(ap
}
ap_fputs(f->next, ctxt->bb, ctxt->cfg->doctype);
- ctxt->parser = htmlCreatePushParserCtxt(&sax, ctxt, buf,
- 4, 0, enc);
- buf += 4;
- bytes -= 4;
+
+ if (ctxt->rlen) {
+ ctxt->parser = htmlCreatePushParserCtxt(&sax, ctxt,
+ ctxt->rbuf,
+ ctxt->rlen,
+ NULL, enc);
+ }
+ else {
+ ctxt->parser = htmlCreatePushParserCtxt(&sax, ctxt, buf, 4,
+ NULL, enc);
+ buf += 4;
+ bytes -= 4;
+ }
if (ctxt->parser == NULL) {
- apr_status_t rv = ap_pass_brigade(f->next, bb);
+ prepend_rbuf(ctxt, bb);
ap_remove_output_filter(f);
- return rv;
+ return ap_pass_brigade(f->next, bb);
}
+ ctxt->rlen = 0;
apr_pool_cleanup_register(f->r->pool, ctxt->parser,
(int(*)(void*))htmlFreeParserCtxt,
apr_pool_cleanup_null);
Re: svn commit: r1819969 -
/httpd/httpd/trunk/modules/filters/mod_proxy_html.c
Posted by Stefan Eissing <st...@greenbytes.de>.
You are right. The continue catches that case. Nice work!
> Am 11.01.2018 um 11:18 schrieb Yann Ylavic <yl...@gmail.com>:
>
> On Thu, Jan 11, 2018 at 10:55 AM, stefan@eissing.org <st...@eissing.org> wrote:
>> Hmm...lines 952+: if the first data bucket has length 0, will the
>> parser not still be opened on potentially uninitialized buf memory?
>
> I tried to make so that zero length buckets (actually any leading
> bucket until "ctxt->rmin" is reached) are caught by the "continue" at
> 945. Do you see an overlook on my side?
>
> See below comments...
>
>>
>>> Am 03.01.2018 um 14:37 schrieb ylavic@apache.org:
>>>
>>> Modified: httpd/httpd/trunk/modules/filters/mod_proxy_html.c
> []
>>> static apr_status_t proxy_html_filter(ap_filter_t *f, apr_bucket_brigade *bb)
>>> {
>>> apr_bucket* b;
> []
>>> @@ -918,9 +936,18 @@ static apr_status_t proxy_html_filter(ap
>>> * HTML rewriting. The URL schema (i.e. 'http') needs four bytes alone.
>>> * And the HTML parser needs at least four bytes to initialise correctly.
>>> */
>>> - if ((bytes < 4) && APR_BUCKET_IS_EOS(APR_BUCKET_NEXT(b))) {
>>> - ap_remove_output_filter(f);
>>> - return ap_pass_brigade(f->next, bb);
>>> + ctxt->rmin += bytes;
>>> + if (ctxt->rmin < sizeof(ctxt->rbuf)) {
>>> + memcpy(ctxt->rbuf + ctxt->rlen, buf, bytes);
>>> + ctxt->rlen += bytes;
>>> + continue;
>
> Here, supposedly, leading empty buckets should be ignored.
>
>
>>> + }
>>> + if (ctxt->rlen && ctxt->rlen < sizeof(ctxt->rbuf)) {
>>> + apr_size_t rem = sizeof(ctxt->rbuf) - ctxt->rlen;
>>> + memcpy(ctxt->rbuf + ctxt->rlen, buf, rem);
>>> + ctxt->rlen += rem;
>>> + buf += rem;
>>> + bytes -= rem;
>>> }
>
> So from here we should either have the bucket's buf >= 4 bytes or
> "ctxt->rbuf" filled in.
>
>>>
>>> if (!xml2enc_charset ||
>>> @@ -949,15 +976,25 @@ static apr_status_t proxy_html_filter(ap
>>> }
>>>
>>> ap_fputs(f->next, ctxt->bb, ctxt->cfg->doctype);
>>> - ctxt->parser = htmlCreatePushParserCtxt(&sax, ctxt, buf,
>>> - 4, 0, enc);
>>> - buf += 4;
>>> - bytes -= 4;
>
> The two cases being handled below:
>>> +
>>> + if (ctxt->rlen) {
>>> + ctxt->parser = htmlCreatePushParserCtxt(&sax, ctxt,
>>> + ctxt->rbuf,
>>> + ctxt->rlen,
>>> + NULL, enc);
>>> + }
>>> + else {
>>> + ctxt->parser = htmlCreatePushParserCtxt(&sax, ctxt, buf, 4,
>>> + NULL, enc);
>>> + buf += 4;
>>> + bytes -= 4;
>>> + }
>
>
> Thanks for the review!
Re: svn commit: r1819969 - /httpd/httpd/trunk/modules/filters/mod_proxy_html.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Jan 11, 2018 at 10:55 AM, stefan@eissing.org <st...@eissing.org> wrote:
> Hmm...lines 952+: if the first data bucket has length 0, will the
> parser not still be opened on potentially uninitialized buf memory?
I tried to make so that zero length buckets (actually any leading
bucket until "ctxt->rmin" is reached) are caught by the "continue" at
945. Do you see an overlook on my side?
See below comments...
>
>> Am 03.01.2018 um 14:37 schrieb ylavic@apache.org:
>>
>> Modified: httpd/httpd/trunk/modules/filters/mod_proxy_html.c
[]
>> static apr_status_t proxy_html_filter(ap_filter_t *f, apr_bucket_brigade *bb)
>> {
>> apr_bucket* b;
[]
>> @@ -918,9 +936,18 @@ static apr_status_t proxy_html_filter(ap
>> * HTML rewriting. The URL schema (i.e. 'http') needs four bytes alone.
>> * And the HTML parser needs at least four bytes to initialise correctly.
>> */
>> - if ((bytes < 4) && APR_BUCKET_IS_EOS(APR_BUCKET_NEXT(b))) {
>> - ap_remove_output_filter(f);
>> - return ap_pass_brigade(f->next, bb);
>> + ctxt->rmin += bytes;
>> + if (ctxt->rmin < sizeof(ctxt->rbuf)) {
>> + memcpy(ctxt->rbuf + ctxt->rlen, buf, bytes);
>> + ctxt->rlen += bytes;
>> + continue;
Here, supposedly, leading empty buckets should be ignored.
>> + }
>> + if (ctxt->rlen && ctxt->rlen < sizeof(ctxt->rbuf)) {
>> + apr_size_t rem = sizeof(ctxt->rbuf) - ctxt->rlen;
>> + memcpy(ctxt->rbuf + ctxt->rlen, buf, rem);
>> + ctxt->rlen += rem;
>> + buf += rem;
>> + bytes -= rem;
>> }
So from here we should either have the bucket's buf >= 4 bytes or
"ctxt->rbuf" filled in.
>>
>> if (!xml2enc_charset ||
>> @@ -949,15 +976,25 @@ static apr_status_t proxy_html_filter(ap
>> }
>>
>> ap_fputs(f->next, ctxt->bb, ctxt->cfg->doctype);
>> - ctxt->parser = htmlCreatePushParserCtxt(&sax, ctxt, buf,
>> - 4, 0, enc);
>> - buf += 4;
>> - bytes -= 4;
The two cases being handled below:
>> +
>> + if (ctxt->rlen) {
>> + ctxt->parser = htmlCreatePushParserCtxt(&sax, ctxt,
>> + ctxt->rbuf,
>> + ctxt->rlen,
>> + NULL, enc);
>> + }
>> + else {
>> + ctxt->parser = htmlCreatePushParserCtxt(&sax, ctxt, buf, 4,
>> + NULL, enc);
>> + buf += 4;
>> + bytes -= 4;
>> + }
Thanks for the review!
Re: svn commit: r1819969 -
/httpd/httpd/trunk/modules/filters/mod_proxy_html.c
Posted by "stefan@eissing.org" <st...@eissing.org>.
Hmm...lines 952+: if the first data bucket has length 0, will the parser not still be opened on potentially uninitialized buf memory?
> Am 03.01.2018 um 14:37 schrieb ylavic@apache.org:
>
> Author: ylavic
> Date: Wed Jan 3 13:37:50 2018
> New Revision: 1819969
>
> URL: http://svn.apache.org/viewvc?rev=1819969&view=rev
> Log:
> mod_proxy_html: follow up to r1599012.
>
> To determine whether or not HTML data are lower than 4 bytes, use a retain
> buffer rather than assuming that all should be contained in a single bucket
> with the next one being EOS (if any).
>
>
> Modified:
> httpd/httpd/trunk/modules/filters/mod_proxy_html.c
>
> Modified: httpd/httpd/trunk/modules/filters/mod_proxy_html.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_proxy_html.c?rev=1819969&r1=1819968&r2=1819969&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/filters/mod_proxy_html.c (original)
> +++ httpd/httpd/trunk/modules/filters/mod_proxy_html.c Wed Jan 3 13:37:50 2018
> @@ -109,6 +109,9 @@ typedef struct {
> const char *encoding;
> urlmap *map;
> const char *etag;
> + char rbuf[4];
> + apr_size_t rlen;
> + apr_size_t rmin;
> } saxctxt;
>
>
> @@ -873,6 +876,17 @@ static saxctxt *check_filter_init (ap_fi
> return f->ctx;
> }
>
> +static void prepend_rbuf(saxctxt *ctxt, apr_bucket_brigade *bb)
> +{
> + if (ctxt->rlen) {
> + apr_bucket *b = apr_bucket_transient_create(ctxt->rbuf,
> + ctxt->rlen,
> + bb->bucket_alloc);
> + APR_BRIGADE_INSERT_HEAD(bb, b);
> + ctxt->rlen = 0;
> + }
> +}
> +
> static apr_status_t proxy_html_filter(ap_filter_t *f, apr_bucket_brigade *bb)
> {
> apr_bucket* b;
> @@ -894,11 +908,15 @@ static apr_status_t proxy_html_filter(ap
> if (APR_BUCKET_IS_METADATA(b)) {
> if (APR_BUCKET_IS_EOS(b)) {
> if (ctxt->parser != NULL) {
> - consume_buffer(ctxt, buf, 0, 1);
> + consume_buffer(ctxt, "", 0, 1);
> + }
> + else {
> + prepend_rbuf(ctxt, ctxt->bb);
> }
> APR_BRIGADE_INSERT_TAIL(ctxt->bb,
> - apr_bucket_eos_create(ctxt->bb->bucket_alloc));
> + apr_bucket_eos_create(ctxt->bb->bucket_alloc));
> ap_pass_brigade(ctxt->f->next, ctxt->bb);
> + apr_brigade_cleanup(ctxt->bb);
> }
> else if (APR_BUCKET_IS_FLUSH(b)) {
> /* pass on flush, except at start where it would cause
> @@ -918,9 +936,18 @@ static apr_status_t proxy_html_filter(ap
> * HTML rewriting. The URL schema (i.e. 'http') needs four bytes alone.
> * And the HTML parser needs at least four bytes to initialise correctly.
> */
> - if ((bytes < 4) && APR_BUCKET_IS_EOS(APR_BUCKET_NEXT(b))) {
> - ap_remove_output_filter(f);
> - return ap_pass_brigade(f->next, bb);
> + ctxt->rmin += bytes;
> + if (ctxt->rmin < sizeof(ctxt->rbuf)) {
> + memcpy(ctxt->rbuf + ctxt->rlen, buf, bytes);
> + ctxt->rlen += bytes;
> + continue;
> + }
> + if (ctxt->rlen && ctxt->rlen < sizeof(ctxt->rbuf)) {
> + apr_size_t rem = sizeof(ctxt->rbuf) - ctxt->rlen;
> + memcpy(ctxt->rbuf + ctxt->rlen, buf, rem);
> + ctxt->rlen += rem;
> + buf += rem;
> + bytes -= rem;
> }
>
> if (!xml2enc_charset ||
> @@ -949,15 +976,25 @@ static apr_status_t proxy_html_filter(ap
> }
>
> ap_fputs(f->next, ctxt->bb, ctxt->cfg->doctype);
> - ctxt->parser = htmlCreatePushParserCtxt(&sax, ctxt, buf,
> - 4, 0, enc);
> - buf += 4;
> - bytes -= 4;
> +
> + if (ctxt->rlen) {
> + ctxt->parser = htmlCreatePushParserCtxt(&sax, ctxt,
> + ctxt->rbuf,
> + ctxt->rlen,
> + NULL, enc);
> + }
> + else {
> + ctxt->parser = htmlCreatePushParserCtxt(&sax, ctxt, buf, 4,
> + NULL, enc);
> + buf += 4;
> + bytes -= 4;
> + }
> if (ctxt->parser == NULL) {
> - apr_status_t rv = ap_pass_brigade(f->next, bb);
> + prepend_rbuf(ctxt, bb);
> ap_remove_output_filter(f);
> - return rv;
> + return ap_pass_brigade(f->next, bb);
> }
> + ctxt->rlen = 0;
> apr_pool_cleanup_register(f->r->pool, ctxt->parser,
> (int(*)(void*))htmlFreeParserCtxt,
> apr_pool_cleanup_null);
>
>