On Fri, Sep 25, 2015 at 09:50:04AM +0200, Yann Ylavic wrote:
> On Fri, Sep 25, 2015 at 1:00 AM, Yann Ylavic <[email protected]> wrote:
> > On Fri, Sep 25, 2015 at 12:22 AM, Eric Covener <[email protected]> wrote:
> >>
> >> two logs (http/https) sorted to top of autoindex here:
> >> http://people.apache.org/~covener/
> >
> > Looks like mod_ssl should also forward EOR buckets.
> >
> > Does this work:
> > Index: modules/ssl/ssl_engine_io.c
> > ===================================================================
> > --- modules/ssl/ssl_engine_io.c (revision 1705160)
> > +++ modules/ssl/ssl_engine_io.c (working copy)
> > @@ -1707,12 +1707,12 @@ static apr_status_t ssl_io_filter_output(ap_filter
>
> I committed this one in r1705194, and also the one preventing the
> FLUSH for non-blocking bio_filter_in_read() in r1705236.
> You may not want to apply the latter, for your testing path to be
> consistent with what you had so far...
The behaviour of that loop is quite bad, it will treat a single brigade
like <EOS EOC> differently to two separate brigades <EOS> <EOC>,
although that should never happen in practice... currently.
I'm not sure what the "correct" behaviour of connection-level filters
should be with metadata buckets. I could argue they should delete
everything they don't understand. mod_ssl should not care at all about
EOS or EOR.
But dodging that issue... simplifying the loop like this, does that
still work for the logio issue?
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (revision 1705264)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -1704,48 +1704,31 @@
return ssl_io_filter_error(f, bb, status);
}
- while (!APR_BRIGADE_EMPTY(bb)) {
+ while (!APR_BRIGADE_EMPTY(bb) && status == APR_SUCCESS) {
apr_bucket *bucket = APR_BRIGADE_FIRST(bb);
- /* If it is a flush or EOS/EOR, we need to pass this down.
- * These types do not require translation by OpenSSL.
- */
- if (APR_BUCKET_IS_EOS(bucket) || AP_BUCKET_IS_EOR(bucket)) {
- /*
- * By definition, nothing can come after EOS/EOR.
- * which also means we can pass the rest of this brigade
- * without creating a new one since it only contains the
- * EOS/EOR bucket.
- */
-
- if ((status = ap_pass_brigade(f->next, bb)) != APR_SUCCESS) {
- return status;
+ if (APR_BUCKET_IS_METADATA(bucket)) {
+ /* Pass through metadata buckets untouched. EOC is
+ * special; terminate the SSL layer first. */
+ if (AP_BUCKET_IS_EOC(bucket)) {
+ ssl_filter_io_shutdown(filter_ctx, f->c, 0);
}
- break;
- }
- else if (APR_BUCKET_IS_FLUSH(bucket)) {
- if (bio_filter_out_flush(filter_ctx->pbioWrite) < 0) {
- status = outctx->rc;
- break;
- }
+ AP_DEBUG_ASSERT(APR_BRIGADE_EMPTY(outctx->bb));
- /* bio_filter_out_flush() already passed down a flush bucket
- * if there was any data to be flushed.
- */
- apr_bucket_delete(bucket);
+ /* Metadata buckets are passed one per brigade; it might
+ * be more efficient (but also more complex) to use
+ * outctx->bb as a true buffer and interleave these with
+ * data buckets. */
+ APR_BUCKET_REMOVE(bucket);
+ APR_BRIGADE_INSERT_HEAD(outctx->bb, bucket);
+ status = ap_pass_brigade(f->next, outctx->bb);
+ if (status == APR_SUCCESS && f->c->aborted)
+ status = APR_ECONNRESET;
+ apr_brigade_cleanup(outctx->bb);
}
- else if (AP_BUCKET_IS_EOC(bucket)) {
- /* The EOC bucket indicates connection closure, so SSL
- * shutdown must now be performed. */
- ssl_filter_io_shutdown(filter_ctx, f->c, 0);
- if ((status = ap_pass_brigade(f->next, bb)) != APR_SUCCESS) {
- return status;
- }
- break;
- }
else {
- /* filter output */
+ /* Filter a data bucket. */
const char *data;
apr_size_t len;
@@ -1758,7 +1741,9 @@
break;
}
rblock = APR_BLOCK_READ;
- continue; /* and try again with a blocking read. */
+ /* and try again with a blocking read. */
+ status = APR_SUCCESS;
+ continue;
}
rblock = APR_NONBLOCK_READ;
@@ -1769,11 +1754,8 @@
status = ssl_filter_write(f, data, len);
apr_bucket_delete(bucket);
+ }
- if (status != APR_SUCCESS) {
- break;
- }
- }
}
return status;
