common/iobuf.c: Better respect boundary conditions in iobuf_read_line.

* 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 <[email protected]>.

4 files changed, 234 insertions, 21 deletions

diff --git a/common/Makefile.am b/common/Makefile.am
index c0df5ef8d..47facd4d6 100644
--- a/common/Makefile.am
+++ b/common/Makefile.am
@@ -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
diff --git a/common/iobuf.c b/common/iobuf.c
index 23d14e6af..664db393f 100644
--- a/common/iobuf.c
+++ b/common/iobuf.c
@@ -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;
 }
 
diff --git a/common/iobuf.h b/common/iobuf.h
index c742c6403..900a12b62 100644
--- a/common/iobuf.h
+++ b/common/iobuf.h
@@ -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
index 000000000..e3d749995
--- /dev/null
+++ b/common/t-iobuf.c
@@ -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;
+}