mod_sed: review fixes, plus a test suite

changes-entries/sed-eos-on-error.txt

@@ -0,0 +1,3 @@
+  *) mod_sed: End the response properly when a sed script fails part-way
+     through it, rather than leaving the client waiting for a response
+     which never finishes.  [Joe Orton]

changes-entries/sed-l-command.txt

@@ -0,0 +1,3 @@
+  *) mod_sed: Fix the octal escapes the "l" command writes for bytes with
+     the high bit set, which came out as nonsense such as "\/77" for 0xff.
+     [Joe Orton]

docs/manual/mod/mod_sed.xml

@@ -108,8 +108,7 @@ page</a>.
 <name>OutputSed</name>
 <description>Sed command for filtering response content</description>
 <syntax>OutputSed <var>sed-command</var></syntax>
-<contextlist><context>directory</context><context>.htaccess</context>
-</contextlist>
+<contextlist><context>directory</context></contextlist>
 
 <usage>
     <p>The <directive>OutputSed</directive> directive specifies the <code>sed</code>
@@ -122,8 +121,7 @@ page</a>.
 <name>InputSed</name>
 <description>Sed command to filter request data (typically <code>POST</code> data)</description>
 <syntax>InputSed <var>sed-command</var></syntax>
-<contextlist><context>directory</context><context>.htaccess</context>
-</contextlist>
+<contextlist><context>directory</context></contextlist>
 
 <usage>
     <p>The <directive>InputSed</directive> directive specifies the <code>sed</code> command

modules/filters/mod_sed.c

@@ -54,6 +54,14 @@ typedef struct sed_filter_ctxt
     apr_size_t bufsize;
     apr_pool_t *tpool;
     int numbuckets;
+    /* Whether anything has been handed to the next filter yet.  Once it
+     * has, a failure can no longer be turned into an error response.
+     */
+    int passed;
+    /* Sticky failure of the evaluation, once the response is beyond
+     * rescue: the rest of the body is dropped but its metadata is not.
+     */
+    apr_status_t evalerr;
 } sed_filter_ctxt;
 
 module AP_MODULE_DECLARE_DATA sed_module;
