[PATCH 3/3] http-push: send out fetch requests on queue

[PATCH 3/3] http-push: send out fetch requests on queue

[Tay Ray Chuan]

Previously, requests for remote files were simply added to the queue
(pointed to by request_queue_head) and no transfer actually took
place[1], even though code that followed may rely on these remote files
to be present (eg. the setup_revisions invocation).

The code that sends out the requests on the request queue is refactored
into the method run_request_queue.

After the get_dav_remote_heads invocation (ie. after fetch requests are
added to the queue), the requests on the queue are sent out through an
invocation to run_request_queue.

This invocation to run_request_queue entails adding a fill function
before pushing checks take place, which may lead to accidental,
unwanted pushes previously.

The flag is_running_queue is introduced to prevent this from occurring.
fill_active_slot is made to check the flag is_running_queue before
the sending of the requests proceeds.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 http-push.c          |   37 ++++++++++++++++++++++++++-----------
 t/t5540-http-push.sh |    4 ++--
 2 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/http-push.c b/http-push.c
index 5138224..1ba7987 100644
--- a/http-push.c
+++ b/http-push.c
@@ -843,11 +843,12 @@ static void finish_request(struct transfer_request *request)
 }

 #ifdef USE_CURL_MULTI
+static int is_running_queue;
 static int fill_active_slot(void *unused)
 {
 	struct transfer_request *request;

-	if (aborted)
+	if (aborted || !is_running_queue)
 		return 0;

 	for (request = request_queue_head; request; request = request->next) {
@@ -2167,6 +2168,25 @@ static int delete_remote_branch(char *pattern, int force)
 	return 0;
 }

+void run_request_queue()
+{
+#ifdef USE_CURL_MULTI
+	is_running_queue = 1;
+	fill_active_slots();
+	add_fill_function(NULL, fill_active_slot);
+#endif
+	do {
+		finish_all_active_slots();
+#ifdef USE_CURL_MULTI
+		fill_active_slots();
+#endif
+	} while (request_queue_head && !aborted);
+
+#ifdef USE_CURL_MULTI
+	is_running_queue = 0;
+#endif
+}
+
 int main(int argc, char **argv)
 {
 	struct transfer_request *request;
@@ -2271,6 +2291,8 @@ int main(int argc, char **argv)
 		repo->url = rewritten_url;
 	}

+	is_running_queue = 0;
+
 	/* Verify DAV compliance/lock support */
 	if (!locking_available()) {
 		rc = 1;
@@ -2300,6 +2322,7 @@ int main(int argc, char **argv)
 	local_refs = get_local_heads();
 	fprintf(stderr, "Fetching remote heads...\n");
 	get_dav_remote_heads();
+	run_request_queue();

 	/* Remove a remote branch if -d or -D was specified */
 	if (delete_branch) {
@@ -2429,16 +2452,8 @@ int main(int argc, char **argv)
 		if (objects_to_send)
 			fprintf(stderr, "    sending %d objects\n",
 				objects_to_send);
-#ifdef USE_CURL_MULTI
-		fill_active_slots();
-		add_fill_function(NULL, fill_active_slot);
-#endif
-		do {
-			finish_all_active_slots();
-#ifdef USE_CURL_MULTI
-			fill_active_slots();
-#endif
-		} while (request_queue_head && !aborted);
+
+		run_request_queue();

 		/* Update the remote branch if all went well */
 		if (aborted || !update_remote(ref->new_sha1, ref_lock))
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index ad0f14b..f4a2cf6 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -67,7 +67,7 @@ test_expect_success ' push to remote repository with unpacked refs' '
 	 test $HEAD = $(git rev-parse --verify HEAD))
 '

-test_expect_failure 'http-push fetches unpacked objects' '
+test_expect_success 'http-push fetches unpacked objects' '
 	cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
 		"$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_unpacked.git &&

@@ -83,7 +83,7 @@ test_expect_failure 'http-push fetches unpacked objects' '
 	 git push -f -v $HTTPD_URL/test_repo_unpacked.git master)
 '

-test_expect_failure 'http-push fetches packed objects' '
+test_expect_success 'http-push fetches packed objects' '
 	cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
 		"$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_packed.git &&

-- 
1.6.3.rc0

Re: [PATCH 3/3] http-push: send out fetch requests on queue

[Clemens Buchacher]

Hi,

On Sat, Apr 25, 2009 at 12:35:57AM +0800, Tay Ray Chuan wrote:
[...]
> --- a/http-push.c
> +++ b/http-push.c
> @@ -2300,6 +2322,7 @@ int main(int argc, char **argv)
>  	local_refs = get_local_heads();
>  	fprintf(stderr, "Fetching remote heads...\n");
>  	get_dav_remote_heads();
> +	run_request_queue();
> 
>  	/* Remove a remote branch if -d or -D was specified */
>  	if (delete_branch) {

The "http-push fetches packed objects" test of t5540 on pu either segfaults
or hangs indefinitely on my machine [1]. The bug is revealed by the added
line above in commit 0d71b15 (http-push: send out fetch requests on queue).

I have been trying to debug this, but I'm at a loss. Below are the backtrace
and an excerpt of the valgrind output with 0d71b15. See
http://gist.github.com/120453 for the full output. Note that the
segfault/hang does not happen if valgrind is used.

Can anybody reproduce this? Set GIT_TEST_HTTPD to enable this test.

Clemens

[1] Debian amd64 stable (mostly), libcurl version 17.18.2
---

(gdb) r
Starting program: /home/drizzd/fridge/linux/src/git/git-http-push --force
--verbose http://127.0.0.1:5540/test_repo_packed.git master
[Thread debugging using libthread_db enabled]
Getting pack list
Getting index for pack ecba592576b66d9aa805891d0a80a0133abd5f5d
Fetching remote heads...
  refs/
  refs/heads/
  fetch 9d498b0bbc2a25438e2fbd19081948da86028c23 for refs/heads/master
  refs/tags/
Fetching pack ecba592576b66d9aa805891d0a80a0133abd5f5d
 which contains 9d498b0bbc2a25438e2fbd19081948da86028c23
[New Thread 0x7f60404a9700 (LWP 21395)]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7f60404a9700 (LWP 21395)]
0x00007f603f49245e in ?? () from /lib/libc.so.6
(gdb) bt
#0  0x00007f603f49245e in ?? () from /lib/libc.so.6
#1  0x00007f603f49104a in ftell () from /lib/libc.so.6
#2  0x0000000000409785 in run_active_slot (slot=0x6bab40) at http.c:536
#3  0x00000000004099f0 in finish_all_active_slots () at http.c:607
#4  0x000000000040f72f in run_request_queue () at http-push.c:2179
#5  0x000000000040fcb4 in main (argc=5, argv=0x7fff485cd748) at
http-push.c:2325


* checking known breakage: 
	cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
		"$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_packed.git &&

	git clone $HTTPD_URL/test_repo_packed.git \
		"$ROOT_PATH"/test_repo_clone_packed &&

	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_packed.git &&
	 git --bare repack &&
	 git --bare prune-packed) &&

	# By reset, we force git to retrieve the packed object
	(cd "$ROOT_PATH"/test_repo_clone_packed &&
	 git reset --hard HEAD^ &&
	 git remote rm origin &&
	 git reflog expire --expire=0 --all &&
	 git prune &&
	 valgrind git-http-push --force --verbose $HTTPD_URL/test_repo_packed.git master)

