Re: svn commit: r1875881 - /httpd/httpd/trunk/modules/ssl/ssl_engine_io.c

[Mike Rumph]

I'm not very familiar with this code, so my questions might not make sense.

But take a look at the following two code segments in
ssl_io_filter_coalesce():

    for (e = APR_BRIGADE_FIRST(bb);
         e != APR_BRIGADE_SENTINEL(bb)
             && !APR_BUCKET_IS_METADATA(e)
             && e->length != (apr_size_t)-1
             && e->length < COALESCE_BYTES
             && (buffered + bytes + e->length) < COALESCE_BYTES;

and

        if (rv == APR_SUCCESS) {
            /* If the read above made the bucket morph, it may now fit
             * entirely within the buffer.  Otherwise, split it so it does
             * fit. */
            if (e->length >= COALESCE_BYTES
                || e->length + buffered + bytes >= COALESCE_BYTES) {
                rv = apr_bucket_split(e, COALESCE_BYTES - (buffered +
bytes));
            }

Does this mean that either buffered or bytes might be negative?
Otherwise, testing both "e->length" and  "e->length + buffered + bytes"
against  COALESCE_BYTES seems redundant.
Just one of these tests should be sufficient.

Also, is it ever possible for "COALESCE_BYTES - (buffered + bytes)" to be
negative?
If so, can the apr_bucket_split() function handle this properly?

Thanks,

Mike




On Wed, Apr 1, 2020 at 12:32 PM Ruediger Pluem <rpl...@apache.org> wrote:

>
>
> On 4/1/20 2:38 PM, Joe Orton wrote:
> > On Wed, Apr 01, 2020 at 02:04:21PM +0200, Ruediger Pluem wrote:
> >> On 3/30/20 3:18 PM, jor...@apache.org wrote:
> >>>
> >>> -        rv = apr_bucket_split(e, COALESCE_BYTES - (buffered + bytes));
> >>> +        /* If the read above made the bucket morph, it may now fit
> >>> +         * entirely within the buffer.  Otherwise, split it so it does
> >>> +         * fit. */
> >>> +        if (e->length < COALESCE_BYTES
> >>> +            && e->length + buffered + bytes < COALESCE_BYTES) {
> >>> +            rv = APR_SUCCESS;
> >>
> >> Hmm. If we had e->length == -1 above and the bucket read failed, e
> might still be the morphing bucket and hence e->length == -1.
> >> I think all the code below assumes e->length >= 0 things can get off
> the rails.
> >
> > Thanks a lot for the review... I tried to keep that as simple as
> > possible but there are too many cases to cover.  Yep, you're right.
> >
> >> How about the following patch (minus whitespace changes) to fix this:
> >
> > +1 that looks correct to me, please commit (or I can...)
>
> Committed as r1876014.
>
> Regards
>
> Rüdiger
>