buffer: do not rely on ->off in advance_last_with_data()

advance_last_with_data() adjusts evbuffer.last_with_datap, and if we
will have empty chain in the middle advance_last_with_data() will stop,
while it should not, since while empty chains is not regular thing they
can pops up in various places like, and while I did not look through all
of them the most tricky I would say is:
  evbuffer_reverse_space()/evbuffer_commit_space()
  evbuffer_add_reference()

Test case from:
  https://github.com/envoyproxy/envoy/pull/6062

Fixes: #778
v2: keep last_with_datap really last with data, i.e. update only if
chain has data in it

diff --git a/buffer.c b/buffer.c
index 7a8c8b696cbaf7bc4bc316a99622f6b738034121..8e947892f7df7605400c5b9a9b66a133f9655181 100644 (file)
--- a/buffer.c
+++ b/buffer.c
@@ -704,13 +704,17 @@ static int
 advance_last_with_data(struct evbuffer *buf)
 {
        int n = 0;
+       struct evbuffer_chain **chainp = buf->last_with_datap;
+
        ASSERT_EVBUFFER_LOCKED(buf);
 
-       if (!*buf->last_with_datap)
+       if (!*chainp)
                return 0;
 
-       while ((*buf->last_with_datap)->next && (*buf->last_with_datap)->next->off) {
-               buf->last_with_datap = &(*buf->last_with_datap)->next;
+       while ((*chainp)->next) {
+               chainp = &(*chainp)->next;
+               if ((*chainp)->off)
+                       buf->last_with_datap = chainp;
                ++n;
        }
        return n;

diff --git a/test/regress_buffer.c b/test/regress_buffer.c
index 67072ec8e03363bececec01ec9b617faa56e3131..479488a03868abe6021d7caa0b7c135f8ecddcc0 100644 (file)
--- a/test/regress_buffer.c
+++ b/test/regress_buffer.c
@@ -450,6 +450,62 @@ test_evbuffer_remove_buffer_with_empty_front(void *ptr)
                evbuffer_free(buf2);
 }
 
+static void
+test_evbuffer_remove_buffer_adjust_last_with_datap_with_empty(void *ptr)
+{
+       struct evbuffer *buf1 = NULL, *buf2 = NULL;
+
+       buf1 = evbuffer_new();
+       tt_assert(buf1);
+
+       buf2 = evbuffer_new();
+       tt_assert(buf2);
+
+       tt_int_op(evbuffer_add(buf1, "aaaaaa", 6), ==, 0);
+
+       // buf1: aaaaaab
+       // buf2:
+       {
+               struct evbuffer_iovec iovecs[2];
+               /** we want two chains, to leave one chain empty */
+               tt_int_op(evbuffer_reserve_space(buf1, 971, iovecs, 2), ==, 2);
+               tt_int_op(iovecs[0].iov_len, >=, 1);
+               tt_int_op(iovecs[1].iov_len, >=, 1);
+               tt_assert(*(char *)(iovecs[0].iov_base) = 'b');
+               tt_assert(iovecs[0].iov_len = 1);
+               tt_int_op(evbuffer_commit_space(buf1, iovecs, 1), ==, 0);
+       }
+
+       // buf1: aaaaaab
+       // buf2: dddcc
+       tt_int_op(evbuffer_add(buf2, "cc", 2), ==, 0);
+       tt_int_op(evbuffer_prepend(buf2, "ddd", 3), ==, 0);
+
+       // buf1:
+       // buf2: aaaaaabdddcc
+       tt_int_op(evbuffer_prepend_buffer(buf2, buf1), ==, 0);
+
+       // buf1: aaaaaabdddcc
+       // buf2:
+       tt_int_op(evbuffer_add_buffer(buf1, buf2), ==, 0);
+
+       // buf1: c
+       // buf2: aaaaaabdddc
+       tt_int_op(evbuffer_remove_buffer(buf1, buf2, 11), ==, 11);
+
+       // This fails today, we observe "aaaaaabcddd" instead!
+       tt_mem_op(evbuffer_pullup(buf2, -1), ==, "aaaaaabdddc", 11);
+
+       evbuffer_validate(buf1);
+       evbuffer_validate(buf2);
+
+ end:
+       if (buf1)
+               evbuffer_free(buf1);
+       if (buf2)
+               evbuffer_free(buf2);
+}
+
 static void
 test_evbuffer_add_buffer_with_empty(void *ptr)
 {
@@ -679,6 +735,28 @@ end:
        evbuffer_free(buf);
 }
 
+static void
+test_evbuffer_reserve_with_empty(void *ptr)
+{
+       struct evbuffer *buf;
+       struct evbuffer_iovec v[2];
+
+       tt_assert(buf = evbuffer_new());
+       evbuffer_add(buf, "a", 1);
+       tt_int_op(evbuffer_reserve_space(buf, 1<<12, v, 2), ==, 2);
+       v[0].iov_len = 1;
+       *(char *)v[0].iov_base = 'b';
+       tt_int_op(evbuffer_commit_space(buf, v, 1), ==, 0);
+       evbuffer_add(buf, "c", 1);
+       tt_mem_op(evbuffer_pullup(buf, -1), ==, "abc", 2);
+
+       evbuffer_validate(buf);
+
+ end:
+       if (buf)
+               evbuffer_free(buf);
+}
+
 static void
 test_evbuffer_expand(void *ptr)
 {
@@ -2550,12 +2628,15 @@ struct testcase_t evbuffer_testcases[] = {
        { "remove_buffer_with_empty2", test_evbuffer_remove_buffer_with_empty2, 0, NULL, NULL },
        { "remove_buffer_with_empty3", test_evbuffer_remove_buffer_with_empty3, 0, NULL, NULL },
        { "remove_buffer_with_empty_front", test_evbuffer_remove_buffer_with_empty_front, 0, NULL, NULL },
+       { "remove_buffer_adjust_last_with_datap_with_empty",
+         test_evbuffer_remove_buffer_adjust_last_with_datap_with_empty, 0, NULL, NULL },
        { "add_buffer_with_empty", test_evbuffer_add_buffer_with_empty, 0, NULL, NULL },
        { "add_buffer_with_empty2", test_evbuffer_add_buffer_with_empty2, 0, NULL, NULL },
        { "reserve2", test_evbuffer_reserve2, 0, NULL, NULL },
        { "reserve_many", test_evbuffer_reserve_many, 0, NULL, NULL },
        { "reserve_many2", test_evbuffer_reserve_many, 0, &nil_setup, (void*)"add" },
        { "reserve_many3", test_evbuffer_reserve_many, 0, &nil_setup, (void*)"fill" },
+       { "reserve_with_empty", test_evbuffer_reserve_with_empty, 0, NULL, NULL },
        { "expand", test_evbuffer_expand, 0, NULL, NULL },
        { "expand_overflow", test_evbuffer_expand_overflow, 0, NULL, NULL },
        { "add1", test_evbuffer_add1, 0, NULL, NULL },