got 9d498b0bbc2a25438e2fbd19081948da86028c23
walk 9d498b0bbc2a25438e2fbd19081948da86028c23
got 50b820aea6d3503362343cdc0e699b760c700b2b
got 0c973ae9bd51902a28466f3850b543fa66a6aaf4
got e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
walk 0c973ae9bd51902a28466f3850b543fa66a6aaf4
got 146e9fd4040abc05619b6c07a3a23cba27a2c722
Initialized empty Git repository in /home/drizzd/fridge/linux/src/git/t/trash directory.t5540-http-push/test_repo_clone_packed/.git/
HEAD is now at 0c973ae initial
==== Memcheck, a memory error detector.
==== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al.
==== Using LibVEX rev 1854, a library for dynamic binary translation.
==== Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP.
==== Using valgrind-3.3.1-Debian, a dynamic binary instrumentation framework.
==== Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al.
==== For more details, rerun with: -v
==== 
==== Invalid read of size 1
====    at 0x409EBE: xml_entities (http-push.c:192)
====    by 0x40DE29: locking_available (http-push.c:1619)
====    by 0x40FBCF: main (http-push.c:2297)
====  Address 0x80b94e4 is 0 bytes after a block of size 44 alloc'd
====    at 0x4C2260E: malloc (vg_replace_malloc.c:207)
====    by 0x44C4E4: xmalloc (wrapper.c:20)
====    by 0x40FB4D: main (http-push.c:2286)
Getting pack list
Getting index for pack ecba592576b66d9aa805891d0a80a0133abd5f5d
Fetching remote heads...
  refs/
  refs/heads/
  fetch 9d498b0bbc2a25438e2fbd19081948da86028c23 for refs/heads/master
  refs/tags/
