On Wed, Apr 01, 2020 at 02:04:21PM +0200, Ruediger Pluem wrote:
> On 3/30/20 3:18 PM, [email protected] 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...)
> Index: ssl_engine_io.c
> ===================================================================
> --- ssl_engine_io.c (revision 1875997)
> +++ ssl_engine_io.c (working copy)
> @@ -1739,7 +1739,7 @@
> && bytes + buffered < COALESCE_BYTES
> && e != APR_BRIGADE_SENTINEL(bb)
> && !APR_BUCKET_IS_METADATA(e)) {
> - apr_status_t rv;
> + apr_status_t rv = APR_SUCCESS;
>
> /* For an indeterminate length bucket (PIPE/CGI/...), try a
> * non-blocking read to have it morph into a HEAP. If the
> @@ -1753,17 +1753,16 @@
> if (rv != APR_SUCCESS && !APR_STATUS_IS_EAGAIN(rv)) {
> ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, f->c, APLOGNO(10232)
> "coalesce failed to read from data bucket");
> + return AP_FILTER_ERROR;
> }
> }
>
> + 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_SUCCESS;
> - }
> - else {
> + if (e->length >= COALESCE_BYTES
> + || e->length + buffered + bytes >= COALESCE_BYTES) {
> rv = apr_bucket_split(e, COALESCE_BYTES - (buffered + bytes));
> }
>
> @@ -1787,6 +1786,7 @@
> return AP_FILTER_ERROR;
> }
> }
> + }
>
> upto = e;
>
>
> Regards
>
> Rüdiger
>
