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.

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");