[PATCH] Make object creation in http fetch a bit safer.

Junio C Hamano· Sep 21, 2005, 09:55 UTC · lore

Unlike write_sha1_file() that tries to create the object file in a temporary location and then move it to the final location, fetch_object could have been interrupted in the middle, leaving a corrupt file.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 I think the packfile side is safer -- the index is verified
 with the call to parse_pack_index() which eventually calls
 check_packed_git_idx() to validate it, and the pack file is
 checked when it is first used, just like any pack file we
 originally had.  Ideally, we should validate the pair after
 downloading with git-verify-pack, though.

 http-fetch.c |   29 ++++++++++++++++++++++++-----
 1 files changed, 24 insertions(+), 5 deletions(-)

09d920831edba17fe983269a9d0d85e592dd4bb3

Show changes to http-fetch.c +24 −5

diff --git a/http-fetch.c b/http-fetch.c
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -350,13 +350,18 @@ int fetch_object(struct alt_base *repo, 
 	char *hex = sha1_to_hex(sha1);
 	char *filename = sha1_file_name(sha1);
 	unsigned char real_sha1[20];
+	char tmpfile[PATH_MAX];
+	int ret;
 	char *url;
 	char *posn;
 
-	local = open(filename, O_WRONLY | O_CREAT | O_EXCL, 0666);
+	snprintf(tmpfile, sizeof(tmpfile), "%s/obj_XXXXXX",
+		 get_object_directory());
 
+	local = mkstemp(tmpfile);
 	if (local < 0)
-		return error("Couldn't open local object %s\n", filename);
+		return error("Couldn't create temporary file %s for %s: %s\n",
+			     tmpfile, filename, strerror(errno));
 
 	memset(&stream, 0, sizeof(stream));
 
@@ -386,18 +391,32 @@ int fetch_object(struct alt_base *repo, 
 		return -1;
 	}
 
+	fchmod(local, 0444);
 	close(local);
 	inflateEnd(&stream);
 	SHA1_Final(real_sha1, &c);
 	if (zret != Z_STREAM_END) {
-		unlink(filename);
+		unlink(tmpfile);
 		return error("File %s (%s) corrupt\n", hex, url);
 	}
 	if (memcmp(sha1, real_sha1, 20)) {
-		unlink(filename);
+		unlink(tmpfile);
 		return error("File %s has bad hash\n", hex);
 	}
-	
+	ret = link(tmpfile, filename);
+	if (ret < 0) {
+		/* Same Coda hack as in write_sha1_file(sha1_file.c) */
+		ret = errno;
+		if (ret == EXDEV && !rename(tmpfile, filename))
+			goto out;
+	}
+	unlink(tmpfile);
+	if (ret) {
+		if (ret != EEXIST)
+			return error("unable to write sha1 filename %s: %s",
+				     filename, strerror(ret));
+	}
+ out:
 	pull_say("got %s\n", hex);
 	return 0;
 }

Daniel Barkalow· Sep 21, 2005, 15:55 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make object creation in http fetch a bit safer.

On Wed, 21 Sep 2005, Junio C Hamano wrote:

> Unlike write_sha1_file() that tries to create the object file in a
> temporary location and then move it to the final location, fetch_object
> could have been interrupted in the middle, leaving a corrupt file.

Looks fine to me, although it should possibly share the code somehow with write_sha1_file().

Show 6 quoted lines

>  I think the packfile side is safer -- the index is verified
>  with the call to parse_pack_index() which eventually calls
>  check_packed_git_idx() to validate it, and the pack file is
>  checked when it is first used, just like any pack file we
>  originally had.  Ideally, we should validate the pair after
>  downloading with git-verify-pack, though.

Well, with verify_pack(), anyway. No need to prepare a command line, fork, exec, and parse it, just to call a library function.

I think we want to have the partial file in the destination location, and maybe teach http-fetch about resuming the download, since it takes significant time to download a single pack, and it would be nice to be able to use the part we've got if we get interrupted.

	-Daniel
*This .sig left intentionally blank*

patchMake object creation in http fetch a bit safer.