Fetching pack ecba592576b66d9aa805891d0a80a0133abd5f5d
 which contains 9d498b0bbc2a25438e2fbd19081948da86028c23
==== 
==== Invalid read of size 2
====    at 0x5ABFFE1: ftell (in /lib/libc-2.7.so)
====    by 0x409784: run_active_slot (http.c:536)
====    by 0x4099EF: finish_all_active_slots (http.c:607)
====    by 0x40F72E: run_request_queue (http-push.c:2179)
====    by 0x40FCB3: main (http-push.c:2325)
====  Address 0x817c688 is 0 bytes inside a block of size 568 free'd
====    at 0x4C2130F: free (vg_replace_malloc.c:323)
====    by 0x5ABEDA0: fclose (in /lib/libc-2.7.so)
====    by 0x40BC64: finish_request (http-push.c:824)
====    by 0x40A0F4: process_response (http-push.c:257)
====    by 0x4099C4: finish_active_slot (http.c:598)
====    by 0x4088E3: process_curl_messages (http.c:101)
====    by 0x409709: step_active_slots (http.c:512)
====    by 0x40975A: run_active_slot (http.c:533)
====    by 0x4099EF: finish_all_active_slots (http.c:607)
====    by 0x40F72E: run_request_queue (http-push.c:2179)
====    by 0x40FCB3: main (http-push.c:2325)
[...]
==== Invalid read of size 4
====    at 0x5AC00AC: ftell (in /lib/libc-2.7.so)
====    by 0x409784: run_active_slot (http.c:536)
====    by 0x4099EF: finish_all_active_slots (http.c:607)
====    by 0x40F72E: run_request_queue (http-push.c:2179)
====    by 0x40FCB3: main (http-push.c:2325)
====  Address 0x817c768 is 224 bytes inside a block of size 568 free'd
====    at 0x4C2130F: free (vg_replace_malloc.c:323)
====    by 0x5ABEDA0: fclose (in /lib/libc-2.7.so)
====    by 0x40BC64: finish_request (http-push.c:824)
====    by 0x40A0F4: process_response (http-push.c:257)
====    by 0x4099C4: finish_active_slot (http.c:598)
====    by 0x4088E3: process_curl_messages (http.c:101)
====    by 0x409709: step_active_slots (http.c:512)
====    by 0x40975A: run_active_slot (http.c:533)
====    by 0x4099EF: finish_all_active_slots (http.c:607)
====    by 0x40F72E: run_request_queue (http-push.c:2179)
====    by 0x40FCB3: main (http-push.c:2325)
updating 'refs/heads/master'
  from 9d498b0bbc2a25438e2fbd19081948da86028c23
  to   0c973ae9bd51902a28466f3850b543fa66a6aaf4
    done
Updating remote server info
==== 
==== ERROR SUMMARY: 46 errors from 42 contexts (suppressed: 8 from 1)
==== malloc/free: in use at exit: 201,408 bytes in 54 blocks.
==== malloc/free: 2,976 allocs, 2,922 frees, 1,512,372 bytes allocated.
==== For counts of detected errors, rerun with: -v
==== searching for pointers to 54 not-freed blocks.
==== checked 648,992 bytes.
==== 
==== LEAK SUMMARY:
====    definitely lost: 10,463 bytes in 26 blocks.
====      possibly lost: 0 bytes in 0 blocks.
====    still reachable: 190,945 bytes in 28 blocks.
====         suppressed: 0 bytes in 0 blocks.
==== Rerun with --leak-check=full to see details of leaked memory.
*   FIXED 6: http-push fetches packed objects

* fixed 1 known breakage(s)
* still have 1 known breakage(s)
* passed all remaining 5 test(s)

Re: [PATCH 3/3] http-push: send out fetch requests on queue

[Tay Ray Chuan]

Hi,

On Sat, May 30, 2009 at 5:17 PM, Clemens Buchacher <drizzd@aon.at> wrote:
> The "http-push fetches packed objects" test of t5540 on pu either segfaults
> or hangs indefinitely on my machine [1]. The bug is revealed by the added
> line above in commit 0d71b15 (http-push: send out fetch requests on queue).

