Re: [9front] git/fetch problem in group writable repositories

[ori@eigenstate.org]

Quoth Anthony Martin <ality@pbrane.org>:
> If you create a git repository in a directory that
> is group writable, running git/fetch will leave
> behind an empty .git/objects/pack/fetch.tmp file
> with mode 0644 in a number of scenarios:
> 
> 	- if passed the -l flag
> 	- if the local repository is up to date
> 	- if there is a fatal error in fetchpack
> 
> If another user in the same group subsequently
> tries to run git/fetch, it will fail since the
> temporary pack file was not removed and cannot
> be overwritten.
> 
> The calls to create need to be at least 0660 for
> files and 0770 for directories.  It may also make
> sense to create the temporary files with OEXCL
> instead of truncating them if they already exist
> or to use a name that's less likely to be reused
> as in serve.c:/^updatepack.

Good catches.

I think the below patch will solve the problems,
as well as cleaning the code up a little.

Looks good?

> Another thing to think about it is if we should
> grab a lock of some kind before operations that
> will modify a repository that is group writable.
> Or just have a policy of "don't do that".
> 
> Thoughts?

Right now, the policy is 'don't do that', but I
am open to changing it.

--- //.git/fs/object/e85aa1089d1d4954aa949cd05c5b6b9c3fca596c/tree/sys/src/cmd/git/fetch.c
+++ sys/src/cmd/git/fetch.c
@@ -5,7 +5,6 @@
 
 char *fetchbranch;
 char *upstream = "origin";
-char *packtmp = ".git/objects/pack/fetch.tmp";
 int listonly;
 
 int
@@ -111,7 +110,7 @@
 	for(p=strchr(s+1, '/'); p; p=strchr(p+1, '/')){
 		*p = 0;
 		if(access(s, AEXIST) != 0){
-			fd = create(s, OREAD, DMDIR | 0755);
+			fd = create(s, OREAD, DMDIR | 0775);
 			if(fd == -1)
 				return -1;
 			close(fd);
@@ -162,13 +161,30 @@
 	}
 }
 
+void
+fail(char *pack, char *idx, char *msg, ...)
+{
+	char buf[ERRMAX];
+	va_list ap;
+
+	va_start(ap, msg);
+	snprint(buf, sizeof(buf), msg, ap);
+	va_end(ap);
+
+	remove(pack);
+	remove(idx);
+	fprint(2, "%s", buf);
+	exits(buf);
+}
+
 int
-fetchpack(Conn *c, int pfd, char *packtmp)
+fetchpack(Conn *c)
 {
-	char buf[Pktmax], idxtmp[256], *sp[3];
+	char buf[Pktmax], *sp[3];
+	char *packtmp, *idxtmp;
 	Hash h, *have, *want;
 	int nref, refsz, first;
-	int i, n, req;
+	int i, n, req, pfd;
 	vlong packsz;
 	Object *o;
 
@@ -249,6 +265,15 @@
 		sysfatal("read: %r");
 	buf[n] = 0;
 
+	if((packtmp = smprint(".git/objects/pack/fetch.%d.pack", getpid())) == nil)
+		sysfatal("smprint: %r");
+	if((idxtmp = smprint(".git/objects/idx/fetch.%d.idx", getpid())) == nil)
+		sysfatal("smprint: %r");
+	if(mkoutpath(packtmp) == -1)
+		sysfatal("could not create %s: %r", packtmp);
+	if((pfd = create(packtmp, ORDWR, 0664)) == -1)
+		sysfatal("could not create %s: %r", packtmp);
+
 	fprint(2, "fetching...\n");
 	packsz = 0;
 	while(1){
@@ -259,19 +284,17 @@
 			sysfatal("fetch packfile: %r");
 		packsz += n;
 	}
+
 	closeconn(c);
 	if(seek(pfd, 0, 0) == -1)
-		sysfatal("packfile seek: %r");
+		fail(packtmp, idxtmp, "packfile seek: %r");
 	if(checkhash(pfd, packsz, &h) == -1)
-		sysfatal("corrupt packfile: %r");
+		fail(packtmp, idxtmp, "corrupt packfile: %r");
 	close(pfd);
-	n = strlen(packtmp) - strlen(".tmp");
-	memcpy(idxtmp, packtmp, n);
-	memcpy(idxtmp + n, ".idx", strlen(".idx") + 1);
 	if(indexpack(packtmp, idxtmp, h) == -1)
-		sysfatal("could not index fetched pack: %r");
+		fail(packtmp, idxtmp, "could not index fetched pack: %r");
 	if(rename(packtmp, idxtmp, h) == -1)
-		sysfatal("could not rename indexed pack: %r");
+		fail(packtmp, idxtmp, "could not rename indexed pack: %r");
 	return 0;
 }
 
@@ -287,7 +310,6 @@
 void
 main(int argc, char **argv)
 {
-	int pfd;
 	Conn c;
 
 	ARGBEGIN{
@@ -302,14 +324,9 @@
 	if(argc != 1)
 		usage();
 
-	if(mkoutpath(packtmp) == -1)
-		sysfatal("could not create %s: %r", packtmp);
-	if((pfd = create(packtmp, ORDWR, 0644)) == -1)
-		sysfatal("could not create %s: %r", packtmp);
-
 	if(gitconnect(&c, argv[0], "upload") == -1)
 		sysfatal("could not dial %s: %r", argv[0]);
-	if(fetchpack(&c, pfd, packtmp) == -1)
+	if(fetchpack(&c) == -1)
 		sysfatal("fetch failed: %r");
 	closeconn(&c);
 	exits(nil);