lib, lib-program-client: Fix i_stream_read_memarea() usage

Calling it may change the stream's buffer and free the old buffer. However,
some istreams didn't change the buffer if i_stream_read_memarea() returned
-2. The old buffer was kept referenced, which could have caused the istream
to read garbage or crash due to accessing freed memory.

This fixes:
 * istream-limit
 * istream-sized
 * istream-rawlog
 * program-client-istream
 * Anything using i_stream_read_copy_from_parent(), which includes:
   * istream-fs-file
   * istream-fs-stats
   * istream-metawrap
   * http-server-istream
   * istream-header-filter
   * istream-mail
   * istream-failure-at
   * istream-hash
   * istream-timeout
   * istream-try

diff --git a/src/lib-program-client/program-client-remote.c b/src/lib-program-client/program-client-remote.c
index 4ded4c3d3212112c162657dab6b2d9a8577684ba..a9e7c33f0e34b6cdfe3e236a78377b6597f5e86d 100644 (file)
--- a/src/lib-program-client/program-client-remote.c
+++ b/src/lib-program-client/program-client-remote.c
@@ -98,17 +98,15 @@ program_client_istream_read(struct istream_private *stream)
                ret = -1;
        } else
                do {
-                       if ((ret=i_stream_read_memarea(stream->parent)) == -2) {
-                               return -2;      /* input buffer full */
-                       }
-
-                       if (ret == 0 || (ret < 0 && !stream->parent->eof))
-                               break;
-
+                       ret = i_stream_read_memarea(stream->parent);
                        stream->istream.stream_errno =
                                stream->parent->stream_errno;
                        stream->buffer =
                                i_stream_get_data(stream->parent, &pos);
+                       if (ret == -2)
+                               return -2;      /* input buffer full */
+                       if (ret == 0 || (ret < 0 && !stream->parent->eof))
+                               break;
 
                        if (stream->parent->eof) {
                                /* Check return code at EOF */

diff --git a/src/lib/istream-limit.c b/src/lib/istream-limit.c
index 35fd68ff264f53420167892861ac2f3dd54533aa..53c035218b2167b19ad6ce11ca347843c74149fa 100644 (file)
--- a/src/lib/istream-limit.c
+++ b/src/lib/istream-limit.c
@@ -46,13 +46,13 @@ static ssize_t i_stream_limit_read(struct istream_private *stream)
        if (pos > stream->pos)
                ret = 0;
        else do {
-               if ((ret = i_stream_read_memarea(stream->parent)) == -2)
-                       return -2;
-
+               ret = i_stream_read_memarea(stream->parent);
                stream->istream.stream_errno = stream->parent->stream_errno;
                stream->istream.eof = stream->parent->eof;
                stream->buffer = i_stream_get_data(stream->parent, &pos);
        } while (pos <= stream->pos && ret > 0);
+       if (ret == -2)
+               return -2;
 
        if (lstream->v_size != (uoff_t)-1) {
                left = lstream->v_size - stream->istream.v_offset;

diff --git a/src/lib/istream-rawlog.c b/src/lib/istream-rawlog.c
index 7e17d6049ed4eca03f9d4a48d057e3f14047f4dd..aa0a75f0f899e71bd45e7017d7798125878b8405 100644 (file)
--- a/src/lib/istream-rawlog.c
+++ b/src/lib/istream-rawlog.c
@@ -51,13 +51,13 @@ static ssize_t i_stream_rawlog_read(struct istream_private *stream)
        if (pos > stream->pos)
                ret = 0;
        else do {
-               if ((ret = i_stream_read_memarea(stream->parent)) == -2)
-                       return -2;
-
+               ret = i_stream_read_memarea(stream->parent);
                stream->istream.stream_errno = stream->parent->stream_errno;
                stream->istream.eof = stream->parent->eof;
                stream->buffer = i_stream_get_data(stream->parent, &pos);
        } while (pos <= stream->pos && ret > 0);
+       if (ret == -2)
+               return -2;
 
        if (pos <= stream->pos)
                ret = ret == 0 ? 0 : -1;

diff --git a/src/lib/istream-sized.c b/src/lib/istream-sized.c
index eed4e6c6402ba21389b9fe4317c82d8b338d3811..d5c1e863bdd16a9224ca6a325628aeb6042b3b7e 100644 (file)
--- a/src/lib/istream-sized.c
+++ b/src/lib/istream-sized.c
@@ -50,9 +50,7 @@ i_stream_sized_parent_read(struct istream_private *stream, size_t *pos_r)
        ssize_t ret;
 
        do {
-               if ((ret = i_stream_read_memarea(stream->parent)) == -2)
-                       break;
-
+               ret = i_stream_read_memarea(stream->parent);
                stream->istream.stream_errno = stream->parent->stream_errno;
                stream->istream.eof = stream->parent->eof;
                stream->buffer = i_stream_get_data(stream->parent, pos_r);

diff --git a/src/lib/istream.c b/src/lib/istream.c
index e04fb50c6f2a997f651fce47336aab9f7d4e2610..161f4a6b2f444488b9b42cecbf05f4082dabe864 100644 (file)
--- a/src/lib/istream.c
+++ b/src/lib/istream.c
@@ -384,11 +384,7 @@ ssize_t i_stream_read_copy_from_parent(struct istream *istream)
        if (pos > stream->pos)
                ret = 0;
        else do {
-               if ((ret = i_stream_read_memarea(stream->parent)) == -2) {
-                       i_stream_update(stream);
-                       return -2;
-               }
-
+               ret = i_stream_read_memarea(stream->parent);
                stream->istream.stream_errno = stream->parent->stream_errno;
                stream->istream.eof = stream->parent->eof;
                stream->buffer = i_stream_get_data(stream->parent, &pos);
@@ -396,6 +392,10 @@ ssize_t i_stream_read_copy_from_parent(struct istream *istream)
                   backwards and the previous read() didn't get us far
                   enough. */
        } while (pos <= stream->pos && ret > 0);
+       if (ret == -2) {
+               i_stream_update(stream);
+               return -2;
+       }
 
        ret = pos > stream->pos ? (ssize_t)(pos - stream->pos) :
                (ret == 0 ? 0 : -1);

diff --git a/src/lib/test-istream-seekable.c b/src/lib/test-istream-seekable.c
index fd44f3e105af619d06711c365f4dbdd55b81a00c..594e0321a6c1b640aadaa3aa51e3681d0dbda201 100644 (file)
--- a/src/lib/test-istream-seekable.c
+++ b/src/lib/test-istream-seekable.c
@@ -1,7 +1,10 @@
 /* Copyright (c) 2009-2018 Dovecot authors, see the included COPYING file */
 
 #include "test-lib.h"
+#include "sha2.h"
 #include "istream-private.h"
+#include "istream-sized.h"
+#include "istream-hash.h"
 #include "istream-seekable.h"
 
 #include <fcntl.h>
@@ -191,6 +194,28 @@ static void test_istream_seekable_early_end(void)
        test_end();
 }
 
+static void test_istream_seekable_invalid_read(void)
+{
+       test_begin("istream seekable + other streams causing invalid read");
+       struct sha256_ctx hash_ctx;
+       sha256_init(&hash_ctx);
+       struct istream *str_input = test_istream_create("123456");
+       str_input->seekable = FALSE;
+       struct istream *seek_inputs[] = { str_input, NULL };
+       struct istream *seek_input = i_stream_create_seekable(seek_inputs, 3, fd_callback, NULL);
+       struct istream *sized_input = i_stream_create_sized(seek_input, 3);
+       struct istream *input = i_stream_create_hash(sized_input, &hash_method_sha256, &hash_ctx);
+       test_assert(i_stream_read(input) == 3);
+       test_assert(i_stream_read(input) == -2);
+       i_stream_skip(input, 3);
+       test_assert(i_stream_read(input) == -1);
+       i_stream_unref(&input);
+       i_stream_unref(&sized_input);
+       i_stream_unref(&seek_input);
+       i_stream_unref(&str_input);
+       test_end();
+}
+
 void test_istream_seekable(void)
 {
        unsigned int i;
@@ -208,4 +233,5 @@ void test_istream_seekable(void)
 
        test_istream_seekable_eof();
        test_istream_seekable_early_end();
+       test_istream_seekable_invalid_read();
 }