Be more careful with user-supplied buffersrb/fix-2333

This adds in missing calls to `git_buf_sanitize` and fixes a
number of places where `git_buf` APIs could inadvertently write
NUL terminator bytes into invalid buffers.  This also changes the
behavior of `git_buf_sanitize` to NUL terminate a buffer if it can
and of `git_buf_shorten` to do nothing if it can.

Adds tests of filtering code with zeroed (i.e. unsanitized) buffer
which was previously triggering a segfault.

7 files changed, 46 insertions, 23 deletions

diff --git a/src/buffer.c b/src/buffer.c
index 5169c3e09..b8f8660ed 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -104,17 +104,20 @@ void git_buf_free(git_buf *buf)
 void git_buf_sanitize(git_buf *buf)
 {
 	if (buf->ptr == NULL) {
-		assert (buf->size == 0 && buf->asize == 0);
+		assert(buf->size == 0 && buf->asize == 0);
 		buf->ptr = git_buf__initbuf;
-	}
+	} else if (buf->asize > buf->size)
+		buf->ptr[buf->size] = '\0';
 }
 
 void git_buf_clear(git_buf *buf)
 {
 	buf->size = 0;
 
-	if (!buf->ptr)
+	if (!buf->ptr) {
 		buf->ptr = git_buf__initbuf;
+		buf->asize = 0;
+	}
 
 	if (buf->asize > 0)
 		buf->ptr[0] = '\0';
@@ -129,8 +132,11 @@ int git_buf_set(git_buf *buf, const void *data, size_t len)
 			ENSURE_SIZE(buf, len + 1);
 			memmove(buf->ptr, data, len);
 		}
+
 		buf->size = len;
-		buf->ptr[buf->size] = '\0';
+		if (buf->asize > buf->size)
+			buf->ptr[buf->size] = '\0';
+
 	}
 	return 0;
 }
@@ -326,19 +332,20 @@ void git_buf_consume(git_buf *buf, const char *end)
 
 void git_buf_truncate(git_buf *buf, size_t len)
 {
-	if (len < buf->size) {
-		buf->size = len;
+	if (len >= buf->size)
+		return;
+
+	buf->size = len;
+	if (buf->size < buf->asize)
 		buf->ptr[buf->size] = '\0';
-	}
 }
 
 void git_buf_shorten(git_buf *buf, size_t amount)
 {
-	if (amount > buf->size)
-		amount = buf->size;
-
-	buf->size = buf->size - amount;
-	buf->ptr[buf->size] = '\0';
+	if (buf->size > amount)
+		git_buf_truncate(buf, buf->size - amount);
+	else
+		git_buf_clear(buf);
 }
 
 void git_buf_rtruncate_at_char(git_buf *buf, char separator)
@@ -574,7 +581,8 @@ void git_buf_rtrim(git_buf *buf)
 		buf->size--;
 	}
 
-	buf->ptr[buf->size] = '\0';
+	if (buf->asize > buf->size)
+		buf->ptr[buf->size] = '\0';
 }
 
 int git_buf_cmp(const git_buf *a, const git_buf *b)
