On Thu, Apr 02, 2020 at 07:30:50AM +0200, Ruediger Pluem wrote:
> On 4/1/20 11:44 PM, Mike Rumph wrote:
> > 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.
>
> I guess they are some sort redundant. Joe?
I was being paranoid about unsigned integer overflow when writing this.
(e->length + X) may overflow, comparing e->length against COALESCE_BYTES
in both cases prevents that since max(bytes + buffered)=COALESCE_BYTES.
There is nothing preventing someone creating a bucket (e.g. FILE) with
e->length = (apr_size_t)-2 and passing it through the filter stack
AFAIK, so I think paranoia is reasonable.
But now you made me read the code *again* I think those tests should be
e->length <= COALESCE_BYTES and e->length > COALESCE_BYTES etc.
And I thought this would be a simple patch... Very much appreciate all
the review!
Regards, Joe
