Re: svn commit: r1819969 - /httpd/httpd/trunk/modules/filters/mod_proxy_html.c
You are right. The continue catches that case. Nice work!
> Am 11.01.2018 um 11:18 schrieb Yann Ylavic :
>
> On Thu, Jan 11, 2018 at 10:55 AM, ste...@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 yla...@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
On Thu, Jan 11, 2018 at 10:55 AM, ste...@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 yla...@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
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 yla...@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_r
