[PATCH] Fix broken sha1 locking

[PATCH] Fix broken sha1 locking

[Petr Baudis]

Current git#next is totally broken wrt. cloning over HTTP, generating refs
at random directories. Of course it's caused by the static get_pathname()
buffer. lock_ref_sha1() stores return value of mkpath()'s get_pathname()
call, then calls lock_ref_sha1_basic() which calls git_path(ref) which
calls get_pathname() at that point returning pointer to the same buffer.
So now you are sprintf()ing a format string into itself, wow! The resulting
pathnames are really cute. (If you've been paying attention, yes, the
mere fact that a format string _could_ write over itself is very wrong
and probably exploitable here. See the other mail I've just sent.)

I've never liked how we use return values of those functions so liberally,
the "allow some random number of get_pathname() return values to work
concurrently" is absolutely horrible pit and we've already fallen in this
before IIRC. I consider it an awful coding practice, you add a call
somewhere and at some other point some distant caller of that breaks since
it reuses the same return values. Not to mention this takes quite some time
to debug.

My gut feeling tells me that there might be more of this.  I don't have
time to review the rest of the users of the refs.c functions though.

Signed-off-by: Petr Baudis <pasky@suse.cz>
---

 refs.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 134c0fc..7bd36e4 100644
--- a/refs.c
+++ b/refs.c
@@ -462,10 +462,12 @@ static struct ref_lock *lock_ref_sha1_ba
 struct ref_lock *lock_ref_sha1(const char *ref,
 	const unsigned char *old_sha1, int mustexist)
 {
+	char refpath[PATH_MAX];
 	if (check_ref_format(ref))
 		return NULL;
-	return lock_ref_sha1_basic(mkpath("refs/%s", ref),
-		5 + strlen(ref), old_sha1, mustexist);
+	strcpy(refpath, mkpath("refs/%s", ref));
+	return lock_ref_sha1_basic(refpath, strlen(refpath),
+		old_sha1, mustexist);
 }
 
 struct ref_lock *lock_any_ref_for_update(const char *ref,

Re: [PATCH] Fix broken sha1 locking

[Linus Torvalds]

On Tue, 19 Sep 2006, Petr Baudis wrote:
>
> Current git#next is totally broken wrt. cloning over HTTP, generating refs
> at random directories. Of course it's caused by the static get_pathname()
> buffer. lock_ref_sha1() stores return value of mkpath()'s get_pathname()
> call, then calls lock_ref_sha1_basic() which calls git_path(ref) which
> calls get_pathname() at that point returning pointer to the same buffer.

Hmm. This was exactly the schenario why I did commit
e7676d2f6454c9c99e600ee2ce3c7205a9fcfb5f - allowing a couple of 
overlapping paths

Isn't that in the "next" branch too?

Of course, that still assumes that you strdup() the result at _some_ time, 
and can't just save it away, but lock_ref_sha1_basic() should do that.

			Linus

Re: [PATCH] Fix broken sha1 locking

[Petr Baudis]

Dear diary, on Tue, Sep 19, 2006 at 11:16:04PM CEST, I got a letter
where Linus Torvalds <torvalds@osdl.org> said that...
> On Tue, 19 Sep 2006, Petr Baudis wrote:
> >
> > Current git#next is totally broken wrt. cloning over HTTP, generating refs
> > at random directories. Of course it's caused by the static get_pathname()
> > buffer. lock_ref_sha1() stores return value of mkpath()'s get_pathname()
> > call, then calls lock_ref_sha1_basic() which calls git_path(ref) which
> > calls get_pathname() at that point returning pointer to the same buffer.
> 
> Hmm. This was exactly the schenario why I did commit
> e7676d2f6454c9c99e600ee2ce3c7205a9fcfb5f - allowing a couple of 
> overlapping paths
> 
> Isn't that in the "next" branch too?

Yes, and between the mkpath() and git_path() calls exactly three other
get_pathname() calls happen.

Of course you could just enlarge the cache, but that will merely make
the bugs even harder to spot, let alone track down. I argue that having
this cache at all is harmful and will result in bugs over time as new
get_pathname() calls are added in the middle of currently safe
"concurrent" calls.

> Of course, that still assumes that you strdup() the result at _some_ time, 
> and can't just save it away, but lock_ref_sha1_basic() should do that.

lock_ref_sha1_basic() never strdup()s ref (at least the reference used
for git_path() later).

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Snow falling on Perl. White noise covering line noise.
Hides all the bugs too. -- J. Putnam

Re: [PATCH] Fix broken sha1 locking

[Linus Torvalds]

On Tue, 19 Sep 2006, Petr Baudis wrote:
>
> lock_ref_sha1_basic() never strdup()s ref (at least the reference used
> for git_path() later).

Ahh. So how did that ever work? Even in the current "master" branch, we 
call "resolve_ref()", which calls "git_path()", and historically we only 
had a single buffer..

So we're doing

	lock_ref_sha1_basic(git_path("refs/%s", ref),...

with that single buffer, and we _do_ do "xstrdup(path)" in between.

[ looks.. ]

Ahh.

We re-assign the "path" to point to the result value of the resolve_ref(), 
and so we do re-use the buffer, but we apparently never have any 
overlapping use. The packed-ref changes make us need pathnames in the 
middle, which we didn't use to do.

I do agree that we tend to use too many static buffers there. The static 
buffers are fine for the low-level functions ("mkpath()" and 
"git_path()"), but once they start getting passed around as arguments to 
other functions, they should be xstrdup'd or something.

		Linus