@@ -119,6 +127,7 @@ static apr_status_t append_bucket(sed_filter_ctxt* ctx, char* buf, apr_size_t sz
         if (ctx->numbuckets >= MAX_TRANSIENT_BUCKETS) {
             b = apr_bucket_flush_create(ctx->r->connection->bucket_alloc);
             APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
+            ctx->passed = 1;
             status = ap_pass_brigade(ctx->f->next, ctx->bb);
             apr_brigade_cleanup(ctx->bb);
             clear_ctxpool(ctx);
@@ -272,11 +281,32 @@ static apr_status_t init_context(ap_filter_t *f, sed_expr_config *sed_cfg, int u
     return APR_SUCCESS;
 }
 
+/* keep_metadata
+ * The response has been broken part-way through.  Whatever is left in bb is
+ * unfiltered content which must not be sent, but its metadata -- above all
+ * the EOS -- still has to reach the next filter, or the response is never
+ * terminated.
+ */
+static void keep_metadata(sed_filter_ctxt *ctx, apr_bucket_brigade *bb)
+{
+    while (!APR_BRIGADE_EMPTY(bb)) {
+        apr_bucket *b = APR_BRIGADE_FIRST(bb);
+        APR_BUCKET_REMOVE(b);
+        if (APR_BUCKET_IS_METADATA(b)) {
+            APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
+        }
+        else {
+            apr_bucket_destroy(b);
+        }
+    }
+}
+
 /* Entry function for Sed output filter */
 static apr_status_t sed_response_filter(ap_filter_t *f,
                                         apr_bucket_brigade *bb)
 {
     apr_bucket *b;
+    apr_status_t rv;
     apr_status_t status = APR_SUCCESS;
     sed_config *cfg = ap_get_module_config(f->r->per_dir_config,
                                            &sed_module);
@@ -291,8 +321,12 @@ static apr_status_t sed_response_filter(ap_filter_t *f,
 
     if (ctx == NULL) {
 
+        if (APR_BRIGADE_EMPTY(bb)) {
+            return ap_pass_brigade(f->next, bb);
+        }
+
         if (APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(bb))) {
-            /* no need to run sed filter for Head requests */
+            /* No body to filter */
             ap_remove_output_filter(f);
             return ap_pass_brigade(f->next, bb);
         }
@@ -305,6 +339,19 @@ static apr_status_t sed_response_filter(ap_filter_t *f,
 
         ctx->bb = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
     }
+    else if (ctx->evalerr != APR_SUCCESS) {
+        /* The evaluation failed for an earlier brigade of this response,
+         * after some of it had already gone out.  Nothing more can be
+         * filtered, but the metadata still has to be carried through so
+         * that the EOS ends the response.
+         */
+        keep_metadata(ctx, bb);
+        if (!APR_BRIGADE_EMPTY(ctx->bb)) {
+            ap_pass_brigade(f->next, ctx->bb);
+            apr_brigade_cleanup(ctx->bb);
+        }
+        return ctx->evalerr;
+    }
 
     /* Here is the main logic. Iterate through all the buckets, read the
      * content of the bucket, call sed_eval_buffer on the data.
@@ -328,49 +375,72 @@ static apr_status_t sed_response_filter(ap_filter_t *f,
      */
     while (!APR_BRIGADE_EMPTY(bb)) {
         b = APR_BRIGADE_FIRST(bb);
-        if (APR_BUCKET_IS_EOS(b)) {
-            /* Now clean up the internal sed buffer */
-            sed_finalize_eval(&ctx->eval, ctx);
-            status = flush_output_buffer(ctx);
-            if (status != APR_SUCCESS) {
-                break;
+        if (APR_BUCKET_IS_METADATA(b)) {
+            if (APR_BUCKET_IS_EOS(b)) {
+                /* Now clean up the internal sed buffer */
+                sed_finalize_eval(&ctx->eval, ctx);
             }
-            /* Move the eos bucket to ctx->bb brigade */
-            APR_BUCKET_REMOVE(b);
-            APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
-        }
-        else if (APR_BUCKET_IS_FLUSH(b)) {
+            /* Flush what has been generated so far, so that the metadata
+             * keeps its place in the stream, and move it across.  Buckets
+             * this filter has no opinion on are carried through rather
+             * than dropped: an error bucket has to reach the filters which
+             * act on it.
+             */
             status = flush_output_buffer(ctx);
             if (status != APR_SUCCESS) {
                 break;
             }
-            /* Move the flush bucket to ctx->bb brigade */
             APR_BUCKET_REMOVE(b);
             APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
         }
         else {
-            if (!APR_BUCKET_IS_METADATA(b)) {
-                const char *buf = NULL;
-                apr_size_t bytes = 0;
+            const char *buf = NULL;
+            apr_size_t bytes = 0;
 
-                status = apr_bucket_read(b, &buf, &bytes, APR_BLOCK_READ);
-                if (status == APR_SUCCESS) {
-                    status = sed_eval_buffer(&ctx->eval, buf, bytes, ctx);
-                }
-                if (status != APR_SUCCESS) {
-                    ap_log_rerror(APLOG_MARK, APLOG_ERR, status, f->r, APLOGNO(10394) "error evaluating sed on output");
-                    break;
-                }
+            status = apr_bucket_read(b, &buf, &bytes, APR_BLOCK_READ);
+            if (status == APR_SUCCESS) {
+                status = sed_eval_buffer(&ctx->eval, buf, bytes, ctx);
+            }
+            if (status != APR_SUCCESS) {
+                ap_log_rerror(APLOG_MARK, APLOG_ERR, status, f->r, APLOGNO(10394) "error evaluating sed on output");
+                break;
             }
             apr_bucket_delete(b);
         }
     }
+
+    /* Flush whatever did evaluate, even after a failure: it is valid output
+     * and the most of the response the client can still be given.
+     */
+    rv = flush_output_buffer(ctx);
     if (status == APR_SUCCESS) {
-        status = flush_output_buffer(ctx);
+        status = rv;
     }
+
+    if (status != APR_SUCCESS) {
+        if (!ctx->passed) {
+            /* None of the response has been written, so the caller can
+             * still turn this into a proper error response.  Say nothing
+             * more than that it failed.
+             */
+            apr_brigade_cleanup(ctx->bb);
+            clear_ctxpool(ctx);
+            return status;
+        }
+        /* Too late for that: part of the response is already on its way,
+         * so it has to be ended as the truncated response it has become.
+         * Dropping the EOS here instead leaves the header filters unrun
+         * and the client waiting for a response that never finishes.
+         */
+        ctx->evalerr = status;
+        keep_metadata(ctx, bb);
+    }
+
     if (!APR_BRIGADE_EMPTY(ctx->bb)) {
+        ctx->passed = 1;
+        rv = ap_pass_brigade(f->next, ctx->bb);
         if (status == APR_SUCCESS) {
-            status = ap_pass_brigade(f->next, ctx->bb);
+            status = rv;
         }
         apr_brigade_cleanup(ctx->bb);
     }

modules/filters/regexp.c

@@ -535,6 +535,13 @@ static int _advance(char *lp, char *ep, step_vars_storage *vars)
             bbeg = vars->braslist[epint];
             ct = vars->braelist[epint] - bbeg;
             ep++;
+            if (ct == 0) {
+                /* The capture matched nothing, so repeating it consumes
+                 * nothing: it can only match once, here.  Both loops below
+                 * step lp by ct and would never make progress.
+                 */
+                continue;
+            }
             curlp = lp;
             while (ecmp(bbeg, lp, ct))
                 lp += ct;

modules/filters/sed1.c

@@ -73,6 +73,8 @@ static apr_status_t command(sed_eval_t *eval, sed_reptr_t *ipc,
                             step_vars_storage *step_vars);
 static apr_status_t wline(sed_eval_t *eval, char *buf, apr_size_t sz);
 static apr_status_t arout(sed_eval_t *eval);
+static void eval_errf(sed_eval_t *eval, const char *fmt, ...)
+                      __attribute__((format(printf,2,3)));
 
 static void eval_errf(sed_eval_t *eval, const char *fmt, ...)
 {
@@ -414,7 +416,7 @@ apr_status_t sed_eval_buffer(sed_eval_t *eval, const char *buf, apr_size_t bufsz
         /* Commands were not finalized properly. */
         const char* error = sed_get_finalize_error(eval->commands, eval->pool);
         if (error) {
-            eval_errf(eval, error);
+            eval_errf(eval, "%s", error);
             return APR_EGENERAL;
         }
     }
@@ -777,7 +779,12 @@ static apr_status_t command(sed_eval_t *eval, sed_reptr_t *ipc,
     switch(ipc->command) {
 
         case ACOM:
-            if (eval->aptr >= &eval->abuf[SED_ABUFSIZE]) {
+            /* One slot has to be left for the NULL which terminates abuf,
+             * or writing it runs off the end of the array and over aptr
+             * itself -- after which the next append writes through a NULL
+             * pointer.
+             */
+            if (eval->aptr >= &eval->abuf[SED_ABUFSIZE - 1]) {
                 eval_errf(eval, SEDERR_TMAMES, eval->lnum);
             } else {
                 *eval->aptr++ = ipc;
@@ -874,6 +881,13 @@ static apr_status_t command(sed_eval_t *eval, sed_reptr_t *ipc,
                         continue;
                     }
                     if (!isprint(*p1 & 0377)) {
+                        /* The three octal digits have to come off the byte
+                         * value, not off a sign-extended char: 0xff is
+                         * \377, and shifting it as a negative number gave
+                         * "\/77".
+                         */
+                        unsigned char uc = (unsigned char)*p1;
+
                         *p2++ = '\\';
                         if (p2 >= eval->lcomend) {
                             *p2 = '\\';
@@ -883,7 +897,7 @@ static apr_status_t command(sed_eval_t *eval, sed_reptr_t *ipc,
                                 return rv;
                             p2 = eval->genbuf;
                         }
-                        *p2++ = (*p1 >> 6) + '0';
+                        *p2++ = (uc >> 6) + '0';
                         if (p2 >= eval->lcomend) {
                             *p2 = '\\';
                             rv = wline(eval, eval->genbuf,
@@ -892,7 +906,7 @@ static apr_status_t command(sed_eval_t *eval, sed_reptr_t *ipc,
                                 return rv;
                             p2 = eval->genbuf;
                         }
-                        *p2++ = ((*p1 >> 3) & 07) + '0';
+                        *p2++ = ((uc >> 3) & 07) + '0';
                         if (p2 >= eval->lcomend) {
                             *p2 = '\\';
                             rv = wline(eval, eval->genbuf,
@@ -901,7 +915,8 @@ static apr_status_t command(sed_eval_t *eval, sed_reptr_t *ipc,
                                 return rv;
                             p2 = eval->genbuf;
                         }
-                        *p2++ = (*p1++ & 07) + '0';
+                        *p2++ = (uc & 07) + '0';
+                        p1++;
                         if (p2 >= eval->lcomend) {
                             *p2 = '\\';
                             rv = wline(eval, eval->genbuf,
@@ -993,7 +1008,8 @@ static apr_status_t command(sed_eval_t *eval, sed_reptr_t *ipc,
             break;
 
         case RCOM:
-            if (eval->aptr >= &eval->abuf[SED_ABUFSIZE]) {
+            /* See ACOM: the terminating NULL needs a slot of its own. */
+            if (eval->aptr >= &eval->abuf[SED_ABUFSIZE - 1]) {
                 eval_errf(eval, SEDERR_TMRMES, eval->lnum);
             } else {
                 *eval->aptr++ = ipc;

test/modules/filters/env.py

@@ -13,6 +13,7 @@ def __init__(self, env: 'HttpdTestEnv'):
         super().__init__(env=env)
         self.add_source_dir(os.path.dirname(inspect.getfile(FiltersTestSetup)))
         self.add_modules(["substitute", "sed"])
+        self.add_cgi_module()
 
 
 class FiltersTestEnv(HttpdTestEnv):

test/modules/filters/htdocs/test1/cgi/echo.py

@@ -0,0 +1,12 @@
+#!/usr/bin/env python3
+# Echo the request body back verbatim, so an InputSed test can see exactly
+# what the input filter handed to the handler.  Read to EOF rather than
+# CONTENT_LENGTH bytes: mod_sed rewrites the body but not the header, so the
+# two need not agree.
+import sys
+
+body = sys.stdin.buffer.read()
+print("Content-Type: text/plain")
+print()
+sys.stdout.flush()
+sys.stdout.buffer.write(body)

test/modules/filters/htdocs/test1/cgi/nobody.py

@@ -0,0 +1,5 @@
+#!/usr/bin/env python3
+# Headers and nothing else, so the output filters see a response whose body
+# is a lone EOS.
+print("Content-Type: text/html")
+print()