Re: svn commit: r1819969 - /httpd/httpd/trunk/modules/filters/mod_proxy_html.c

[Yann Ylavic]

On Thu, Jan 11, 2018 at 10:55 AM, ste...@eissing.org <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!