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.

diff --git a/src/clone.c b/src/clone.c
index 070daf94dd27571b3489e40ebc878aeb31b3bdd2..6b4b7ae530ca15c59f6e366b37acb09e9a6ae7b2 100644 (file)
--- 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 2b1dedc01463a34a9d25c0f49f10e77c4ae5168a..6dca36fbd4d2220dc12e00824a74173772968cb8 100644 (file)
--- 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 973326c00cda7de9d375fc3ea66e3a3745e55472..91acc354146df96f327ce1d11bd98a264210b838 100644 (file)
--- 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 73f0b3d266914706c32acf8e22f5381a5a110755..e424698bb96adf8d06a39ebb80c5081d11b2fcb4 100644 (file)
--- 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 cf51a340db89b33aa74419247113ac0780f58292..900ffdcaa8b3ebd89b5dc05fd6c8a26c68df6c92 100644 (file)
--- 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 25bbbd5f6dc454aa005072b58d59ef55e4f41828..ab18951a667403347398043f053e514be92f1ee2 100644 (file)
--- 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");