From: Niels Provos Date: Mon, 31 Mar 2008 02:04:34 +0000 (+0000) Subject: fix a bug in which evbuffer_add_vfprintf would loop forever; avoid X-Git-Tag: release-2.0.1-alpha~388 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=193c06a7ed9730f5f08ca3958fbacee2c3e28e36;p=libevent fix a bug in which evbuffer_add_vfprintf would loop forever; avoid fragmentation in evbuffer_expand by increasing the size of the last buffer in the chain; as a result with have to keep track of the previous_to_last chain; provide a evbuffer_validate() function in the regression test to make sure that all evbuffer are internally consistent. svn:r699 --- diff --git a/buffer.c b/buffer.c index a3574ce7..c669460b 100644 --- a/buffer.c +++ b/buffer.c @@ -122,6 +122,37 @@ evbuffer_length(struct evbuffer *buffer) return (buffer->total_len); } +#define ZERO_CHAIN(dst) do { \ + (dst)->first = NULL; \ + (dst)->last = NULL; \ + (dst)->previous_to_last = NULL; \ + (dst)->total_len = 0; \ + } while (0) + +#define COPY_CHAIN(dst, src) do { \ + (dst)->first = (src)->first; \ + (dst)->previous_to_last = (src)->previous_to_last; \ + (dst)->last = (src)->last; \ + (dst)->total_len = (src)->total_len; \ + } while (0) + +#define APPEND_CHAIN(dst, src) do { \ + (dst)->last->next = (src)->first; \ + (dst)->previous_to_last = (src)->previous_to_last ? \ + (src)->previous_to_last : (dst)->last; \ + (dst)->last = (src)->last; \ + (dst)->total_len += (src)->total_len; \ + } while (0) + +#define PREPEND_CHAIN(dst, src) do { \ + (src)->last->next = (dst)->first; \ + (dst)->first = (src)->first; \ + (dst)->total_len += (src)->total_len; \ + if ((dst)->previous_to_last == NULL) \ + (dst)->previous_to_last = (src)->last; \ + } while (0) + + int evbuffer_add_buffer(struct evbuffer *outbuf, struct evbuffer *inbuf) { @@ -129,19 +160,13 @@ evbuffer_add_buffer(struct evbuffer *outbuf, struct evbuffer *inbuf) size_t in_total_len = inbuf->total_len; if (out_total_len == 0) { - outbuf->first = inbuf->first; - outbuf->last = inbuf->last; - outbuf->total_len = in_total_len; + COPY_CHAIN(outbuf, inbuf); } else { - outbuf->last->next = inbuf->first; - outbuf->last = inbuf->last; - outbuf->total_len += in_total_len; + APPEND_CHAIN(outbuf, inbuf); } /* remove everything from inbuf */ - inbuf->first = NULL; - inbuf->last = NULL; - inbuf->total_len = 0; + ZERO_CHAIN(inbuf); if (inbuf->cb != NULL && inbuf->total_len != in_total_len) (*inbuf->cb)(inbuf, in_total_len, inbuf->total_len, @@ -163,19 +188,13 @@ evbuffer_prepend_buffer(struct evbuffer *outbuf, struct evbuffer *inbuf) return; if (out_total_len == 0) { - outbuf->first = inbuf->first; - outbuf->last = inbuf->last; - outbuf->total_len = in_total_len; + COPY_CHAIN(outbuf, inbuf); } else { - inbuf->last->next = outbuf->first; - outbuf->first = inbuf->first; - outbuf->total_len += in_total_len; + PREPEND_CHAIN(outbuf, inbuf); } /* remove everything from inbuf */ - inbuf->first = NULL; - inbuf->last = NULL; - inbuf->total_len = 0; + ZERO_CHAIN(inbuf); if (inbuf->cb != NULL && inbuf->total_len != in_total_len) (*inbuf->cb)(inbuf, in_total_len, inbuf->total_len, @@ -201,8 +220,7 @@ evbuffer_drain(struct evbuffer *buf, size_t len) event_free(chain); } - buf->total_len = 0; - buf->first = buf->last = NULL; + ZERO_CHAIN(buf); } else { buf->total_len -= len; @@ -214,6 +232,8 @@ evbuffer_drain(struct evbuffer *buf, size_t len) } buf->first = chain; + if (buf->first == buf->last) + buf->previous_to_last = NULL; chain->misalign += len; chain->off -= len; } @@ -254,6 +274,8 @@ evbuffer_remove(struct evbuffer *buf, void *data_out, size_t datlen) buf->first = chain; if (chain == NULL) buf->last = NULL; + if (buf->first == buf->last) + buf->previous_to_last = NULL; if (datlen) { memcpy(data, chain->buffer + chain->misalign, datlen); @@ -276,7 +298,8 @@ int evbuffer_remove_buffer(struct evbuffer *src, struct evbuffer *dst, size_t datlen) { - struct evbuffer_chain *chain = src->first, *previous = chain; + struct evbuffer_chain *chain = src->first; + struct evbuffer_chain *previous = chain, *previous_to_previous = NULL; size_t nread = 0; /* short-cut if there is no more data buffered */ @@ -293,6 +316,7 @@ evbuffer_remove_buffer(struct evbuffer *src, struct evbuffer *dst, while (chain->off <= datlen) { nread += chain->off; datlen -= chain->off; + previous_to_previous = previous; previous = chain; chain = chain->next; } @@ -304,9 +328,12 @@ evbuffer_remove_buffer(struct evbuffer *src, struct evbuffer *dst, } else { dst->last->next = src->first; } + dst->previous_to_last = previous_to_previous; dst->last = previous; previous->next = NULL; src->first = chain; + if (src->first == src->last) + src->previous_to_last = NULL; dst->total_len += nread; } @@ -383,8 +410,12 @@ evbuffer_pullup(struct evbuffer *buf, int size) memcpy(buffer, chain->buffer + chain->misalign, size); chain->misalign += size; chain->off -= size; + if (chain == buf->last) + buf->previous_to_last = tmp; } else { buf->last = tmp; + /* the last is already the first, so we have no previous */ + buf->previous_to_last = NULL; } tmp->next = chain; @@ -605,6 +636,7 @@ evbuffer_add(struct evbuffer *buf, const void *data_in, size_t datlen) if (chain->next == NULL) return (-1); buf->last = chain->next; + buf->previous_to_last = chain; buf->total_len += datlen; memcpy(chain->buffer + chain->misalign + chain->off, @@ -646,13 +678,15 @@ evbuffer_prepend(struct evbuffer *buf, const void *data, size_t datlen) chain->misalign -= datlen; } else { struct evbuffer_chain *tmp; - /* XXX we should copy as much of the data into chain as possible - * before we put any into tmp. */ + /* XXX we should copy as much of the data into chain + * as possible before we put any into tmp. */ /* we need to add another chain */ if ((tmp = evbuffer_chain_new(datlen)) == NULL) return (-1); buf->first = tmp; + if (buf->previous_to_last == NULL) + buf->previous_to_last = tmp; tmp->next = chain; tmp->off = datlen; @@ -691,6 +725,7 @@ evbuffer_expand(struct evbuffer *buf, size_t datlen) return (-1); buf->first = buf->last = chain; + buf->previous_to_last = NULL; return (0); } @@ -700,25 +735,34 @@ evbuffer_expand(struct evbuffer *buf, size_t datlen) if (chain->buffer_len >= need) return (0); - /* If the misalignment plus the remaining space fulfils our data needs, we - * just force an alignment to happen. Afterwards, we have enough space. + /* If the misalignment plus the remaining space fulfils our + * data needs, we just force an alignment to happen. + * Afterwards, we have enough space. */ if (chain->buffer_len - chain->off >= datlen) { evbuffer_chain_align(chain); return (0); } - /* avoid a memcpy if we can just append a new chain */ - /* XXX in practice, does this result in lots of leftover space? */ - length = chain->buffer_len << 1; - if (length < datlen) - length = datlen; + /* figure out how much space we need */ + length = chain->buffer_len - chain->misalign + datlen; tmp = evbuffer_chain_new(length); if (tmp == NULL) return (-1); - chain->next = tmp; + /* copy the data over that we had so far */ + tmp->off = chain->off; + tmp->misalign = 0; + memcpy(tmp->buffer, chain->buffer + chain->misalign, chain->off); + + /* fix up the chain */ + if (buf->first == chain) + buf->first = tmp; + if (buf->previous_to_last) + buf->previous_to_last->next = tmp; buf->last = tmp; + event_free(chain); + return (0); } @@ -851,7 +895,6 @@ evbuffer_find(struct evbuffer *buffer, const u_char *what, size_t len) int evbuffer_add_vprintf(struct evbuffer *buf, const char *fmt, va_list ap) { - struct evbuffer_chain *chain; char *buffer; size_t space; size_t old_len = buf->total_len; @@ -862,8 +905,8 @@ evbuffer_add_vprintf(struct evbuffer *buf, const char *fmt, va_list ap) if (evbuffer_expand(buf, 64) == -1) return (-1); - chain = buf->last; for (;;) { + struct evbuffer_chain *chain = buf->last; size_t used = chain->misalign + chain->off; buffer = (char *)chain->buffer + chain->misalign + chain->off; assert(chain->buffer_len >= used); diff --git a/evbuffer-internal.h b/evbuffer-internal.h index 5df1b1a2..0f14d990 100644 --- a/evbuffer-internal.h +++ b/evbuffer-internal.h @@ -45,6 +45,7 @@ struct evbuffer_chain; struct evbuffer { struct evbuffer_chain *first; struct evbuffer_chain *last; + struct evbuffer_chain *previous_to_last; size_t total_len; /* total length of all buffers */ diff --git a/test/regress.c b/test/regress.c index 48d3a712..573cc8c2 100644 --- a/test/regress.c +++ b/test/regress.c @@ -53,10 +53,12 @@ #include #include #include +#include #include "event.h" #include "evutil.h" #include "event-internal.h" +#include "evbuffer-internal.h" #include "log.h" #include "regress.h" @@ -890,6 +892,38 @@ test_nonpersist_readd(void) cleanup_test(); } +/* validates that an evbuffer is good */ +static void +evbuffer_validate(struct evbuffer *buf) +{ + struct evbuffer_chain *chain, *previous = NULL; + size_t sum = 0; + + if (buf->first == NULL) { + assert(buf->last == NULL); + assert(buf->previous_to_last == NULL); + assert(buf->total_len == 0); + } + + if (buf->previous_to_last == NULL) { + assert(buf->first == buf->last); + } + + chain = buf->first; + while (chain != NULL) { + sum += chain->off; + if (chain->next == NULL) { + assert(buf->previous_to_last == previous); + assert(buf->last == chain); + } + assert(chain->buffer_len >= chain->misalign + chain->off); + previous = chain; + chain = chain->next; + } + + assert(sum == buf->total_len); +} + static void test_evbuffer(void) { @@ -900,19 +934,24 @@ test_evbuffer(void) int i; setup_test("Testing Evbuffer: "); + evbuffer_validate(evb); evbuffer_add_printf(evb, "%s/%d", "hello", 1); + evbuffer_validate(evb); if (EVBUFFER_LENGTH(evb) != 7 || strcmp((char*)EVBUFFER_DATA(evb), "hello/1") != 0) goto out; evbuffer_drain(evb, strlen("hello/")); + evbuffer_validate(evb); if (EVBUFFER_LENGTH(evb) != 1 || strcmp((char*)EVBUFFER_DATA(evb), "1") != 0) goto out; evbuffer_add_printf(evb_two, "%s", "/hello"); + evbuffer_validate(evb); evbuffer_add_buffer(evb, evb_two); + evbuffer_validate(evb); if (EVBUFFER_LENGTH(evb_two) != 0 || EVBUFFER_LENGTH(evb) != 7 || @@ -921,6 +960,7 @@ test_evbuffer(void) memset(buffer, 0, sizeof(buffer)); evbuffer_add(evb, buffer, sizeof(buffer)); + evbuffer_validate(evb); if (EVBUFFER_LENGTH(evb) != 7 + 512) goto out; @@ -931,20 +971,29 @@ test_evbuffer(void) goto out; if (memcmp(tmp + 7, buffer, sizeof(buffer)) != 0) goto out; + evbuffer_validate(evb); evbuffer_prepend(evb, "something", 9); + evbuffer_validate(evb); evbuffer_prepend(evb, "else", 4); + evbuffer_validate(evb); tmp = (char *)evbuffer_pullup(evb, 4 + 9 + 7); if (strncmp(tmp, "elsesomething1/hello", 4 + 9 + 7) != 0) goto out; + evbuffer_validate(evb); evbuffer_drain(evb, -1); + evbuffer_validate(evb); evbuffer_drain(evb_two, -1); + evbuffer_validate(evb); for (i = 0; i < 3; ++i) { evbuffer_add(evb_two, buffer, sizeof(buffer)); + evbuffer_validate(evb_two); evbuffer_add_buffer(evb, evb_two); + evbuffer_validate(evb); + evbuffer_validate(evb_two); } if (EVBUFFER_LENGTH(evb_two) != 0 || @@ -957,12 +1006,14 @@ test_evbuffer(void) if (EVBUFFER_LENGTH(evb_two) != sz_tmp || EVBUFFER_LENGTH(evb) != sizeof(buffer) / 2) goto out; + evbuffer_validate(evb); if (memcmp(evbuffer_pullup( evb, -1), buffer, sizeof(buffer) / 2) != 0 || memcmp(evbuffer_pullup( evb_two, -1), buffer, sizeof(buffer) != 0)) goto out; + evbuffer_validate(evb); test_ok = 1; @@ -986,123 +1037,169 @@ test_evbuffer_readln(void) /* Test EOL_ANY. */ s = "complex silly newline\r\n\n\r\n\n\rmore\0\n"; evbuffer_add(evb, s, strlen(s)+2); + evbuffer_validate(evb); cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_ANY); if (!cp || sz != strlen(cp) || strcmp(cp, "complex silly newline")) goto done; free(cp); + evbuffer_validate(evb); cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_ANY); if (!cp || sz != 5 || memcmp(cp, "more\0\0", 6)) goto done; if (EVBUFFER_LENGTH(evb) != 0) goto done; + evbuffer_validate(evb); s = "\nno newline"; evbuffer_add(evb, s, strlen(s)); free(cp); + evbuffer_validate(evb); cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_ANY); if (!cp || sz || strcmp(cp, "")) goto done; free(cp); + evbuffer_validate(evb); cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_ANY); if (cp) goto done; + evbuffer_validate(evb); evbuffer_drain(evb, EVBUFFER_LENGTH(evb)); if (EVBUFFER_LENGTH(evb) != 0) goto done; + evbuffer_validate(evb); /* Test EOL_CRLF */ s = "Line with\rin the middle\nLine with good crlf\r\n\nfinal\n"; evbuffer_add(evb, s, strlen(s)); + evbuffer_validate(evb); cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_CRLF); if (!cp || sz != strlen(cp) || strcmp(cp, "Line with\rin the middle")) goto done; - free(cp); + evbuffer_validate(evb); + cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_CRLF); if (!cp || sz != strlen(cp) || strcmp(cp, "Line with good crlf")) goto done; free(cp); + evbuffer_validate(evb); + cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_CRLF); if (!cp || sz != strlen(cp) || strcmp(cp, "")) goto done; free(cp); + evbuffer_validate(evb); + cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_CRLF); if (!cp || sz != strlen(cp) || strcmp(cp, "final")) goto done; s = "x"; + evbuffer_validate(evb); evbuffer_add(evb, s, 1); + evbuffer_validate(evb); free(cp); cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_CRLF); if (cp) goto done; + evbuffer_validate(evb); /* Test CRLF_STRICT */ s = " and a bad crlf\nand a good one\r\n\r\nMore\r"; evbuffer_add(evb, s, strlen(s)); + evbuffer_validate(evb); cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_CRLF_STRICT); if (!cp || sz != strlen(cp) || strcmp(cp, "x and a bad crlf\nand a good one")) goto done; free(cp); + evbuffer_validate(evb); + cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_CRLF_STRICT); if (!cp || sz != strlen(cp) || strcmp(cp, "")) goto done; free(cp); + evbuffer_validate(evb); + cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_CRLF_STRICT); if (cp) goto done; + evbuffer_validate(evb); evbuffer_add(evb, "\n", 1); + evbuffer_validate(evb); + cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_CRLF_STRICT); if (!cp || sz != strlen(cp) || strcmp(cp, "More")) goto done; if (EVBUFFER_LENGTH(evb) != 0) goto done; + evbuffer_validate(evb); /* Test LF */ s = "An\rand a nl\n\nText"; evbuffer_add(evb, s, strlen(s)); + evbuffer_validate(evb); + cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_LF); if (!cp || sz != strlen(cp) || strcmp(cp, "An\rand a nl")) goto done; free(cp); + evbuffer_validate(evb); + cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_LF); if (!cp || sz != strlen(cp) || strcmp(cp, "")) goto done; free(cp); + evbuffer_validate(evb); + cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_LF); if (cp) goto done; evbuffer_add(evb, "\n", 1); + evbuffer_validate(evb); cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_LF); if (!cp || sz != strlen(cp) || strcmp(cp, "Text")) goto done; + evbuffer_validate(evb); /* Test CRLF_STRICT - across boundaries*/ s = " and a bad crlf\nand a good one\r"; evbuffer_add(evb_tmp, s, strlen(s)); + evbuffer_validate(evb); evbuffer_add_buffer(evb, evb_tmp); + evbuffer_validate(evb); s = "\n\r"; evbuffer_add(evb_tmp, s, strlen(s)); + evbuffer_validate(evb); evbuffer_add_buffer(evb, evb_tmp); + evbuffer_validate(evb); s = "\nMore\r"; evbuffer_add(evb_tmp, s, strlen(s)); + evbuffer_validate(evb); evbuffer_add_buffer(evb, evb_tmp); + evbuffer_validate(evb); cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_CRLF_STRICT); if (!cp || sz != strlen(cp) || strcmp(cp, " and a bad crlf\nand a good one")) goto done; free(cp); + evbuffer_validate(evb); + cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_CRLF_STRICT); if (!cp || sz != strlen(cp) || strcmp(cp, "")) goto done; free(cp); + evbuffer_validate(evb); + cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_CRLF_STRICT); if (cp) goto done; + evbuffer_validate(evb); evbuffer_add(evb, "\n", 1); + evbuffer_validate(evb); cp = evbuffer_readln(evb, &sz, EVBUFFER_EOL_CRLF_STRICT); if (!cp || sz != strlen(cp) || strcmp(cp, "More")) goto done; + evbuffer_validate(evb); if (EVBUFFER_LENGTH(evb) != 0) goto done; @@ -1115,6 +1212,36 @@ test_evbuffer_readln(void) cleanup_test(); } +static void +test_evbuffer_iterative(void) +{ + struct evbuffer *buf = evbuffer_new(); + const char *abc = "abcdefghijklmnopqrstvuwxyzabcdefghijklmnopqrstvuwxyzabcdefghijklmnopqrstvuwxyzabcdefghijklmnopqrstvuwxyz"; + int i, j, sum; + + setup_test("Testing evbuffer() iterative: "); + + sum = 0; + for (i = 0; i < 1000; ++i) { + for (j = 1; j < strlen(abc); ++j) { + char format[32]; + + snprintf(format, sizeof(format), "%%%d.%ds", j, j); + evbuffer_add_printf(buf, format, abc); + evbuffer_validate(buf); + + sum += j; + } + } + + if (sum == EVBUFFER_LENGTH(buf)) + test_ok = 1; + + evbuffer_free(buf); + + cleanup_test(); +} + static void test_evbuffer_find(void) { @@ -1129,8 +1256,11 @@ test_evbuffer_find(void) /* make sure evbuffer_find doesn't match past the end of the buffer */ fprintf(stdout, "Testing evbuffer_find 1: "); evbuffer_add(buf, (u_char*)test1, strlen(test1)); + evbuffer_validate(buf); evbuffer_drain(buf, strlen(test1)); + evbuffer_validate(buf); evbuffer_add(buf, (u_char*)test2, strlen(test2)); + evbuffer_validate(buf); p = evbuffer_find(buf, (u_char*)"\r\n", 2); if (p == NULL) { fprintf(stdout, "OK\n"); @@ -1145,10 +1275,12 @@ test_evbuffer_find(void) */ fprintf(stdout, "Testing evbuffer_find 2: "); evbuffer_drain(buf, strlen(test2)); + evbuffer_validate(buf); for (i = 0; i < EVBUFFER_INITIAL_LENGTH; ++i) test3[i] = 'a'; test3[EVBUFFER_INITIAL_LENGTH - 1] = 'x'; evbuffer_add(buf, (u_char *)test3, EVBUFFER_INITIAL_LENGTH); + evbuffer_validate(buf); p = evbuffer_find(buf, (u_char *)"xy", 2); if (p == NULL) { printf("OK\n"); @@ -1718,6 +1850,7 @@ main (int argc, char **argv) test_priorities(3); test_evbuffer(); + test_evbuffer_iterative(); test_evbuffer_readln(); test_evbuffer_find();