if you don't mind, could you try running this test on the
http-fix-push-fetching branch in git://github.com/rctay/git.git and
see if this still happens? It contains these patches only, applied on
master.

-- 
Cheers,
Ray Chuan

Re: [PATCH 3/3] http-push: send out fetch requests on queue

[Clemens Buchacher]

On Sat, May 30, 2009 at 05:31:36PM +0800, Tay Ray Chuan wrote:
> On Sat, May 30, 2009 at 5:17 PM, Clemens Buchacher <drizzd@aon.at> wrote:
> > The "http-push fetches packed objects" test of t5540 on pu either segfaults
> > or hangs indefinitely on my machine [1]. The bug is revealed by the added
> > line above in commit 0d71b15 (http-push: send out fetch requests on queue).
> 
> if you don't mind, could you try running this test on the
> http-fix-push-fetching branch in git://github.com/rctay/git.git and
> see if this still happens? It contains these patches only, applied on
> master.

Sure. The same thing happens.

Re: [PATCH 3/3] http-push: send out fetch requests on queue

[Tay Ray Chuan]

Hi,

On Sat, May 30, 2009 at 5:37 PM, Clemens Buchacher <drizzd@aon.at> wrote:
> Sure. The same thing happens.

curiously, I wasn't able to reproduce on my cygwin setup, but on my ubuntu box.

-- 
Cheers,
Ray Chuan

Re: [PATCH 3/3] http-push: send out fetch requests on queue

[Tay Ray Chuan]

Set slot->local to NULL after doing a fclose on the FILE* pointer it
points to.

Move NULL assignment to request->slot from
http-push.c::finish_request() to http-push.c::release_request(). This
is safe, since the functions finish_request() invoke will overwrite
request->slot anyway.

Refactor http-push.c::fetch_index(), http-walker.c::fetch_index() and
http-walker.c::fetch_pack() to use labels while cleaning up.

This fixes the issue raised by Clemens Buchacher on 30th May:

  http://www.spinics.net/lists/git/msg104623.html

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

This applies on master.

 http-push.c   |   23 ++++++++++++++---------
 http-walker.c |   36 +++++++++++++++++++++++-------------
 2 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/http-push.c b/http-push.c
index dac2c6e..816824a 100644
--- a/http-push.c
+++ b/http-push.c
@@ -715,6 +715,8 @@ static void release_request(struct transfer_request *request)
 		close(request->local_fileno);
 	if (request->local_stream)
 		fclose(request->local_stream);
+	if (request->slot)
+		request->slot = NULL;
 	free(request->url);
 	free(request);
 }
@@ -727,7 +729,6 @@ static void finish_request(struct transfer_request *request)
 
 	request->curl_result = request->slot->curl_result;
 	request->http_code = request->slot->http_code;
-	request->slot = NULL;
 
 	/* Keep locks active */
 	check_locks();
@@ -823,6 +824,7 @@ static void finish_request(struct transfer_request *request)
 
 			fclose(request->local_stream);
 			request->local_stream = NULL;
+			request->slot->local = NULL;
 			if (!move_temp_to_file(request->tmpfile,
 					       request->filename)) {
 				target = (struct packed_git *)request->userData;
@@ -946,6 +948,7 @@ static int add_send_request(struct object *obj, struct remote_lock *lock)
 
 static int fetch_index(unsigned char *sha1)
 {
+	int ret;
 	char *hex = sha1_to_hex(sha1);
 	char *filename;
 	char *url;
@@ -1022,21 +1025,23 @@ static int fetch_index(unsigned char *sha1)
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
 		if (results.curl_result != CURLE_OK) {
-			free(url);
-			fclose(indexfile);
-			return error("Unable to get pack index %s\n%s", url,
-				     curl_errorstr);
+			ret = error("Unable to get pack index %s\n%s", url,
+				    curl_errorstr);
+			goto cleanup;
 		}
 	} else {
-		free(url);
-		fclose(indexfile);
-		return error("Unable to start request");
+		ret = error("Unable to start request");
+		goto cleanup;
 	}
 
+	ret = move_temp_to_file(tmpfile, filename);
+
+cleanup:
 	free(url);
 	fclose(indexfile);
+	slot->local = NULL;
 
-	return move_temp_to_file(tmpfile, filename);
+	return ret;
 }
 
 static int setup_index(unsigned char *sha1)
