{"thread":{"id":"48680","subject":"[PATCH] fsck: avoid looking at NULL blob->object","startedAt":"2018-06-09T08:32:06Z","lastAt":"2018-06-12T09:51:26Z","messageCount":17,"participants":["Jeff King","Eric Sunshine","Duy Nguyen","Martin Ågren","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"349782","messageId":"20180609083159.GA6158@sigill.intra.peff.net","threadId":"48680","inReplyTo":null,"subject":"[PATCH] fsck: avoid looking at NULL blob->object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-09T08:32:00Z","receivedAt":"2018-06-09T08:32:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 159e7b080b (fsck: detect gitmodules files,\n2018-05-02) taught fsck to look at the content of\n.gitmodules files. If the object turns out not to be a blob\nat all, we just complain and punt on checking the content.\nAnd since this was such an obvious and trivial code path, I\ndidn't even bother to add a test.\n\nExcept it _does_ do one non-trivial thing, which is call the\nreport() function, which wants us to pass a pointer to a\n\"struct object\". Which we don't have (we have only a \"struct\nobject_id\"). So we erroneously passed the NULL object, which\nends up segfaulting.\n\nIt seems like we could refactor report() to just take the\nobject_id itself. But we pass the object pointer along to\na callback function, and indeed this ends up in\nbuiltin/fsck.c's objreport() which does want to look at\nother parts of the object (like the type).\n\nSo instead, let's just use lookup_unknown_object() to get\nthe real \"struct object\", and pass that.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe problem is in v2.17.1, and of course the v2.18 release candidates.\n\n fsck.c                     |  3 ++-\n t/t7415-submodule-names.sh | 18 ++++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex bcae2c30e6..48e7e36869 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -1036,7 +1036,8 @@ int fsck_finish(struct fsck_options *options)\n \n \t\tblob = lookup_blob(oid);\n \t\tif (!blob) {\n-\t\t\tret |= report(options, &blob->object,\n+\t\t\tstruct object *obj = lookup_unknown_object(oid->hash);\n+\t\t\tret |= report(options, obj,\n \t\t\t\t      FSCK_MSG_GITMODULES_BLOB,\n \t\t\t\t      \"non-blob found at .gitmodules\");\n \t\t\tcontinue;\ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex a770d92a55..63c767bf99 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -151,4 +151,22 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t)\n '\n \n+test_expect_success 'fsck detects non-blob .gitmodules' '\n+\tgit init non-blob &&\n+\t(\n+\t\tcd non-blob &&\n+\n+\t\t# As above, make the funny directly to avoid index restrictions.\n+\t\tmkdir subdir &&\n+\t\tcp ../.gitmodules subdir/file &&\n+\t\tgit add subdir/file &&\n+\t\tgit commit -m ok &&\n+\t\ttree=$(git ls-tree HEAD | sed s/subdir/.gitmodules/ | git mktree) &&\n+\t\tcommit=$(git commit-tree $tree) &&\n+\n+\t\ttest_must_fail git fsck 2>output &&\n+\t\tgrep gitmodulesBlob output\n+\t)\n+'\n+\n test_done\n-- \n2.18.0.rc1.446.g4486251e51\n"},{"id":"349783","messageId":"CAPig+cTgCD5=96XG=Z5FwOsPbN409DxzAfPy0p=wnoLywu++dw@mail.gmail.com","threadId":"48680","inReplyTo":"20180609083159.GA6158@sigill.intra.peff.net","subject":"Re: [PATCH] fsck: avoid looking at NULL blob->object","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-09T08:38:54Z","receivedAt":"2018-06-09T08:38:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jun 9, 2018 at 4:32 AM, Jeff King <peff@peff.net> wrote:\n> Commit 159e7b080b (fsck: detect gitmodules files,\n> 2018-05-02) taught fsck to look at the content of\n> .gitmodules files. If the object turns out not to be a blob\n> at all, we just complain and punt on checking the content.\n> And since this was such an obvious and trivial code path, I\n> didn't even bother to add a test.\n> [...]\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\n> @@ -151,4 +151,22 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n> +test_expect_success 'fsck detects non-blob .gitmodules' '\n> +       git init non-blob &&\n> +       (\n> +               cd non-blob &&\n> +\n> +               # As above, make the funny directly to avoid index restrictions.\n\nIs there a word missing after \"funny\"?\n\n> +               mkdir subdir &&\n> +               cp ../.gitmodules subdir/file &&\n> +               git add subdir/file &&\n> +               git commit -m ok &&\n> +               tree=$(git ls-tree HEAD | sed s/subdir/.gitmodules/ | git mktree) &&\n> +               commit=$(git commit-tree $tree) &&\n\nI see that this is just mirroring the preceding test, but do you need\nto assign to variable 'commit' which is never consulted by anything\nlater in the test?\n\n> +               test_must_fail git fsck 2>output &&\n> +               grep gitmodulesBlob output\n> +       )\n> +'\n"},{"id":"349785","messageId":"CACsJy8Cpd_0CR9F_xyQfcw0j8ceOf777KcWXLHuxY1NxmRnXqw@mail.gmail.com","threadId":"48680","inReplyTo":"20180609083159.GA6158@sigill.intra.peff.net","subject":"Re: [PATCH] fsck: avoid looking at NULL blob->object","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-06-09T08:45:15Z","receivedAt":"2018-06-09T08:45:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jun 9, 2018 at 10:34 AM Jeff King <peff@peff.net> wrote:\n>\n> Commit 159e7b080b (fsck: detect gitmodules files,\n> 2018-05-02) taught fsck to look at the content of\n> .gitmodules files. If the object turns out not to be a blob\n> at all, we just complain and punt on checking the content.\n> And since this was such an obvious and trivial code path, I\n> didn't even bother to add a test.\n>\n> Except it _does_ do one non-trivial thing, which is call the\n> report() function, which wants us to pass a pointer to a\n> \"struct object\". Which we don't have (we have only a \"struct\n> object_id\"). So we erroneously passed the NULL object, which\n> ends up segfaulting.\n>\n> It seems like we could refactor report() to just take the\n> object_id itself. But we pass the object pointer along to\n> a callback function, and indeed this ends up in\n> builtin/fsck.c's objreport() which does want to look at\n> other parts of the object (like the type).\n\nAnd objreport() can handle OBJ_NONE well, which is the type given by\nlookup_unknown_object(). So yeah this looks good.\n\n> So instead, let's just use lookup_unknown_object() to get\n> the real \"struct object\", and pass that.\n-- \nDuy\n"},{"id":"349786","messageId":"CAN0heSqHRpOerHV8tgA-ntU6QvYQMxgrXPCPsrVcHLKUSwANXQ@mail.gmail.com","threadId":"48680","inReplyTo":"20180609083159.GA6158@sigill.intra.peff.net","subject":"Re: [PATCH] fsck: avoid looking at NULL blob->object","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-06-09T08:50:36Z","receivedAt":"2018-06-09T08:50:40Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 9 June 2018 at 10:32, Jeff King <peff@peff.net> wrote:\n> Except it _does_ do one non-trivial thing, which is call the\n> report() function, which wants us to pass a pointer to a\n> \"struct object\". Which we don't have (we have only a \"struct\n> object_id\"). So we erroneously passed the NULL object, which\n\ns/passed/dereferenced/? Probably doesn't affect the fix though.\n\n> ends up segfaulting.\n\n>                 blob = lookup_blob(oid);\n>                 if (!blob) {\n> -                       ret |= report(options, &blob->object,\n> +                       struct object *obj = lookup_unknown_object(oid->hash);\n> +                       ret |= report(options, obj,\n>                                       FSCK_MSG_GITMODULES_BLOB,\n>                                       \"non-blob found at .gitmodules\");\n"},{"id":"349787","messageId":"20180609091945.GA6817@sigill.intra.peff.net","threadId":"48680","inReplyTo":"CAPig+cTgCD5=96XG=Z5FwOsPbN409DxzAfPy0p=wnoLywu++dw@mail.gmail.com","subject":"Re: [PATCH] fsck: avoid looking at NULL blob->object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-09T09:19:45Z","receivedAt":"2018-06-09T09:19:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 09, 2018 at 04:38:54AM -0400, Eric Sunshine wrote:\n\n> On Sat, Jun 9, 2018 at 4:32 AM, Jeff King <peff@peff.net> wrote:\n> > Commit 159e7b080b (fsck: detect gitmodules files,\n> > 2018-05-02) taught fsck to look at the content of\n> > .gitmodules files. If the object turns out not to be a blob\n> > at all, we just complain and punt on checking the content.\n> > And since this was such an obvious and trivial code path, I\n> > didn't even bother to add a test.\n> > [...]\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> > diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\n> > @@ -151,4 +151,22 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n> > +test_expect_success 'fsck detects non-blob .gitmodules' '\n> > +       git init non-blob &&\n> > +       (\n> > +               cd non-blob &&\n> > +\n> > +               # As above, make the funny directly to avoid index restrictions.\n> \n> Is there a word missing after \"funny\"?\n\nOops, should be \"funny tree\" (that's what I get for trying to wordsmith\nit at the last minute).\n\n> > +               mkdir subdir &&\n> > +               cp ../.gitmodules subdir/file &&\n> > +               git add subdir/file &&\n> > +               git commit -m ok &&\n> > +               tree=$(git ls-tree HEAD | sed s/subdir/.gitmodules/ | git mktree) &&\n> > +               commit=$(git commit-tree $tree) &&\n> \n> I see that this is just mirroring the preceding test, but do you need\n> to assign to variable 'commit' which is never consulted by anything\n> later in the test?\n\nNo (nor above). I think originally I had planned to points refs to these\ncommits, but it isn't necessary for fsck. In fact, in the final form of\nthe patches, we do not even need the commit at all, since we will\ncomplain about .gitmodules in _any_ tree (early versions actually did an\nextra pass to find which trees were root trees).\n\n-Peff\n"},{"id":"349788","messageId":"20180609092056.GB6817@sigill.intra.peff.net","threadId":"48680","inReplyTo":"CACsJy8Cpd_0CR9F_xyQfcw0j8ceOf777KcWXLHuxY1NxmRnXqw@mail.gmail.com","subject":"Re: [PATCH] fsck: avoid looking at NULL blob->object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-09T09:20:56Z","receivedAt":"2018-06-09T09:21:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 09, 2018 at 10:45:15AM +0200, Duy Nguyen wrote:\n\n> > It seems like we could refactor report() to just take the\n> > object_id itself. But we pass the object pointer along to\n> > a callback function, and indeed this ends up in\n> > builtin/fsck.c's objreport() which does want to look at\n> > other parts of the object (like the type).\n> \n> And objreport() can handle OBJ_NONE well, which is the type given by\n> lookup_unknown_object(). So yeah this looks good.\n\nActually, we know that we have some \"real\" type, because otherwise\nlookup_blob() would not have returned NULL. In fact, we could actually\nuse lookup_object() here to find that object, knowing that it exists in\nthe hash. But IMHO that would be depending too much on how lookup_blob()\nworks. :)\n\n-Peff\n"},{"id":"349789","messageId":"20180609092135.GC6817@sigill.intra.peff.net","threadId":"48680","inReplyTo":"CAN0heSqHRpOerHV8tgA-ntU6QvYQMxgrXPCPsrVcHLKUSwANXQ@mail.gmail.com","subject":"Re: [PATCH] fsck: avoid looking at NULL blob->object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-09T09:21:36Z","receivedAt":"2018-06-09T09:21:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 09, 2018 at 10:50:36AM +0200, Martin Ågren wrote:\n\n> On 9 June 2018 at 10:32, Jeff King <peff@peff.net> wrote:\n> > Except it _does_ do one non-trivial thing, which is call the\n> > report() function, which wants us to pass a pointer to a\n> > \"struct object\". Which we don't have (we have only a \"struct\n> > object_id\"). So we erroneously passed the NULL object, which\n> \n> s/passed/dereferenced/? Probably doesn't affect the fix though.\n\nWell, we passed it, and then that function dereferenced it. :)\n\nI'm going to re-roll for the minor bits that Eric pointed out, so I'll\ntry to word this better.\n\n-Peff\n"},{"id":"349790","messageId":"20180609093028.GA8864@sigill.intra.peff.net","threadId":"48680","inReplyTo":"20180609083159.GA6158@sigill.intra.peff.net","subject":"[PATCH v2 0/2] .gitmodules fsck cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-09T09:30:28Z","receivedAt":"2018-06-09T09:30:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a v2 which fixes the comment typo and drops the unnecessary\ncommit creation in the tests. I've added a preparatory patch to do the\nsame cleanup in the existing test for consistency.\n\nAs before, these should apply on 'maint'.\n\n  [1/2]: t7415: don't bother creating commit for symlink test\n  [2/2]: fsck: avoid looking at NULL blob->object\n\n fsck.c                     |  3 ++-\n t/t7415-submodule-names.sh | 19 ++++++++++++++++++-\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\n-Peff\n"},{"id":"349791","messageId":"20180609093118.GA8942@sigill.intra.peff.net","threadId":"48680","inReplyTo":"20180609093028.GA8864@sigill.intra.peff.net","subject":"[PATCH v2 1/2] t7415: don't bother creating commit for symlink test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-09T09:31:18Z","receivedAt":"2018-06-09T09:31:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Early versions of the fsck .gitmodules detection code\nactually required a tree to be at the root of a commit for\nit to be checked for .gitmodules. What we ended up with in\n159e7b080b (fsck: detect gitmodules files, 2018-05-02),\nthough, finds a .gitmodules file in _any_ tree (see that\ncommit for more discussion).\n\nAs a result, there's no need to create a commit in our\ntests. Let's drop it in the name of simplicity.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t7415-submodule-names.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex a770d92a55..e2aae587ae 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -141,7 +141,6 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t\t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n \t\t\t} | git mktree\n \t\t) &&\n-\t\tcommit=$(git commit-tree $tree) &&\n \n \t\t# Check not only that we fail, but that it is due to the\n \t\t# symlink detector; this grep string comes from the config\n-- \n2.18.0.rc1.446.g4486251e51\n\n"},{"id":"349792","messageId":"20180609093141.GB8942@sigill.intra.peff.net","threadId":"48680","inReplyTo":"20180609093028.GA8864@sigill.intra.peff.net","subject":"[PATCH v2 2/2] fsck: avoid looking at NULL blob->object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-09T09:31:41Z","receivedAt":"2018-06-09T09:31:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 159e7b080b (fsck: detect gitmodules files,\n2018-05-02) taught fsck to look at the content of\n.gitmodules files. If the object turns out not to be a blob\nat all, we just complain and punt on checking the content.\nAnd since this was such an obvious and trivial code path, I\ndidn't even bother to add a test.\n\nExcept it _does_ do one non-trivial thing, which is call the\nreport() function, which wants us to pass a pointer to a\n\"struct object\". Which we don't have (we have only a \"struct\nobject_id\"). So we erroneously pass a NULL object to\nreport(), which gets dereferenced and causes a segfault.\n\nIt seems like we could refactor report() to just take the\nobject_id itself. But we pass the object pointer along to\na callback function, and indeed this ends up in\nbuiltin/fsck.c's objreport() which does want to look at\nother parts of the object (like the type).\n\nSo instead, let's just use lookup_unknown_object() to get\nthe real \"struct object\", and pass that.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n fsck.c                     |  3 ++-\n t/t7415-submodule-names.sh | 18 ++++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex bcae2c30e6..48e7e36869 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -1036,7 +1036,8 @@ int fsck_finish(struct fsck_options *options)\n \n \t\tblob = lookup_blob(oid);\n \t\tif (!blob) {\n-\t\t\tret |= report(options, &blob->object,\n+\t\t\tstruct object *obj = lookup_unknown_object(oid->hash);\n+\t\t\tret |= report(options, obj,\n \t\t\t\t      FSCK_MSG_GITMODULES_BLOB,\n \t\t\t\t      \"non-blob found at .gitmodules\");\n \t\t\tcontinue;\ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex e2aae587ae..d7868c11fa 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -150,4 +150,22 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t)\n '\n \n+test_expect_success 'fsck detects non-blob .gitmodules' '\n+\tgit init non-blob &&\n+\t(\n+\t\tcd non-blob &&\n+\n+\t\t# As above, make the funny tree directly to avoid index\n+\t\t# restrictions.\n+\t\tmkdir subdir &&\n+\t\tcp ../.gitmodules subdir/file &&\n+\t\tgit add subdir/file &&\n+\t\tgit commit -m ok &&\n+\t\ttree=$(git ls-tree HEAD | sed s/subdir/.gitmodules/ | git mktree) &&\n+\n+\t\ttest_must_fail git fsck 2>output &&\n+\t\tgrep gitmodulesBlob output\n+\t)\n+'\n+\n test_done\n-- \n2.18.0.rc1.446.g4486251e51\n"},{"id":"349793","messageId":"20180609094613.17675-1-szeder.dev@gmail.com","threadId":"48680","inReplyTo":"20180609093118.GA8942@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] t7415: don't bother creating commit for symlink test","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-06-09T09:46:13Z","receivedAt":"2018-06-09T09:46:23Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> As a result, there's no need to create a commit in our\n> tests. Let's drop it in the name of simplicity.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t7415-submodule-names.sh | 1 -\n>  1 file changed, 1 deletion(-)\n> \n> diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\n> index a770d92a55..e2aae587ae 100755\n> --- a/t/t7415-submodule-names.sh\n> +++ b/t/t7415-submodule-names.sh\n> @@ -141,7 +141,6 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n\nI add few more lines of context here:\n\n                tree=$(\n                        {\n                                printf \"100644 blob $content\\t$tricky\\n\" &&\n\n>  \t\t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n>  \t\t\t} | git mktree\n>  \t\t) &&\n> -\t\tcommit=$(git commit-tree $tree) &&\n\nThis was the only case where that $tree variable was used, so perhaps\nthat can go away as well, in the name of even more simplicity?\n\n>  \n>  \t\t# Check not only that we fail, but that it is due to the\n>  \t\t# symlink detector; this grep string comes from the config\n> -- \n> 2.18.0.rc1.446.g4486251e51\n> \n> \n"},{"id":"349794","messageId":"20180609095014.17972-1-szeder.dev@gmail.com","threadId":"48680","inReplyTo":"20180609093141.GB8942@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] fsck: avoid looking at NULL blob->object","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-06-09T09:50:14Z","receivedAt":"2018-06-09T09:50:20Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> +test_expect_success 'fsck detects non-blob .gitmodules' '\n> +\tgit init non-blob &&\n> +\t(\n> +\t\tcd non-blob &&\n> +\n> +\t\t# As above, make the funny tree directly to avoid index\n> +\t\t# restrictions.\n> +\t\tmkdir subdir &&\n> +\t\tcp ../.gitmodules subdir/file &&\n> +\t\tgit add subdir/file &&\n> +\t\tgit commit -m ok &&\n> +\t\ttree=$(git ls-tree HEAD | sed s/subdir/.gitmodules/ | git mktree) &&\n\nAnd $tree won't be used here, either.\n\n> +\n> +\t\ttest_must_fail git fsck 2>output &&\n> +\t\tgrep gitmodulesBlob output\n> +\t)\n> +'\n> +\n>  test_done\n> -- \n> 2.18.0.rc1.446.g4486251e51\n> \n"},{"id":"349798","messageId":"CAN0heSqkcZ5KBpaq7R45A7wPxQ3F=6PKeHqwAQztdJyz31Vdwg@mail.gmail.com","threadId":"48680","inReplyTo":"20180609092135.GC6817@sigill.intra.peff.net","subject":"Re: [PATCH] fsck: avoid looking at NULL blob->object","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-06-09T13:44:30Z","receivedAt":"2018-06-09T13:44:34Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 9 June 2018 at 11:21, Jeff King <peff@peff.net> wrote:\n> On Sat, Jun 09, 2018 at 10:50:36AM +0200, Martin Ågren wrote:\n>\n>> On 9 June 2018 at 10:32, Jeff King <peff@peff.net> wrote:\n>> > Except it _does_ do one non-trivial thing, which is call the\n>> > report() function, which wants us to pass a pointer to a\n>> > \"struct object\". Which we don't have (we have only a \"struct\n>> > object_id\"). So we erroneously passed the NULL object, which\n>>\n>> s/passed/dereferenced/? Probably doesn't affect the fix though.\n>\n> Well, we passed it, and then that function dereferenced it. :)\n>\n> I'm going to re-roll for the minor bits that Eric pointed out, so I'll\n> try to word this better.\n\nMy bad. I somehow thought we get into trouble already before we call\n`report()`. Well, we do, since we have undefined behavior. But for all\npractical purposes `&blob->object` and `blob` are the same\n(NULL-)pointer so we only crash after we call `report()`.\n\nAnyway, obviously no need to do anything about this in a v3.\n\nMartin\n"},{"id":"349881","messageId":"20180611083515.GA1815@sigill.intra.peff.net","threadId":"48680","inReplyTo":"20180609094613.17675-1-szeder.dev@gmail.com","subject":"[PATCH v3 0/2] .gitmodules fsck cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-11T08:35:15Z","receivedAt":"2018-06-11T08:35:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 09, 2018 at 11:46:13AM +0200, SZEDER Gábor wrote:\n\n> I add few more lines of context here:\n> \n>                 tree=$(\n>                         {\n>                                 printf \"100644 blob $content\\t$tricky\\n\" &&\n> \n> >  \t\t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n> >  \t\t\t} | git mktree\n> >  \t\t) &&\n> > -\t\tcommit=$(git commit-tree $tree) &&\n> \n> This was the only case where that $tree variable was used, so perhaps\n> that can go away as well, in the name of even more simplicity?\n\nYep, that is simpler. Here's a v3 just dropping those $tree variables,\nand adjusting the commit message as appropriate.\n\n  [1/2]: t7415: don't bother creating commit for symlink test\n  [2/2]: fsck: avoid looking at NULL blob->object\n\n fsck.c                     |  3 ++-\n t/t7415-submodule-names.sh | 29 ++++++++++++++++++++++-------\n 2 files changed, 24 insertions(+), 8 deletions(-)\n\n-Peff\n"},{"id":"349882","messageId":"20180611083540.GA16131@sigill.intra.peff.net","threadId":"48680","inReplyTo":"20180611083515.GA1815@sigill.intra.peff.net","subject":"[PATCH v3 1/2] t7415: don't bother creating commit for symlink test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-11T08:35:40Z","receivedAt":"2018-06-11T08:35:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Early versions of the fsck .gitmodules detection code\nactually required a tree to be at the root of a commit for\nit to be checked for .gitmodules. What we ended up with in\n159e7b080b (fsck: detect gitmodules files, 2018-05-02),\nthough, finds a .gitmodules file in _any_ tree (see that\ncommit for more discussion).\n\nAs a result, there's no need to create a commit in our\ntests. Let's drop it in the name of simplicity. And since\nthat was the only thing referencing $tree, we can pull our\ntree creation out of a command substitution.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t7415-submodule-names.sh | 11 ++++-------\n 1 file changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex a770d92a55..541bd81684 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -135,13 +135,10 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t\ttricky=\"[foo]bar=true\" &&\n \t\tcontent=$(git hash-object -w ../.gitmodules) &&\n \t\ttarget=$(printf \"$tricky\" | git hash-object -w --stdin) &&\n-\t\ttree=$(\n-\t\t\t{\n-\t\t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n-\t\t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n-\t\t\t} | git mktree\n-\t\t) &&\n-\t\tcommit=$(git commit-tree $tree) &&\n+\t\t{\n+\t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n+\t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n+\t\t} | git mktree &&\n \n \t\t# Check not only that we fail, but that it is due to the\n \t\t# symlink detector; this grep string comes from the config\n-- \n2.18.0.rc1.446.g4486251e51\n\n"},{"id":"349883","messageId":"20180611083544.GB16131@sigill.intra.peff.net","threadId":"48680","inReplyTo":"20180611083515.GA1815@sigill.intra.peff.net","subject":"[PATCH v3 2/2] fsck: avoid looking at NULL blob->object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-11T08:35:45Z","receivedAt":"2018-06-11T08:35:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 159e7b080b (fsck: detect gitmodules files,\n2018-05-02) taught fsck to look at the content of\n.gitmodules files. If the object turns out not to be a blob\nat all, we just complain and punt on checking the content.\nAnd since this was such an obvious and trivial code path, I\ndidn't even bother to add a test.\n\nExcept it _does_ do one non-trivial thing, which is call the\nreport() function, which wants us to pass a pointer to a\n\"struct object\". Which we don't have (we have only a \"struct\nobject_id\"). So we erroneously pass a NULL object to\nreport(), which gets dereferenced and causes a segfault.\n\nIt seems like we could refactor report() to just take the\nobject_id itself. But we pass the object pointer along to\na callback function, and indeed this ends up in\nbuiltin/fsck.c's objreport() which does want to look at\nother parts of the object (like the type).\n\nSo instead, let's just use lookup_unknown_object() to get\nthe real \"struct object\", and pass that.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n fsck.c                     |  3 ++-\n t/t7415-submodule-names.sh | 18 ++++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex bcae2c30e6..48e7e36869 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -1036,7 +1036,8 @@ int fsck_finish(struct fsck_options *options)\n \n \t\tblob = lookup_blob(oid);\n \t\tif (!blob) {\n-\t\t\tret |= report(options, &blob->object,\n+\t\t\tstruct object *obj = lookup_unknown_object(oid->hash);\n+\t\t\tret |= report(options, obj,\n \t\t\t\t      FSCK_MSG_GITMODULES_BLOB,\n \t\t\t\t      \"non-blob found at .gitmodules\");\n \t\t\tcontinue;\ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex 541bd81684..3c0f1a102a 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -148,4 +148,22 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t)\n '\n \n+test_expect_success 'fsck detects non-blob .gitmodules' '\n+\tgit init non-blob &&\n+\t(\n+\t\tcd non-blob &&\n+\n+\t\t# As above, make the funny tree directly to avoid index\n+\t\t# restrictions.\n+\t\tmkdir subdir &&\n+\t\tcp ../.gitmodules subdir/file &&\n+\t\tgit add subdir/file &&\n+\t\tgit commit -m ok &&\n+\t\tgit ls-tree HEAD | sed s/subdir/.gitmodules/ | git mktree &&\n+\n+\t\ttest_must_fail git fsck 2>output &&\n+\t\tgrep gitmodulesBlob output\n+\t)\n+'\n+\n test_done\n-- \n2.18.0.rc1.446.g4486251e51\n"},{"id":"349978","messageId":"20180612095121.GA27181@sigill.intra.peff.net","threadId":"48680","inReplyTo":"CAN0heSqkcZ5KBpaq7R45A7wPxQ3F=6PKeHqwAQztdJyz31Vdwg@mail.gmail.com","subject":"Re: [PATCH] fsck: avoid looking at NULL blob->object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-12T09:51:21Z","receivedAt":"2018-06-12T09:51:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 09, 2018 at 03:44:30PM +0200, Martin Ågren wrote:\n\n> On 9 June 2018 at 11:21, Jeff King <peff@peff.net> wrote:\n> > On Sat, Jun 09, 2018 at 10:50:36AM +0200, Martin Ågren wrote:\n> >\n> >> On 9 June 2018 at 10:32, Jeff King <peff@peff.net> wrote:\n> >> > Except it _does_ do one non-trivial thing, which is call the\n> >> > report() function, which wants us to pass a pointer to a\n> >> > \"struct object\". Which we don't have (we have only a \"struct\n> >> > object_id\"). So we erroneously passed the NULL object, which\n> >>\n> >> s/passed/dereferenced/? Probably doesn't affect the fix though.\n> >\n> > Well, we passed it, and then that function dereferenced it. :)\n> >\n> > I'm going to re-roll for the minor bits that Eric pointed out, so I'll\n> > try to word this better.\n> \n> My bad. I somehow thought we get into trouble already before we call\n> `report()`. Well, we do, since we have undefined behavior. But for all\n> practical purposes `&blob->object` and `blob` are the same\n> (NULL-)pointer so we only crash after we call `report()`.\n> \n> Anyway, obviously no need to do anything about this in a v3.\n\nAh, yeah, I didn't really think of it that way. But certainly you are\nright that the moment we look at &blob->object, we are invoking\nundefined behavior according to the standard.  Hopefully the wording\ntweak I made covers both ways of thinking about it. :)\n\n-Peff\n"}]}