Merge pull request #3351 from ethomson/error_buf

Error handling: use buffers, improved OOM handling

9 files changed, 149 insertions, 72 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
@@ -114,18 +114,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.
  *
  * This function is public so that custom ODB backends and the like can
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 7a2600586..91acc3541 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)
@@ -116,45 +131,65 @@ void giterr_clear(void)
 #endif
 }
 
-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 *error_buf = &GIT_GLOBAL->error_buf;
 
-	assert(cpy);
+	memset(state, 0, sizeof(git_error_state));
 
-	if (!error)
-		return -1;
+	if (!error_code)
+		return 0;
 
-	cpy->message = error->message;
-	cpy->klass = error->klass;
+	state->error_code = error_code;
+	state->oom = (error == &g_git_oom_error);
 
-	error->message = NULL;
-	giterr_clear();
+	if (error) {
+		state->error_msg.klass = error->klass;
 
-	return 0;
-}
+		if (state->oom)
+			state->error_msg.message = g_git_oom_error.message;
+		else
+			state->error_msg.message = git_buf_detach(error_buf);
+	}
 
-const git_error *giterr_last(void)
-{
-	return GIT_GLOBAL->last_error;
+	giterr_clear();
+	return error_code;
 }
 
-int giterr_capture(git_error_state *state, int error_code)
+int giterr_state_restore(git_error_state *state)
 {
-	state->error_code = error_code;
-	if (error_code)
-		giterr_detach(&state->error_msg);
-	return error_code;
+	int ret = 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;
 }
 
-int giterr_restore(git_error_state *state)
+void giterr_state_free(git_error_state *state)
 {
-	if (state && state->error_code && state->error_msg.message)
-		set_error(state->error_msg.klass, state->error_msg.message);
-	else
-		giterr_clear();
+	if (!state)
+		return;
+
+	if (!state->oom)
+		git__free(state->error_msg.message);
 
-	return state ? state->error_code : 0;
+	memset(state, 0, sizeof(git_error_state));
 }
 
 int giterr_system_last(void)
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;
 
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 a06ec4abc..ab18951a6 100644
--- a/tests/core/errors.c
+++ b/tests/core/errors.c
@@ -93,23 +93,69 @@ 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};
+	const git_error *oom_error = NULL;
+
+	giterr_clear();
+
+	giterr_set_oom(); /* internal fn */
+	oom_error = giterr_last();
+	cl_assert(oom_error);
+
+	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_state_restore(&err_state);
+
+	cl_assert(giterr_last()->klass == GITERR_NOMEMORY);
+	cl_assert_(giterr_last() == oom_error, "static oom error not restored");
+
+	giterr_clear();
+}
+
 static int test_arraysize_multiply(size_t nelem, size_t size)
 {
 	size_t out;