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

@@ -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;

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

src/errors.c

@@ -131,53 +131,65 @@ void giterr_clear(void)
 #endif
 }
 
-static 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 = git_buf_detach(buf);
-	cpy->klass = error->klass;
-
-	if (error != &g_git_oom_error) {
-		error->message = NULL;
-	}
-	giterr_clear();
-
-	return 0;
-}
-
 const git_error *giterr_last(void)
 {
 	return GIT_GLOBAL->last_error;
 }
 
-int giterr_capture(git_error_state *state, int error_code)
+int giterr_state_capture(git_error_state *state, int error_code)
 {
+	git_error *error = GIT_GLOBAL->last_error;
+	git_buf *error_buf = &GIT_GLOBAL->error_buf;
+
+	memset(state, 0, sizeof(git_error_state));
+
+	if (!error_code)
+		return 0;
+
 	state->error_code = error_code;
-	if (error_code)
-		giterr_detach(&state->error_msg);
+	state->oom = (error == &g_git_oom_error);
+
+	if (error) {
+		state->error_msg.klass = error->klass;
+
+		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 error_code;
 }
 
-int giterr_restore(git_error_state *state)
+int giterr_state_restore(git_error_state *state)
 {
-	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();
+	int ret = 0;
 
-	return state ? state->error_code : 0;
+	giterr_clear();
+
+	if (state && state->error_msg.message) {
+		if (state->oom)
+			giterr_set_oom();
+		else
+			set_error(state->error_msg.klass, state->error_msg.message);
+
+		ret = state->error_code;
+		memset(state, 0, sizeof(git_error_state));
+	}
+
+	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)

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

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) {

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