On 4/2/20 9:36 AM, Joe Orton wrote:
> 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.
Sounds reasonable. Thanks for explaining. Do you fix <= vs < and > vs >= or
should I?
Regards
Rüdiger
