Merge pull request #3246 from libgit2/cmn/dont-grow-borrowed

Don't allow growing borrowed buffers

CHANGELOG.md

@@ -127,6 +127,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.
@@ -267,6 +270,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
 ------
 

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

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 */

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,11 @@ int git_buf_try_grow(
 	if (buf->ptr == git_buf__oom)
 		return -1;
 
+	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;
 
@@ -82,9 +87,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 +100,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 +112,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)

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

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;
 

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_EINVALID, git_buf_grow(&buf, 1024));
+}