filebuf: add GIT_FILEBUF_INIT and protect multiple opens and cleanups

Update all stack allocations of git_filebuf to use GIT_FILEBUF_INIT
and make git_filebuf_open and git_filebuf_cleanup safe to be called
multiple times on the same buffer.

Signed-off-by: Vicent Marti <tanoku@gmail.com>

.gitignore

@@ -23,3 +23,4 @@ msvc/Release/
 CMake*
 *.cmake
 .DS_Store
+*~

CMakeLists.txt

@@ -64,7 +64,7 @@ IF (MSVC)
 	SET(CMAKE_C_FLAGS_RELEASE "/MT /O2")
 	SET(WIN_RC "src/win32/git2.rc")
 ELSE ()
-	SET(CMAKE_C_FLAGS "-O2 -g -Wall -Wextra -Wstrict-aliasing=2 -Wstrict-prototypes -Wmissing-prototypes ${CMAKE_C_FLAGS}")
+	SET(CMAKE_C_FLAGS "-O2 -g -Wall -Wextra -Wno-missing-field-initializers -Wstrict-aliasing=2 -Wstrict-prototypes -Wmissing-prototypes ${CMAKE_C_FLAGS}")
 	SET(CMAKE_C_FLAGS_DEBUG "-O0 -g")
 	IF (NOT MINGW) # MinGW always does PIC and complains if we tell it to
 		SET(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fPIC")

src/config_file.c

@@ -877,7 +877,7 @@ static int config_write(diskfile_backend *cfg, cvar_t *var)
 	int section_matches = 0, last_section_matched = 0;
 	char *current_section = NULL;
 	char *var_name, *var_value, *data_start;
-	git_filebuf file;
+	git_filebuf file = GIT_FILEBUF_INIT;
 	const char *pre_end = NULL, *post_start = NULL;
 
 	/* We need to read in our own config file */

src/fetch.c

@@ -138,7 +138,7 @@ int git_fetch_download_pack(char **out, git_remote *remote)
 int git_fetch__download_pack(char **out, const char *buffered, size_t buffered_size,
                              GIT_SOCKET fd, git_repository *repo)
 {
-	git_filebuf file;
+	git_filebuf file = GIT_FILEBUF_INIT;
 	int error;
 	char buff[1024], path[GIT_PATH_MAX];
 	static const char suff[] = "/objects/pack/pack-received";

src/filebuf.c

@@ -66,13 +66,22 @@ void git_filebuf_cleanup(git_filebuf *file)
 	if (file->digest)
 		git_hash_free_ctx(file->digest);
 
-	git__free(file->buffer);
-	git__free(file->z_buf);
+	if (file->buffer)
+		git__free(file->buffer);
 
-	deflateEnd(&file->zs);
+	/* use the presence of z_buf to decide if we need to deflateEnd */
+	if (file->z_buf) {
+		git__free(file->z_buf);
+		deflateEnd(&file->zs);
+	}
 
-	git__free(file->path_original);
-	git__free(file->path_lock);
+	if (file->path_original)
+		git__free(file->path_original);
+	if (file->path_lock)
+		git__free(file->path_lock);
+
+	memset(file, 0x0, sizeof(git_filebuf));
+	file->fd = -1;
 }
 
 GIT_INLINE(int) flush_buffer(git_filebuf *file)
@@ -137,6 +146,9 @@ int git_filebuf_open(git_filebuf *file, const char *path, int flags)
 
 	assert(file && path);
 
+	if (file->buffer)
+		return git__throw(GIT_EINVALIDARGS, "Tried to reopen an open filebuf");
+
 	memset(file, 0x0, sizeof(git_filebuf));
 
 	file->buf_size = WRITE_BUFFER_SIZE;

src/filebuf.h

@@ -44,6 +44,21 @@ struct git_filebuf {
 
 typedef struct git_filebuf git_filebuf;
 
+#define GIT_FILEBUF_INIT {0}
+
+/* The git_filebuf object lifecycle is:
+ * - Allocate git_filebuf, preferably using GIT_FILEBUF_INIT.
+ * - Call git_filebuf_open() to initialize the filebuf for use.
+ * - Make as many calls to git_filebuf_write(), git_filebuf_printf(),
+ *   git_filebuf_reserve() as you like.
+ * - While you are writing, you may call git_filebuf_hash() to get
+ *   the hash of all you have written so far.
+ * - To close the git_filebuf, you may call git_filebuf_commit() or
+ *   git_filebuf_commit_at() to save the file, or
+ *   git_filebuf_cleanup() to abandon the file.  All of these will
+ *   clear the git_filebuf object.
+ */
+
 int git_filebuf_write(git_filebuf *lock, const void *buff, size_t len);
 int git_filebuf_reserve(git_filebuf *file, void **buff, size_t len);
 int git_filebuf_printf(git_filebuf *file, const char *format, ...) GIT_FORMAT_PRINTF(2, 3);

src/index.c

@@ -248,7 +248,7 @@ int git_index_read(git_index *index)
 
 int git_index_write(git_index *index)
 {
-	git_filebuf file;
+	git_filebuf file = GIT_FILEBUF_INIT;
 	struct stat indexst;
 	int error;
 

src/odb_loose.c

@@ -769,7 +769,7 @@ static int loose_backend__write(git_oid *oid, git_odb_backend *_backend, const v
 {
 	int error, header_len;
 	char final_path[GIT_PATH_MAX], header[64];
-	git_filebuf fbuf;
+	git_filebuf fbuf = GIT_FILEBUF_INIT;
 	loose_backend *backend;
 
 	backend = (loose_backend *)_backend;

src/reflog.c

@@ -41,7 +41,7 @@ static int reflog_write(const char *log_path, const char *oid_old,
 {
 	int error;
 	git_buf log = GIT_BUF_INIT;
-	git_filebuf fbuf;
+	git_filebuf fbuf = GIT_FILEBUF_INIT;
 
 	assert(log_path && oid_old && oid_new && committer);
 

src/refs.c

@@ -285,7 +285,7 @@ cleanup:
 
 static int loose_write(git_reference *ref)
 {
-	git_filebuf file;
+	git_filebuf file = GIT_FILEBUF_INIT;
 	char ref_path[GIT_PATH_MAX];
 	int error;
 	struct stat st;
@@ -744,7 +744,7 @@ static int packed_sort(const void *a, const void *b)
  */
 static int packed_write(git_repository *repo)
 {
-	git_filebuf pack_file;
+	git_filebuf pack_file = GIT_FILEBUF_INIT;
 	int error;
 	unsigned int i;
 	char pack_file_path[GIT_PATH_MAX];

tests-clay/clay.h

@@ -70,6 +70,9 @@ extern void test_core_dirent__traverse_weird_filenames(void);
 extern void test_core_filebuf__0(void);
 extern void test_core_filebuf__1(void);
 extern void test_core_filebuf__2(void);
+extern void test_core_filebuf__3(void);
+extern void test_core_filebuf__4(void);
+extern void test_core_filebuf__5(void);
 extern void test_core_oid__initialize(void);
 extern void test_core_oid__streq(void);
 extern void test_core_path__0(void);

tests-clay/clay_main.c

@@ -121,7 +121,10 @@ static const struct clay_func _clay_cb_core_dirent[] = {
 static const struct clay_func _clay_cb_core_filebuf[] = {
     {"0", &test_core_filebuf__0},
 	{"1", &test_core_filebuf__1},
-	{"2", &test_core_filebuf__2}
+	{"2", &test_core_filebuf__2},
+	{"3", &test_core_filebuf__3},
+	{"4", &test_core_filebuf__4},
+	{"5", &test_core_filebuf__5}
 };
 static const struct clay_func _clay_cb_core_oid[] = {
     {"streq", &test_core_oid__streq}
@@ -241,7 +244,7 @@ static const struct clay_suite _clay_suites[] = {
         "core::filebuf",
         {NULL, NULL},
         {NULL, NULL},
-        _clay_cb_core_filebuf, 3
+        _clay_cb_core_filebuf, 6
     },
 	{
         "core::oid",
@@ -360,7 +363,7 @@ static const struct clay_suite _clay_suites[] = {
 };
 
 static size_t _clay_suite_count = 23;
-static size_t _clay_callback_count = 67;
+static size_t _clay_callback_count = 70;
 
 /* Core test functions */
 static void

tests-clay/core/filebuf.c

@@ -4,7 +4,7 @@
 /* make sure git_filebuf_open doesn't delete an existing lock */
 void test_core_filebuf__0(void)
 {
-	git_filebuf file;
+	git_filebuf file = GIT_FILEBUF_INIT;
 	int fd;
 	char test[] = "test", testlock[] = "test.lock";
 
@@ -23,7 +23,7 @@ void test_core_filebuf__0(void)
 /* make sure GIT_FILEBUF_APPEND works as expected */
 void test_core_filebuf__1(void)
 {
-	git_filebuf file;
+	git_filebuf file = GIT_FILEBUF_INIT;
 	int fd;
 	char test[] = "test";
 
@@ -43,7 +43,7 @@ void test_core_filebuf__1(void)
 /* make sure git_filebuf_write writes large buffer correctly */
 void test_core_filebuf__2(void)
 {
-	git_filebuf file;
+	git_filebuf file = GIT_FILEBUF_INIT;
 	char test[] = "test";
 	unsigned char buf[4096 * 4]; /* 2 * WRITE_BUFFER_SIZE */
 
@@ -56,3 +56,51 @@ void test_core_filebuf__2(void)
 	cl_must_pass(p_unlink(test));
 }
 
+
+/* make sure git_filebuf_open won't reopen an open buffer */
+void test_core_filebuf__3(void)
+{
+	git_filebuf file = GIT_FILEBUF_INIT;
+	char test[] = "test";
+
+	cl_git_pass(git_filebuf_open(&file, test, 0));
+	cl_git_fail(git_filebuf_open(&file, test, 0));
+
+	git_filebuf_cleanup(&file);
+}
+
+
+/* make sure git_filebuf_cleanup clears the buffer */
+void test_core_filebuf__4(void)
+{
+	git_filebuf file = GIT_FILEBUF_INIT;
+	char test[] = "test";
+
+	cl_assert(file.buffer == NULL);
+
+	cl_git_pass(git_filebuf_open(&file, test, 0));
+	cl_assert(file.buffer != NULL);
+
+	git_filebuf_cleanup(&file);
+	cl_assert(file.buffer == NULL);
+}
+
+
+/* make sure git_filebuf_commit clears the buffer */
+void test_core_filebuf__5(void)
+{
+	git_filebuf file = GIT_FILEBUF_INIT;
+	char test[] = "test";
+
+	cl_assert(file.buffer == NULL);
+
+	cl_git_pass(git_filebuf_open(&file, test, 0));
+	cl_assert(file.buffer != NULL);
+	cl_git_pass(git_filebuf_printf(&file, "%s\n", "libgit2 rocks"));
+	cl_assert(file.buffer != NULL);
+
+	cl_git_pass(git_filebuf_commit(&file, 0666));
+	cl_assert(file.buffer == NULL);
+
+	cl_must_pass(p_unlink(test));
+}

tests/t00-core.c

@@ -462,7 +462,7 @@ BEGIN_TEST(dirent4, "make sure that strange looking filenames ('..c') are traver
 END_TEST
 
 BEGIN_TEST(filebuf0, "make sure git_filebuf_open doesn't delete an existing lock")
-	git_filebuf file;
+	git_filebuf file = GIT_FILEBUF_INIT;
 	int fd;
 	char test[] = "test", testlock[] = "test.lock";
 
@@ -475,7 +475,7 @@ BEGIN_TEST(filebuf0, "make sure git_filebuf_open doesn't delete an existing lock
 END_TEST
 
 BEGIN_TEST(filebuf1, "make sure GIT_FILEBUF_APPEND works as expected")
-	git_filebuf file;
+	git_filebuf file = GIT_FILEBUF_INIT;
 	int fd;
 	char test[] = "test";
 
@@ -492,7 +492,7 @@ BEGIN_TEST(filebuf1, "make sure GIT_FILEBUF_APPEND works as expected")
 END_TEST
 
 BEGIN_TEST(filebuf2, "make sure git_filebuf_write writes large buffer correctly")
-	git_filebuf file;
+	git_filebuf file = GIT_FILEBUF_INIT;
 	char test[] = "test";
 	unsigned char buf[4096 * 4]; /* 2 * WRITE_BUFFER_SIZE */
 

tests/t06-index.c

@@ -163,7 +163,7 @@ END_TEST
 
 BEGIN_TEST(add0, "add a new file to the index")
 	git_index *index;
-	git_filebuf file;
+	git_filebuf file = GIT_FILEBUF_INIT;
 	git_repository *repo;
 	git_index_entry *entry;
 	git_oid id1;

tests/t15-config.c

@@ -306,7 +306,7 @@ END_TEST
 BEGIN_TEST(config16, "add a variable in a new section")
 	git_config *cfg;
 	int32_t i;
-	git_filebuf buf;
+	git_filebuf buf = GIT_FILEBUF_INIT;
 
 	/* By freeing the config, we make sure we flush the values  */
 	must_pass(git_config_open_ondisk(&cfg, CONFIG_BASE "/config10"));