Merge pull request libgit2#4196 from pks-t/pks/filter-segfault

filter: only close filter if it's been initialized correctly

Author: ethomson

src/filter.c

@@ -911,14 +911,19 @@ static int stream_list_init(
 				last_stream);
 
 		if (error < 0)
-			return error;
+			goto out;
 
 		git_vector_insert(streams, filter_stream);
 		last_stream = filter_stream;
 	}
 
-	*out = last_stream;
-	return 0;
+out:
+	if (error)
+		last_stream->close(last_stream);
+	else
+		*out = last_stream;
+
+	return error;
 }
 
 void stream_list_free(git_vector *streams)
@@ -943,12 +948,13 @@ int git_filter_list_stream_file(
 	git_vector filter_streams = GIT_VECTOR_INIT;
 	git_writestream *stream_start;
 	ssize_t readlen;
-	int fd = -1, error;
+	int fd = -1, error, initialized = 0;
 
 	if ((error = stream_list_init(
 			&stream_start, &filter_streams, filters, target)) < 0 ||
 		(error = git_path_join_unrooted(&abspath, path, base, NULL)) < 0)
 		goto done;
+	initialized = 1;
 
 	if ((fd = git_futils_open_ro(abspath.ptr)) < 0) {
 		error = fd;
@@ -960,13 +966,13 @@ int git_filter_list_stream_file(
 			goto done;
 	}
 
-	if (!readlen)
-		error = stream_start->close(stream_start);
-	else if (readlen < 0)
+	if (readlen < 0)
 		error = readlen;
 
-
 done:
+	if (initialized)
+		error |= stream_start->close(stream_start);
+
 	if (fd >= 0)
 		p_close(fd);
 	stream_list_free(&filter_streams);
@@ -981,20 +987,24 @@ int git_filter_list_stream_data(
 {
 	git_vector filter_streams = GIT_VECTOR_INIT;
 	git_writestream *stream_start;
-	int error = 0, close_error;
+	int error, initialized = 0;
 
 	git_buf_sanitize(data);
 
 	if ((error = stream_list_init(&stream_start, &filter_streams, filters, target)) < 0)
 		goto out;
+	initialized = 1;
 
-	error = stream_start->write(stream_start, data->ptr, data->size);
+	if ((error = stream_start->write(
+			stream_start, data->ptr, data->size)) < 0)
+		goto out;
 
 out:
-	close_error = stream_start->close(stream_start);
+	if (initialized)
+		error |= stream_start->close(stream_start);
+
 	stream_list_free(&filter_streams);
-	/* propagate the stream init or write error */
-	return error < 0 ? error : close_error;
+	return error;
 }
 
 int git_filter_list_stream_blob(

tests/filter/custom.c

@@ -55,6 +55,7 @@ void test_filter_custom__initialize(void)
 		"hero* bitflip reverse\n"
 		"herofile text\n"
 		"heroflip -reverse binary\n"
+		"villain erroneous\n"
 		"*.bin binary\n");
 }
 
@@ -82,6 +83,11 @@ static void register_custom_filters(void)
 			create_reverse_filter("+prereverse"),
 			GIT_FILTER_DRIVER_PRIORITY));
 
+		cl_git_pass(git_filter_register(
+			"erroneous",
+			create_erroneous_filter("+erroneous"),
+			GIT_FILTER_DRIVER_PRIORITY));
+
 		filters_registered = 1;
 	}
 }
@@ -235,3 +241,18 @@ void test_filter_custom__filter_registry_failure_cases(void)
 	cl_git_fail(git_filter_unregister(GIT_FILTER_IDENT));
 	cl_assert_equal_i(GIT_ENOTFOUND, git_filter_unregister("not-a-filter"));
 }
+
+void test_filter_custom__erroneous_filter_fails(void)
+{
+	git_filter_list *filters;
+	git_buf out = GIT_BUF_INIT;
+	git_buf in = GIT_BUF_INIT_CONST(workdir_data, strlen(workdir_data));
+
+	cl_git_pass(git_filter_list_load(
+		&filters, g_repo, NULL, "villain", GIT_FILTER_TO_WORKTREE, 0));
+
+	cl_git_fail(git_filter_list_apply_to_data(&out, filters, &in));
+
+	git_filter_list_free(filters);
+	git_buf_free(&out);
+}

tests/filter/custom_helpers.c

@@ -106,3 +106,36 @@ git_filter *create_reverse_filter(const char *attrs)
 
 	return filter;
 }
+
+int erroneous_filter_stream(
+	git_writestream **out,
+	git_filter *self,
+	void **payload,
+	const git_filter_source *src,
+	git_writestream *next)
+{
+	GIT_UNUSED(out);
+	GIT_UNUSED(self);
+	GIT_UNUSED(payload);
+	GIT_UNUSED(src);
+	GIT_UNUSED(next);
+	return -1;
+}
+
+static void erroneous_filter_free(git_filter *f)
+{
+	git__free(f);
+}
+
+git_filter *create_erroneous_filter(const char *attrs)
+{
+	git_filter *filter = git__calloc(1, sizeof(git_filter));
+	cl_assert(filter);
+
+	filter->version = GIT_FILTER_VERSION;
+	filter->attributes = attrs;
+	filter->stream = erroneous_filter_stream;
+	filter->shutdown = erroneous_filter_free;
+
+	return filter;
+}

tests/filter/custom_helpers.h

@@ -2,6 +2,7 @@
 
 extern git_filter *create_bitflip_filter(void);
 extern git_filter *create_reverse_filter(const char *attr);
+extern git_filter *create_erroneous_filter(const char *attr);
 
 extern int bitflip_filter_apply(
 	git_filter     *self,