patchfsck: avoid looking at NULL blob->object

Commit 159e7b080b (fsck: detect gitmodules files, 2018-05-02) taught fsck to look at the content of .gitmodules files. If the object turns out not to be a blob at all, we just complain and punt on checking the content. And since this was such an obvious and trivial code path, I didn't even bother to add a test.

Except it _does_ do one non-trivial thing, which is call the report() function, which wants us to pass a pointer to a "struct object". Which we don't have (we have only a "struct object_id"). So we erroneously passed the NULL object, which ends up segfaulting.

It seems like we could refactor report() to just take the object_id itself. But we pass the object pointer along to a callback function, and indeed this ends up in builtin/fsck.c's objreport() which does want to look at other parts of the object (like the type).

So instead, let's just use lookup_unknown_object() to get the real "struct object", and pass that.

Signed-off-by: Jeff King <peff@peff.net>
---
The problem is in v2.17.1, and of course the v2.18 release candidates.

 fsck.c                     |  3 ++-
 t/t7415-submodule-names.sh | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

On Sat, Jun 9, 2018 at 4:32 AM, Jeff King <peff@peff.net> wrote:

Is there a word missing after "funny"?

I see that this is just mirroring the preceding test, but do you need to assign to variable 'commit' which is never consulted by anything later in the test?

On Sat, Jun 09, 2018 at 04:38:54AM -0400, Eric Sunshine wrote:

Oops, should be "funny tree" (that's what I get for trying to wordsmith it at the last minute).

No (nor above). I think originally I had planned to points refs to these commits, but it isn't necessary for fsck. In fact, in the final form of the patches, we do not even need the commit at all, since we will complain about .gitmodules in _any_ tree (early versions actually did an extra pass to find which trees were root trees).

-Peff

On Sat, Jun 9, 2018 at 10:34 AM Jeff King <peff@peff.net> wrote:

And objreport() can handle OBJ_NONE well, which is the type given by lookup_unknown_object(). So yeah this looks good.

On Sat, Jun 09, 2018 at 10:45:15AM +0200, Duy Nguyen wrote:

Actually, we know that we have some "real" type, because otherwise lookup_blob() would not have returned NULL. In fact, we could actually use lookup_object() here to find that object, knowing that it exists in the hash. But IMHO that would be depending too much on how lookup_blob() works. :)

-Peff

On 9 June 2018 at 10:32, Jeff King <peff@peff.net> wrote:

s/passed/dereferenced/? Probably doesn't affect the fix though.

On Sat, Jun 09, 2018 at 10:50:36AM +0200, Martin Ågren wrote:

Well, we passed it, and then that function dereferenced it. :)

I'm going to re-roll for the minor bits that Eric pointed out, so I'll try to word this better.

-Peff

On 9 June 2018 at 11:21, Jeff King <peff@peff.net> wrote:

My bad. I somehow thought we get into trouble already before we call `report()`. Well, we do, since we have undefined behavior. But for all practical purposes `&blob->object` and `blob` are the same (NULL-)pointer so we only crash after we call `report()`.

Anyway, obviously no need to do anything about this in a v3.

Martin

On Sat, Jun 09, 2018 at 03:44:30PM +0200, Martin Ågren wrote:

Ah, yeah, I didn't really think of it that way. But certainly you are right that the moment we look at &blob->object, we are invoking undefined behavior according to the standard. Hopefully the wording tweak I made covers both ways of thinking about it. :)

-Peff

Here's a v2 which fixes the comment typo and drops the unnecessary commit creation in the tests. I've added a preparatory patch to do the same cleanup in the existing test for consistency.

As before, these should apply on 'maint'.

  [1/2]: t7415: don't bother creating commit for symlink test
  [2/2]: fsck: avoid looking at NULL blob->object

 fsck.c                     |  3 ++-
 t/t7415-submodule-names.sh | 19 ++++++++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

-Peff

Early versions of the fsck .gitmodules detection code actually required a tree to be at the root of a commit for it to be checked for .gitmodules. What we ended up with in 159e7b080b (fsck: detect gitmodules files, 2018-05-02), though, finds a .gitmodules file in _any_ tree (see that commit for more discussion).

As a result, there's no need to create a commit in our tests. Let's drop it in the name of simplicity.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7415-submodule-names.sh | 1 -
 1 file changed, 1 deletion(-)

I add few more lines of context here:

                tree=$(
                        {
                                printf "100644 blob $content\t$tricky\n" &&

This was the only case where that $tree variable was used, so perhaps that can go away as well, in the name of even more simplicity?

On Sat, Jun 09, 2018 at 11:46:13AM +0200, SZEDER Gábor wrote:

Yep, that is simpler. Here's a v3 just dropping those $tree variables, and adjusting the commit message as appropriate.

  [1/2]: t7415: don't bother creating commit for symlink test
  [2/2]: fsck: avoid looking at NULL blob->object

 fsck.c                     |  3 ++-
 t/t7415-submodule-names.sh | 29 ++++++++++++++++++++++-------
 2 files changed, 24 insertions(+), 8 deletions(-)

-Peff

Early versions of the fsck .gitmodules detection code actually required a tree to be at the root of a commit for it to be checked for .gitmodules. What we ended up with in 159e7b080b (fsck: detect gitmodules files, 2018-05-02), though, finds a .gitmodules file in _any_ tree (see that commit for more discussion).

As a result, there's no need to create a commit in our tests. Let's drop it in the name of simplicity. And since that was the only thing referencing $tree, we can pull our tree creation out of a command substitution.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7415-submodule-names.sh | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Commit 159e7b080b (fsck: detect gitmodules files, 2018-05-02) taught fsck to look at the content of .gitmodules files. If the object turns out not to be a blob at all, we just complain and punt on checking the content. And since this was such an obvious and trivial code path, I didn't even bother to add a test.

Except it _does_ do one non-trivial thing, which is call the report() function, which wants us to pass a pointer to a "struct object". Which we don't have (we have only a "struct object_id"). So we erroneously pass a NULL object to report(), which gets dereferenced and causes a segfault.

It seems like we could refactor report() to just take the object_id itself. But we pass the object pointer along to a callback function, and indeed this ends up in builtin/fsck.c's objreport() which does want to look at other parts of the object (like the type).

So instead, let's just use lookup_unknown_object() to get the real "struct object", and pass that.

Signed-off-by: Jeff King <peff@peff.net>
---
 fsck.c                     |  3 ++-
 t/t7415-submodule-names.sh | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

Commit 159e7b080b (fsck: detect gitmodules files, 2018-05-02) taught fsck to look at the content of .gitmodules files. If the object turns out not to be a blob at all, we just complain and punt on checking the content. And since this was such an obvious and trivial code path, I didn't even bother to add a test.

Except it _does_ do one non-trivial thing, which is call the report() function, which wants us to pass a pointer to a "struct object". Which we don't have (we have only a "struct object_id"). So we erroneously pass a NULL object to report(), which gets dereferenced and causes a segfault.

It seems like we could refactor report() to just take the object_id itself. But we pass the object pointer along to a callback function, and indeed this ends up in builtin/fsck.c's objreport() which does want to look at other parts of the object (like the type).

So instead, let's just use lookup_unknown_object() to get the real "struct object", and pass that.

Signed-off-by: Jeff King <peff@peff.net>
---
 fsck.c                     |  3 ++-
 t/t7415-submodule-names.sh | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

And $tree won't be used here, either.