From b762e576c6d0118664320f50be2e5810dbed4c15 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Thu, 17 Nov 2011 15:10:27 -0800 Subject: [PATCH] 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 --- .gitignore | 1 + CMakeLists.txt | 2 +- src/config_file.c | 2 +- src/fetch.c | 2 +- src/filebuf.c | 22 ++++++++++++---- src/filebuf.h | 15 +++++++++++ src/index.c | 2 +- src/odb_loose.c | 2 +- src/reflog.c | 2 +- src/refs.c | 4 +-- tests-clay/clay.h | 3 +++ tests-clay/clay_main.c | 9 ++++--- tests-clay/core/filebuf.c | 54 ++++++++++++++++++++++++++++++++++++--- tests/t00-core.c | 6 ++--- tests/t06-index.c | 2 +- tests/t15-config.c | 2 +- 16 files changed, 106 insertions(+), 24 deletions(-) diff --git a/.gitignore b/.gitignore index 87ba3f34f3c..6594f147873 100644 --- a/.gitignore +++ b/.gitignore @@ -23,3 +23,4 @@ msvc/Release/ CMake* *.cmake .DS_Store +*~ diff --git a/CMakeLists.txt b/CMakeLists.txt index 7655e0ba5ad..5505a96ca01 100644 --- a/CMakeLists.txt +++ b/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") diff --git a/src/config_file.c b/src/config_file.c index aec29d4e234..87a430759e7 100644 --- a/src/config_file.c +++ b/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 */ diff --git a/src/fetch.c b/src/fetch.c index af7dbaffdf1..a427329257e 100644 --- a/src/fetch.c +++ b/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"; diff --git a/src/filebuf.c b/src/filebuf.c index 199418032c6..6600bfa4bbb 100644 --- a/src/filebuf.c +++ b/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; diff --git a/src/filebuf.h b/src/filebuf.h index d08505e8dc4..6c283bc5c6d 100644 --- a/src/filebuf.h +++ b/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); diff --git a/src/index.c b/src/index.c index 1a9745a2c0e..aad117164fe 100644 --- a/src/index.c +++ b/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; diff --git a/src/odb_loose.c b/src/odb_loose.c index 57a0b0a8e8d..f1789e0719a 100644 --- a/src/odb_loose.c +++ b/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; diff --git a/src/reflog.c b/src/reflog.c index e0fa7a0603f..c136987b162 100644 --- a/src/reflog.c +++ b/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); diff --git a/src/refs.c b/src/refs.c index 569efbf7877..5a297a5167e 100644 --- a/src/refs.c +++ b/src/refs.c @@ -285,7 +285,7 @@ static int loose_lookup_to_packfile( 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]; diff --git a/tests-clay/clay.h b/tests-clay/clay.h index 2c687ee5a1b..812209be18c 100644 --- a/tests-clay/clay.h +++ b/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); diff --git a/tests-clay/clay_main.c b/tests-clay/clay_main.c index 16cc516fa45..6d964b1baad 100644 --- a/tests-clay/clay_main.c +++ b/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 diff --git a/tests-clay/core/filebuf.c b/tests-clay/core/filebuf.c index e1ecb27985d..5b233fe8e37 100644 --- a/tests-clay/core/filebuf.c +++ b/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)); +} diff --git a/tests/t00-core.c b/tests/t00-core.c index 94824b438d2..16a5c6f9363 100644 --- a/tests/t00-core.c +++ b/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 */ diff --git a/tests/t06-index.c b/tests/t06-index.c index 44562d004b4..7b0f05129b7 100644 --- a/tests/t06-index.c +++ b/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; diff --git a/tests/t15-config.c b/tests/t15-config.c index a97f820679d..9f0deb3e305 100644 --- a/tests/t15-config.c +++ b/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"));