From c63728cd73c14093665880b26505418581d7a29a Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Tue, 29 Nov 2011 16:39:49 -0800 Subject: [PATCH 1/3] Make git_buf functions always maintain a valid cstr. At a tiny cost of 1 extra byte per allocation, this makes git_buf_cstr into basically a noop, which simplifies error checking when trying to convert things to use dynamic allocation. This patch also adds a new function (git_buf_copy_cstr) for copying the cstr data directly into an external buffer. --- src/buffer.c | 46 ++++++++++++++++++++++++++++------------ src/buffer.h | 1 + tests-clay/core/buffer.c | 9 ++++---- 3 files changed, 37 insertions(+), 19 deletions(-) diff --git a/src/buffer.c b/src/buffer.c index c56a75598..aa66261c9 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -53,9 +53,10 @@ void git_buf_set(git_buf *buf, const char *data, size_t len) if (len == 0 || data == NULL) { git_buf_clear(buf); } else { - ENSURE_SIZE(buf, len); + ENSURE_SIZE(buf, len + 1); memmove(buf->ptr, data, len); buf->size = len; + buf->ptr[buf->size] = '\0'; } } @@ -66,15 +67,17 @@ void git_buf_sets(git_buf *buf, const char *string) void git_buf_putc(git_buf *buf, char c) { - ENSURE_SIZE(buf, buf->size + 1); + ENSURE_SIZE(buf, buf->size + 2); buf->ptr[buf->size++] = c; + buf->ptr[buf->size] = '\0'; } void git_buf_put(git_buf *buf, const char *data, size_t len) { - ENSURE_SIZE(buf, buf->size + len); + ENSURE_SIZE(buf, buf->size + len + 1); memmove(buf->ptr + buf->size, data, len); buf->size += len; + buf->ptr[buf->size] = '\0'; } void git_buf_puts(git_buf *buf, const char *string) @@ -111,12 +114,25 @@ void git_buf_printf(git_buf *buf, const char *format, ...) const char *git_buf_cstr(git_buf *buf) { - if (buf->size + 1 > buf->asize && - git_buf_grow(buf, buf->size + 1) < GIT_SUCCESS) - return NULL; + return buf->ptr ? buf->ptr : ""; +} - buf->ptr[buf->size] = '\0'; - return buf->ptr; +void git_buf_copy_cstr(char *data, size_t datasize, git_buf *buf) +{ + size_t copylen; + + assert(data && datasize); + + data[0] = '\0'; + + if (buf->size == 0 || buf->asize <= 0) + return; + + copylen = buf->size; + if (copylen > datasize - 1) + copylen = datasize - 1; + memmove(data, buf->ptr, copylen); + data[copylen] = '\0'; } void git_buf_free(git_buf *buf) @@ -132,6 +148,8 @@ void git_buf_free(git_buf *buf) void git_buf_clear(git_buf *buf) { buf->size = 0; + if (buf->ptr) + *buf->ptr = '\0'; } void git_buf_consume(git_buf *buf, const char *end) @@ -140,6 +158,7 @@ void git_buf_consume(git_buf *buf, const char *end) size_t consumed = end - buf->ptr; memmove(buf->ptr, end, buf->size - consumed); buf->size -= consumed; + buf->ptr[buf->size] = '\0'; } } @@ -157,12 +176,9 @@ char *git_buf_take_cstr(git_buf *buf) if (buf->ptr == NULL) return NULL; - if (buf->size + 1 > buf->asize && - git_buf_grow(buf, buf->size + 1) < GIT_SUCCESS) - return NULL; + assert(buf->asize > buf->size); data = buf->ptr; - data[buf->size] = '\0'; buf->ptr = NULL; buf->asize = 0; @@ -199,7 +215,7 @@ void git_buf_join_n(git_buf *buf, char separator, int nbuf, ...) } va_end(ap); - ENSURE_SIZE(buf, buf->size + total_size); + ENSURE_SIZE(buf, buf->size + total_size + 1); out = buf->ptr + buf->size; @@ -235,6 +251,7 @@ void git_buf_join_n(git_buf *buf, char separator, int nbuf, ...) /* set size based on num characters actually written */ buf->size = out - buf->ptr; + buf->ptr[buf->size] = '\0'; } void git_buf_join( @@ -277,7 +294,7 @@ void git_buf_join( if (!add_size) return; - ENSURE_SIZE(buf, buf->size + add_size); + ENSURE_SIZE(buf, buf->size + add_size + 1); /* concatenate strings */ ptr = buf->ptr + buf->size; @@ -296,4 +313,5 @@ void git_buf_join( /* set size based on num characters actually written */ buf->size = ptr - buf->ptr; + buf->ptr[buf->size] = '\0'; } diff --git a/src/buffer.h b/src/buffer.h index 2ed9047ca..9e8ebd058 100644 --- a/src/buffer.h +++ b/src/buffer.h @@ -42,6 +42,7 @@ void git_buf_join(git_buf *buf, char separator, const char *str_a, const char *s const char *git_buf_cstr(git_buf *buf); char *git_buf_take_cstr(git_buf *buf); +void git_buf_copy_cstr(char *data, size_t datasize, git_buf *buf); #define git_buf_PUTS(buf, str) git_buf_put(buf, str, sizeof(str) - 1) diff --git a/tests-clay/core/buffer.c b/tests-clay/core/buffer.c index bf88a3073..cf6e45bd9 100644 --- a/tests-clay/core/buffer.c +++ b/tests-clay/core/buffer.c @@ -57,14 +57,13 @@ void test_core_buffer__2(void) /* this must be safe to do */ git_buf_free(&buf); - cl_assert(buf.size == 0); cl_assert(buf.asize == 0); /* empty buffer should be empty string */ cl_assert_strequal("", git_buf_cstr(&buf)); cl_assert(buf.size == 0); - cl_assert(buf.asize > 0); + /* cl_assert(buf.asize == 0); -- should not assume what git_buf does */ /* free should set us back to the beginning */ git_buf_free(&buf); @@ -277,15 +276,15 @@ void test_core_buffer__5(void) */ check_buf_append("abcdefgh", "/", "abcdefgh/", 9, 16); - check_buf_append("abcdefgh", "ijklmno", "abcdefghijklmno", 15, 24); + check_buf_append("abcdefgh", "ijklmno", "abcdefghijklmno", 15, 16); check_buf_append("abcdefgh", "ijklmnop", "abcdefghijklmnop", 16, 24); check_buf_append("0123456789", "0123456789", "01234567890123456789", 20, 24); check_buf_append(REP16("x"), REP16("o"), REP16("x") REP16("o"), 32, 40); - check_buf_append(test_4096, "", test_4096, 4096, 6144); - check_buf_append(test_4096, test_4096, test_8192, 8192, 9216); + check_buf_append(test_4096, "", test_4096, 4096, 4104); + check_buf_append(test_4096, test_4096, test_8192, 8192, 9240); /* check sequences of appends */ check_buf_append_abc("a", "b", "c", From 7df41387f5d09308acda0d4b54eccc1431c71610 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Tue, 29 Nov 2011 21:44:54 -0800 Subject: [PATCH 2/3] Adding unit tests for git_buf_copy_cstr --- tests-clay/core/buffer.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests-clay/core/buffer.c b/tests-clay/core/buffer.c index cf6e45bd9..1abee3e8c 100644 --- a/tests-clay/core/buffer.c +++ b/tests-clay/core/buffer.c @@ -52,6 +52,7 @@ void test_core_buffer__2(void) { git_buf buf = GIT_BUF_INIT; int i; + char data[100]; cl_assert(buf.size == 0); @@ -129,6 +130,27 @@ void test_core_buffer__2(void) cl_assert_strequal("", git_buf_cstr(&buf)); git_buf_free(&buf); + + /* test extracting data into buffer */ + git_buf_puts(&buf, REP4("0123456789")); + cl_assert(git_buf_oom(&buf) == 0); + + git_buf_copy_cstr(data, 100, &buf); + cl_assert_strequal(data, REP4("0123456789")); + git_buf_copy_cstr(data, 11, &buf); + cl_assert_strequal(data, "0123456789"); + git_buf_copy_cstr(data, 3, &buf); + cl_assert_strequal(data, "01"); + git_buf_copy_cstr(data, 1, &buf); + cl_assert_strequal(data, ""); + + git_buf_copy_cstr(data, 100, &buf); + cl_assert_strequal(data, REP4("0123456789")); + + git_buf_free(&buf); + + git_buf_copy_cstr(data, 100, &buf); + cl_assert_strequal(data, ""); } /* let's do some tests with larger buffers to push our limits */ From 309113c984c1f3157659dc1174e5d4218f610ae4 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Tue, 29 Nov 2011 23:45:17 -0800 Subject: [PATCH 3/3] Make initial value of git_buf ptr always be a valid empty string. Taking a page from core git's strbuf, this introduces git_buf_initbuf which is an empty string that is used to initialize the git_buf ptr value even for new buffers. Now the git_buf ptr will always point to a valid NUL-terminated string. This change required jumping through a few hoops for git_buf_grow and git_buf_free to distinguish between a actual allocated buffer and the global initial value. Also, this moves the allocation related functions to be next to each other near the top of buffer.c. --- src/buffer.c | 90 +++++++++++++++++++++++++--------------- src/buffer.h | 5 ++- tests-clay/core/buffer.c | 9 ++-- 3 files changed, 63 insertions(+), 41 deletions(-) diff --git a/src/buffer.c b/src/buffer.c index aa66261c9..3dff813e3 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -8,13 +8,29 @@ #include "posix.h" #include +/* Used as default value for git_buf->ptr so that people can always + * assume ptr is non-NULL and zero terminated even for new git_bufs. + */ +char git_buf_initbuf[1]; + #define ENSURE_SIZE(b, d) \ if ((ssize_t)(d) > buf->asize && git_buf_grow(b, (d)) < GIT_SUCCESS)\ return; +void git_buf_init(git_buf *buf, size_t initial_size) +{ + buf->asize = 0; + buf->size = 0; + buf->ptr = git_buf_initbuf; + + if (initial_size) + git_buf_grow(buf, initial_size); +} + int git_buf_grow(git_buf *buf, size_t target_size) { char *new_ptr; + size_t new_size; if (buf->asize < 0) return GIT_ENOMEM; @@ -22,27 +38,56 @@ int git_buf_grow(git_buf *buf, size_t target_size) if (target_size <= (size_t)buf->asize) return GIT_SUCCESS; - if (buf->asize == 0) - buf->asize = target_size; + if (buf->asize == 0) { + new_size = target_size; + new_ptr = NULL; + } else { + new_size = (size_t)buf->asize; + new_ptr = buf->ptr; + } /* grow the buffer size by 1.5, until it's big enough * to fit our target size */ - while (buf->asize < (int)target_size) - buf->asize = (buf->asize << 1) - (buf->asize >> 1); + while (new_size < target_size) + new_size = (new_size << 1) - (new_size >> 1); /* round allocation up to multiple of 8 */ - buf->asize = (buf->asize + 7) & ~7; + new_size = (new_size + 7) & ~7; - new_ptr = git__realloc(buf->ptr, buf->asize); + new_ptr = git__realloc(new_ptr, new_size); if (!new_ptr) { buf->asize = -1; return GIT_ENOMEM; } - buf->ptr = new_ptr; + buf->asize = new_size; + buf->ptr = new_ptr; + + /* truncate the existing buffer size if necessary */ + if (buf->size >= buf->asize) + buf->size = buf->asize - 1; + buf->ptr[buf->size] = '\0'; + return GIT_SUCCESS; } +void git_buf_free(git_buf *buf) +{ + if (!buf) return; + + if (buf->ptr != git_buf_initbuf) + git__free(buf->ptr); + + git_buf_init(buf, 0); +} + +void git_buf_clear(git_buf *buf) +{ + buf->size = 0; + if (buf->asize > 0) + buf->ptr[0] = '\0'; +} + int git_buf_oom(const git_buf *buf) { return (buf->asize < 0); @@ -114,7 +159,7 @@ void git_buf_printf(git_buf *buf, const char *format, ...) const char *git_buf_cstr(git_buf *buf) { - return buf->ptr ? buf->ptr : ""; + return buf->ptr; } void git_buf_copy_cstr(char *data, size_t datasize, git_buf *buf) @@ -135,23 +180,6 @@ void git_buf_copy_cstr(char *data, size_t datasize, git_buf *buf) data[copylen] = '\0'; } -void git_buf_free(git_buf *buf) -{ - if (!buf) return; - - git__free(buf->ptr); - buf->ptr = NULL; - buf->asize = 0; - buf->size = 0; -} - -void git_buf_clear(git_buf *buf) -{ - buf->size = 0; - if (buf->ptr) - *buf->ptr = '\0'; -} - void git_buf_consume(git_buf *buf, const char *end) { if (end > buf->ptr && end <= buf->ptr + buf->size) { @@ -171,18 +199,12 @@ void git_buf_swap(git_buf *buf_a, git_buf *buf_b) char *git_buf_take_cstr(git_buf *buf) { - char *data = NULL; + char *data = buf->ptr; - if (buf->ptr == NULL) + if (buf->asize <= 0) return NULL; - assert(buf->asize > buf->size); - - data = buf->ptr; - - buf->ptr = NULL; - buf->asize = 0; - buf->size = 0; + git_buf_init(buf, 0); return data; } diff --git a/src/buffer.h b/src/buffer.h index 9e8ebd058..fa0c7f0b8 100644 --- a/src/buffer.h +++ b/src/buffer.h @@ -14,8 +14,11 @@ typedef struct { ssize_t asize, size; } git_buf; -#define GIT_BUF_INIT {NULL, 0, 0} +extern char git_buf_initbuf[]; +#define GIT_BUF_INIT { git_buf_initbuf, 0, 0 } + +void git_buf_init(git_buf *buf, size_t initial_size); int git_buf_grow(git_buf *buf, size_t target_size); void git_buf_free(git_buf *buf); void git_buf_swap(git_buf *buf_a, git_buf *buf_b); diff --git a/tests-clay/core/buffer.c b/tests-clay/core/buffer.c index 1abee3e8c..acde8c0a7 100644 --- a/tests-clay/core/buffer.c +++ b/tests-clay/core/buffer.c @@ -224,10 +224,7 @@ check_buf_append( cl_assert(git_buf_oom(&tgt) == 0); git_buf_puts(&tgt, data_b); cl_assert(git_buf_oom(&tgt) == 0); - if (expected_data == NULL) - cl_assert(tgt.ptr == NULL); - else - cl_assert_strequal(expected_data, git_buf_cstr(&tgt)); + cl_assert_strequal(expected_data, git_buf_cstr(&tgt)); cl_assert(tgt.size == expected_size); if (expected_asize > 0) cl_assert(tgt.asize == expected_asize); @@ -356,13 +353,13 @@ void test_core_buffer__7(void) b = git_buf_take_cstr(&a); cl_assert_strequal("foo", b); - cl_assert_strequal(NULL, a.ptr); + cl_assert_strequal("", a.ptr); git__free(b); b = git_buf_take_cstr(&a); cl_assert_strequal(NULL, b); - cl_assert_strequal(NULL, a.ptr); + cl_assert_strequal("", a.ptr); git_buf_free(&a); }