lib: istream-try - Fix detecting istream when its input buffer is full

The previous check didn't work when the stream's buffer_size was 0,
which happened with istream-concat parent.

Added also a unit test that tries to test for these kind of situations.
It doesn't actually reproduce this specific bug, but it tests that the
code paths works at least in the generic situation.

diff --git a/src/lib/istream-try.c b/src/lib/istream-try.c
index df94a486690d7de1270a3b7a8a2c341ec1e01d24..1a31a7fb69e63338e354fd19a7b0e7aa9aaf908d 100644 (file)
--- a/src/lib/istream-try.c
+++ b/src/lib/istream-try.c
@@ -7,6 +7,7 @@
 struct try_istream {
        struct istream_private istream;
 
+       size_t min_buffer_full_size;
        unsigned int try_input_count;
        struct istream **try_input;
        unsigned int try_idx;
@@ -40,7 +41,9 @@ static void i_stream_try_close(struct iostream_private *stream,
        i_stream_unref_try_inputs(tstream);
 }
 
-static bool i_stream_try_is_buffer_full(struct istream *try_input)
+static bool
+i_stream_try_is_buffer_full(struct try_istream *tstream,
+                           struct istream *try_input)
 {
        /* See if one of the parent istreams have their buffer full.
           This is mainly intended to check with istream-tee whether its
@@ -48,11 +51,20 @@ static bool i_stream_try_is_buffer_full(struct istream *try_input)
           a full buffer of input, but it hasn't decided to return anything
           yet. But it also hasn't failed, so we'll assume that the input is
           correct for it and it simply needs a lot more input before it can
-          return anything (e.g. istream-bzlib). */
+          return anything (e.g. istream-bzlib).
+
+          Note that it's common for buffer_size to be 0 for all parents. This
+          could be e.g. because the root is istream-concat, which breaks the
+          parent hierarchy since it has multiple parents. So the buffer_size
+          check can be thought of just as an optional extra check that
+          sometimes works and sometimes doesn't.
+
+          Note that we don't check whether skip==pos. An istream could be
+          reading its buffer full without skipping over anything. */
        while (try_input->real_stream->parent != NULL) {
                try_input = try_input->real_stream->parent;
-               if (try_input->real_stream->pos == try_input->real_stream->buffer_size &&
-                   try_input->real_stream->buffer_size > 0)
+               if (try_input->real_stream->pos >= try_input->real_stream->buffer_size &&
+                   try_input->real_stream->pos > tstream->min_buffer_full_size)
                        return TRUE;
        }
        return FALSE;
@@ -67,7 +79,7 @@ static int i_stream_try_detect(struct try_istream *tstream)
                        tstream->try_input[tstream->try_idx];
 
                ret = i_stream_read(try_input);
-               if (ret == 0 && i_stream_try_is_buffer_full(try_input))
+               if (ret == 0 && i_stream_try_is_buffer_full(tstream, try_input))
                        ret = 1;
                if (ret > 0) {
                        i_stream_init_parent(&tstream->istream, try_input);
@@ -114,7 +126,8 @@ i_stream_try_read(struct istream_private *stream)
        return i_stream_read_copy_from_parent(&stream->istream);
 }
 
-struct istream *istream_try_create(struct istream *const input[])
+struct istream *istream_try_create(struct istream *const input[],
+                                  size_t min_buffer_full_size)
 {
        struct try_istream *tstream;
        unsigned int count;
@@ -133,6 +146,7 @@ struct istream *istream_try_create(struct istream *const input[])
        i_assert(count != 0);
 
        tstream = i_new(struct try_istream, 1);
+       tstream->min_buffer_full_size = min_buffer_full_size;
        tstream->try_input_count = count;
        tstream->try_input = p_memdup(default_pool, input,
                                      sizeof(*input) * count);

diff --git a/src/lib/istream-try.h b/src/lib/istream-try.h
index f2040b3b9b761d7b6e1181bdb51df487d4ec55e3..6f0f8b7dbd4c253a07b414663e2872dfb041c128 100644 (file)
--- a/src/lib/istream-try.h
+++ b/src/lib/istream-try.h
@@ -8,7 +8,18 @@
    be a fallback stream that always succeeds.
 
    Once the stream is detected, all the other streams are unreferenced.
-   The streams should usually be children of the same parent tee-istream. */
-struct istream *istream_try_create(struct istream *const input[]);
+   The streams should usually be children of the same parent tee-istream.
+
+   Detecting whether istream-tee buffer is full or not is a bit tricky.
+   There's no visible difference between non-blocking istream returning 0 and
+   istream-tee buffer being full. To work around this, we treat used buffer
+   sizes <= min_buffer_full_size as being non-blocking istreams, while
+   buffer sizes > min_buffer_full_size are assumed to be due to istream-tee
+   max buffer size being reached. Practically this means that
+   min_buffer_full_size must be smaller than the smallest of the istreams'
+   maximum buffer sizes, but large enough that all the istreams would have
+   returned EINVAL on invalid input by that position. */
+struct istream *istream_try_create(struct istream *const input[],
+                                  size_t min_buffer_full_size);
 
 #endif

diff --git a/src/lib/test-istream-try.c b/src/lib/test-istream-try.c
index 91ac37ddd16a999f6f31fa1037c4a1a04ffc6e0d..afad516e9dcde865c3a672d057ef88d06502b35d 100644 (file)
--- a/src/lib/test-istream-try.c
+++ b/src/lib/test-istream-try.c
@@ -1,9 +1,12 @@
 /* Copyright (c) 2009-2018 Dovecot authors, see the included COPYING file */
 
 #include "test-lib.h"
-#include "istream.h"
+#include "istream-private.h"
+#include "istream-base64.h"
 #include "istream-try.h"
 
+#define MIN_FULL_SIZE 1
+
 static void test_istream_try_normal(void)
 {
        bool finished = FALSE;
@@ -17,7 +20,7 @@ static void test_istream_try_normal(void)
                test_inputs[2] = NULL;
                test_istream_set_size(test_inputs[0], 0);
                test_istream_set_size(test_inputs[1], 0);
-               try_input = istream_try_create(test_inputs);
+               try_input = istream_try_create(test_inputs, MIN_FULL_SIZE);
 
                /* nonblocking read */
                test_assert_idx(i_stream_read(try_input) == 0, test);
@@ -137,7 +140,8 @@ static void test_istream_try_empty(void)
                test_istream_create(""),
                NULL
        };
-       struct istream *try_input = istream_try_create(test_inputs);
+       struct istream *try_input =
+               istream_try_create(test_inputs, MIN_FULL_SIZE);
        test_assert(i_stream_read(try_input) == -1);
        test_assert(try_input->eof);
        test_assert(try_input->stream_errno == 0);
@@ -147,8 +151,45 @@ static void test_istream_try_empty(void)
        test_end();
 }
 
+static void test_istream_try_buffer_full(void)
+{
+       const char *test_strings[] = { "Zm9v", "YmFy" };
+       struct istream *test_inputs[3], *try_input, *input, *input2;
+
+       test_begin("istream try");
+
+       for (unsigned int i = 0; i < 2; i++) {
+               input = test_istream_create(test_strings[i]);
+               test_istream_set_size(input, 1);
+               test_istream_set_max_buffer_size(input, 1);
+               input2 = i_stream_create_base64_decoder(input);
+               i_stream_unref(&input);
+               test_inputs[i] = input2;
+       };
+       test_inputs[2] = NULL;
+
+       try_input = istream_try_create(test_inputs, MIN_FULL_SIZE);
+
+       test_assert(i_stream_read(try_input) == 0);
+       test_assert(try_input->real_stream->parent != NULL);
+       test_assert(i_stream_get_data_size(test_inputs[0]) == 0);
+       test_assert(i_stream_get_data_size(test_inputs[1]) == 0);
+
+       test_istream_set_size(test_inputs[0], 2);
+       test_assert(i_stream_read(try_input) == 1);
+       test_assert(i_stream_get_data_size(test_inputs[0]) == 1);
+       test_assert(i_stream_get_data_size(test_inputs[1]) == 0);
+
+       i_stream_unref(&test_inputs[0]);
+       i_stream_unref(&test_inputs[1]);
+       i_stream_unref(&try_input);
+
+       test_end();
+}
+
 void test_istream_try(void)
 {
        test_istream_try_normal();
        test_istream_try_empty();
+       test_istream_try_buffer_full();
 }

diff --git a/src/plugins/fs-compress/fs-compress.c b/src/plugins/fs-compress/fs-compress.c
index dd6d76e42542686cd7738a569d56e8ca67d156be..74605efa3436b89f34424dce80c905f42586a256 100644 (file)
--- a/src/plugins/fs-compress/fs-compress.c
+++ b/src/plugins/fs-compress/fs-compress.c
@@ -188,7 +188,7 @@ fs_compress_try_create_stream(struct compress_fs_file *file,
        array_append_zero(&try_inputs_arr);
 
        try_inputs = array_idx_modifiable(&try_inputs_arr, 0);
-       ret_input = istream_try_create(try_inputs);
+       ret_input = istream_try_create(try_inputs, COMPRESSION_HDR_MAX_SIZE);
        for (i = 0; i < count; i++)
                i_stream_unref(&try_inputs[i]);
        return ret_input;