threads / patch / 5607

patchFix broken sha1 locking

Subject: [PATCH] Fix broken sha1 locking

## tl;dr

4 messages between Sep 19, 2006 and Sep 19, 2006. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Petr Baudis· Sep 19, 2006, 20:58 UTC · lore

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(-)
Show changes to refs.c +4 −2
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,
Linus Torvalds· Sep 19, 2006, 21:16 UTC · re: Petr Baudis · lore

Re: [PATCH] Fix broken sha1 locking

On Tue, 19 Sep 2006, Petr Baudis wrote:
Show 6 quoted lines
>
> 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
Petr Baudis· Sep 19, 2006, 21:23 UTC · re: Linus Torvalds · lore

Re: [PATCH] Fix broken sha1 locking

Dear diary, on Tue, Sep 19, 2006 at 11:16:04PM CEST, I got a letter where Linus Torvalds <torvalds@osdl.org> said that...

Show 13 quoted lines
> 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
Linus Torvalds· Sep 19, 2006, 22:10 UTC · re: Petr Baudis · lore

Re: [PATCH] Fix broken sha1 locking

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

← back to recent threads