Merge pull request #999 from carlosmn/cmn/strarray-dispose

Use a more explicit way of correctly disposing `git_strarray` when we're using ruby strings

Author: carlosmn

ext/rugged/rugged.c

@@ -448,14 +448,23 @@ void rugged_rb_ary_to_strarray(VALUE rb_array, git_strarray *str_array)
 		Check_Type(rb_ary_entry(rb_array, i), T_STRING);
 
 	str_array->count = RARRAY_LEN(rb_array);
-	str_array->strings = xmalloc(str_array->count * sizeof(char *));
+	str_array->strings = xcalloc(str_array->count, sizeof(char *));
 
 	for (i = 0; i < RARRAY_LEN(rb_array); ++i) {
 		VALUE rb_string = rb_ary_entry(rb_array, i);
 		str_array->strings[i] = StringValueCStr(rb_string);
 	}
 }
 
+void rugged_strarray_dispose(git_strarray *str_array)
+{
+	/* We only free this one pointer as the strings inside are owned by ruby */
+	xfree(str_array->strings);
+
+	str_array->strings = NULL;
+	str_array->count = 0;
+}
+
 void rugged_parse_merge_file_options(git_merge_file_options *opts, VALUE rb_options)
 {
 	VALUE rb_value;

ext/rugged/rugged.h

@@ -101,7 +101,27 @@ const char * rugged_refname_from_string_or_ref(VALUE rb_name_or_ref);
 
 VALUE rugged_signature_from_buffer(const char *buffer, const char *encoding_name);
 
+/**
+ * Convert the given array of strings to a git_strarray referencing the ruby C strings
+ *
+ * The git_strarray must be allocated somewhere (typically it's in the stack)
+ * and this function will allocate an array of strings and set all the values to
+ * the ruby-owned C string pointer for the corresponding ruby string.
+ *
+ * The pointer in the strings field is owend by us, the memory it points to is
+ * owned by ruby.
+ */
 void rugged_rb_ary_to_strarray(VALUE rb_array, git_strarray *str_array);
+
+/**
+ * Free a git_strarray allocated by rugged_rb_ary_to_strarray
+ *
+ * This is different from git_strarray_dispose because we use pointers into the
+ * ruby C strings and don't own the strings. On top of that the allocators could
+ * be different (though we set it to be the same one at the moment).
+ */
+void rugged_strarray_dispose(git_strarray *str_array);
+
 VALUE rugged_strarray_to_rb_ary(git_strarray *str_array);
 
 #define CALLABLE_OR_RAISE(ret, name) \

ext/rugged/rugged_commit.c

@@ -651,7 +651,7 @@ static VALUE rb_git_commit_to_mbox(int argc, VALUE *argv, VALUE self)
 
 	cleanup:
 
-	xfree(opts.pathspec.strings);
+	rugged_strarray_dispose(&opts.pathspec);
 	git_buf_dispose(&email_patch);
 	rugged_exception_check(error);
 

ext/rugged/rugged_diff.c

@@ -138,19 +138,12 @@ void rugged_parse_diff_options(git_diff_options *opts, VALUE rb_options)
 
 		rb_value = rb_hash_aref(rb_options, CSTR2SYM("paths"));
 		if (!NIL_P(rb_value)) {
-			int i;
+			/*
+			 * rugged_rb_ary_to_strarray allows single values and we
+			 * want to be more specific here
+			 */
 			Check_Type(rb_value, T_ARRAY);
-
-			for (i = 0; i < RARRAY_LEN(rb_value); ++i)
-				Check_Type(rb_ary_entry(rb_value, i), T_STRING);
-
-			opts->pathspec.count = RARRAY_LEN(rb_value);
-			opts->pathspec.strings = xmalloc(opts->pathspec.count * sizeof(char *));
-
-			for (i = 0; i < RARRAY_LEN(rb_value); ++i) {
-				VALUE rb_path = rb_ary_entry(rb_value, i);
-				opts->pathspec.strings[i] = StringValueCStr(rb_path);
-			}
+			rugged_rb_ary_to_strarray(rb_value, &opts->pathspec);
 		}
 
 		rb_value = rb_hash_aref(rb_options, CSTR2SYM("context_lines"));

ext/rugged/rugged_index.c

@@ -408,7 +408,7 @@ static VALUE rb_git_index_add_all(int argc, VALUE *argv, VALUE self)
 	error = git_index_add_all(index, &pathspecs, flags,
 		rb_block_given_p() ? rugged__index_matched_path_cb : NULL, &exception);
 
-	xfree(pathspecs.strings);
+	rugged_strarray_dispose(&pathspecs);
 
 	if (exception)
 		rb_jump_tag(exception);
@@ -458,7 +458,7 @@ static VALUE rb_git_index_update_all(int argc, VALUE *argv, VALUE self)
 	error = git_index_update_all(index, &pathspecs,
 		rb_block_given_p() ? rugged__index_matched_path_cb : NULL, &exception);
 
-	xfree(pathspecs.strings);
+	rugged_strarray_dispose(&pathspecs);
 
 	if (exception)
 		rb_jump_tag(exception);
@@ -504,7 +504,7 @@ static VALUE rb_git_index_remove_all(int argc, VALUE *argv, VALUE self)
 	error = git_index_remove_all(index, &pathspecs,
 		rb_block_given_p() ? rugged__index_matched_path_cb : NULL, &exception);
 
-	xfree(pathspecs.strings);
+	rugged_strarray_dispose(&pathspecs);
 
 	if (exception)
 		rb_jump_tag(exception);
@@ -705,7 +705,7 @@ static VALUE rb_git_diff_tree_to_index(VALUE self, VALUE rb_other, VALUE rb_opti
 	TypedData_Get_Struct(rb_other, git_tree, &rugged_object_type, other_tree);
 	error = git_diff_tree_to_index(&diff, repo, other_tree, index, &opts);
 
-	xfree(opts.pathspec.strings);
+	rugged_strarray_dispose(&opts.pathspec);
 	rugged_exception_check(error);
 
 	return rugged_diff_new(rb_cRuggedDiff, owner, diff);
@@ -728,7 +728,7 @@ static VALUE rb_git_diff_index_to_workdir(VALUE self, VALUE rb_options)
 
 	error = git_diff_index_to_workdir(&diff, repo, index, &opts);
 
-	xfree(opts.pathspec.strings);
+	rugged_strarray_dispose(&opts.pathspec);
 	rugged_exception_check(error);
 
 	return rugged_diff_new(rb_cRuggedDiff, owner, diff);

ext/rugged/rugged_remote.c

@@ -354,7 +354,7 @@ static VALUE rb_git_remote_ls(int argc, VALUE *argv, VALUE self)
 	cleanup:
 
 	git_remote_disconnect(remote);
-	xfree(custom_headers.strings);
+	rugged_strarray_dispose(&custom_headers);
 
 	if (payload.exception)
 		rb_jump_tag(payload.exception);
@@ -527,7 +527,7 @@ static VALUE rb_git_remote_check_connection(int argc, VALUE *argv, VALUE self)
 	git_remote *remote;
 	git_remote_callbacks callbacks = GIT_REMOTE_CALLBACKS_INIT;
 	git_proxy_options proxy_options = GIT_PROXY_OPTIONS_INIT;
-	git_strarray custom_headers = {0};
+	git_strarray custom_headers = { NULL, 0 };
 	struct rugged_remote_cb_payload payload = { Qnil, Qnil, Qnil, Qnil, Qnil, Qnil, Qnil, 0 };
 	VALUE rb_direction, rb_options;
 	ID id_direction;
@@ -552,7 +552,7 @@ static VALUE rb_git_remote_check_connection(int argc, VALUE *argv, VALUE self)
 	error = git_remote_connect(remote, direction, &callbacks, &proxy_options, &custom_headers);
 	git_remote_disconnect(remote);
 
-	xfree(custom_headers.strings);
+	rugged_strarray_dispose(&custom_headers);
 
 	if (payload.exception)
 		rb_jump_tag(payload.exception);
@@ -655,8 +655,8 @@ static VALUE rb_git_remote_fetch(int argc, VALUE *argv, VALUE self)
 
 	error = git_remote_fetch(remote, &refspecs, &opts, log_message);
 
-	xfree(refspecs.strings);
-	xfree(opts.custom_headers.strings);
+	rugged_strarray_dispose(&refspecs);
+	rugged_strarray_dispose(&opts.custom_headers);
 
 	if (payload.exception)
 		rb_jump_tag(payload.exception);
@@ -740,8 +740,8 @@ static VALUE rb_git_remote_push(int argc, VALUE *argv, VALUE self)
 
 	error = git_remote_push(remote, &refspecs, &opts);
 
-	xfree(refspecs.strings);
-	xfree(opts.custom_headers.strings);
+	rugged_strarray_dispose(&refspecs);
+	rugged_strarray_dispose(&opts.custom_headers);
 
 	if (payload.exception)
 		rb_jump_tag(payload.exception);

ext/rugged/rugged_repo.c

@@ -1893,7 +1893,7 @@ static VALUE rb_git_repo_reset_path(int argc, VALUE *argv, VALUE self)
 
 	error = git_reset_default(repo, target, &pathspecs);
 
-	xfree(pathspecs.strings);
+	rugged_strarray_dispose(&pathspecs);
 	git_object_free(target);
 
 	rugged_exception_check(error);
@@ -2391,7 +2391,7 @@ static VALUE rb_git_checkout_tree(int argc, VALUE *argv, VALUE self)
 	rugged_parse_checkout_options(&opts, rb_options);
 
 	error = git_checkout_tree(repo, treeish, &opts);
-	xfree(opts.paths.strings);
+	rugged_strarray_dispose(&opts.paths);
 
 	if ((payload = opts.notify_payload) != NULL) {
 		exception = payload->exception;
@@ -2439,7 +2439,7 @@ static VALUE rb_git_checkout_index(int argc, VALUE *argv, VALUE self)
 	rugged_parse_checkout_options(&opts, rb_options);
 
 	error = git_checkout_index(repo, index, &opts);
-	xfree(opts.paths.strings);
+	rugged_strarray_dispose(&opts.paths);
 
 	if ((payload = opts.notify_payload) != NULL) {
 		exception = payload->exception;
@@ -2482,7 +2482,7 @@ static VALUE rb_git_checkout_head(int argc, VALUE *argv, VALUE self)
 	rugged_parse_checkout_options(&opts, rb_options);
 
 	error = git_checkout_head(repo, &opts);
-	xfree(opts.paths.strings);
+	rugged_strarray_dispose(&opts.paths);
 
 	if ((payload = opts.notify_payload) != NULL) {
 		exception = payload->exception;

ext/rugged/rugged_tree.c

@@ -381,7 +381,7 @@ static VALUE rb_git_diff_tree_to_index(VALUE self, VALUE rb_repo, VALUE rb_self,
 
 	error = git_diff_tree_to_index(&diff, repo, tree, index, &opts);
 
-	xfree(opts.pathspec.strings);
+	rugged_strarray_dispose(&opts.pathspec);
 	rugged_exception_check(error);
 
 	return rugged_diff_new(rb_cRuggedDiff, rb_repo, diff);
@@ -430,7 +430,7 @@ static VALUE rb_git_diff_tree_to_tree(VALUE self, VALUE rb_repo, VALUE rb_tree,
 
 	diff = rb_thread_call_without_gvl(rb_git_diff_tree_to_tree_nogvl, &args, RUBY_UBF_PROCESS, NULL);
 
-	xfree(opts.pathspec.strings);
+	rugged_strarray_dispose(&opts.pathspec);
 	rugged_exception_check(args.error);
 
 	return rugged_diff_new(rb_cRuggedDiff, rb_repo, diff);
@@ -465,7 +465,7 @@ static VALUE rb_git_tree_diff_workdir(int argc, VALUE *argv, VALUE self)
 
 	error = git_diff_tree_to_workdir(&diff, repo, tree, &opts);
 
-	xfree(opts.pathspec.strings);
+	rugged_strarray_dispose(&opts.pathspec);
 	rugged_exception_check(error);
 
 	return rugged_diff_new(rb_cRuggedDiff, owner, diff);