From caab22c0d468e90b6a95072f3092d5dcf331b3ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 23 Jun 2015 15:41:58 +0200 Subject: [PATCH 1/5] buffer: don't allow growing borrowed buffers When we don't own a buffer (asize=0) we currently allow the usage of grow to copy the memory into a buffer we do own. This muddles the meaning of grow, and lets us be a bit cavalier with ownership semantics. Don't allow this any more. Usage of grow should be restricted to buffers which we know own their own memory. If unsure, we must not attempt to modify it. --- src/buffer.c | 12 ++++++------ src/buffer.h | 2 +- src/path.c | 4 ++-- tests/core/buffer.c | 13 +++++++++++++ 4 files changed, 22 insertions(+), 9 deletions(-) diff --git a/src/buffer.c b/src/buffer.c index f633c5e02..c066d8e84 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -33,7 +33,7 @@ void git_buf_init(git_buf *buf, size_t initial_size) } int git_buf_try_grow( - git_buf *buf, size_t target_size, bool mark_oom, bool preserve_external) + git_buf *buf, size_t target_size, bool mark_oom) { char *new_ptr; size_t new_size; @@ -41,6 +41,9 @@ int git_buf_try_grow( if (buf->ptr == git_buf__oom) return -1; + if (buf->asize == 0 && buf->size != 0) + return GIT_EINVALIDSPEC; + if (!target_size) target_size = buf->size; @@ -82,9 +85,6 @@ int git_buf_try_grow( return -1; } - if (preserve_external && !buf->asize && buf->ptr != NULL && buf->size > 0) - memcpy(new_ptr, buf->ptr, min(buf->size, new_size)); - buf->asize = new_size; buf->ptr = new_ptr; @@ -98,7 +98,7 @@ int git_buf_try_grow( int git_buf_grow(git_buf *buffer, size_t target_size) { - return git_buf_try_grow(buffer, target_size, true, true); + return git_buf_try_grow(buffer, target_size, true); } int git_buf_grow_by(git_buf *buffer, size_t additional_size) @@ -110,7 +110,7 @@ int git_buf_grow_by(git_buf *buffer, size_t additional_size) return -1; } - return git_buf_try_grow(buffer, newsize, true, true); + return git_buf_try_grow(buffer, newsize, true); } void git_buf_free(git_buf *buf) diff --git a/src/buffer.h b/src/buffer.h index 093ed9b60..e46ee5dd7 100644 --- a/src/buffer.h +++ b/src/buffer.h @@ -59,7 +59,7 @@ extern int git_buf_grow_by(git_buf *buffer, size_t additional_size); * into the newly allocated buffer. */ extern int git_buf_try_grow( - git_buf *buf, size_t target_size, bool mark_oom, bool preserve_external); + git_buf *buf, size_t target_size, bool mark_oom); /** * Sanitizes git_buf structures provided from user input. Users of the diff --git a/src/path.c b/src/path.c index c2c90e48d..2558058dd 100644 --- a/src/path.c +++ b/src/path.c @@ -640,7 +640,7 @@ static bool _check_dir_contents( /* leave base valid even if we could not make space for subdir */ if (GIT_ADD_SIZET_OVERFLOW(&alloc_size, dir_size, sub_size) || GIT_ADD_SIZET_OVERFLOW(&alloc_size, alloc_size, 2) || - git_buf_try_grow(dir, alloc_size, false, false) < 0) + git_buf_try_grow(dir, alloc_size, false) < 0) return false; /* save excursion */ @@ -847,7 +847,7 @@ int git_path_make_relative(git_buf *path, const char *parent) /* save the offset as we might realllocate the pointer */ offset = p - path->ptr; - if (git_buf_try_grow(path, alloclen, 1, 0) < 0) + if (git_buf_try_grow(path, alloclen, 1) < 0) return -1; p = path->ptr + offset; diff --git a/tests/core/buffer.c b/tests/core/buffer.c index fef37f8f7..cc2d7bbcc 100644 --- a/tests/core/buffer.c +++ b/tests/core/buffer.c @@ -1153,3 +1153,16 @@ void test_core_buffer__lf_and_crlf_conversions(void) git_buf_free(&src); git_buf_free(&tgt); } + +void test_core_buffer__dont_grow_borrowed(void) +{ + const char *somestring = "blah blah"; + git_buf buf = GIT_BUF_INIT; + + git_buf_attach_notowned(&buf, somestring, strlen(somestring) + 1); + cl_assert_equal_p(somestring, buf.ptr); + cl_assert_equal_i(0, buf.asize); + cl_assert_equal_i(strlen(somestring) + 1, buf.size); + + cl_git_fail_with(GIT_EINVALIDSPEC, git_buf_grow(&buf, 1024)); +} From bd470d0034e7b9217e4084a71d63dec64d2bfcd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 23 Jun 2015 15:21:12 +0200 Subject: [PATCH 2/5] blob: don't recomment using git_buf_grow We currently recommend using `git_buf_grow` in order to make a buffer make an owned copy of the memory it points to. This is not behaviour we should encourage, so remove this recommendation. The function itself is not changed, as we need to remain compatible, but it will be changed not to allow usage on borrowed buffers. --- include/git2/blob.h | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/include/git2/blob.h b/include/git2/blob.h index c24ff7e7f..4a6d8e50a 100644 --- a/include/git2/blob.h +++ b/include/git2/blob.h @@ -107,12 +107,10 @@ GIT_EXTERN(git_off_t) git_blob_rawsize(const git_blob *blob); * The output is written into a `git_buf` which the caller must free * when done (via `git_buf_free`). * - * If no filters need to be applied, then the `out` buffer will just be - * populated with a pointer to the raw content of the blob. In that case, - * be careful to *not* free the blob until done with the buffer. To keep - * the data detached from the blob, call `git_buf_grow` on the buffer - * with a `want_size` of 0 and the buffer will be reallocated to be - * detached from the blob. + * If no filters need to be applied, then the `out` buffer will just + * be populated with a pointer to the raw content of the blob. In + * that case, be careful to *not* free the blob until done with the + * buffer or copy it into memory you own. * * @param out The git_buf to be filled in * @param blob Pointer to the blob From 189aad45af733316ddecb01e750daed5f792fa07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 24 Jun 2015 19:32:07 +0200 Subject: [PATCH 3/5] errors: introduce EINVALID We've been using EINVALIDSPEC for a while to mean this, but that name is too specific. Introduce this to be more explicit. --- include/git2/errors.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/git2/errors.h b/include/git2/errors.h index dda0f8a57..5ff0f35b5 100644 --- a/include/git2/errors.h +++ b/include/git2/errors.h @@ -46,6 +46,7 @@ typedef enum { GIT_EAPPLIED = -18, /**< Patch/merge has already been applied */ GIT_EPEEL = -19, /**< The requested peel operation is not possible */ GIT_EEOF = -20, /**< Unexpected EOF */ + GIT_EINVALID = -21, /**< Invalid operation or input */ GIT_PASSTHROUGH = -30, /**< Internal only */ GIT_ITEROVER = -31, /**< Signals end of iteration with iterator */ From a65992355dac71490f4b22b3f2c0d55a1beca01b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 24 Jun 2015 19:32:56 +0200 Subject: [PATCH 4/5] buffer: make use of EINVALID for growing a borrowed buffer This explains more closely what happens. While here, set an error message. --- src/buffer.c | 6 ++++-- tests/core/buffer.c | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/buffer.c b/src/buffer.c index c066d8e84..1a5809cca 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -41,8 +41,10 @@ int git_buf_try_grow( if (buf->ptr == git_buf__oom) return -1; - if (buf->asize == 0 && buf->size != 0) - return GIT_EINVALIDSPEC; + if (buf->asize == 0 && buf->size != 0) { + giterr_set(GITERR_INVALID, "cannot grow a borrowed buffer"); + return GIT_EINVALID; + } if (!target_size) target_size = buf->size; diff --git a/tests/core/buffer.c b/tests/core/buffer.c index cc2d7bbcc..0e7026a9c 100644 --- a/tests/core/buffer.c +++ b/tests/core/buffer.c @@ -1164,5 +1164,5 @@ void test_core_buffer__dont_grow_borrowed(void) cl_assert_equal_i(0, buf.asize); cl_assert_equal_i(strlen(somestring) + 1, buf.size); - cl_git_fail_with(GIT_EINVALIDSPEC, git_buf_grow(&buf, 1024)); + cl_git_fail_with(GIT_EINVALID, git_buf_grow(&buf, 1024)); } From 3cf91d98e28f2519e69c9ce77a6d6454adc713d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 24 Jun 2015 20:21:54 +0200 Subject: [PATCH 5/5] Add CHANGELOG entries --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1340f78f9..2c7aa3164 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -124,6 +124,9 @@ support for HTTPS connections insead of OpenSSL. configuration of the server, and tools can use this to show messages about failing to communicate with the server. +* A new error code `GIT_EINVALID` indicates that an argument to a + function is invalid, or an invalid operation was requested. + * `git_diff_index_to_workdir()` and `git_diff_tree_to_index()` will now produce deltas of type `GIT_DELTA_CONFLICTED` to indicate that the index side of the delta is a conflict. @@ -264,6 +267,9 @@ support for HTTPS connections insead of OpenSSL. * `GIT_EMERGECONFLICT` is now `GIT_ECONFLICT`, which more accurately describes the nature of the error. +* It is no longer allowed to call `git_buf_grow()` on buffers + borrowing the memory they point to. + v0.22 ------