diff --git a/http-walker.c b/http-walker.c
index 7321ccc..779947d 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -364,6 +364,7 @@ static void prefetch(struct walker *walker, unsigned char *sha1)
 
 static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned char *sha1)
 {
+	int ret;
 	char *hex = sha1_to_hex(sha1);
 	char *filename;
 	char *url;
@@ -417,18 +418,21 @@ static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned ch
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
 		if (results.curl_result != CURLE_OK) {
-			fclose(indexfile);
-			return error("Unable to get pack index %s\n%s", url,
-				     curl_errorstr);
+			ret = error("Unable to get pack index %s\n%s", url,
+				    curl_errorstr);
+			goto cleanup;
 		}
 	} else {
-		fclose(indexfile);
-		return error("Unable to start request");
+		ret = error("Unable to start request");
+		goto cleanup;
 	}
 
-	fclose(indexfile);
+	ret = move_temp_to_file(tmpfile, filename);
 
-	return move_temp_to_file(tmpfile, filename);
+cleanup:
+	fclose(indexfile);
+	slot->local = NULL;
+	return ret;
 }
 
 static int setup_index(struct walker *walker, struct alt_base *repo, unsigned char *sha1)
@@ -718,7 +722,7 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha
 	FILE *packfile;
 	char *filename;
 	char tmpfile[PATH_MAX];
-	int ret;
+	int ret = 0;
 	long prev_posn = 0;
 	char range[RANGE_HEADER_SIZE];
 	struct curl_slist *range_header = NULL;
@@ -775,17 +779,23 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
 		if (results.curl_result != CURLE_OK) {
-			fclose(packfile);
-			return error("Unable to get pack file %s\n%s", url,
-				     curl_errorstr);
+			ret = error("Unable to get pack file %s\n%s", url,
+				    curl_errorstr);
+			goto cleanup;
 		}
 	} else {
-		fclose(packfile);
-		return error("Unable to start request");
+		ret = error("Unable to start request");
+		goto cleanup;
 	}
 
 	target->pack_size = ftell(packfile);
+
+cleanup:
 	fclose(packfile);
+	slot->local = NULL;
+
+	if (ret)
+		return ret;
 
 	ret = move_temp_to_file(tmpfile, filename);
 	if (ret)
-- 
1.6.3.1

[PATCH] http*: cleanup slot->local after fclose

[Tay Ray Chuan]

Set slot->local to NULL after doing a fclose on the FILE* pointer it
points to.

Move NULL assignment to request->slot from
http-push.c::finish_request() to http-push.c::release_request(). This
is safe, since the functions finish_request() invoke will overwrite
request->slot anyway.

Refactor http-push.c::fetch_index(), http-walker.c::fetch_index() and
http-walker.c::fetch_pack() to use labels while cleaning up.

This fixes the issue raised by Clemens Buchacher on 30th May:

  http://www.spinics.net/lists/git/msg104623.html

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

 This applies on master, and is a resend, this time with the subject.

 http-push.c   |   23 ++++++++++++++---------
 http-walker.c |   36 +++++++++++++++++++++++-------------
 2 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/http-push.c b/http-push.c
index dac2c6e..816824a 100644
--- a/http-push.c
+++ b/http-push.c
@@ -715,6 +715,8 @@ static void release_request(struct transfer_request *request)
 		close(request->local_fileno);
 	if (request->local_stream)
 		fclose(request->local_stream);
+	if (request->slot)
+		request->slot = NULL;
 	free(request->url);
 	free(request);
 }
@@ -727,7 +729,6 @@ static void finish_request(struct transfer_request *request)
 
 	request->curl_result = request->slot->curl_result;
 	request->http_code = request->slot->http_code;
-	request->slot = NULL;
 
 	/* Keep locks active */
 	check_locks();
@@ -823,6 +824,7 @@ static void finish_request(struct transfer_request *request)
 
 			fclose(request->local_stream);
 			request->local_stream = NULL;
