From 97eb5ef0262bc0acf2ce66481105960b79e10ed7 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 7 Jun 2017 10:05:54 +0200 Subject: [PATCH 1/5] buffer: rely on `GITERR_OOM` set by `git_buf_try_grow` The function `git_buf_try_grow` consistently calls `giterr_set_oom` whenever growing the buffer fails due to insufficient memory being available. So in fact, we do not have to do this ourselves when a call to any buffer-growing function has failed due to an OOM situation. But we still do so in two functions, which this patch cleans up. --- src/pack.c | 1 - src/transports/winhttp.c | 1 - 2 files changed, 2 deletions(-) diff --git a/src/pack.c b/src/pack.c index 60b757e90..d24c13815 100644 --- a/src/pack.c +++ b/src/pack.c @@ -324,7 +324,6 @@ static int pack_index_open(struct git_pack_file *p) git_buf_put(&idx_name, p->pack_name, name_len - strlen(".pack")); git_buf_puts(&idx_name, ".idx"); if (git_buf_oom(&idx_name)) { - giterr_set_oom(); return -1; } diff --git a/src/transports/winhttp.c b/src/transports/winhttp.c index e8e848026..fb504c912 100644 --- a/src/transports/winhttp.c +++ b/src/transports/winhttp.c @@ -429,7 +429,6 @@ static int winhttp_stream_connect(winhttp_stream *s) git_buf_printf(&processed_url, ":%s", t->proxy_connection_data.port); if (git_buf_oom(&processed_url)) { - giterr_set_oom(); error = -1; goto on_error; } From e82dd8130fa1d944b5846f5165a990b97b2590ed Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 8 Jun 2017 11:52:32 +0200 Subject: [PATCH 2/5] buffer: fix `ENSURE_SIZE` macro referencing wrong variable While the `ENSURE_SIZE` macro gets a reference to both the buffer that is to be resized and a new size, we were not consistently referencing the passed buffer, but instead a variable `buf`, which is not passed in. Funnily enough, we never noticed because our buffers seem to always be named `buf` whenever the macro was being used. Fix the macro by always using the passed-in buffer. While at it, add braces around all mentions of passed-in variables as should be done with macros to avoid subtle errors. Found-by: Edward Thompson --- src/buffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/buffer.c b/src/buffer.c index fdb732d9e..ba8bd82d0 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -18,7 +18,7 @@ char git_buf__initbuf[1]; char git_buf__oom[1]; #define ENSURE_SIZE(b, d) \ - if ((d) > buf->asize && git_buf_grow(b, (d)) < 0)\ + if ((d) > (b)->asize && git_buf_grow((b), (d)) < 0)\ return -1; From 9a8386a2c649fa74cf90fe95931bc3fc0466f2f6 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 7 Jun 2017 09:50:54 +0200 Subject: [PATCH 3/5] buffer: consistently use `ENSURE_SIZE` to grow buffers on-demand The `ENSURE_SIZE` macro can be used to grow a buffer if its currently allocated size does not suffice a required target size. While most of the code already uses this macro, the `git_buf_join` and `git_buf_join3` functions do not yet use it. Due to the macro first checking whether we have to grow the buffer at all, this has the benefit of saving a function call when it is not needed. While this is nice to have, it will probably not matter at all performance-wise -- instead, this only serves for consistency across the code. --- src/buffer.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/buffer.c b/src/buffer.c index ba8bd82d0..40bed5c98 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -724,9 +724,7 @@ int git_buf_join( GITERR_CHECK_ALLOC_ADD(&alloc_len, strlen_a, strlen_b); GITERR_CHECK_ALLOC_ADD(&alloc_len, alloc_len, need_sep); GITERR_CHECK_ALLOC_ADD(&alloc_len, alloc_len, 1); - if (git_buf_grow(buf, alloc_len) < 0) - return -1; - assert(buf->ptr); + ENSURE_SIZE(buf, alloc_len); /* fix up internal pointers */ if (offset_a >= 0) @@ -780,8 +778,7 @@ int git_buf_join3( GITERR_CHECK_ALLOC_ADD(&len_total, len_total, sep_b); GITERR_CHECK_ALLOC_ADD(&len_total, len_total, len_c); GITERR_CHECK_ALLOC_ADD(&len_total, len_total, 1); - if (git_buf_grow(buf, len_total) < 0) - return -1; + ENSURE_SIZE(buf, len_total); tgt = buf->ptr; From 4796c916d376af528d8bbf07e8a5e176da6ee928 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 7 Jun 2017 09:56:31 +0200 Subject: [PATCH 4/5] buffer: return errors for `git_buf_init` and `git_buf_attach` Both the `git_buf_init` and `git_buf_attach` functions may call `git_buf_grow` in case they were given an allocation length as parameter. As such, it is possible for these functions to fail when we run out of memory. While it won't probably be used anytime soon, it does indeed make sense to also record this fact by returning an error code from both functions. As they belong to the internal API only, this change does not break our interface. --- src/buffer.c | 14 ++++++++------ src/buffer.h | 4 ++-- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/buffer.c b/src/buffer.c index 40bed5c98..6dfcbfbe6 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -22,14 +22,15 @@ char git_buf__oom[1]; return -1; -void git_buf_init(git_buf *buf, size_t initial_size) +int 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); + ENSURE_SIZE(buf, initial_size); + + return 0; } int git_buf_try_grow( @@ -577,7 +578,7 @@ char *git_buf_detach(git_buf *buf) return data; } -void git_buf_attach(git_buf *buf, char *ptr, size_t asize) +int git_buf_attach(git_buf *buf, char *ptr, size_t asize) { git_buf_free(buf); @@ -588,9 +589,10 @@ void git_buf_attach(git_buf *buf, char *ptr, size_t asize) buf->asize = (asize < buf->size) ? buf->size + 1 : asize; else /* pass 0 to fall back on strlen + 1 */ buf->asize = buf->size + 1; - } else { - git_buf_grow(buf, asize); } + + ENSURE_SIZE(buf, asize); + return 0; } void git_buf_attach_notowned(git_buf *buf, const char *ptr, size_t size) diff --git a/src/buffer.h b/src/buffer.h index a76b2d771..b0aece488 100644 --- a/src/buffer.h +++ b/src/buffer.h @@ -34,7 +34,7 @@ GIT_INLINE(bool) git_buf_is_allocated(const git_buf *buf) * For the cases where GIT_BUF_INIT cannot be used to do static * initialization. */ -extern void git_buf_init(git_buf *buf, size_t initial_size); +extern int git_buf_init(git_buf *buf, size_t initial_size); /** * Resize the buffer allocation to make more space. @@ -73,7 +73,7 @@ extern void git_buf_sanitize(git_buf *buf); extern void git_buf_swap(git_buf *buf_a, git_buf *buf_b); extern char *git_buf_detach(git_buf *buf); -extern void git_buf_attach(git_buf *buf, char *ptr, size_t asize); +extern int git_buf_attach(git_buf *buf, char *ptr, size_t asize); /* Populates a `git_buf` where the contents are not "owned" by the * buffer, and calls to `git_buf_free` will not free the given buf. From a693b8734941889bc9a4c31752d317658162246a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 7 Jun 2017 10:20:44 +0200 Subject: [PATCH 5/5] buffer: use `git_buf_init` with length The `git_buf_init` function has an optional length parameter, which will cause the buffer to be initialized and allocated in one step. This can be used instead of static initialization with `GIT_BUF_INIT` followed by a `git_buf_grow`. This patch does so for two functions where it is applicable. --- src/config_file.c | 5 +++-- src/pack.c | 7 +++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/config_file.c b/src/config_file.c index 2302d3343..e15d57bbb 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -1267,7 +1267,7 @@ static const char *escaped = "\n\t\b\"\\"; /* Escape the values to write them to the file */ static char *escape_value(const char *ptr) { - git_buf buf = GIT_BUF_INIT; + git_buf buf; size_t len; const char *esc; @@ -1277,7 +1277,8 @@ static char *escape_value(const char *ptr) if (!len) return git__calloc(1, sizeof(char)); - git_buf_grow(&buf, len); + if (git_buf_init(&buf, len) < 0) + return NULL; while (*ptr != '\0') { if ((esc = strchr(escaped, *ptr)) != NULL) { diff --git a/src/pack.c b/src/pack.c index d24c13815..f8d0dc9ac 100644 --- a/src/pack.c +++ b/src/pack.c @@ -312,7 +312,7 @@ static int pack_index_open(struct git_pack_file *p) { int error = 0; size_t name_len; - git_buf idx_name = GIT_BUF_INIT; + git_buf idx_name; if (p->index_version > -1) return 0; @@ -320,10 +320,13 @@ static int pack_index_open(struct git_pack_file *p) name_len = strlen(p->pack_name); assert(name_len > strlen(".pack")); /* checked by git_pack_file alloc */ - git_buf_grow(&idx_name, name_len); + if (git_buf_init(&idx_name, name_len) < 0) + return -1; + git_buf_put(&idx_name, p->pack_name, name_len - strlen(".pack")); git_buf_puts(&idx_name, ".idx"); if (git_buf_oom(&idx_name)) { + git_buf_free(&idx_name); return -1; }