common/iobuf.c: Better respect boundary conditions in iobuf_read_line.
authorNeal H. Walfield <neal@g10code.com>
Wed, 12 Aug 2015 00:19:05 +0000 (02:19 +0200)
committerNeal H. Walfield <neal@g10code.com>
Thu, 20 Aug 2015 12:16:19 +0000 (14:16 +0200)
* common/iobuf.c (iobuf_read_line): Be more careful with boundary
conditions.
* common/iobuf.h: Include <gpg-error.h>.
* common/t-iobuf.c: New file.
* common/Makefile.am (module_tests): Add t-iobuf.
(t_mbox_util_LDADD): New variable.

--
Signed-off-by: Neal H. Walfield <neal@g10code.com>.
common/Makefile.am
common/iobuf.c
common/iobuf.h
common/t-iobuf.c [new file with mode: 0644]

index c0df5ef..47facd4 100644 (file)
@@ -169,7 +169,7 @@ endif
 module_tests = t-stringhelp t-timestuff \
                t-convert t-percent t-gettime t-sysutils t-sexputil \
               t-session-env t-openpgp-oid t-ssh-utils \
-              t-mapstrings t-zb32 t-mbox-util
+              t-mapstrings t-zb32 t-mbox-util t-iobuf
 if !HAVE_W32CE_SYSTEM
 module_tests += t-exechelp
 endif
@@ -213,6 +213,7 @@ t_ssh_utils_LDADD = $(t_common_ldadd)
 t_mapstrings_LDADD = $(t_common_ldadd)
 t_zb32_LDADD = $(t_common_ldadd)
 t_mbox_util_LDADD = $(t_common_ldadd)
+t_iobuf_LDADD = $(t_common_ldadd)
 
 # System specific test
 if HAVE_W32_SYSTEM