+			request->slot->local = NULL;
 			if (!move_temp_to_file(request->tmpfile,
 					       request->filename)) {
 				target = (struct packed_git *)request->userData;
@@ -946,6 +948,7 @@ static int add_send_request(struct object *obj, struct remote_lock *lock)
 
 static int fetch_index(unsigned char *sha1)
 {
+	int ret;
 	char *hex = sha1_to_hex(sha1);
 	char *filename;
 	char *url;
@@ -1022,21 +1025,23 @@ static int fetch_index(unsigned char *sha1)
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
 		if (results.curl_result != CURLE_OK) {
-			free(url);
-			fclose(indexfile);
-			return error("Unable to get pack index %s\n%s", url,
-				     curl_errorstr);
+			ret = error("Unable to get pack index %s\n%s", url,
+				    curl_errorstr);
+			goto cleanup;
 		}
 	} else {
-		free(url);
-		fclose(indexfile);
-		return error("Unable to start request");
+		ret = error("Unable to start request");
+		goto cleanup;
 	}
 
+	ret = move_temp_to_file(tmpfile, filename);
+
+cleanup:
 	free(url);
 	fclose(indexfile);
+	slot->local = NULL;
 
-	return move_temp_to_file(tmpfile, filename);
+	return ret;
 }
 
 static int setup_index(unsigned char *sha1)
diff --git a/http-walker.c b/http-walker.c
index 7321ccc..779947d 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -364,6 +364,7 @@ static void prefetch(struct walker *walker, unsigned char *sha1)
 
 static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned char *sha1)
 {
+	int ret;
 	char *hex = sha1_to_hex(sha1);
 	char *filename;
 	char *url;
@@ -417,18 +418,21 @@ static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned ch
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
 		if (results.curl_result != CURLE_OK) {
-			fclose(indexfile);
-			return error("Unable to get pack index %s\n%s", url,
-				     curl_errorstr);
+			ret = error("Unable to get pack index %s\n%s", url,
+				    curl_errorstr);
+			goto cleanup;
 		}
 	} else {
-		fclose(indexfile);
-		return error("Unable to start request");
+		ret = error("Unable to start request");
+		goto cleanup;
 	}
 
-	fclose(indexfile);
+	ret = move_temp_to_file(tmpfile, filename);
 
-	return move_temp_to_file(tmpfile, filename);
+cleanup:
+	fclose(indexfile);
+	slot->local = NULL;
+	return ret;
 }
 
 static int setup_index(struct walker *walker, struct alt_base *repo, unsigned char *sha1)
@@ -718,7 +722,7 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha
 	FILE *packfile;
 	char *filename;
 	char tmpfile[PATH_MAX];
-	int ret;
+	int ret = 0;
 	long prev_posn = 0;
 	char range[RANGE_HEADER_SIZE];
 	struct curl_slist *range_header = NULL;
@@ -775,17 +779,23 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
 		if (results.curl_result != CURLE_OK) {
-			fclose(packfile);
-			return error("Unable to get pack file %s\n%s", url,
-				     curl_errorstr);
+			ret = error("Unable to get pack file %s\n%s", url,
+				    curl_errorstr);
+			goto cleanup;
 		}
 	} else {
-		fclose(packfile);
-		return error("Unable to start request");
+		ret = error("Unable to start request");
+		goto cleanup;
 	}
 
 	target->pack_size = ftell(packfile);
+
+cleanup:
 	fclose(packfile);
+	slot->local = NULL;
+
+	if (ret)
+		return ret;
 
 	ret = move_temp_to_file(tmpfile, filename);
 	if (ret)
-- 
1.6.3.1

Re: [PATCH] http*: cleanup slot->local after fclose

[Clemens Buchacher]

On Sun, May 31, 2009 at 12:09:55AM +0800, Tay Ray Chuan wrote:
> Set slot->local to NULL after doing a fclose on the FILE* pointer it
> points to.

Thanks. That fixes it.

Clemens

Re: [PATCH] http*: cleanup slot->local after fclose

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> Set slot->local to NULL after doing a fclose on the FILE* pointer it
> points to.
>
> Move NULL assignment to request->slot from
> http-push.c::finish_request() to http-push.c::release_request(). This
> is safe, since the functions finish_request() invoke will overwrite
> request->slot anyway.
>
> Refactor http-push.c::fetch_index(), http-walker.c::fetch_index() and
> http-walker.c::fetch_pack() to use labels while cleaning up.
>
> This fixes the issue raised by Clemens Buchacher on 30th May:
>
>   http://www.spinics.net/lists/git/msg104623.html
>
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
> ---
>
>  This applies on master, and is a resend, this time with the subject.

Shouldn't a fix instead be queued for 'maint', without "refactoring"?

Is there a reproducible repipe you can add to the test script?

>  http-push.c   |   23 ++++++++++++++---------
>  http-walker.c |   36 +++++++++++++++++++++++-------------
>  2 files changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/http-push.c b/http-push.c
> index dac2c6e..816824a 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -715,6 +715,8 @@ static void release_request(struct transfer_request *request)
>  		close(request->local_fileno);
>  	if (request->local_stream)
>  		fclose(request->local_stream);
> +	if (request->slot)
> +		request->slot = NULL;
>  	free(request->url);
>  	free(request);

