9front - general discussion about 9front
 help / color / mirror / Atom feed
* [9front] git/fetch problem in group writable repositories
@ 2021-07-16  5:49 Anthony Martin
  2021-07-16 16:05 ` ori
  0 siblings, 1 reply; 6+ messages in thread
From: Anthony Martin @ 2021-07-16  5:49 UTC (permalink / raw)
  To: 9front

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.

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?
  Anthony

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [9front] git/fetch problem in group writable repositories
  2021-07-16  5:49 [9front] git/fetch problem in group writable repositories Anthony Martin
@ 2021-07-16 16:05 ` ori
  2021-07-16 18:44   ` cinap_lenrek
  0 siblings, 1 reply; 6+ messages in thread
From: ori @ 2021-07-16 16:05 UTC (permalink / raw)
  To: 9front

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);


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [9front] git/fetch problem in group writable repositories
  2021-07-16 16:05 ` ori
@ 2021-07-16 18:44   ` cinap_lenrek
  2021-07-16 19:36     ` ori
  2021-07-17  0:11     ` Anthony Martin
  0 siblings, 2 replies; 6+ messages in thread
From: cinap_lenrek @ 2021-07-16 18:44 UTC (permalink / raw)
  To: 9front

looks good.

but why not ORCLOSE?

then it would get automatically removed when the program
exits or crashes.

--
cinap

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [9front] git/fetch problem in group writable repositories
  2021-07-16 18:44   ` cinap_lenrek
@ 2021-07-16 19:36     ` ori
  2021-07-17  0:12       ` cinap_lenrek
  2021-07-17  0:11     ` Anthony Martin
  1 sibling, 1 reply; 6+ messages in thread
From: ori @ 2021-07-16 19:36 UTC (permalink / raw)
  To: 9front

Quoth cinap_lenrek@felloff.net:
> looks good.
> 
> but why not ORCLOSE?
> 
> then it would get automatically removed when the program
> exits or crashes.
> 

How does that interact with renaming? I want to keep the
files around if the fetching and indexing succeed.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [9front] git/fetch problem in group writable repositories
  2021-07-16 18:44   ` cinap_lenrek
  2021-07-16 19:36     ` ori
@ 2021-07-17  0:11     ` Anthony Martin
  1 sibling, 0 replies; 6+ messages in thread
From: Anthony Martin @ 2021-07-17  0:11 UTC (permalink / raw)
  To: 9front

Looks good to me too.

cinap_lenrek@felloff.net once said:
> but why not ORCLOSE?

The only reason I didn't suggest ORCLOSE was because
I thought it might be useful to have the pack file
linger around for debugging a failed fetch.

Cheers,
  Anthony

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [9front] git/fetch problem in group writable repositories
  2021-07-16 19:36     ` ori
@ 2021-07-17  0:12       ` cinap_lenrek
  0 siblings, 0 replies; 6+ messages in thread
From: cinap_lenrek @ 2021-07-17  0:12 UTC (permalink / raw)
  To: 9front

> How does that interact with renaming? I want to keep the
> files around if the fetching and indexing succeed.

yeah, you'r right. that wont work.

--
cinap

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-07-17  6:29 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-16  5:49 [9front] git/fetch problem in group writable repositories Anthony Martin
2021-07-16 16:05 ` ori
2021-07-16 18:44   ` cinap_lenrek
2021-07-16 19:36     ` ori
2021-07-17  0:12       ` cinap_lenrek
2021-07-17  0:11     ` Anthony Martin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).