Re: [PATCH] fsck: avoid looking at NULL blob->object
On Sat, Jun 09, 2018 at 03:44:30PM +0200, Martin Ågren wrote: > On 9 June 2018 at 11:21, Jeff King wrote: > > On Sat, Jun 09, 2018 at 10:50:36AM +0200, Martin Ågren wrote: > > > >> On 9 June 2018 at 10:32, Jeff King wrote: > >> > 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 > >> > >> s/passed/dereferenced/? Probably doesn't affect the fix though. > > > > 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. > > 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 `>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. Ah, yeah, I didn't really think of it that way. But certainly you are right that the moment we look at >object, we are invoking undefined behavior according to the standard. Hopefully the wording tweak I made covers both ways of thinking about it. :) -Peff
Re: [PATCH] fsck: avoid looking at NULL blob->object
On Sat, Jun 09, 2018 at 10:50:36AM +0200, Martin Ågren wrote: > On 9 June 2018 at 10:32, Jeff King wrote: > > 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 > > s/passed/dereferenced/? Probably doesn't affect the fix though. 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
Re: [PATCH] fsck: avoid looking at NULL blob->object
On Sat, Jun 09, 2018 at 10:45:15AM +0200, Duy Nguyen wrote: > > 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). > > And objreport() can handle OBJ_NONE well, which is the type given by > lookup_unknown_object(). So yeah this looks good. 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
Re: [PATCH] fsck: avoid looking at NULL blob->object
On Sat, Jun 09, 2018 at 04:38:54AM -0400, Eric Sunshine wrote: > On Sat, Jun 9, 2018 at 4:32 AM, Jeff King wrote: > > 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. > > [...] > > Signed-off-by: Jeff King > > --- > > diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh > > @@ -151,4 +151,22 @@ test_expect_success 'fsck detects symlinked > > .gitmodules file' ' > > +test_expect_success 'fsck detects non-blob .gitmodules' ' > > + git init non-blob && > > + ( > > + cd non-blob && > > + > > + # As above, make the funny directly to avoid index > > restrictions. > > Is there a word missing after "funny"? Oops, should be "funny tree" (that's what I get for trying to wordsmith it at the last minute). > > + mkdir subdir && > > + cp ../.gitmodules subdir/file && > > + git add subdir/file && > > + git commit -m ok && > > + tree=$(git ls-tree HEAD | sed s/subdir/.gitmodules/ | git > > mktree) && > > + commit=$(git commit-tree $tree) && > > 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? 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
Re: [PATCH] fsck: avoid looking at NULL blob->object
On 9 June 2018 at 10:32, Jeff King wrote:
> 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
s/passed/dereferenced/? Probably doesn't affect the fix though.
> ends up segfaulting.
> blob = lookup_blob(oid);
> if (!blob) {
> - ret |= report(options, >object,
> + struct object *obj = lookup_unknown_object(oid->hash);
> + ret |= report(options, obj,
> FSCK_MSG_GITMODULES_BLOB,
> "non-blob found at .gitmodules");
Re: [PATCH] fsck: avoid looking at NULL blob->object
On Sat, Jun 9, 2018 at 10:34 AM Jeff King wrote: > > 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). And objreport() can handle OBJ_NONE well, which is the type given by lookup_unknown_object(). So yeah this looks good. > So instead, let's just use lookup_unknown_object() to get > the real "struct object", and pass that. -- Duy
Re: [PATCH] fsck: avoid looking at NULL blob->object
On Sat, Jun 9, 2018 at 4:32 AM, Jeff King wrote: > 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. > [...] > Signed-off-by: Jeff King > --- > diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh > @@ -151,4 +151,22 @@ test_expect_success 'fsck detects symlinked .gitmodules > file' ' > +test_expect_success 'fsck detects non-blob .gitmodules' ' > + git init non-blob && > + ( > + cd non-blob && > + > + # As above, make the funny directly to avoid index > restrictions. Is there a word missing after "funny"? > + mkdir subdir && > + cp ../.gitmodules subdir/file && > + git add subdir/file && > + git commit -m ok && > + tree=$(git ls-tree HEAD | sed s/subdir/.gitmodules/ | git > mktree) && > + commit=$(git commit-tree $tree) && 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? > + test_must_fail git fsck 2>output && > + grep gitmodulesBlob output > + ) > +'
[PATCH] fsck: 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
---
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(-)
diff --git a/fsck.c b/fsck.c
index bcae2c30e6..48e7e36869 100644
--- a/fsck.c
+++ b/fsck.c
@@ -1036,7 +1036,8 @@ int fsck_finish(struct fsck_options *options)
blob = lookup_blob(oid);
if (!blob) {
- ret |= report(options, >object,
+ struct object *obj = lookup_unknown_object(oid->hash);
+ ret |= report(options, obj,
FSCK_MSG_GITMODULES_BLOB,
"non-blob found at .gitmodules");
continue;
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index a770d92a55..63c767bf99 100755
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -151,4 +151,22 @@ test_expect_success 'fsck detects symlinked .gitmodules
file' '
)
'
+test_expect_success 'fsck detects non-blob .gitmodules' '
+ git init non-blob &&
+ (
+ cd non-blob &&
+
+ # As above, make the funny directly to avoid index restrictions.
+ mkdir subdir &&
+ cp ../.gitmodules subdir/file &&
+ git add subdir/file &&
+ git commit -m ok &&
+ tree=$(git ls-tree HEAD | sed s/subdir/.gitmodules/ | git
mktree) &&
+ commit=$(git commit-tree $tree) &&
+
+ test_must_fail git fsck 2>output &&
+ grep gitmodulesBlob output
+ )
+'
+
test_done
--
2.18.0.rc1.446.g4486251e51