Hmm, what's the point of setting NULL to request->slot if you are already
freeing "request" that contains the field?

Re: [PATCH] http*: cleanup slot->local after fclose

[Tay Ray Chuan]

Hi,

On Sun, May 31, 2009 at 2:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Shouldn't a fix instead be queued for 'maint', without "refactoring"?

one on the way.

> Is there a reproducible repipe you can add to the test script?

what do you mean by "repipe"? To my mind, reproducing Clemens' issue
would require some code manipulation and compiling.

> Hmm, what's the point of setting NULL to request->slot if you are already
> freeing "request" that contains the field?

you do have a point. :)

-- 
Cheers,
Ray Chuan

[PATCH v2] http*: cleanup slot->local after fclose

[Tay Ray Chuan]

Set slot->local to NULL after doing a fclose on the FILE* pointer it
points to.

This fixes the issue raised by Clemens Buchacher on 30th May:

  http://www.spinics.net/lists/git/msg104623.html

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

On Sat, 30 May 2009 23:17:19 -0700
Junio C Hamano <gitster@pobox.com> wrote:
> Shouldn't a fix instead be queued for 'maint', without "refactoring"?

I tested this on top of the first 4 patches in 'rc/http-push' in 'pu',
applied on 'maint'. [1] I wonder if this should instead be queued for
'pu' [2], since the issue only occurs there, although, conceivably, it
*could* happen without those patches in 'pu'.

Footnotes:
[1] You can find this in the 'http-cleanup-slot-local_maint branch' at
    git://github.com/rctay/git.git. It's quite a minimal testcase, if
    you're looking for one.

[2] Found in 'http-cleanup-slot-local_pu'.

 http-push.c   |    6 ++++++
 http-walker.c |    6 ++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/http-push.c b/http-push.c
index e16a0ad..0b12ffe 100644
--- a/http-push.c
+++ b/http-push.c
@@ -724,9 +724,11 @@ static void finish_request(struct transfer_request *request)
 	struct stat st;
 	struct packed_git *target;
 	struct packed_git **lst;
+	struct active_request_slot *slot;
 
 	request->curl_result = request->slot->curl_result;
 	request->http_code = request->slot->http_code;
+	slot = request->slot;
 	request->slot = NULL;
 
 	/* Keep locks active */
@@ -823,6 +825,7 @@ static void finish_request(struct transfer_request *request)
 
 			fclose(request->local_stream);
 			request->local_stream = NULL;
+			slot->local = NULL;
 			if (!move_temp_to_file(request->tmpfile,
 					       request->filename)) {
 				target = (struct packed_git *)request->userData;
@@ -1024,17 +1027,20 @@ static int fetch_index(unsigned char *sha1)
 		if (results.curl_result != CURLE_OK) {
 			free(url);
 			fclose(indexfile);
+			slot->local = NULL;
 			return error("Unable to get pack index %s\n%s", url,
 				     curl_errorstr);
 		}
 	} else {
 		free(url);
 		fclose(indexfile);
+		slot->local = NULL;
 		return error("Unable to start request");
 	}
 
 	free(url);
 	fclose(indexfile);
+	slot->local = NULL;
 
 	return move_temp_to_file(tmpfile, filename);
 }
diff --git a/http-walker.c b/http-walker.c
index 7321ccc..9377851 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -418,15 +418,18 @@ static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned ch
 		run_active_slot(slot);
 		if (results.curl_result != CURLE_OK) {
 			fclose(indexfile);
+			slot->local = NULL;
 			return error("Unable to get pack index %s\n%s", url,
 				     curl_errorstr);
 		}
 	} else {
 		fclose(indexfile);
+		slot->local = NULL;
 		return error("Unable to start request");
 	}
 
 	fclose(indexfile);
+	slot->local = NULL;
 
 	return move_temp_to_file(tmpfile, filename);
 }
@@ -776,16 +779,19 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha
 		run_active_slot(slot);
 		if (results.curl_result != CURLE_OK) {
 			fclose(packfile);
+			slot->local = NULL;
 			return error("Unable to get pack file %s\n%s", url,
 				     curl_errorstr);
 		}
 	} else {
 		fclose(packfile);
+		slot->local = NULL;
 		return error("Unable to start request");
 	}
 
 	target->pack_size = ftell(packfile);
 	fclose(packfile);