@@ -598,8 +606,7 @@ int git_buf_splice(
 	/* Ported from git.git
 	 * https://github.com/git/git/blob/16eed7c/strbuf.c#L159-176
 	 */
-	if (git_buf_grow(buf, git_buf_len(buf) + nb_to_insert - nb_to_remove) < 0)
-		return -1;
+	ENSURE_SIZE(buf, buf->size + nb_to_insert - nb_to_insert + 1);
 
 	memmove(buf->ptr + where + nb_to_insert,
 			buf->ptr + where + nb_to_remove,
diff --git a/src/config.c b/src/config.c
index f9d697197..4dddab6df 100644
--- a/src/config.c
+++ b/src/config.c
@@ -967,16 +967,19 @@ void git_config_iterator_free(git_config_iterator *iter)
 
 int git_config_find_global(git_buf *path)
 {
+	git_buf_sanitize(path);
 	return git_sysdir_find_global_file(path, GIT_CONFIG_FILENAME_GLOBAL);
 }
 
 int git_config_find_xdg(git_buf *path)
 {
+	git_buf_sanitize(path);
 	return git_sysdir_find_xdg_file(path, GIT_CONFIG_FILENAME_XDG);
 }
 
 int git_config_find_system(git_buf *path)
 {
+	git_buf_sanitize(path);
 	return git_sysdir_find_system_file(path, GIT_CONFIG_FILENAME_SYSTEM);
 }
 
diff --git a/src/diff_print.c b/src/diff_print.c
index 07c1f8577..08e1e7f90 100644
--- a/src/diff_print.c
+++ b/src/diff_print.c
@@ -597,9 +597,9 @@ int git_diff_print_callback__to_file_handle(
 }
 
 /* print a git_patch to a git_buf */
-int git_patch_to_buf(
-	git_buf *out,
-	git_patch *patch)
+int git_patch_to_buf(git_buf *out, git_patch *patch)
 {
+	assert(out && patch);
+	git_buf_sanitize(out);
 	return git_patch_print(patch, git_diff_print_callback__to_buf, out);
 }
diff --git a/src/filter.c b/src/filter.c
index b2f57964a..3c36aaf36 100644
--- a/src/filter.c
+++ b/src/filter.c
@@ -578,6 +578,9 @@ int git_filter_list_apply_to_data(
 	git_buf *dbuffer[2], local = GIT_BUF_INIT;
 	unsigned int si = 0;
 
+	git_buf_sanitize(tgt);
+	git_buf_sanitize(src);
+
 	if (!fl)
 		return filter_list_out_buffer_from_raw(tgt, src->ptr, src->size);
 
@@ -613,7 +616,7 @@ int git_filter_list_apply_to_data(
 			/* PASSTHROUGH means filter decided not to process the buffer */
 			error = 0;
 		} else if (!error) {
-			git_buf_shorten(dbuffer[di], 0); /* force NUL termination */
+			git_buf_sanitize(dbuffer[di]); /* force NUL termination */
 			si = di; /* swap buffers */
 		} else {
 			tgt->size = 0;
diff --git a/src/pack-objects.c b/src/pack-objects.c
index ace8afd17..b50338578 100644
--- a/src/pack-objects.c
+++ b/src/pack-objects.c
@@ -1276,6 +1276,7 @@ int git_packbuilder_foreach(git_packbuilder *pb, int (*cb)(void *buf, size_t siz
 int git_packbuilder_write_buf(git_buf *buf, git_packbuilder *pb)
 {
 	PREPARE_PACK;
+	git_buf_sanitize(buf);
 	return write_pack(pb, &write_pack_buf, buf);
 }
 
diff --git a/src/submodule.c b/src/submodule.c
index 5ddbfe828..b1291df8e 100644
--- a/src/submodule.c
+++ b/src/submodule.c
@@ -641,7 +641,9 @@ int git_submodule_resolve_url(git_buf *out, git_repository *repo, const char *ur
 {
 	int error = 0;
 
-	assert(url);
+	assert(out && repo && url);
+
+	git_buf_sanitize(out);
 
 	if (git_path_is_relative(url)) {
 		if (!(error = get_url_base(out, repo)))
diff --git a/tests/object/blob/filter.c b/tests/object/blob/filter.c
index 0b2d6bf9e..3cc8fdc3d 100644
--- a/tests/object/blob/filter.c
+++ b/tests/object/blob/filter.c
@@ -112,7 +112,7 @@ void test_object_blob_filter__to_odb(void)
 	git_config *cfg;
 	int i;
 	git_blob *blob;
-	git_buf out = GIT_BUF_INIT;
+	git_buf out = GIT_BUF_INIT, zeroed;
 
 	cl_git_pass(git_repository_config(&cfg, g_repo));
 	cl_assert(cfg);
@@ -127,13 +127,20 @@ void test_object_blob_filter__to_odb(void)
 	for (i = 0; i < CRLF_NUM_TEST_OBJECTS; i++) {
 		cl_git_pass(git_blob_lookup(&blob, g_repo, &g_crlf_oids[i]));
 
+		/* try once with allocated blob */
 		cl_git_pass(git_filter_list_apply_to_blob(&out, fl, blob));
-
 		cl_assert_equal_sz(g_crlf_filtered[i].size, out.size);
-
 		cl_assert_equal_i(
 			0, memcmp(out.ptr, g_crlf_filtered[i].ptr, out.size));
 
+		/* try again with zeroed blob */
+		memset(&zeroed, 0, sizeof(zeroed));
+		cl_git_pass(git_filter_list_apply_to_blob(&zeroed, fl, blob));
+		cl_assert_equal_sz(g_crlf_filtered[i].size, zeroed.size);
+		cl_assert_equal_i(
+			0, memcmp(zeroed.ptr, g_crlf_filtered[i].ptr, zeroed.size));
+		git_buf_free(&zeroed);
+
 		git_blob_free(blob);
 	}