index 23d14e6..664db39 100644 (file)
@@ -2423,45 +2423,66 @@ iobuf_read_line (iobuf_t a, byte ** addr_of_buffer,
   unsigned maxlen = *max_length;
   char *p;
 
-  if (!buffer)
-    {                          /* must allocate a new buffer */
-      length = 256;
-      buffer = xmalloc (length);
+  /* The code assumes that we have space for at least a newline and a
+     NUL character in the buffer.  This requires at least 2 bytes.  We
+     don't complicate the code by handling the stupid corner case, but
+     simply assert that it can't happen.  */
+  assert (length >= 2 || maxlen >= 2);
+
+  if (!buffer || length <= 1)
+    /* must allocate a new buffer */
+    {
+      length = 256 <= maxlen ? 256 : maxlen;
+      buffer = xrealloc (buffer, length);
       *addr_of_buffer = (unsigned char *)buffer;
       *length_of_buffer = length;
     }
 
-  length -= 3;                 /* reserve 3 bytes (cr,lf,eol) */
   p = buffer;
   while ((c = iobuf_get (a)) != -1)
     {
-      if (nbytes == length)
-       {                       /* increase the buffer */
-         if (length > maxlen)
-           {                   /* this is out limit */
-             /* skip the rest of the line */
+      *p++ = c;
+      nbytes++;
+      if (c == '\n')
+       break;
+
+      if (nbytes == length - 1)
+       /* We don't have enough space to add a \n and a \0.  Increase
+          the buffer size.  */
+       {
+         if (length == maxlen)
+           /* We reached the buffer's size limit!  */
+           {
+             /* Skip the rest of the line.  */
              while (c != '\n' && (c = iobuf_get (a)) != -1)
                ;
-             *p++ = '\n';      /* always append a LF (we have reserved space) */
-             nbytes++;
-             *max_length = 0;  /* indicate truncation */
+
+             /* p is pointing at the last byte in the buffer.  We
+                always terminate the line with "\n\0" so overwrite
+                the previous byte with a \n.  */
+             assert (p > buffer);
+             p[-1] = '\n';
+
+             /* Indicate truncation.  */
+             *max_length = 0;
              break;
            }
-         length += 3;          /* correct for the reserved byte */
+
          length += length < 1024 ? 256 : 1024;
+         if (length > maxlen)
+           length = maxlen;
+
          buffer = xrealloc (buffer, length);
          *addr_of_buffer = (unsigned char *)buffer;
          *length_of_buffer = length;
-         length -= 3;          /* and reserve again */
          p = buffer + nbytes;
        }
-      *p++ = c;
-      nbytes++;
-      if (c == '\n')
-       break;
     }
-  *p = 0;                      /* make sure the line is a string */
+  /* Add the terminating NUL.  */
+  *p = 0;
 
+  /* Return the number of characters written to the buffer including
+     the newline, but not including the terminating NUL.  */
   return nbytes;
 }
 
index c742c64..900a12b 100644 (file)
@@ -31,6 +31,9 @@
 #ifndef GNUPG_COMMON_IOBUF_H
 #define GNUPG_COMMON_IOBUF_H
 
+/* For estream_t.  */
+#include <gpg-error.h>
+
 #include "../common/types.h"
 #include "../common/sysutils.h"
 
diff --git a/common/t-iobuf.c b/common/t-iobuf.c
new file mode 100644 (file)
index 0000000..e3d7499
--- /dev/null
@@ -0,0 +1,188 @@
+#include <config.h>
+#include <stdio.h>
+#include <string.h>
+#include <assert.h>
+#include <stdlib.h>
+
+#include "iobuf.h"
+
+static int
+every_other_filter (void *opaque, int control,
+                   iobuf_t chain, byte *buf, size_t *len)
+{
+  (void) opaque;
+
+  if (control == IOBUFCTRL_DESC)
+    {
+      *(char **) buf = "every_other_filter";
+    }
+  if (control == IOBUFCTRL_UNDERFLOW)
+    {
+      int c = iobuf_readbyte (chain);
+      int c2;
+      if (c == -1)
+       c2 = -1;
+      else
+       c2 = iobuf_readbyte (chain);
+
+      // printf ("Discarding %d (%c); return %d (%c)\n", c, c, c2, c2);
+
+      if (c2 == -1)
+       {
+         *len = 0;
+         return -1;
+       }
+
+      *buf = c2;
+      *len = 1;
+
+      return 0;
+    }
+
+  return 0;
+}
+
+int
+main (int argc, char *argv[])
+{
+  (void) argc;
+  (void) argv;
+
+  /* A simple test to make sure filters work.  We use a static buffer
+     and then add a filter in front of it that returns every other
+     character.  */
+  {
+    char *content = "0123456789abcdefghijklm";
+    iobuf_t iobuf;
+    int c;
+    int n;
+    int rc;
+
+    iobuf = iobuf_temp_with_content (content, strlen (content));
+    rc = iobuf_push_filter (iobuf, every_other_filter, NULL);
+    assert (rc == 0);
+
+    n = 0;
+    while ((c = iobuf_readbyte (iobuf)) != -1)
+      {
+       // printf ("%d: %c\n", n + 1, (char) c);
+       assert (content[2 * n + 1] == c);
+       n ++;
+      }
+    // printf ("Got EOF after reading %d bytes (content: %d)\n",
+    // n, strlen (content));
+    assert (n == strlen (content) / 2);
+
+    iobuf_close (iobuf);
+  }
+
+  /* A simple test to check buffering.  Make sure that when we add a
+     filter to a pipeline, any buffered data gets processed by the */
+  {
+    char *content = "0123456789abcdefghijklm";
+    iobuf_t iobuf;
+    int c;
+    int n;
+    int rc;
+    int i;
+
+    iobuf = iobuf_temp_with_content (content, strlen (content));
+
+    n = 0;
+    for (i = 0; i < 10; i ++)
+      {
+       c = iobuf_readbyte (iobuf);
+       assert (content[i] == c);
+       n ++;
+      }
+
+    rc = iobuf_push_filter (iobuf, every_other_filter, NULL);
+    assert (rc == 0);
+
+    while ((c = iobuf_readbyte (iobuf)) != -1)
+      {
+       // printf ("%d: %c\n", n + 1, (char) c);
+       assert (content[2 * (n - 5) + 1] == c);
+       n ++;
+      }
+    assert (n == 10 + (strlen (content) - 10) / 2);
+  }
+
+
+  /* A simple test to check that iobuf_read_line works.  */
+  {
+    /* - 3 characters plus new line
+       - 4 characters plus new line
+       - 5 characters plus new line
+       - 5 characters, no new line
+     */
+    char *content = "abc\ndefg\nhijkl\nmnopq";
+    iobuf_t iobuf;
+    byte *buffer;
+    unsigned size;
+    unsigned max_len;
+    int n;
+
+    iobuf = iobuf_temp_with_content (content, strlen(content));
+
+    /* We read a line with 3 characters plus a newline.  If we
+       allocate a buffer that is 5 bytes long, then no reallocation
+       should be required.  */
+    size = 5;
+    buffer = malloc (size);
+    assert (buffer);
+    max_len = 100;
+    n = iobuf_read_line (iobuf, &buffer, &size, &max_len);
+    assert (n == 4);
+    assert (strcmp (buffer, "abc\n") == 0);
+    assert (size == 5);
+    assert (max_len == 100);
+    free (buffer);
+
+    /* We now read a line with 4 characters plus a newline.  This
+       requires 6 bytes of storage.  We pass a buffer that is 5 bytes
+       large and we allow the buffer to be grown.  */
+    size = 5;
+    buffer = malloc (size);
+    max_len = 100;
+    n = iobuf_read_line (iobuf, &buffer, &size, &max_len);
+    assert (n == 5);
+    assert (strcmp (buffer, "defg\n") == 0);
+    assert (size >= 6);
+    /* The string shouldn't have been truncated (max_len == 0).  */
+    assert (max_len == 100);
+    free (buffer);
+
+    /* We now read a line with 5 characters plus a newline.  This
+       requires 7 bytes of storage.  We pass a buffer that is 5 bytes
+       large and we don't allow the buffer to be grown.  */
+    size = 5;
+    buffer = malloc (size);
+    max_len = 5;
+    n = iobuf_read_line (iobuf, &buffer, &size, &max_len);
+    assert (n == 4);
+    /* Note: the string should still have a trailing \n.  */
+    assert (strcmp (buffer, "hij\n") == 0);
+    assert (size == 5);
+    /* The string should have been truncated (max_len == 0).  */
+    assert (max_len == 0);
+    free (buffer);
+
+    /* We now read a line with 6 characters without a newline.  This
+       requires 7 bytes of storage.  We pass a NULL buffer and we
+       don't allow the buffer to be grown larger than 5 bytes.  */
+    size = 5;
+    buffer = NULL;
+    max_len = 5;
+    n = iobuf_read_line (iobuf, &buffer, &size, &max_len);
+    assert (n == 4);
+    /* Note: the string should still have a trailing \n.  */
+    assert (strcmp (buffer, "mno\n") == 0);
+    assert (size == 5);
+    /* The string should have been truncated (max_len == 0).  */
+    assert (max_len == 0);
+    free (buffer);
+  }
+
+  return 0;
+}