+	slot->local = NULL;
 
 	ret = move_temp_to_file(tmpfile, filename);
 	if (ret)
-- 
1.6.3.1

Re: [PATCH v2] http*: cleanup slot->local after fclose

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> I tested this on top of the first 4 patches in 'rc/http-push' in 'pu',
> applied on 'maint'. [1] I wonder if this should instead be queued for
> 'pu' [2], since the issue only occurs there, although, conceivably, it
> *could* happen without those patches in 'pu'.

That "*could*" leaves me puzzled even more.  Could you clarify?

Do you mean "earlier, nobody tried to fclose(slot->local) twice, but with
the four patches there are codepaths that do so, which triggered the bug
reported by Clemens"?  But then the issue couldn't have happened without
those patches.

If existing code always knew when slot->local is and is not valid without
having to rely on its NULLness, and that was the reason why nobody tried
to fclose(slot->local) twice, then I'd agree that it is a bug waiting to
happen, and stuffing NULL to slot->local after the code fclose() it would
be a good clean-up.

Re: [PATCH v2] http*: cleanup slot->local after fclose

[Tay Ray Chuan]

Hi,

On Mon, Jun 1, 2009 at 4:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
> That "*could*" leaves me puzzled even more.  Could you clarify?
>
> Do you mean "earlier, nobody tried to fclose(slot->local) twice, but with
> the four patches there are codepaths that do so, which triggered the bug
> reported by Clemens"?  But then the issue couldn't have happened without
> those patches.
>
> If existing code always knew when slot->local is and is not valid without
> having to rely on its NULLness, and that was the reason why nobody tried
> to fclose(slot->local) twice, then I'd agree that it is a bug waiting to
> happen, and stuffing NULL to slot->local after the code fclose() it would
> be a good clean-up.

the problem isn't triggered by fclose(slot->local) being done twice,
but a ftell() being done on slot->local, which points to a FILE*
handle that has already been fclose()'d (that is what the valgrind log
provided by Clemens reported). The offending ftell() is found in
http.c::run_active_slot(), and the code there does indeed depend on
the NULLness of slot->local (it checks whether slot->local is NULL
before doing the ftell()).

run_active_slot() is used rather heavily, and users of slot->local
always neglect to set it to NULL doing a fclose() on the FILE* handle
it points to. This statement is true in both 'master', and 'pu'
('rc/http-push'). Therefore, I said "*could*": although the problem
Clemens reported occurs only in 'pu' (or more accurately, the first
four patches of 'rc/http-push'), it's not totally impossible for the
problem to occur without those four patches.

-- 
Cheers,
Ray Chuan

Re: [PATCH v2] http*: cleanup slot->local after fclose

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> the problem isn't triggered by fclose(slot->local) being done twice,
> but a ftell() being done on slot->local, which points to a FILE*
> handle that has already been fclose()'d (that is what the valgrind log
> provided by Clemens reported). The offending ftell() is found in
> http.c::run_active_slot(), and the code there does indeed depend on
> the NULLness of slot->local (it checks whether slot->local is NULL
> before doing the ftell()).
>
> run_active_slot() is used rather heavily, and users of slot->local
> always neglect to set it to NULL doing a fclose() on the FILE* handle
> it points to.

Thanks; that clearly indicates that you are fixing an existing bug.

I wish the commit log message explained that from the beginning without
being asked.

So what would we want to do?

No patch from your http-push topic is in 'next' yet, and I presume that
the same issue exists in 'maint', so I'd say:

 (0) queue your slimmed down fix to 'maint' (or rc/maint-http-local-slot-fix
     that is forked from 'maint', to later merge to 'maint');

 (1) create a new rc/http-push topic branch from 'master';

 (2) merge (0) into the result of (1);
 
 (3) re-apply the patches queued on the current rc/http-push topic on top
     of the result of (2).

to rebuild it, perhaps?

Re: [PATCH v2] http*: cleanup slot->local after fclose

[Tay Ray Chuan]

Hi,

On Tue, Jun 2, 2009 at 12:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I wish the commit log message explained that from the beginning without
> being asked.

Indeed, I guess I only hinted at the bug being fixed, and in the last
line, at that.

> So what would we want to do?

You can go ahead and apply this patch, many thanks. Don't bother with
re-applying the rc/http-push though; I'll send in the revised patches
sometime this weekend.

-- 
Cheers,
Ray Chuan