Re: SSL Input Filter bogosity

[Justin Erenkrantz]

--On Wednesday, October 30, 2002 5:56 PM -0600 "William A. Rowe, Jr." <[EMAIL PROTECTED]> wrote:

@@ -358,6 +357,12 @@
     SSLConnRec *sslconn = myConnConfig(inbio->f->c);
     int len = 0;

+    inbio->rc = APR_SUCCESS;
+
+    /* OpenSSL catches this case, so should we. */
+    if (!in)
+        return 0;
+
     /* XXX: flush here only required for SSLv2;
      * OpenSSL calls BIO_flush() at the appropriate times for
      * the other protocols.

Why are we attempting to catch NULL pointer exceptions? I don't believe that there are any cases where our code could have a NULL ctx->buffer as those are well-defined char arrays. Within mod_ssl, there shouldn't be any other entry points to the OpenSSL read calls but by entering this path with ctx->buffer, so I don't see why we should bother. (Yes, photons could hit, so what?)

@@ -366,11 +371,11 @@
         BIO_bucket_flush(inbio->wbio);
     }

-    inbio->rc = APR_SUCCESS;
-
+    BIO_clear_retry_flags(bio);
+
     /* first use data already read from socket if any */
     if ((len = char_buffer_read(&inbio->cbuf, in, inl))) {
-        if ((len <= inl) || inbio->mode == AP_MODE_GETLINE) {
+        if (len >= inl) {
             return len;
         }
         inl -= len;

Hmm, what is the point of this conditional change? char_buffer_read shouldn't be returning anything larger than inl. To me, it actually sounds that this segment should just be returning len all the time (as that conditional was *always* true). No need for a conditional here. If there's anything that we've already read, just return it. Clear out that buffer - if the user still wants more, then they can call again. (Due to our structure, we're not sure if the caller really wants all of inl or not - almost certainly not for AP_MODE_GETLINE.)

@@ -396,21 +399,58 @@
AP_MODE_READBYTES,

inbio->block,

inl);

- if ((inbio->rc != APR_SUCCESS)

||APR_BRIGADE_EMPTY(inbio->bb))

-            {
+            if (APR_STATUS_IS_EAGAIN(inbio->rc)
+                    || APR_STATUS_IS_EINTR(inbio->rc)) {
+                break;
+            }
+
+            if (inbio->rc != APR_SUCCESS) {
+                /* Unexpected errors discard the brigade */
+                apr_brigade_cleanup(inbio->bb);
+                inbio->bb = NULL;
+                return -1;
+            }
+
+            if (APR_BRIGADE_EMPTY(inbio->bb)) {
+                /* Treat an empty brigade as a retry case */
                 break;
             }
         }

We should do a conditional on the APR_BRIGADE_EMPTY() check if inbio->block is non-blocking. It's considered a design error if a filter returns an empty brigade on a blocking call.

-        inbio->bucket = APR_BRIGADE_FIRST(inbio->bb);
+        bucket = APR_BRIGADE_FIRST(inbio->bb);
+
+        if (APR_BUCKET_IS_EOS(bucket)) {
+            if (len) {
+                /* Leave this EOS on the brigade */
+                break;
+            }
+            /* Consume the EOS and return 0, we already
+             * reset the retry flag above
+             */
+            apr_brigade_cleanup(inbio->bb);
+            inbio->bb = NULL;
+            return 0;
+        }

Should the APR_BUCKET_IS_EOS rather be APR_BUCKET_IS_METADATA? Not sure here. Perhaps.

- if (inbio->mode == AP_MODE_GETLINE) {
- /* only read from the socket once in getline mode.
- * since callers buffer size is likely much larger than
- * the request headers. caller can always come back

for more

-             * if first read didn't get all the headers.
-             */
-            break;
-        }

Again, what is the rationale for removing this? We don't have a predetermined size for a GETLINE call - well, not 100% true, it sort of ends up being sizeof(ctx->buffer). Yet, I think that should at most translate to only one socket read. Not sure why we would want to do a full 8k read all the time. The hope is that on a GETLINE mode, the odds are that the lines are really short - one socket call should be sufficient to get mulitple lines. (Furthermore, we can probably read from the buffer since multiple headers will be read at once!)

Also, I'm not totally sold on the collasping of ssl_io_hook_read. I think there's an advantage to creating a fine line between the OpenSSL calls and our more abstract calls. But, whatever, no big deal. I think shorter, concise functions are better... -- justin