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

[Ruediger Pluem]

On 3/30/20 3:18 PM, jor...@apache.org wrote:
> Author: jorton
> Date: Mon Mar 30 13:18:29 2020
> New Revision: 1875881
> 
> URL: http://svn.apache.org/viewvc?rev=1875881&view=rev
> Log:
> * modules/ssl/ssl_engine_io.c: (ssl_io_filter_coalesce): Handle the
>   case of a bucket which morphs to a bucket short enough to fit within
>   the buffer without needing to split.
> 
> Modified:
>     httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
> 
> Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_io.c?rev=1875881&r1=1875880&r2=1875881&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_engine_io.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_io.c Mon Mar 30 13:18:29 2020
> @@ -1749,7 +1749,17 @@ static apr_status_t ssl_io_filter_coales
>              }
>          }
>  
> -        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.

How about the following patch (minus whitespace changes) to fix this:

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