From f85fc367e01eb0f5918263b1fcdfc8b7d16065c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sun, 26 Jul 2015 21:12:00 +0200 Subject: error: store the error messages in a reusable buffer Instead of allocating a brand new buffer for each error string we want to store, we can use a per-thread buffer to store the error string and re-use the underlying storage. We already use the buffer to format the string, so this mostly makes that more direct. --- src/errors.c | 50 +++++++++++++++++++++++++++++++++----------------- src/global.c | 14 ++++++++------ src/global.h | 1 + 3 files changed, 42 insertions(+), 23 deletions(-) diff --git a/src/errors.c b/src/errors.c index 7a2600586..56fad009f 100644 --- a/src/errors.c +++ b/src/errors.c @@ -18,19 +18,30 @@ static git_error g_git_oom_error = { GITERR_NOMEMORY }; -static void set_error(int error_class, char *string) +static void set_error_from_buffer(int error_class) { git_error *error = &GIT_GLOBAL->error_t; + git_buf *buf = &GIT_GLOBAL->error_buf; - if (error->message != string) - git__free(error->message); - - error->message = string; + error->message = buf->ptr; error->klass = error_class; GIT_GLOBAL->last_error = error; } +static void set_error(int error_class, char *string) +{ + git_buf *buf = &GIT_GLOBAL->error_buf; + + git_buf_clear(buf); + if (string) { + git_buf_puts(buf, string); + git__free(string); + } + + set_error_from_buffer(error_class); +} + void giterr_set_oom(void) { GIT_GLOBAL->last_error = &g_git_oom_error; @@ -38,27 +49,28 @@ void giterr_set_oom(void) void giterr_set(int error_class, const char *string, ...) { - git_buf buf = GIT_BUF_INIT; va_list arglist; #ifdef GIT_WIN32 DWORD win32_error_code = (error_class == GITERR_OS) ? GetLastError() : 0; #endif int error_code = (error_class == GITERR_OS) ? errno : 0; + git_buf *buf = &GIT_GLOBAL->error_buf; + git_buf_clear(buf); if (string) { va_start(arglist, string); - git_buf_vprintf(&buf, string, arglist); + git_buf_vprintf(buf, string, arglist); va_end(arglist); if (error_class == GITERR_OS) - git_buf_PUTS(&buf, ": "); + git_buf_PUTS(buf, ": "); } if (error_class == GITERR_OS) { #ifdef GIT_WIN32 char * win32_error = git_win32_get_error_message(win32_error_code); if (win32_error) { - git_buf_puts(&buf, win32_error); + git_buf_puts(buf, win32_error); git__free(win32_error); SetLastError(0); @@ -66,26 +78,29 @@ void giterr_set(int error_class, const char *string, ...) else #endif if (error_code) - git_buf_puts(&buf, strerror(error_code)); + git_buf_puts(buf, strerror(error_code)); if (error_code) errno = 0; } - if (!git_buf_oom(&buf)) - set_error(error_class, git_buf_detach(&buf)); + if (!git_buf_oom(buf)) + set_error_from_buffer(error_class); } void giterr_set_str(int error_class, const char *string) { - char *message; + git_buf *buf = &GIT_GLOBAL->error_buf; assert(string); - message = git__strdup(string); + if (!string) + return; - if (message) - set_error(error_class, message); + git_buf_clear(buf); + git_buf_puts(buf, string); + if (!git_buf_oom(buf)) + set_error_from_buffer(error_class); } int giterr_set_regex(const regex_t *regex, int error_code) @@ -119,13 +134,14 @@ void giterr_clear(void) int giterr_detach(git_error *cpy) { git_error *error = GIT_GLOBAL->last_error; + git_buf *buf = &GIT_GLOBAL->error_buf; assert(cpy); if (!error) return -1; - cpy->message = error->message; + cpy->message = git_buf_detach(buf); cpy->klass = error->klass; error->message = NULL; diff --git a/src/global.c b/src/global.c index 37a47bd27..3d37ee4de 100644 --- a/src/global.c +++ b/src/global.c @@ -279,18 +279,19 @@ int git_libgit2_shutdown(void) git_global_st *git__global_state(void) { - void *ptr; + git_global_st *ptr; assert(git_atomic_get(&git__n_inits) > 0); if ((ptr = TlsGetValue(_tls_index)) != NULL) return ptr; - ptr = git__malloc(sizeof(git_global_st)); + ptr = git__calloc(1, sizeof(git_global_st)); if (!ptr) return NULL; - memset(ptr, 0x0, sizeof(git_global_st)); + git_buf_init(&ptr->error_buf, 0); + TlsSetValue(_tls_index, ptr); return ptr; } @@ -378,18 +379,18 @@ int git_libgit2_shutdown(void) git_global_st *git__global_state(void) { - void *ptr; + git_global_st *ptr; assert(git_atomic_get(&git__n_inits) > 0); if ((ptr = pthread_getspecific(_tls_key)) != NULL) return ptr; - ptr = git__malloc(sizeof(git_global_st)); + ptr = git__calloc(1, sizeof(git_global_st)); if (!ptr) return NULL; - memset(ptr, 0x0, sizeof(git_global_st)); + git_buf_init(&ptr->error_buf, 0); pthread_setspecific(_tls_key, ptr); return ptr; } @@ -407,6 +408,7 @@ int git_libgit2_init(void) ssl_inited = 1; } + git_buf_init(&__state.error_buf, 0); return git_atomic_inc(&git__n_inits); } diff --git a/src/global.h b/src/global.h index fdad6ba89..37e909ac6 100644 --- a/src/global.h +++ b/src/global.h @@ -14,6 +14,7 @@ typedef struct { git_error *last_error; git_error error_t; + git_buf error_buf; char oid_fmt[GIT_OID_HEXSZ+1]; } git_global_st; -- cgit v1.2.3 From 5ef4b86015309c157b20260905cb5d0c9bb47ca8 Mon Sep 17 00:00:00 2001 From: Michael Procter Date: Thu, 23 Jul 2015 13:16:19 +0100 Subject: Add failing test for capture/restore oom error --- tests/core/errors.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/core/errors.c b/tests/core/errors.c index a06ec4abc..6aceb30fd 100644 --- a/tests/core/errors.c +++ b/tests/core/errors.c @@ -110,6 +110,29 @@ void test_core_errors__restore(void) cl_assert_equal_s("Foo: bar", giterr_last()->message); } +void test_core_errors__restore_oom(void) +{ + git_error_state err_state = {0}; + const char *static_message = NULL; + + giterr_clear(); + + giterr_set_oom(); /* internal fn */ + static_message = giterr_last()->message; + + cl_assert_equal_i(-1, giterr_capture(&err_state, -1)); + + cl_assert(giterr_last() == NULL); + + cl_assert_(err_state.error_msg.message != static_message, "pointer to static buffer exposed"); + + giterr_restore(&err_state); + + cl_assert(giterr_last()->klass == GITERR_NOMEMORY); + + giterr_clear(); +} + static int test_arraysize_multiply(size_t nelem, size_t size) { size_t out; -- cgit v1.2.3 From c2f17bda074b2e5718456aed75fedd2196c8dc94 Mon Sep 17 00:00:00 2001 From: Michael Procter Date: Thu, 23 Jul 2015 13:17:08 +0100 Subject: Ensure static oom error message not detached Error messages that are detached are assumed to be dynamically allocated. Passing a pointer to the static oom error message can cause an attempt to free the static buffer later. This change checks if the oom error message is about to be detached and detaches a copy instead. --- src/errors.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/errors.c b/src/errors.c index 7a2600586..979602a2a 100644 --- a/src/errors.c +++ b/src/errors.c @@ -125,10 +125,14 @@ int giterr_detach(git_error *cpy) if (!error) return -1; - cpy->message = error->message; + if (error == &g_git_oom_error) { + cpy->message = git__strdup(error->message); + } else { + cpy->message = error->message; + error->message = NULL; + } cpy->klass = error->klass; - error->message = NULL; giterr_clear(); return 0; -- cgit v1.2.3 From 25dbcf34993cad3cdc3981f1ed394d3374fb640f Mon Sep 17 00:00:00 2001 From: Michael Procter Date: Mon, 27 Jul 2015 09:59:07 +0100 Subject: Make giterr_detach no longer public --- include/git2/errors.h | 12 ------------ src/errors.c | 2 +- 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/include/git2/errors.h b/include/git2/errors.h index e189e55f1..4698366d8 100644 --- a/include/git2/errors.h +++ b/include/git2/errors.h @@ -113,18 +113,6 @@ GIT_EXTERN(const git_error *) giterr_last(void); */ GIT_EXTERN(void) giterr_clear(void); -/** - * Get the last error data and clear it. - * - * This copies the last error into the given `git_error` struct - * and returns 0 if the copy was successful, leaving the error - * cleared as if `giterr_clear` had been called. - * - * If there was no existing error in the library, -1 will be returned - * and the contents of `cpy` will be left unmodified. - */ -GIT_EXTERN(int) giterr_detach(git_error *cpy); - /** * Set the error message string for this thread. * diff --git a/src/errors.c b/src/errors.c index 979602a2a..95c62176c 100644 --- a/src/errors.c +++ b/src/errors.c @@ -116,7 +116,7 @@ void giterr_clear(void) #endif } -int giterr_detach(git_error *cpy) +static int giterr_detach(git_error *cpy) { git_error *error = GIT_GLOBAL->last_error; -- cgit v1.2.3 From 0fcfb60dc4f5e6cfd91c902d844f5d8665a5c1a7 Mon Sep 17 00:00:00 2001 From: Michael Procter Date: Mon, 27 Jul 2015 10:10:18 +0100 Subject: Make giterr_restore aware of g_git_oom_error Allow restoring a previously captured oom error, by detecting when the captured message pointer points to the static oom error message. This means there is no need to strdup the message in giterr_detach. --- src/errors.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/errors.c b/src/errors.c index 95c62176c..f10430c10 100644 --- a/src/errors.c +++ b/src/errors.c @@ -125,14 +125,12 @@ static int giterr_detach(git_error *cpy) if (!error) return -1; - if (error == &g_git_oom_error) { - cpy->message = git__strdup(error->message); - } else { - cpy->message = error->message; - error->message = NULL; - } + cpy->message = error->message; cpy->klass = error->klass; + if (error != &g_git_oom_error) { + error->message = NULL; + } giterr_clear(); return 0; @@ -153,8 +151,13 @@ int giterr_capture(git_error_state *state, int error_code) int giterr_restore(git_error_state *state) { - if (state && state->error_code && state->error_msg.message) - set_error(state->error_msg.klass, state->error_msg.message); + if (state && state->error_code && state->error_msg.message) { + if (state->error_msg.message == g_git_oom_error.message) { + giterr_set_oom(); + } else { + set_error(state->error_msg.klass, state->error_msg.message); + } + } else giterr_clear(); -- cgit v1.2.3 From 988ea59443b71f4a07b19fff837ccaa1659dbcc0 Mon Sep 17 00:00:00 2001 From: Michael Procter Date: Mon, 27 Jul 2015 10:13:49 +0100 Subject: Test: check restored oom error points to static buffer --- tests/core/errors.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/core/errors.c b/tests/core/errors.c index 6aceb30fd..25bbbd5f6 100644 --- a/tests/core/errors.c +++ b/tests/core/errors.c @@ -113,22 +113,22 @@ void test_core_errors__restore(void) void test_core_errors__restore_oom(void) { git_error_state err_state = {0}; - const char *static_message = NULL; + const git_error *oom_error = NULL; giterr_clear(); giterr_set_oom(); /* internal fn */ - static_message = giterr_last()->message; + oom_error = giterr_last(); + cl_assert(oom_error); cl_assert_equal_i(-1, giterr_capture(&err_state, -1)); cl_assert(giterr_last() == NULL); - cl_assert_(err_state.error_msg.message != static_message, "pointer to static buffer exposed"); - giterr_restore(&err_state); cl_assert(giterr_last()->klass == GITERR_NOMEMORY); + cl_assert_(giterr_last() == oom_error, "static oom error not restored"); giterr_clear(); } -- cgit v1.2.3 From ef4857c2b3d4a61fd1d840199afc92eaf2e15345 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Mon, 3 Aug 2015 16:50:27 -0500 Subject: errors: tighten up git_error_state OOMs a bit more When an error state is an OOM, make sure that we treat is specially and do not try to free it. --- src/clone.c | 4 +-- src/common.h | 13 +++++++--- src/errors.c | 72 +++++++++++++++++++++++++++++++---------------------- src/index.c | 6 ++--- src/iterator.c | 4 +-- tests/core/errors.c | 33 ++++++++++++++++++++---- 6 files changed, 86 insertions(+), 46 deletions(-) diff --git a/src/clone.c b/src/clone.c index 070daf94d..6b4b7ae53 100644 --- a/src/clone.c +++ b/src/clone.c @@ -440,14 +440,14 @@ int git_clone( if (error != 0) { git_error_state last_error = {0}; - giterr_capture(&last_error, error); + giterr_state_capture(&last_error, error); git_repository_free(repo); repo = NULL; (void)git_futils_rmdir_r(local_path, NULL, rmdir_flags); - giterr_restore(&last_error); + giterr_state_restore(&last_error); } *out = repo; diff --git a/src/common.h b/src/common.h index 2b1dedc01..6dca36fbd 100644 --- a/src/common.h +++ b/src/common.h @@ -141,20 +141,25 @@ void giterr_system_set(int code); * Structure to preserve libgit2 error state */ typedef struct { - int error_code; + int error_code; + unsigned int oom : 1; git_error error_msg; } git_error_state; /** * Capture current error state to restore later, returning error code. - * If `error_code` is zero, this does nothing and returns zero. + * If `error_code` is zero, this does not clear the current error state. + * You must either restore this error state, or free it. */ -int giterr_capture(git_error_state *state, int error_code); +extern int giterr_state_capture(git_error_state *state, int error_code); /** * Restore error state to a previous value, returning saved error code. */ -int giterr_restore(git_error_state *state); +extern int giterr_state_restore(git_error_state *state); + +/** Free an error state. */ +extern void giterr_state_free(git_error_state *state); /** * Check a versioned structure for validity diff --git a/src/errors.c b/src/errors.c index 973326c00..91acc3541 100644 --- a/src/errors.c +++ b/src/errors.c @@ -131,53 +131,65 @@ void giterr_clear(void) #endif } -static int giterr_detach(git_error *cpy) +const git_error *giterr_last(void) +{ + return GIT_GLOBAL->last_error; +} + +int giterr_state_capture(git_error_state *state, int error_code) { git_error *error = GIT_GLOBAL->last_error; - git_buf *buf = &GIT_GLOBAL->error_buf; + git_buf *error_buf = &GIT_GLOBAL->error_buf; - assert(cpy); + memset(state, 0, sizeof(git_error_state)); - if (!error) - return -1; + if (!error_code) + return 0; + + state->error_code = error_code; + state->oom = (error == &g_git_oom_error); - cpy->message = git_buf_detach(buf); - cpy->klass = error->klass; + if (error) { + state->error_msg.klass = error->klass; - if (error != &g_git_oom_error) { - error->message = NULL; + if (state->oom) + state->error_msg.message = g_git_oom_error.message; + else + state->error_msg.message = git_buf_detach(error_buf); } - giterr_clear(); - return 0; + giterr_clear(); + return error_code; } -const git_error *giterr_last(void) +int giterr_state_restore(git_error_state *state) { - return GIT_GLOBAL->last_error; -} + int ret = 0; -int giterr_capture(git_error_state *state, int error_code) -{ - state->error_code = error_code; - if (error_code) - giterr_detach(&state->error_msg); - return error_code; -} + giterr_clear(); -int giterr_restore(git_error_state *state) -{ - if (state && state->error_code && state->error_msg.message) { - if (state->error_msg.message == g_git_oom_error.message) { + if (state && state->error_msg.message) { + if (state->oom) giterr_set_oom(); - } else { + else set_error(state->error_msg.klass, state->error_msg.message); - } + + ret = state->error_code; + memset(state, 0, sizeof(git_error_state)); } - else - giterr_clear(); - return state ? state->error_code : 0; + return ret; +} + +void giterr_state_free(git_error_state *state) +{ + if (!state) + return; + + if (!state->oom) + git__free(state->error_msg.message); + + memset(state, 0, sizeof(git_error_state)); } int giterr_system_last(void) diff --git a/src/index.c b/src/index.c index 73f0b3d26..e424698bb 100644 --- a/src/index.c +++ b/src/index.c @@ -1286,13 +1286,13 @@ int git_index_add_bypath(git_index *index, const char *path) git_submodule *sm; git_error_state err; - giterr_capture(&err, ret); + giterr_state_capture(&err, ret); ret = git_submodule_lookup(&sm, INDEX_OWNER(index), path); if (ret == GIT_ENOTFOUND) - return giterr_restore(&err); + return giterr_state_restore(&err); - git__free(err.error_msg.message); + giterr_state_free(&err); /* * EEXISTS means that there is a repository at that path, but it's not known diff --git a/src/iterator.c b/src/iterator.c index cf51a340d..900ffdcaa 100644 --- a/src/iterator.c +++ b/src/iterator.c @@ -1120,7 +1120,7 @@ static int fs_iterator__expand_dir(fs_iterator *fi) if (error < 0) { git_error_state last_error = { 0 }; - giterr_capture(&last_error, error); + giterr_state_capture(&last_error, error); /* these callbacks may clear the error message */ fs_iterator__free_frame(ff); @@ -1128,7 +1128,7 @@ static int fs_iterator__expand_dir(fs_iterator *fi) /* next time return value we skipped to */ fi->base.flags &= ~GIT_ITERATOR_FIRST_ACCESS; - return giterr_restore(&last_error); + return giterr_state_restore(&last_error); } if (ff->entries.length == 0) { diff --git a/tests/core/errors.c b/tests/core/errors.c index 25bbbd5f6..ab18951a6 100644 --- a/tests/core/errors.c +++ b/tests/core/errors.c @@ -93,23 +93,44 @@ void test_core_errors__restore(void) giterr_clear(); cl_assert(giterr_last() == NULL); - cl_assert_equal_i(0, giterr_capture(&err_state, 0)); + cl_assert_equal_i(0, giterr_state_capture(&err_state, 0)); memset(&err_state, 0x0, sizeof(git_error_state)); giterr_set(42, "Foo: %s", "bar"); - cl_assert_equal_i(-1, giterr_capture(&err_state, -1)); + cl_assert_equal_i(-1, giterr_state_capture(&err_state, -1)); cl_assert(giterr_last() == NULL); giterr_set(99, "Bar: %s", "foo"); - giterr_restore(&err_state); + giterr_state_restore(&err_state); cl_assert_equal_i(42, giterr_last()->klass); cl_assert_equal_s("Foo: bar", giterr_last()->message); } +void test_core_errors__free_state(void) +{ + git_error_state err_state = {0}; + + giterr_clear(); + + giterr_set(42, "Foo: %s", "bar"); + cl_assert_equal_i(-1, giterr_state_capture(&err_state, -1)); + + giterr_set(99, "Bar: %s", "foo"); + + giterr_state_free(&err_state); + + cl_assert_equal_i(99, giterr_last()->klass); + cl_assert_equal_s("Bar: foo", giterr_last()->message); + + giterr_state_restore(&err_state); + + cl_assert(giterr_last() == NULL); +} + void test_core_errors__restore_oom(void) { git_error_state err_state = {0}; @@ -121,11 +142,13 @@ void test_core_errors__restore_oom(void) oom_error = giterr_last(); cl_assert(oom_error); - cl_assert_equal_i(-1, giterr_capture(&err_state, -1)); + cl_assert_equal_i(-1, giterr_state_capture(&err_state, -1)); cl_assert(giterr_last() == NULL); + cl_assert_equal_i(GITERR_NOMEMORY, err_state.error_msg.klass); + cl_assert_equal_s("Out of memory", err_state.error_msg.message); - giterr_restore(&err_state); + giterr_state_restore(&err_state); cl_assert(giterr_last()->klass == GITERR_NOMEMORY); cl_assert_(giterr_last() == oom_error, "static oom error not restored"); -- cgit v1.2.3