{"thread":{"id":"11389","subject":"[PATCH] Don't dereference NULL upon lookup_tree failure.","startedAt":"2007-12-21T22:32:45Z","lastAt":"2007-12-22T18:32:53Z","messageCount":10,"participants":["Jim Meyering","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"63961","messageId":"873atvwueq.fsf@rho.meyering.net","threadId":"11389","inReplyTo":null,"subject":"[PATCH] Don't dereference NULL upon lookup_tree failure.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-12-21T22:32:45Z","receivedAt":"2007-12-21T22:32:45Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"While Matthew Farrellee was working on converting the Condor repository\nfrom cvs to git, he managed to make git segfault (with help from parsecvs)\nwhile producing a 100MB .git repository. He found the single offending\n,v file that led parsecvs to generate a bogus repository, and deduced\nthat adding a single well-placed branch tag[*] was enough to avoid\nthe problem. I fixed the bug in git along with a few in parsecvs that\nwere exposed while paring the ,v file down from a 150KB monstrosity to\nsomething manageable.\n\n[*] Adding this tag avoids the problem: FOO:1.30.2.5.0.8\n\nFirst, here's how to reproduce the git segfault, using the file, \"min,v\"\nincluded below. Run this in an empty directory:\n\n rm -rf k .git .git-cvs\n parsecvs min,v >& log\n git clone -q . k\n\nI noticed that while I get a segfault both on x86 and x86_64, I see\nclear evidence of it only on x86:\n\n 0 blocks\n error: Object 0d57588da39d10795486bd5451bc2660832228e6 is a commit, not a tree\n Segmentation fault\n fatal: The remote end hung up unexpectedly\n [Exit 1]\n\nWhen running on an x86_64 system (either debian unstable or rawhide)\nI see only this:\n\n 0 blocks\n error: Object 0d57588da39d10795486bd5451bc2660832228e6 is a commit, not a tree\n fatal: The remote end hung up unexpectedly\n\nOf course, valgrind shows you the segfault in both cases.\n\nThe above was using git version 1.5.4.rc1.3.gec692\nand the latest parsecvs from here:\ngit://people.freedesktop.org/~keithp/parsecvs\n\n commit 2b0113ffb0055620193397c025d6f6bca3b110cd\n Author: Finn Arne Gangstad \n Date: Sun Nov 18 15:26:35 2007 -0800\n\nThis patch avoids the NULL dereference by treating a failed lookup_tree the\nsame way an invalid \"type\" is handled in the \"else\" block just below.\nThe only difference is that for a failed lookup_tree, the failing\nfunction has already produced a diagnostic.\n\n-----------------------------------------------------\nFrom 4cd649160d8174b23727b3d7276f1bd7246d0aff Mon Sep 17 00:00:00 2001\nFrom: Jim Meyering \nDate: Fri, 21 Dec 2007 11:56:32 +0100\nSubject: [PATCH] Don't dereference NULL upon lookup_tree failure.\n\n\nSigned-off-by: Jim Meyering \n---\n object.c | 12 ++++++++----\n 1 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 16793d9..eb59550 100644\n--- a/object.c\n+++ b/object.c\n@@ -142,10 +142,14 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t\tobj = &blob->object;\n \t} else if (type == OBJ_TREE) {\n \t\tstruct tree *tree = lookup_tree(sha1);\n-\t\tobj = &tree->object;\n-\t\tif (!tree->object.parsed) {\n-\t\t\tparse_tree_buffer(tree, buffer, size);\n-\t\t\teaten = 1;\n+\t\tif (!tree)\n+\t\t obj = NULL;\n+\t\telse {\n+\t\t obj = &tree->object;\n+\t\t if (!tree->object.parsed) {\n+\t\t\t parse_tree_buffer(tree, buffer, size);\n+\t\t\t eaten = 1;\n+\t\t }\n \t\t}\n \t} else if (type == OBJ_COMMIT) {\n \t\tstruct commit *commit = lookup_commit(sha1);\n--\n1.5.4.rc0.76.g55ee\n\n\nHere's the contents of min,v\n------------------------------------------------\nhead\t1.31;\naccess;\nsymbols\n\ta:1.30.2.18.4.14.4.29.0.6\n\tb:1.30.2.18.4.14.4.29.0.4\n\tc:1.30.2.18.4.14.4.29.0.2\n\td:1.30.2.18.4.14.4.27.0.6\n\te:1.30.2.18.4.14.4.27.0.4\n\tf:1.30.2.18.4.14.4.27.0.2\n\tg:1.30.2.18.4.14.4.25.0.6\n\th:1.30.2.18.4.14.4.25.0.4\n\ti:1.30.2.18.4.14.4.25.0.2\n\tj:1.30.2.18.4.14.4.24.0.24\n\tk:1.30.2.18.4.14.4.24.0.22\n\tl:1.30.2.18.4.14.4.24.0.20\n\tm:1.30.2.18.4.14.4.24.0.18\n\tn:1.30.2.18.4.14.4.24.0.16\n\to:1.30.2.18.4.14.4.24.0.14\n\tp:1.30.2.18.4.14.4.24.0.12\n\tq:1.30.2.18.4.14.4.24.0.10\n\tr:1.30.2.18.4.14.4.24.0.8\n\ts:1.30.2.18.4.14.4.24.0.6\n\tt:1.30.2.18.4.14.4.24.0.4\n\tu:1.30.2.18.4.14.4.24.0.2\n\ta0:1.30.2.18.4.14.4.23.0.10\n\ta1:1.30.2.18.4.14.4.23.0.8\n\ta2:1.30.2.18.4.14.4.23.0.6\n\ta3:1.30.2.18.4.14.4.23.0.4\n\ta4:1.30.2.18.4.14.4.23.0.2\n\ta5:1.30.2.18.4.14.4.22.0.2\n\ta6:1.30.2.18.4.14.4.21.0.2\n\ta7:1.30.2.18.4.14.4.18.4.1.0.2\n\ta8:1.30.2.18.4.14.4.18.0.6\n\ta9:1.30.2.18.4.14.4.18.0.4\n\ta10:1.30.2.18.4.14.4.17.0.12\n\ta11:1.30.2.18.4.14.4.18.0.2\n\ta12:1.30.2.18.4.14.4.17.0.10\n\ta13:1.30.2.18.4.14.4.17.0.8\n\ta14:1.30.2.18.4.14.4.17.0.6\n\ta15:1.30.2.18.4.14.4.17.0.4\n\ta16:1.30.2.18.4.14.4.17.0.2\n\ta17:1.30.2.18.4.14.4.16.0.2\n\ta18:1.30.2.18.4.14.4.15.0.2\n\ta19:1.30.2.18.4.14.4.14.0.10\n\ta20:1.30.2.18.4.14.4.14.0.8\n\ta21:1.30.2.18.4.14.4.14.0.6\n\ta22:1.30.2.18.4.14.4.14.0.4\n\ta23:1.30.2.18.4.14.4.14.0.2\n\ta24:1.30.2.18.4.14.4.13.0.4\n\ta25:1.30.2.18.4.14.4.13.0.2\n\ta26:1.30.2.18.4.14.4.7.0.6\n\ta27:1.30.2.18.4.14.4.12.0.8\n\ta28:1.30.2.18.4.14.4.12.0.6\n\ta29:1.30.2.18.4.14.4.12.0.4\n\ta30:1.30.2.18.4.14.4.12.0.2\n\ta31:1.30.2.18.4.14.4.11.0.4\n\ta32:1.30.2.18.4.14.4.11.0.2\n\ta33:1.30.2.18.4.14.4.9.0.16\n\ta34:1.30.2.18.4.14.4.9.0.14\n\ta35:1.30.2.18.4.14.4.9.0.12\n\ta36:1.30.2.18.4.14.4.9.0.10\n\ta37:1.30.2.18.4.14.4.9.0.8\n\ta38:1.30.2.18.4.14.4.9.0.6\n\ta39:1.30.2.18.4.14.4.9.0.4\n\ta40:1.30.2.18.4.14.4.9.0.2\n\ta41:1.30.2.18.4.14.4.8.0.16\n\ta42:1.30.2.18.4.14.4.8.0.14\n\ta43:1.30.2.18.4.14.4.8.0.12\n\ta44:1.30.2.18.4.14.4.8.0.10\n\ta45:1.30.2.18.4.14.4.8.0.8\n\ta46:1.30.2.18.4.14.4.8.0.6\n\ta47:1.30.2.18.4.14.4.8.0.4\n\ta48:1.30.2.18.4.14.4.8.0.2\n\ta49:1.30.2.18.4.14.4.7.0.4\n\ta50:1.30.2.18.4.14.4.7.0.2\n\ta51:1.30.2.18.4.14.4.6.0.2\n\ta52:1.30.2.18.4.14.4.3.4.1.0.2\n\ta53:1.30.2.18.4.14.4.5.0.2\n\ta54:1.30.2.18.4.14.4.5.0.14\n\ta55:1.30.2.18.4.14.4.5.0.12\n\ta56:1.30.2.18.4.14.4.5.0.10\n\ta57:1.30.2.18.4.14.4.5.0.8\n\ta58:1.30.2.18.4.14.4.5.0.6\n\ta59:1.30.2.18.4.14.4.5.0.4\n\ta60:1.30.2.18.4.14.4.3.0.4\n\ta61:1.30.2.18.4.14.4.3.0.2\n\ta62:1.30.2.18.4.14.4.2.0.16\n\ta63:1.30.2.18.4.14.4.2.0.14\n\ta64:1.30.2.18.4.14.4.2.0.12\n\ta65:1.30.2.18.4.14.4.2.0.10\n\ta66:1.30.2.18.4.14.4.2.0.8\n\ta67:1.30.2.18.4.14.4.2.0.6\n\ta68:1.30.2.18.4.14.4.2.0.4\n\ta69:1.30.2.18.4.14.4.2.0.2\n\ta70:1.30.2.18.4.14.4.1.0.4\n\ta71:1.30.2.18.4.14.4.1.0.2\n\ta72:1.30.2.18.4.14.0.4\n\ta73:1.30.2.18.4.14.0.2\n\ta74:1.30.2.18.4.13.0.2\n\ta75:1.30.2.18.4.11.0.4\n\ta76:1.30.2.18.4.11.0.2\n\ta77:1.30.2.18.4.10.0.2\n\ta78:1.30.2.18.4.6.0.2\n\ta79:1.30.2.18.4.3.0.4\n\ta80:1.30.2.18.4.5.0.4\n\ta81:1.30.2.18.4.5.0.2\n\ta82:1.30.2.18.4.3.0.2\n\ta83:1.30.2.18.2.8.0.2\n\ta84:1.30.2.18.4.1.0.8\n\ta85:1.30.2.18.4.1.0.6\n\ta86:1.30.2.18.4.1.0.4\n\ta87:1.30.2.18.4.1.0.2\n\ta88:1.30.2.18.0.4\n\ta89:1.30.2.18.0.2\n\ta90:1.30.2.14.0.2\n\ta91:1.31.0.4\n\ta92:1.31.0.2\n\ta93:1.30.2.5.0.14\n\ta94:1.30.2.5.0.12\n\ta95:1.30.2.5.0.10\n\ta96:1.30.2.5.0.6\n\ta97:1.30.2.5.0.4\n\ta98:1.30.2.5.0.2\n\ta99:1.30.2.4.0.2\n\ta100:1.30.2.3.0.8\n\ta101:1.30.2.3.0.6\n\ta102:1.30.2.3.0.2\n\ta103:1.30.2.3.0.4\n\ta104:1.30.2.1.0.2\n\ta105:1.30.0.2\n\tnew-syscall-branch:1.14.0.2\n\tV6_0-branch:1.6.0.2;\nlocks; strict;\ncomment\t@ * @;\n\n\n1.31\ndate\t2000.07.06.19.46.55;\tauthor x;\tstate Exp;\nbranches\n\t1.31.4.1;\nnext\t1.30;\n\n1.30\ndate\t2000.07.06.17.21.26;\tauthor x;\tstate Exp;\nbranches\n\t1.30.2.1;\nnext\t1.14;\n\n1.14\ndate\t99.03.09.23.33.53;\tauthor x;\tstate Exp;\nbranches\n\t1.14.6.1;\nnext\t;\n\n1.14.6.1\ndate\t99.03.17.05.07.10;\tauthor x;\tstate Exp;\nbranches;\nnext\t;\n\n1.30.2.1\ndate\t2000.12.13.20.13.52;\tauthor x;\tstate Exp;\nbranches;\nnext\t1.30.2.3;\n\n1.30.2.3\ndate\t2001.03.28.18.40.01;\tauthor x;\tstate Exp;\nbranches;\nnext\t1.30.2.4;\n\n1.30.2.4\ndate\t2001.08.21.21.14.44;\tauthor x;\tstate Exp;\nbranches;\nnext\t1.30.2.5;\n\n1.30.2.5\ndate\t2001.10.17.20.05.09;\tauthor x;\tstate Exp;\nbranches\n\t1.30.2.5.8.1;\nnext\t1.30.2.14;\n\n1.30.2.14\ndate\t2002.06.13.17.02.53;\tauthor x;\tstate Exp;\nbranches;\nnext\t1.30.2.18;\n\n1.30.2.18\ndate\t2002.07.18.22.25.03;\tauthor x;\tstate Exp;\nbranches;\nnext\t;\n\n1.30.2.5.8.1\ndate\t2002.01.29.00.03.25;\tauthor x;\tstate Exp;\nbranches;\nnext\t;\n\n1.31.4.1\ndate\t2002.04.16.22.46.40;\tauthor x;\tstate Exp;\nbranches;\nnext\t;\n\n\ndesc\n@@\n\n\n1.31\nlog\n@6.5.0 version string, on the trunk.\n@\ntext\n@\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n@\n\n\n1.31.4.1\nlog\n@\n@\ntext\n@d35 1\na35 1\n\n@\n\n\n1.30\nlog\n@\n@\ntext\n@d35 1\na35 1\n\n@\n\n\n1.30.2.1\nlog\n@\n@\ntext\n@d35 1\na35 1\n\n@\n\n\n1.30.2.3\nlog\n@\n@\ntext\n@d35 1\na35 1\n\n@\n\n\n1.30.2.4\nlog\n@\n@\ntext\n@d35 1\na35 1\n\n@\n\n\n1.30.2.5\nlog\n@\n@\ntext\n@d35 1\na35 1\n\n@\n\n\n1.30.2.14\nlog\n@\n@\ntext\n@d35 1\na35 1\n\n@\n\n\n1.30.2.18\nlog\n@\n@\ntext\n@d35 1\na35 1\n\n@\n\n\n1.30.2.5.8.1\nlog\n@\n@\ntext\n@d35 1\na35 1\n\n@\n\n\n1.14\nlog\n@\n@\ntext\n@d25 43\na67 1\ns\nd72 1\na72 1\nC\nd77 6\nd84 1\n@\n\n\n1.14.6.1\nlog\n@\n@\ntext\n@d25 1\na25 1\n\n@\n"},{"id":"63963","messageId":"7v4peb4qbj.fsf@gitster.siamese.dyndns.org","threadId":"11389","inReplyTo":"873atvwueq.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't dereference NULL upon lookup_tree failure.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-21T22:48:32Z","receivedAt":"2007-12-21T22:48:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n> When running on an x86_64 system (either debian unstable or rawhide)\n> I see only this:\n>\n> 0 blocks\n> error: Object 0d57588da39d10795486bd5451bc2660832228e6 is a commit, not a tree\n> fatal: The remote end hung up unexpectedly\n>...\n> diff --git a/object.c b/object.c\n> index 16793d9..eb59550 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -142,10 +142,14 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n> \t\tobj = &blob->object;\n> \t} else if (type == OBJ_TREE) {\n> \t\tstruct tree *tree = lookup_tree(sha1);\n> -\t\tobj = &tree->object;\n> -\t\tif (!tree->object.parsed) {\n> -\t\t\tparse_tree_buffer(tree, buffer, size);\n> -\t\t\teaten = 1;\n> +\t\tif (!tree)\n> +\t\t obj = NULL;\n> +\t\telse {\n> +\t\t obj = &tree->object;\n> +\t\t if (!tree->object.parsed) {\n> +\t\t\t parse_tree_buffer(tree, buffer, size);\n> +\t\t\t eaten = 1;\n> +\t\t }\n> \t\t}\n> \t} else if (type == OBJ_COMMIT) {\n> \t\tstruct commit *commit = lookup_commit(sha1);\n\nWhile this change may be a prudent safeguard, there is something\nelse going on. Can you provide the callchain that led to the\nparse_object_buffer() that gave SHA1 of a commit object with\ntype set to OBJ_TREE? Which caller does that bogus combination?\n"},{"id":"63964","messageId":"87hcibveut.fsf@rho.meyering.net","threadId":"11389","inReplyTo":"7v4peb4qbj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Don't dereference NULL upon lookup_tree failure.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-12-21T22:54:02Z","receivedAt":"2007-12-21T22:54:02Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n\n> Jim Meyering writes:\n>\n>> When running on an x86_64 system (either debian unstable or rawhide)\n>> I see only this:\n>>\n>> 0 blocks\n>> error: Object 0d57588da39d10795486bd5451bc2660832228e6 is a commit, not a tree\n>> fatal: The remote end hung up unexpectedly\n>>...\n>> diff --git a/object.c b/object.c\n>> index 16793d9..eb59550 100644\n>> --- a/object.c\n>> +++ b/object.c\n>> @@ -142,10 +142,14 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n>> \t\tobj = &blob->object;\n>> \t} else if (type == OBJ_TREE) {\n>> \t\tstruct tree *tree = lookup_tree(sha1);\n>> -\t\tobj = &tree->object;\n>> -\t\tif (!tree->object.parsed) {\n>> -\t\t\tparse_tree_buffer(tree, buffer, size);\n>> -\t\t\teaten = 1;\n>> +\t\tif (!tree)\n>> +\t\t obj = NULL;\n>> +\t\telse {\n>> +\t\t obj = &tree->object;\n>> +\t\t if (!tree->object.parsed) {\n>> +\t\t\t parse_tree_buffer(tree, buffer, size);\n>> +\t\t\t eaten = 1;\n>> +\t\t }\n>> \t\t}\n>> \t} else if (type == OBJ_COMMIT) {\n>> \t\tstruct commit *commit = lookup_commit(sha1);\n>\n> While this change may be a prudent safeguard, there is something\n> else going on. Can you provide the callchain that led to the\n> parse_object_buffer() that gave SHA1 of a commit object with\n> type set to OBJ_TREE? Which caller does that bogus combination?\n\nSure.\nHere's valgrind output from running this (from my reproducer):\n\n valgrind --trace-children=yes git clone . k\n---------------\n error: Object 0d57588da39d10795486bd5451bc2660832228e6 is a commit, not a tree\n ==9483== Invalid read of size 1\n ==9483== at 0x405C27: parse_object_buffer (object.c:146)\n ==9483== by 0x405CE4: parse_object (object.c:187)\n ==9483== by 0x403185: send_ref (upload-pack.c:561)\n ==9483== by 0x408EEF: do_for_each_ref (refs.c:546)\n ==9483== by 0x4036EC: main (upload-pack.c:587)\n ==9483== Address 0x0 is not stack'd, malloc'd or (recently) free'd\n ==9483==\n ==9483== Process terminating with default action of signal 11 (SIGSEGV)\n ==9483== Access not within mapped region at address 0x0\n ==9483== at 0x405C27: parse_object_buffer (object.c:146)\n ==9483== by 0x405CE4: parse_object (object.c:187)\n ==9483== by 0x403185: send_ref (upload-pack.c:561)\n ==9483== by 0x408EEF: do_for_each_ref (refs.c:546)\n ==9483== by 0x4036EC: main (upload-pack.c:587)\n ...\n fatal: The remote end hung up unexpectedly\n"},{"id":"63968","messageId":"7vzlw33a1o.fsf@gitster.siamese.dyndns.org","threadId":"11389","inReplyTo":"87hcibveut.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't dereference NULL upon lookup_tree failure.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-21T23:25:23Z","receivedAt":"2007-12-21T23:25:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n>> While this change may be a prudent safeguard, there is something\n>> else going on. Can you provide the callchain that led to the\n>> parse_object_buffer() that gave SHA1 of a commit object with\n>> type set to OBJ_TREE? Which caller does that bogus combination?\n>\n> Sure.\n> Here's valgrind output from running this (from my reproducer):\n>\n> valgrind --trace-children=yes git clone . k\n> ---------------\n> error: Object 0d57588da39d10795486bd5451bc2660832228e6 is a commit, not a tree\n> ==9483== Invalid read of size 1\n> ==9483== at 0x405C27: parse_object_buffer (object.c:146)\n> ==9483== by 0x405CE4: parse_object (object.c:187)\n> ==9483== by 0x403185: send_ref (upload-pack.c:561)\n> ==9483== by 0x408EEF: do_for_each_ref (refs.c:546)\n> ==9483== by 0x4036EC: main (upload-pack.c:587)\n\nSorry, I asked for a wrong thing. parse_object() reads and\nfinds out the type, so type there is presumably the right type\nof the object (which is OBJ_TREE). Then parse_object_buffer()\nchecks if it has already seen the object of the same SHA-1 and\nfinds that somebody had earlier told that SHA-1 name is of a\ncommit object. Either you found a SHA-1 collision (highly\nunlikely) or the earlier caller had lied. And I think what\nreally needs to be fixed is that lying caller. That is not in\nthe above call chain.\n"},{"id":"63969","messageId":"87bq8jvd1g.fsf@rho.meyering.net","threadId":"11389","inReplyTo":"7vzlw33a1o.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Don't dereference NULL upon lookup_tree failure.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-12-21T23:33:15Z","receivedAt":"2007-12-21T23:33:15Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n> Jim Meyering writes:\n>\n>>> While this change may be a prudent safeguard, there is something\n>>> else going on. Can you provide the callchain that led to the\n>>> parse_object_buffer() that gave SHA1 of a commit object with\n>>> type set to OBJ_TREE? Which caller does that bogus combination?\n>>\n>> Sure.\n>> Here's valgrind output from running this (from my reproducer):\n>>\n>> valgrind --trace-children=yes git clone . k\n>> ---------------\n>> error: Object 0d57588da39d10795486bd5451bc2660832228e6 is a commit, not a tree\n>> ==9483== Invalid read of size 1\n>> ==9483== at 0x405C27: parse_object_buffer (object.c:146)\n>> ==9483== by 0x405CE4: parse_object (object.c:187)\n>> ==9483== by 0x403185: send_ref (upload-pack.c:561)\n>> ==9483== by 0x408EEF: do_for_each_ref (refs.c:546)\n>> ==9483== by 0x4036EC: main (upload-pack.c:587)\n>\n> Sorry, I asked for a wrong thing. parse_object() reads and\n> finds out the type, so type there is presumably the right type\n> of the object (which is OBJ_TREE). Then parse_object_buffer()\n> checks if it has already seen the object of the same SHA-1 and\n> finds that somebody had earlier told that SHA-1 name is of a\n> commit object. Either you found a SHA-1 collision (highly\n> unlikely) or the earlier caller had lied. And I think what\n> really needs to be fixed is that lying caller. That is not in\n> the above call chain.\n\nI haven't debugged it enough to find/fix the bug in parsecvs.\n\nI presume that parsecvs is the culprit, in constructing\nan invalid repository -- especially, since inserting the\nsingle missing tag into the ,v file is enough to make it so\nparsecvs creates a repository that no longer triggers the git bug.\n"},{"id":"63970","messageId":"7vve6r39bp.fsf@gitster.siamese.dyndns.org","threadId":"11389","inReplyTo":"87bq8jvd1g.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't dereference NULL upon lookup_tree failure.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-21T23:40:58Z","receivedAt":"2007-12-21T23:40:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n> Junio C Hamano wrote:\n> ...\n>> Sorry, I asked for a wrong thing. parse_object() reads and\n>> finds out the type, so type there is presumably the right type\n>> of the object (which is OBJ_TREE). Then parse_object_buffer()\n>> checks if it has already seen the object of the same SHA-1 and\n>> finds that somebody had earlier told that SHA-1 name is of a\n>> commit object. Either you found a SHA-1 collision (highly\n>> unlikely) or the earlier caller had lied. And I think what\n>> really needs to be fixed is that lying caller. That is not in\n>> the above call chain.\n>\n> I presume that parsecvs is the culprit, in constructing\n> an invalid repository...\n\nIndependent of who creates a \"invalid repository\", our tools\nshould be prepared to be fed bad data and not get upset.\nSomebody in our code read a non-commit (or it did not read\nanything) and told the object layer it is a commit. That\ncodepath needs to be tightened up, no?\n"},{"id":"63971","messageId":"8763yrvb3g.fsf@rho.meyering.net","threadId":"11389","inReplyTo":"7vve6r39bp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Don't dereference NULL upon lookup_tree failure.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-12-22T00:15:15Z","receivedAt":"2007-12-22T00:15:15Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n\n> Jim Meyering writes:\n>\n>> Junio C Hamano wrote:\n>> ...\n>>> Sorry, I asked for a wrong thing. parse_object() reads and\n>>> finds out the type, so type there is presumably the right type\n>>> of the object (which is OBJ_TREE). Then parse_object_buffer()\n>>> checks if it has already seen the object of the same SHA-1 and\n>>> finds that somebody had earlier told that SHA-1 name is of a\n>>> commit object. Either you found a SHA-1 collision (highly\n>>> unlikely) or the earlier caller had lied. And I think what\n>>> really needs to be fixed is that lying caller. That is not in\n>>> the above call chain.\n>>\n>> I presume that parsecvs is the culprit, in constructing\n>> an invalid repository...\n>\n> Independent of who creates a \"invalid repository\", our tools\n> should be prepared to be fed bad data and not get upset.\n> Somebody in our code read a non-commit (or it did not read\n> anything) and told the object layer it is a commit. That\n> codepath needs to be tightened up, no?\n\nYes, of course.\nUnfortunately, I won't have time to investigate for a while.\n"},{"id":"63991","messageId":"7vzlw3xeqy.fsf@gitster.siamese.dyndns.org","threadId":"11389","inReplyTo":"8763yrvb3g.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't dereference NULL upon lookup_tree failure.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-22T09:25:41Z","receivedAt":"2007-12-22T09:25:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n>>> Junio C Hamano wrote:\n>>> ...\n>> Independent of who creates a \"invalid repository\", our tools\n>> should be prepared to be fed bad data and not get upset.\n>> Somebody in our code read a non-commit (or it did not read\n>> anything) and told the object layer it is a commit. That\n>> codepath needs to be tightened up, no?\n>\n> Yes, of course.\n> Unfortunately, I won't have time to investigate for a while.\n\nAlthough I haven't seen the problematic repository, I think what\nis going on is that the repository has objects with pointers to\nother objects labelled with wrong types.\n\nFor example, a tag can tag any type of object.\n\n $ git cat-file tag v1.5.4-rc1\n object 74f6b03c5c8fceef416de9f9a18e5d64712b6d96\n type commit\n tag v1.5.4-rc1\n tagger Junio C Hamano 1198114975 -0800\n\n GIT 1.5.4-rc1\n ...\n\nWhen parsing such a tag object, it instantiates an in-core tag\nobject that has a pointer to an in-core commit object, because\nthe tag says that it points at commit 74f6b0, without reading\nthe tagged object itself. This is to allow the user of that\nin-core tag object to ask \"what do you point at\" and get an\nin-core object that represents the pointee. If (and only if)\nthe user choose to follow that pointer and look at the contents\nof the pointee, it can then call parse_object() on that in-core\nobject. In other words, the object layer is designed to work\nlazily.\n\nSo if you earlier parsed such a tag but if the object 74f6b0\nwere an object of different type, we would hit the codepath you\nidentified that leads to a NULL dereference. And \"lying caller\"\ncannot really do much better (except perhaps actually validating\nthe presense and the type of the object, which would incur\nadditional overhead). The situation is the same if:\n\n - a tree object records an entry with: \n - mode bits 040000 pointing at an object that is not a tree; or\n - mode bits 100(644|755) pointing at an object that is not a blob; or\n - mode bits 160000 pointing at an object that is not a commit; or\n - a commit object has a \"parent\" line that is not a commit; or\n - a commit object has a \"tree\" line that is not a tree.\n\nAlso we cannot just say \"then we would take safety over\noverhead\" and make the lying callers validate the pointee;\nsubproject commits pointed by gitlinks (i.e. entry with 160000\nmode bits in a tree object) are not even required to be\navailable.\n\nYour fix is all we can sensibly do. I however think you would\nneed similar fix to the same function for other object types, as\nthey dereference a potentially NULL pointer the same way.\n"},{"id":"63993","messageId":"87prwyu9r0.fsf@rho.meyering.net","threadId":"11389","inReplyTo":"7vzlw3xeqy.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Don't dereference NULL upon lookup_tree failure.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-12-22T13:41:55Z","receivedAt":"2007-12-22T13:41:55Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n...\n> Your fix is all we can sensibly do. I however think you would\n> need similar fix to the same function for other object types, as\n> they dereference a potentially NULL pointer the same way.\n\nGood point. Patch below. I wondered if these lookup functions have\nalways been able to return NULL, since there are many remaining uses\nwhere the possibility of a NULL return value is not handled.\nHere are a few that stood out in the output of a quick search:\n\n git-grep -E -A3 'lookup_(commit|tag|blob|tree) *\\('\n\nNote: I did not look at cases where the return value is an argument\nto some other function.\n\n------------------\nbuiltin-fmt-merge-msg.c:\n\t\thead = lookup_commit(head_sha1);\n\t\t...\n\t\tfor (i = 0; i < origins.nr; i++)\n\t\t\tshortlog(origins.list[i], origins.payload[i],\n\t\t\t\t\thead, &rev, limit);\n------------------\nbuiltin-read-tree.c: struct tree *subtree = lookup_tree(entry.\nsha1);\nbuiltin-read-tree.c- if (!subtree->object.parsed)\n\n------------------\ncombine-diff.c: struct commit *commit = lookup_commit(sha1);\ncombine-diff.c- struct commit_list *parents;\n\n------------------\nhttp-push.c: struct commit *head = lookup_commit(head_sha1);\nhttp-push.c: struct commit *branch = lookup_commit(branch_sha1);\nhttp-push.c- struct commit_list *merge_bases = get_merge_bases(head, branch, 1);\n\n------------------\nreachable.c: struct tree *tree = lookup_tree(sha1);\nreachable.c- add_pending_object(revs, &tree->object, \"\");\n\n------------------\ntag.c: item->tagged = &lookup_blob(sha1)->object;\ntag.c- } else if (!strcmp(type, tree_type)) {\ntag.c: item->tagged = &lookup_tree(sha1)->object;\ntag.c- } else if (!strcmp(type, commit_type)) {\ntag.c: item->tagged = &lookup_commit(sha1)->object;\ntag.c- } else if (!strcmp(type, tag_type)) {\ntag.c: item->tagged = &lookup_tag(sha1)->object;\ntag.c- } else {\n\n--------------------------\nunpack-trees.c: struct tree *tree = lookup_tree(posns[i]->sha1);\n...\nunpack-trees.c- parse_tree(tree);\n\n\nHere's the revised patch:\n\n==================================================================\nFrom 94152217e8e57d3932b4ba6f7ee014da1f4346d3 Mon Sep 17 00:00:00 2001\nFrom: Jim Meyering \nDate: Fri, 21 Dec 2007 11:56:32 +0100\nSubject: [PATCH] Don't dereference NULL upon lookup failure.\n\n\nSigned-off-by: Jim Meyering \n---\n object.c | 42 +++++++++++++++++++++++++++++-------------\n 1 files changed, 29 insertions(+), 13 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 16793d9..9945b25 100644\n--- a/object.c\n+++ b/object.c\n@@ -138,27 +138,43 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n\n \tif (type == OBJ_BLOB) {\n \t\tstruct blob *blob = lookup_blob(sha1);\n-\t\tparse_blob_buffer(blob, buffer, size);\n-\t\tobj = &blob->object;\n+\t\tif (!blob)\n+\t\t obj = NULL;\n+\t\telse {\n+\t\t parse_blob_buffer(blob, buffer, size);\n+\t\t obj = &blob->object;\n+\t\t}\n \t} else if (type == OBJ_TREE) {\n \t\tstruct tree *tree = lookup_tree(sha1);\n-\t\tobj = &tree->object;\n-\t\tif (!tree->object.parsed) {\n-\t\t\tparse_tree_buffer(tree, buffer, size);\n-\t\t\teaten = 1;\n+\t\tif (!tree)\n+\t\t obj = NULL;\n+\t\telse {\n+\t\t obj = &tree->object;\n+\t\t if (!tree->object.parsed) {\n+\t\t\t parse_tree_buffer(tree, buffer, size);\n+\t\t\t eaten = 1;\n+\t\t }\n \t\t}\n \t} else if (type == OBJ_COMMIT) {\n \t\tstruct commit *commit = lookup_commit(sha1);\n-\t\tparse_commit_buffer(commit, buffer, size);\n-\t\tif (!commit->buffer) {\n-\t\t\tcommit->buffer = buffer;\n-\t\t\teaten = 1;\n+\t\tif (!commit)\n+\t\t obj = NULL;\n+\t\telse {\n+\t\t parse_commit_buffer(commit, buffer, size);\n+\t\t if (!commit->buffer) {\n+\t\t\t commit->buffer = buffer;\n+\t\t\t eaten = 1;\n+\t\t }\n+\t\t obj = &commit->object;\n \t\t}\n-\t\tobj = &commit->object;\n \t} else if (type == OBJ_TAG) {\n \t\tstruct tag *tag = lookup_tag(sha1);\n-\t\tparse_tag_buffer(tag, buffer, size);\n-\t\tobj = &tag->object;\n+\t\tif (!tag)\n+\t\t obj = NULL;\n+\t\telse {\n+\t\t parse_tag_buffer(tag, buffer, size);\n+\t\t obj = &tag->object;\n+\t\t}\n \t} else {\n \t\twarning(\"object %s has unknown type id %d\\n\", sha1_to_hex(sha1), type);\n \t\tobj = NULL;\n--\n1.5.4.rc1.16.g60f3b\n"},{"id":"64012","messageId":"7v63yqy3ze.fsf@gitster.siamese.dyndns.org","threadId":"11389","inReplyTo":"87prwyu9r0.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't dereference NULL upon lookup_tree failure.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-22T18:32:53Z","receivedAt":"2007-12-22T18:32:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"}]}