git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Petr Baudis <pasky@suse.cz>
To: Junio C Hamano <junkio@cox.net>
Cc: <git@vger.kernel.org>
Subject: [PATCH] Fix broken sha1 locking
Date: Tue, 19 Sep 2006 22:58:23 +0200	[thread overview]
Message-ID: <20060919205823.18579.59604.stgit@machine.or.cz> (raw)

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,

             reply	other threads:[~2006-09-19 20:58 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-09-19 20:58 Petr Baudis [this message]
2006-09-19 21:16 ` [PATCH] Fix broken sha1 locking Linus Torvalds
2006-09-19 21:23   ` Petr Baudis
2006-09-19 22:10     ` Linus Torvalds

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20060919205823.18579.59604.stgit@machine.or.cz \
    --to=pasky@suse.cz \
    --cc=git@vger.kernel.org \
    --cc=junkio@cox.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).