{"thread":{"id":"5607","subject":"[PATCH] Fix broken sha1 locking","startedAt":"2006-09-19T20:58:23Z","lastAt":"2006-09-19T22:10:58Z","messageCount":4,"participants":["Petr Baudis","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"27167","messageId":"20060919205823.18579.59604.stgit@machine.or.cz","threadId":"5607","inReplyTo":null,"subject":"[PATCH] Fix broken sha1 locking","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2006-09-19T20:58:23Z","receivedAt":"2006-09-19T20:58:23Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Current git#next is totally broken wrt. cloning over HTTP, generating refs\nat random directories. Of course it's caused by the static get_pathname()\nbuffer. lock_ref_sha1() stores return value of mkpath()'s get_pathname()\ncall, then calls lock_ref_sha1_basic() which calls git_path(ref) which\ncalls get_pathname() at that point returning pointer to the same buffer.\nSo now you are sprintf()ing a format string into itself, wow! The resulting\npathnames are really cute. (If you've been paying attention, yes, the\nmere fact that a format string _could_ write over itself is very wrong\nand probably exploitable here. See the other mail I've just sent.)\n\nI've never liked how we use return values of those functions so liberally,\nthe \"allow some random number of get_pathname() return values to work\nconcurrently\" is absolutely horrible pit and we've already fallen in this\nbefore IIRC. I consider it an awful coding practice, you add a call\nsomewhere and at some other point some distant caller of that breaks since\nit reuses the same return values. Not to mention this takes quite some time\nto debug.\n\nMy gut feeling tells me that there might be more of this.  I don't have\ntime to review the rest of the users of the refs.c functions though.\n\nSigned-off-by: Petr Baudis <pasky@suse.cz>\n---\n\n refs.c |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 134c0fc..7bd36e4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -462,10 +462,12 @@ static struct ref_lock *lock_ref_sha1_ba\n struct ref_lock *lock_ref_sha1(const char *ref,\n \tconst unsigned char *old_sha1, int mustexist)\n {\n+\tchar refpath[PATH_MAX];\n \tif (check_ref_format(ref))\n \t\treturn NULL;\n-\treturn lock_ref_sha1_basic(mkpath(\"refs/%s\", ref),\n-\t\t5 + strlen(ref), old_sha1, mustexist);\n+\tstrcpy(refpath, mkpath(\"refs/%s\", ref));\n+\treturn lock_ref_sha1_basic(refpath, strlen(refpath),\n+\t\told_sha1, mustexist);\n }\n \n struct ref_lock *lock_any_ref_for_update(const char *ref,\n"},{"id":"27170","messageId":"Pine.LNX.4.64.0609191411460.4388@g5.osdl.org","threadId":"5607","inReplyTo":"20060919205823.18579.59604.stgit@machine.or.cz","subject":"Re: [PATCH] Fix broken sha1 locking","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-09-19T21:16:04Z","receivedAt":"2006-09-19T21:16:04Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 19 Sep 2006, Petr Baudis wrote:\n>\n> Current git#next is totally broken wrt. cloning over HTTP, generating refs\n> at random directories. Of course it's caused by the static get_pathname()\n> buffer. lock_ref_sha1() stores return value of mkpath()'s get_pathname()\n> call, then calls lock_ref_sha1_basic() which calls git_path(ref) which\n> calls get_pathname() at that point returning pointer to the same buffer.\n\nHmm. This was exactly the schenario why I did commit\ne7676d2f6454c9c99e600ee2ce3c7205a9fcfb5f - allowing a couple of \noverlapping paths\n\nIsn't that in the \"next\" branch too?\n\nOf course, that still assumes that you strdup() the result at _some_ time, \nand can't just save it away, but lock_ref_sha1_basic() should do that.\n\n\t\t\tLinus\n"},{"id":"27172","messageId":"20060919212340.GC8259@pasky.or.cz","threadId":"5607","inReplyTo":"Pine.LNX.4.64.0609191411460.4388@g5.osdl.org","subject":"Re: [PATCH] Fix broken sha1 locking","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2006-09-19T21:23:40Z","receivedAt":"2006-09-19T21:23:40Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Tue, Sep 19, 2006 at 11:16:04PM CEST, I got a letter\nwhere Linus Torvalds <torvalds@osdl.org> said that...\n> On Tue, 19 Sep 2006, Petr Baudis wrote:\n> >\n> > Current git#next is totally broken wrt. cloning over HTTP, generating refs\n> > at random directories. Of course it's caused by the static get_pathname()\n> > buffer. lock_ref_sha1() stores return value of mkpath()'s get_pathname()\n> > call, then calls lock_ref_sha1_basic() which calls git_path(ref) which\n> > calls get_pathname() at that point returning pointer to the same buffer.\n> \n> Hmm. This was exactly the schenario why I did commit\n> e7676d2f6454c9c99e600ee2ce3c7205a9fcfb5f - allowing a couple of \n> overlapping paths\n> \n> Isn't that in the \"next\" branch too?\n\nYes, and between the mkpath() and git_path() calls exactly three other\nget_pathname() calls happen.\n\nOf course you could just enlarge the cache, but that will merely make\nthe bugs even harder to spot, let alone track down. I argue that having\nthis cache at all is harmful and will result in bugs over time as new\nget_pathname() calls are added in the middle of currently safe\n\"concurrent\" calls.\n\n> Of course, that still assumes that you strdup() the result at _some_ time, \n> and can't just save it away, but lock_ref_sha1_basic() should do that.\n\nlock_ref_sha1_basic() never strdup()s ref (at least the reference used\nfor git_path() later).\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nSnow falling on Perl. White noise covering line noise.\nHides all the bugs too. -- J. Putnam\n"},{"id":"27181","messageId":"Pine.LNX.4.64.0609191501311.4388@g5.osdl.org","threadId":"5607","inReplyTo":"20060919212340.GC8259@pasky.or.cz","subject":"Re: [PATCH] Fix broken sha1 locking","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-09-19T22:10:58Z","receivedAt":"2006-09-19T22:10:58Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 19 Sep 2006, Petr Baudis wrote:\n>\n> lock_ref_sha1_basic() never strdup()s ref (at least the reference used\n> for git_path() later).\n\nAhh. So how did that ever work? Even in the current \"master\" branch, we \ncall \"resolve_ref()\", which calls \"git_path()\", and historically we only \nhad a single buffer..\n\nSo we're doing\n\n\tlock_ref_sha1_basic(git_path(\"refs/%s\", ref),...\n\nwith that single buffer, and we _do_ do \"xstrdup(path)\" in between.\n\n[ looks.. ]\n\nAhh.\n\nWe re-assign the \"path\" to point to the result value of the resolve_ref(), \nand so we do re-use the buffer, but we apparently never have any \noverlapping use. The packed-ref changes make us need pathnames in the \nmiddle, which we didn't use to do.\n\nI do agree that we tend to use too many static buffers there. The static \nbuffers are fine for the low-level functions (\"mkpath()\" and \n\"git_path()\"), but once they start getting passed around as arguments to \nother functions, they should be xstrdup'd or something.\n\n\t\tLinus\n"}]}