{"thread":{"id":"41304","subject":"parse_object does check_sha1_signature but not parse_object_buffer?","startedAt":"2016-02-02T01:57:01Z","lastAt":"2016-02-04T17:40:54Z","messageCount":7,"participants":["Mike Hommey","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"277201","messageId":"20160202015701.GA30444@glandium.org","threadId":"41304","inReplyTo":null,"subject":"parse_object does check_sha1_signature but not parse_object_buffer?","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-02-02T01:57:01Z","receivedAt":"2016-02-02T01:57:01Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Hi,\n\nYou might or might not be aware of this thread:\nhttps://groups.google.com/forum/#!topic/binary-transparency/f-BI4o8HZW0\n\nAnyways, this got me to take a look around, and I noticed that\nparse_object does SHA-1 validation through check_sha1_signature. What\nsurprised me is that parse_object_buffer doesn't. So we end up with\ninconsistent behavior across commands:\n\n$ git init\n$ echo a > a ; echo b > b\n$ git add a b\n$ git cat-file blob 78981922613b2afb6025042ff6bd878ac1994e85\na\n$ cp -f .git/objects/61/780798228d17af2d34fce4cfbdf35556832472 .git/objects/78/981922613b2afb6025042ff6bd878ac1994e85\n$ git cat-file blob 78981922613b2afb6025042ff6bd878ac1994e85\nb\n$ git show 78981922613b2afb6025042ff6bd878ac1994e85\nerror: sha1 mismatch 78981922613b2afb6025042ff6bd878ac1994e85\nfatal: bad object 78981922613b2afb6025042ff6bd878ac1994e85\n\nShouldn't parse_object_buffer also do check_sha1_signature?\n\nMike\n"},{"id":"277202","messageId":"20160202020611.GA30613@glandium.org","threadId":"41304","inReplyTo":"20160202015701.GA30444@glandium.org","subject":"Re: parse_object does check_sha1_signature but not parse_object_buffer?","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-02-02T02:06:11Z","receivedAt":"2016-02-02T02:06:11Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, Feb 02, 2016 at 10:57:01AM +0900, Mike Hommey wrote:\n> Hi,\n> \n> You might or might not be aware of this thread:\n> https://groups.google.com/forum/#!topic/binary-transparency/f-BI4o8HZW0\n> \n> Anyways, this got me to take a look around, and I noticed that\n> parse_object does SHA-1 validation through check_sha1_signature. What\n> surprised me is that parse_object_buffer doesn't. So we end up with\n> inconsistent behavior across commands:\n> \n> $ git init\n> $ echo a > a ; echo b > b\n> $ git add a b\n> $ git cat-file blob 78981922613b2afb6025042ff6bd878ac1994e85\n> a\n> $ cp -f .git/objects/61/780798228d17af2d34fce4cfbdf35556832472 .git/objects/78/981922613b2afb6025042ff6bd878ac1994e85\n> $ git cat-file blob 78981922613b2afb6025042ff6bd878ac1994e85\n> b\n> $ git show 78981922613b2afb6025042ff6bd878ac1994e85\n> error: sha1 mismatch 78981922613b2afb6025042ff6bd878ac1994e85\n> fatal: bad object 78981922613b2afb6025042ff6bd878ac1994e85\n> \n> Shouldn't parse_object_buffer also do check_sha1_signature?\n\nWell, except cat-file doesn't use parse_object_buffer either...\n\nMike\n"},{"id":"277204","messageId":"xmqq60y7u7sj.fsf@gitster.mtv.corp.google.com","threadId":"41304","inReplyTo":"20160202015701.GA30444@glandium.org","subject":"Re: parse_object does check_sha1_signature but not parse_object_buffer?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-02T03:10:04Z","receivedAt":"2016-02-02T03:10:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> Shouldn't parse_object_buffer also do check_sha1_signature?\n\nIn general, it shouldn't; its callers are supposed to do it as\nadditional check when/if needed. Callers like the one in fsck.c\ndoes not want to die after seeing one bad one. We want to report\nand keep checking other things.\n"},{"id":"277207","messageId":"20160202043628.GA10253@glandium.org","threadId":"41304","inReplyTo":"xmqq60y7u7sj.fsf@gitster.mtv.corp.google.com","subject":"Re: parse_object does check_sha1_signature but not parse_object_buffer?","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-02-02T04:36:28Z","receivedAt":"2016-02-02T04:36:28Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Mon, Feb 01, 2016 at 07:10:04PM -0800, Junio C Hamano wrote:\n> Mike Hommey writes:\n> \n> > Shouldn't parse_object_buffer also do check_sha1_signature?\n> \n> In general, it shouldn't; its callers are supposed to do it as\n> additional check when/if needed. Callers like the one in fsck.c\n> does not want to die after seeing one bad one. We want to report\n> and keep checking other things.\n\nShouldn't some things like, at least, `git checkout`, still check\nthe sha1s, though?\n\nMike\n"},{"id":"277268","messageId":"xmqqwpqmsymf.fsf@gitster.mtv.corp.google.com","threadId":"41304","inReplyTo":"20160202043628.GA10253@glandium.org","subject":"Re: parse_object does check_sha1_signature but not parse_object_buffer?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-02T19:25:44Z","receivedAt":"2016-02-02T19:25:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> On Mon, Feb 01, 2016 at 07:10:04PM -0800, Junio C Hamano wrote:\n>> Mike Hommey writes:\n>> \n>> > Shouldn't parse_object_buffer also do check_sha1_signature?\n>> \n>> In general, it shouldn't; its callers are supposed to do it as\n>> additional check when/if needed. Callers like the one in fsck.c\n>> does not want to die after seeing one bad one. We want to report\n>> and keep checking other things.\n>\n> Shouldn't some things like, at least, `git checkout`, still check\n> the sha1s, though?\n\nThat is a different issue--my answer was about the quoted question\nregarding parse_object_buffer(). Its callers are supposed to do\nadditional check when/if needed, and there may be codepaths that\ncurrently use parse_object_buffer() that may want to do their own\ncheck, or call a different function that does the check for them.\n\nHaving said that, I do not necessarily think \"git checkout\" should\nrevalidate the object name. The repository that you use for your\ndaily work would have the same error/corruption rate as your working\ntree files, and I do not think you would constantly \"validate\" what\nis in your working tree by comparing their contents with what you\nthink ought to be there.\n\nIf you are working on extremely poor quality disks and SSDs, it\nmight make sense to constantly revalidating the object data to catch\ncorruption early, as that is what we can do (as opposed to the\nworking tree files, corruption to which you probably do not have\nanything to catch bitflipping on).\n\nUnless you are placing your working tree on a filesystem with\nchecksums, but your object data would also be protected against\ncorruption in that case, so an extra revalidation at \"git checkout\"\ntime would not buy you much if anything at all.\n\nhttp://article.gmane.org/gmane.comp.version-control.git/283380 (not\nnecessarily the entire thread, but that exact article) is a\nreasonable summary that illustrates the way how we view the object\nintegrity.\n\n So \"index-pack\" is the enforcement point, and the rest of the\n git commands generally assume that we can trust what is on disk\n (as it is has either been generated by us, or checked by\n index-pack). The rest of the commands do not spend time\n checking that the on-disk contents are sane (though you can run\n git-fsck if you want to do that).\n\nIf anything, we may want to reduce the number of codepaths that\ncalls check_sha1_signature() on data that we know we have read from\nour own repository. Even though I am not opposed to an idea to have\na \"paranoid\" mode that revalidates the object name every time (and\nif \"git checkout\" does not currently, allow it to revalidate when we\nare operating under the \"paranoid\" mode), I do not think it should\nbe on by default.\n\nIn fact, I have this suspicion that the original justification to\nhave the call to check_sha1_signature() in parse_object() might have\nbeen invalidated by the restructuring of the code over the past 10\nyears. e9eefa67 ([PATCH] Add function to parse an object of\nunspecified type (take 2), 2005-04-28) says\n\n It also checks the hash of the file, due to its heritage as part\n of fsck-cache.\n\nI.e. we do not need this call here, as long as we make sure that\nfsck codepath does not depend on the fact that parse_object() calls\ncheck_sha1_signature() to validate the consistency between the data\nand the object name.\n\nIn fact, we do this, which is quite suboptimal:\n\n static int fsck_sha1(const unsigned char *sha1)\n {\n struct object *obj = parse_object(sha1);\n if (!obj) {\n errors_found |= ERROR_OBJECT;\n return error(\"%s: object corrupt or missing\",\n sha1_to_hex(sha1));\n }\n obj->flags |= HAS_OBJ;\n return fsck_obj(obj);\n }\n\nThis function is called for each loose object file we find in\nfsck_object_dir(), and there are a few problems:\n\n * The function parse_object() called from here would issue an error\n message and returns NULL; then you get another \"corrupt or\n missing\" error message, because this code cannot tell from the\n NULL which is the case.\n\n * The intent of the callchain to fsck_sha1() is to iterate over\n loose object files xx/x{38} and validate what is contained in\n them, but that behaviour is not guaranteed because it calls\n parse_object(), which may get the object data from a packfile\n if the loose object is also in the packfile.\n\nThis function should instead take \"path\" (the only caller of this\nfunction fsck_loose() has it), read the data in the file, hash the\ndata to validate that it matches \"sha1\" and then create the object\nout of that data it read by calling parse_object_buffer().\n\nI didn't check other callers of parse_object(), but I doubt that\nthey need or want a check_sha1_signature() call in the function.\n"},{"id":"277367","messageId":"20160204071919.GA14032@sigill.intra.peff.net","threadId":"41304","inReplyTo":"xmqqwpqmsymf.fsf@gitster.mtv.corp.google.com","subject":"Re: parse_object does check_sha1_signature but not parse_object_buffer?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-04T07:19:19Z","receivedAt":"2016-02-04T07:19:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 02, 2016 at 11:25:44AM -0800, Junio C Hamano wrote:\n\n> Having said that, I do not necessarily think \"git checkout\" should\n> revalidate the object name. The repository that you use for your\n> daily work would have the same error/corruption rate as your working\n> tree files, and I do not think you would constantly \"validate\" what\n> is in your working tree by comparing their contents with what you\n> think ought to be there.\n\nNo, but there is a question of how much it costs to do so.\n\nIn the past, I've spent a lot of time trying to speed up object access,\nbut it was usually about replacing `parse_object` with\n`lookup_object` or similar to avoid loading from disk in the first\nplace, as most of the time goes to zlib. If we are accessing the object\nalready (and obviously we have to for something like checkout), I'm not\nsure what the marginal cost is of computing the sha1 on the data as it\npasses through.\n\nIt might be significant, but I don't have numbers.\n\nIf it's not, then I think it's a nice feature that we would notice\nproblems earlier rather than later.\n\n> If you are working on extremely poor quality disks and SSDs, it\n> might make sense to constantly revalidating the object data to catch\n> corruption early, as that is what we can do (as opposed to the\n> working tree files, corruption to which you probably do not have\n> anything to catch bitflipping on).\n\nHopefully if your working tree files bit-flip, then you notice via `git\ndiff`. I guess you wouldn't for stat-clean ones. But if a bit flips in\nthe forest, and it's not stat-dirty enough for anyone to hear it, does\nit make a sound?\n\n> http://article.gmane.org/gmane.comp.version-control.git/283380 (not\n> necessarily the entire thread, but that exact article) is a\n> reasonable summary that illustrates the way how we view the object\n> integrity.\n> \n> So \"index-pack\" is the enforcement point, and the rest of the\n> git commands generally assume that we can trust what is on disk\n> (as it is has either been generated by us, or checked by\n> index-pack). The rest of the commands do not spend time\n> checking that the on-disk contents are sane (though you can run\n> git-fsck if you want to do that).\n\nI think that's me your quoting. I was specifically talking about\nmalicious tampering there. Which isn't to say I disagree with the\nworld-view you're proposing, but I think for random errors it's a little\nmore complicated. We certainly should be checking incoming data (and we\ndo).\n\nFor local repository operations, most of them are about reading data.\nAnd there I generally favor performance over extra validation, with the\ncaveat that we should always be aware of the tradeoff. An extra\ncomparison to make sure we are not going out-of-bounds on a pack .idx\npointer is cheap. Loading a blob just to make sure its sha1 is valid\nbefore we mention it in `diff --raw` output is stupid. Checking the sha1\non objects we are otherwise accessing is somewhere in between. :)\n\nFor local write operations, like repacking, we should err on the careful\nside. And I think we do a good job of balancing performance and\nvalidation there (e.g., we reuse deltas without reconstructing the\nobject, but _with_ a crc check on the delta data itself).\n\n> In fact, we do this, which is quite suboptimal:\n> \n> static int fsck_sha1(const unsigned char *sha1)\n> {\n> struct object *obj = parse_object(sha1);\n> if (!obj) {\n> errors_found |= ERROR_OBJECT;\n> return error(\"%s: object corrupt or missing\",\n> sha1_to_hex(sha1));\n> }\n> obj->flags |= HAS_OBJ;\n> return fsck_obj(obj);\n> }\n> \n> This function is called for each loose object file we find in\n> fsck_object_dir(), and there are a few problems:\n> \n> * The function parse_object() called from here would issue an error\n> message and returns NULL; then you get another \"corrupt or\n> missing\" error message, because this code cannot tell from the\n> NULL which is the case.\n> \n> * The intent of the callchain to fsck_sha1() is to iterate over\n> loose object files xx/x{38} and validate what is contained in\n> them, but that behaviour is not guaranteed because it calls\n> parse_object(), which may get the object data from a packfile\n> if the loose object is also in the packfile.\n> \n> This function should instead take \"path\" (the only caller of this\n> function fsck_loose() has it), read the data in the file, hash the\n> data to validate that it matches \"sha1\" and then create the object\n> out of that data it read by calling parse_object_buffer().\n\nYeah, I agree. I think as it is written, we also end up loading the\nloose objects twice (once in parse_object, and then later again in\nfsck_object to do the real work). Your solution would fix that, too.\n\nIt looks like we _don't_ load loose commits twice, though. We never turn\noff save_commit_buffer, so we happily cache the buffer for every single\ncommit, even though we won't need it again after fsck_object returns.\n\nI guess nobody has really noticed either issue, because repositories\nlarge enough for it to make a difference will usually be packed.\n\n-Peff\n"},{"id":"277436","messageId":"xmqq60y4jrvd.fsf@gitster.mtv.corp.google.com","threadId":"41304","inReplyTo":"20160204071919.GA14032@sigill.intra.peff.net","subject":"Re: parse_object does check_sha1_signature but not parse_object_buffer?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-04T17:40:54Z","receivedAt":"2016-02-04T17:40:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> For local repository operations, most of them are about reading data.\n> And there I generally favor performance over extra validation, with the\n> caveat that we should always be aware of the tradeoff. An extra\n> comparison to make sure we are not going out-of-bounds on a pack .idx\n> pointer is cheap. Loading a blob just to make sure its sha1 is valid\n> before we mention it in `diff --raw` output is stupid. Checking the sha1\n> on objects we are otherwise accessing is somewhere in between. :)\n>\n> For local write operations, like repacking, we should err on the careful\n> side. And I think we do a good job of balancing performance and\n> validation there (e.g., we reuse deltas without reconstructing the\n> object, but _with_ a crc check on the delta data itself).\n\nOK, that is a reasonable set of rules. We can say \"checkout\" is\nwriting out the contents to the filesystem, and make the \"somewhere\nin between\" be closer to the \"error on the careful side\".\n\n>> In fact, we do this, which is quite suboptimal:\n>> \n>> static int fsck_sha1(const unsigned char *sha1)\n>> {\n>> struct object *obj = parse_object(sha1);\n>> if (!obj) {\n>> errors_found |= ERROR_OBJECT;\n>> return error(\"%s: object corrupt or missing\",\n>> sha1_to_hex(sha1));\n>> }\n>> obj->flags |= HAS_OBJ;\n>> return fsck_obj(obj);\n>> }\n>> \n>> This function is called for each loose object file we find in\n>> fsck_object_dir(), and there are a few problems:\n>> \n>> * The function parse_object() called from here would issue an error\n>> message and returns NULL; then you get another \"corrupt or\n>> missing\" error message, because this code cannot tell from the\n>> NULL which is the case.\n>> \n>> * The intent of the callchain to fsck_sha1() is to iterate over\n>> loose object files xx/x{38} and validate what is contained in\n>> them, but that behaviour is not guaranteed because it calls\n>> parse_object(), which may get the object data from a packfile\n>> if the loose object is also in the packfile.\n>> \n>> This function should instead take \"path\" (the only caller of this\n>> function fsck_loose() has it), read the data in the file, hash the\n>> data to validate that it matches \"sha1\" and then create the object\n>> out of that data it read by calling parse_object_buffer().\n>\n> Yeah, I agree. I think as it is written, we also end up loading the\n> loose objects twice (once in parse_object, and then later again in\n> fsck_object to do the real work). Your solution would fix that, too.\n> ...\n> I guess nobody has really noticed either issue, because repositories\n> large enough for it to make a difference will usually be packed.\n\nI'll throw it in the leftover-bits list, then ;-)\n"}]}