common/iobuf.c: Buffered data should not be processed by new filters.
authorNeal H. Walfield <neal@g10code.com>
Mon, 17 Aug 2015 09:56:42 +0000 (11:56 +0200)
committerNeal H. Walfield <neal@g10code.com>
Thu, 20 Aug 2015 12:16:24 +0000 (14:16 +0200)
* common/iobuf.c (iobuf_push_filter2): If the pipeline is an output or
temp pipeline, the new filter shouldn't assume ownership of the old
head's internal buffer: the data was written before the filter was
added.
* common/t-iobuf.c (double_filter): New function.
(main): Add test cases for the above bug.

--
Signed-off-by: Neal H. Walfield <neal@g10code.com>.
common/iobuf.c
common/t-iobuf.c

index e6b70a1..6d85124 100644 (file)
@@ -1607,20 +1607,21 @@ iobuf_push_filter2 (iobuf_t a,
     /* make a write stream from a temp stream */
     a->use = IOBUF_OUTPUT;
 
-  if (a->use == IOBUF_OUTPUT)
-    {                          /* allocate a fresh buffer for the
-                                   original stream */
-      b->d.buf = xmalloc (a->d.size);
-      b->d.len = 0;
-      b->d.start = 0;
-    }
-  else
-    {                          /* allocate a fresh buffer for the new
-                                   stream */
-      a->d.buf = xmalloc (a->d.size);
-      a->d.len = 0;
-      a->d.start = 0;
-    }
+  /* The new filter (A) gets a new buffer.
+
+     If the pipeline is an output or temp pipeline, then giving the
+     buffer to the new filter means that data that was written before
+     the filter was pushed gets sent to the filter.  That's clearly
+     wrong.
+
+     If the pipeline is an input pipeline, then giving the buffer to
+     the new filter (A) means that data that has read from (B), but
+     not yet read from the pipeline won't be processed by the new
+     filter (A)!  That's certainly not what we want.  */
+  a->d.buf = xmalloc (a->d.size);
+  a->d.len = 0;
+  a->d.start = 0;
+
   /* disable nlimit for the new stream */
   a->ntotal = b->ntotal + b->nbytes;
   a->nlimit = a->nbytes = 0;
index 0f21c35..01c94a3 100644 (file)
@@ -6,6 +6,8 @@
 
 #include "iobuf.h"
 
+/* Return every other byte.  In particular, reads two bytes, returns
+   the second one.  */
 static int
 every_other_filter (void *opaque, int control,
                    iobuf_t chain, byte *buf, size_t *len)
@@ -42,6 +44,36 @@ every_other_filter (void *opaque, int control,
   return 0;
 }
 
+static int
+double_filter (void *opaque, int control,
+              iobuf_t chain, byte *buf, size_t *len)
+{
+  (void) opaque;
+
+  if (control == IOBUFCTRL_DESC)
+    {
+      * (char **) buf = "double_filter";
+    }
+  if (control == IOBUFCTRL_FLUSH)
+    {
+      int i;
+
+      for (i = 0; i < *len; i ++)
+       {
+         int rc;
+
+         rc = iobuf_writebyte (chain, buf[i]);
+         if (rc)
+           return rc;
+         rc = iobuf_writebyte (chain, buf[i]);
+         if (rc)
+           return rc;
+       }
+    }
+
+  return 0;
+}
+
 struct content_filter_state
 {
   int pos;
@@ -261,21 +293,87 @@ main (int argc, char *argv[])
        c = iobuf_readbyte (iobuf);
        if (c == -1 && lastc == -1)
          {
-           printf("Two EOFs in a row.  Done.\n");
+           // printf("Two EOFs in a row.  Done.\n");
+           assert (n == 44);
            break;
          }
 
        lastc = c;
 
        if (c == -1)
-         printf("After %d bytes, got EOF.\n", n);
+         {
+           // printf("After %d bytes, got EOF.\n", n);
+           assert (n == 27 || n == 44);
+         }
        else
          {
            n ++;
-           printf ("%d: '%c' (%d)\n", n, c, c);
+           // printf ("%d: '%c' (%d)\n", n, c, c);
          }
       }
   }
 
+  /* Write some data to a temporary filter.  Push a new filter.  The
+     already written data should not be processed by the new
+     filter.  */
+  {
+    iobuf_t iobuf;
+    int rc;
+    char *content = "0123456789";
+    char *content2 = "abc";
+    char buffer[4096];
+    int n;
+
+    iobuf = iobuf_temp ();
+    assert (iobuf);
+
+    rc = iobuf_write (iobuf, content, strlen (content));
+    assert (rc == 0);
+
+    rc = iobuf_push_filter (iobuf, double_filter, NULL);
+    assert (rc == 0);
+
+    /* Include a NUL.  */
+    rc = iobuf_write (iobuf, content2, strlen (content2) + 1);
+    assert (rc == 0);
+
+    n = iobuf_temp_to_buffer (iobuf, buffer, sizeof (buffer));
+    printf ("Got %d bytes\n", n);
+    printf ("buffer: `");
+    fwrite (buffer, n, 1, stdout);
+    fputc ('\'', stdout);
+    fputc ('\n', stdout);
+
+    assert (n == strlen (content) + 2 * (strlen (content2) + 1));
+    assert (strcmp (buffer, "0123456789aabbcc") == 0);
+  }
+
+  {
+    iobuf_t iobuf;
+    int rc;
+    char *content = "0123456789";
+    int n;
+    int c;
+    char buffer[strlen (content)];
+
+    iobuf = iobuf_temp_with_content (content, strlen (content));
+    assert (iobuf);
+
+    rc = iobuf_push_filter (iobuf, every_other_filter, NULL);
+    assert (rc == 0);
+    rc = iobuf_push_filter (iobuf, every_other_filter, NULL);
+    assert (rc == 0);
+
+    for (n = 0; (c = iobuf_get (iobuf)) != -1; n ++)
+      {
+       // printf ("%d: `%c'\n", n, c);
+       buffer[n] = c;
+      }
+
+    assert (n == 2);
+    assert (buffer[0] == '3');
+    assert (buffer[1] == '7');
+  }
+
   return 0;
 }