{"thread":{"id":"46869","subject":"[PATCH] tag: avoid NULL pointer arithmetic","startedAt":"2017-10-01T14:45:20Z","lastAt":"2017-10-06T16:21:27Z","messageCount":13,"participants":["René Scharfe","Junio C Hamano","Jeff King","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"329284","messageId":"99e56671-bdf9-a59f-ae7b-758f1b7a8f14@web.de","threadId":"46869","inReplyTo":null,"subject":"[PATCH] tag: avoid NULL pointer arithmetic","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-10-01T14:45:13Z","receivedAt":"2017-10-01T14:45:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"lookup_blob() etc. can return NULL if the referenced object isn't of the\nexpected type.  In theory it's wrong to reference the object member in\nthat case.  In practice it's OK because it's located at offset 0 for all\ntypes, so the pointer arithmetic (NULL + 0) is optimized out by the\ncompiler.  The issue is reported by Clang's AddressSanitizer, though.\n\nAvoid the ASan error by casting the results of the lookup functions to\nstruct object pointers.  That works fine with NULL pointers as well.  We\nalready rely on the object member being first in all object types in\nother places in the code.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n tag.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex 7e10acfb6e..fcbe012f7a 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -142,13 +142,13 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)\n \tbufptr = nl + 1;\n \n \tif (!strcmp(type, blob_type)) {\n-\t\titem->tagged = &lookup_blob(&oid)->object;\n+\t\titem->tagged = (struct object *)lookup_blob(&oid);\n \t} else if (!strcmp(type, tree_type)) {\n-\t\titem->tagged = &lookup_tree(&oid)->object;\n+\t\titem->tagged = (struct object *)lookup_tree(&oid);\n \t} else if (!strcmp(type, commit_type)) {\n-\t\titem->tagged = &lookup_commit(&oid)->object;\n+\t\titem->tagged = (struct object *)lookup_commit(&oid);\n \t} else if (!strcmp(type, tag_type)) {\n-\t\titem->tagged = &lookup_tag(&oid)->object;\n+\t\titem->tagged = (struct object *)lookup_tag(&oid);\n \t} else {\n \t\terror(\"Unknown type %s\", type);\n \t\titem->tagged = NULL;\n-- \n2.14.2\n"},{"id":"329376","messageId":"xmqqwp4ep4qb.fsf@gitster.mtv.corp.google.com","threadId":"46869","inReplyTo":"99e56671-bdf9-a59f-ae7b-758f1b7a8f14@web.de","subject":"Re: [PATCH] tag: avoid NULL pointer arithmetic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-02T04:13:00Z","receivedAt":"2017-10-02T04:13:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> lookup_blob() etc. can return NULL if the referenced object isn't of the\n> expected type.  In theory it's wrong to reference the object member in\n> that case.  In practice it's OK because it's located at offset 0 for all\n> types, so the pointer arithmetic (NULL + 0) is optimized out by the\n> compiler.  The issue is reported by Clang's AddressSanitizer, though.\n>\n> Avoid the ASan error by casting the results of the lookup functions to\n> struct object pointers.  That works fine with NULL pointers as well.  We\n> already rely on the object member being first in all object types in\n> other places in the code.\n>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n\nYikes, that is tricky.\n\nTaking the address of .object field appears to be a lot cleaner and\nmore kosher than casting, but from the compiler's point of view it's\nquite the opposite.  Sigh.\n\nThanks.  The patch itself looks like the right solution to me.\n\n>  tag.c | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/tag.c b/tag.c\n> index 7e10acfb6e..fcbe012f7a 100644\n> --- a/tag.c\n> +++ b/tag.c\n> @@ -142,13 +142,13 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)\n>  \tbufptr = nl + 1;\n>  \n>  \tif (!strcmp(type, blob_type)) {\n> -\t\titem->tagged = &lookup_blob(&oid)->object;\n> +\t\titem->tagged = (struct object *)lookup_blob(&oid);\n>  \t} else if (!strcmp(type, tree_type)) {\n> -\t\titem->tagged = &lookup_tree(&oid)->object;\n> +\t\titem->tagged = (struct object *)lookup_tree(&oid);\n>  \t} else if (!strcmp(type, commit_type)) {\n> -\t\titem->tagged = &lookup_commit(&oid)->object;\n> +\t\titem->tagged = (struct object *)lookup_commit(&oid);\n>  \t} else if (!strcmp(type, tag_type)) {\n> -\t\titem->tagged = &lookup_tag(&oid)->object;\n> +\t\titem->tagged = (struct object *)lookup_tag(&oid);\n>  \t} else {\n>  \t\terror(\"Unknown type %s\", type);\n>  \t\titem->tagged = NULL;\n"},{"id":"329385","messageId":"20171002050810.ryj4nv3nwqp7gb7p@sigill.intra.peff.net","threadId":"46869","inReplyTo":"99e56671-bdf9-a59f-ae7b-758f1b7a8f14@web.de","subject":"Re: [PATCH] tag: avoid NULL pointer arithmetic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T05:08:11Z","receivedAt":"2017-10-02T05:08:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 01, 2017 at 04:45:13PM +0200, René Scharfe wrote:\n\n> lookup_blob() etc. can return NULL if the referenced object isn't of the\n> expected type.  In theory it's wrong to reference the object member in\n> that case.  In practice it's OK because it's located at offset 0 for all\n> types, so the pointer arithmetic (NULL + 0) is optimized out by the\n> compiler.  The issue is reported by Clang's AddressSanitizer, though.\n> \n> Avoid the ASan error by casting the results of the lookup functions to\n> struct object pointers.  That works fine with NULL pointers as well.  We\n> already rely on the object member being first in all object types in\n> other places in the code.\n\nOut of curiosity, did you have to do anything to coax this out of ASan\n(e.g., a specific version)?  I've been running it pretty regularly and\ndidn't see this one (I did switch from clang to gcc a month or two ago,\nbut this code is pretty old, I think).\n\n-Peff\n"},{"id":"329438","messageId":"32a8b949-638a-1784-7fba-948ae32206fc@web.de","threadId":"46869","inReplyTo":"20171002050810.ryj4nv3nwqp7gb7p@sigill.intra.peff.net","subject":"Re: [PATCH] tag: avoid NULL pointer arithmetic","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-10-02T13:06:57Z","receivedAt":"2017-10-02T13:07:18Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.10.2017 um 07:08 schrieb Jeff King:\n> On Sun, Oct 01, 2017 at 04:45:13PM +0200, René Scharfe wrote:\n> \n>> lookup_blob() etc. can return NULL if the referenced object isn't of the\n>> expected type.  In theory it's wrong to reference the object member in\n>> that case.  In practice it's OK because it's located at offset 0 for all\n>> types, so the pointer arithmetic (NULL + 0) is optimized out by the\n>> compiler.  The issue is reported by Clang's AddressSanitizer, though.\n>>\n>> Avoid the ASan error by casting the results of the lookup functions to\n>> struct object pointers.  That works fine with NULL pointers as well.  We\n>> already rely on the object member being first in all object types in\n>> other places in the code.\n> \n> Out of curiosity, did you have to do anything to coax this out of ASan\n> (e.g., a specific version)?  I've been running it pretty regularly and\n> didn't see this one (I did switch from clang to gcc a month or two ago,\n> but this code is pretty old, I think).\n\nI did \"make -j4 SANITIZE=undefined,address BLK_SHA1=1 test\" with\nclang version 4.0.1-1 (tags/RELEASE_401/final), and t1450-fsck.sh failed.\n\nRené\n\n\n"},{"id":"329475","messageId":"20171002192327.xyynkh47r2kgir7f@sigill.intra.peff.net","threadId":"46869","inReplyTo":"32a8b949-638a-1784-7fba-948ae32206fc@web.de","subject":"Re: [PATCH] tag: avoid NULL pointer arithmetic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T19:23:27Z","receivedAt":"2017-10-02T19:23:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 02, 2017 at 03:06:57PM +0200, René Scharfe wrote:\n\n> >> Avoid the ASan error by casting the results of the lookup functions to\n> >> struct object pointers.  That works fine with NULL pointers as well.  We\n> >> already rely on the object member being first in all object types in\n> >> other places in the code.\n> > \n> > Out of curiosity, did you have to do anything to coax this out of ASan\n> > (e.g., a specific version)?  I've been running it pretty regularly and\n> > didn't see this one (I did switch from clang to gcc a month or two ago,\n> > but this code is pretty old, I think).\n> \n> I did \"make -j4 SANITIZE=undefined,address BLK_SHA1=1 test\" with\n> clang version 4.0.1-1 (tags/RELEASE_401/final), and t1450-fsck.sh failed.\n\nInteresting. I can trigger it with the same setup, but not if:\n\n  1. I use gcc instead of clang.\n\n  2. If I only use one of UBSan and ASan. It's the combination that\n     triggers it.\n\n-Peff\n"},{"id":"329548","messageId":"20171003102215.9952-1-szeder.dev@gmail.com","threadId":"46869","inReplyTo":"99e56671-bdf9-a59f-ae7b-758f1b7a8f14@web.de","subject":"Re: [PATCH] tag: avoid NULL pointer arithmetic","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-10-03T10:22:15Z","receivedAt":"2017-10-03T10:22:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> lookup_blob() etc. can return NULL if the referenced object isn't of the\n> expected type.  In theory it's wrong to reference the object member in\n> that case.  In practice it's OK because it's located at offset 0 for all\n> types, so the pointer arithmetic (NULL + 0) is optimized out by the\n> compiler.  The issue is reported by Clang's AddressSanitizer, though.\n> \n> Avoid the ASan error by casting the results of the lookup functions to\n> struct object pointers.  That works fine with NULL pointers as well.  We\n> already rely on the object member being first in all object types in\n> other places in the code.\n\nThis sounds like the main goal of the patch is to avoid an ASan error,\nbut I think it's more important to avoid (and to be more explicit\nabout avoiding) the undefined behavior.  I.e. along the lines of\ns/In theory it's wrong/It's undefined behavior/ and\ns/ASan error/undefined behavior/\n\nFurthermore, fsck.c:fsck_walk_tree() does the same \"immediately\nreference the object member in lookup_blob()'s and lookup_tree()'s\nreturn value\" thing.  I think those should receive the same treatment\nas well.\n\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  tag.c | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/tag.c b/tag.c\n> index 7e10acfb6e..fcbe012f7a 100644\n> --- a/tag.c\n> +++ b/tag.c\n> @@ -142,13 +142,13 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)\n>  \tbufptr = nl + 1;\n>  \n>  \tif (!strcmp(type, blob_type)) {\n> -\t\titem->tagged = &lookup_blob(&oid)->object;\n> +\t\titem->tagged = (struct object *)lookup_blob(&oid);\n>  \t} else if (!strcmp(type, tree_type)) {\n> -\t\titem->tagged = &lookup_tree(&oid)->object;\n> +\t\titem->tagged = (struct object *)lookup_tree(&oid);\n>  \t} else if (!strcmp(type, commit_type)) {\n> -\t\titem->tagged = &lookup_commit(&oid)->object;\n> +\t\titem->tagged = (struct object *)lookup_commit(&oid);\n>  \t} else if (!strcmp(type, tag_type)) {\n> -\t\titem->tagged = &lookup_tag(&oid)->object;\n> +\t\titem->tagged = (struct object *)lookup_tag(&oid);\n>  \t} else {\n>  \t\terror(\"Unknown type %s\", type);\n>  \t\titem->tagged = NULL;\n> -- \n> 2.14.2\n> \n"},{"id":"329557","messageId":"19a08fb1-2fb3-f368-772b-36646a179975@web.de","threadId":"46869","inReplyTo":"20171003102215.9952-1-szeder.dev@gmail.com","subject":"Re: [PATCH] tag: avoid NULL pointer arithmetic","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-10-03T12:51:05Z","receivedAt":"2017-10-03T12:51:18Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.10.2017 um 12:22 schrieb SZEDER Gábor:\n>> lookup_blob() etc. can return NULL if the referenced object isn't of the\n>> expected type.  In theory it's wrong to reference the object member in\n>> that case.  In practice it's OK because it's located at offset 0 for all\n>> types, so the pointer arithmetic (NULL + 0) is optimized out by the\n>> compiler.  The issue is reported by Clang's AddressSanitizer, though.\n>>\n>> Avoid the ASan error by casting the results of the lookup functions to\n>> struct object pointers.  That works fine with NULL pointers as well.  We\n>> already rely on the object member being first in all object types in\n>> other places in the code.\n> \n> This sounds like the main goal of the patch is to avoid an ASan error,\n> but I think it's more important to avoid (and to be more explicit\n> about avoiding) the undefined behavior.  I.e. along the lines of\n> s/In theory it's wrong/It's undefined behavior/ and\n> s/ASan error/undefined behavior/\n\nYou are probably right, but I struggle to pin down the difference.  Another\ntry: Adding zero to a NULL pointer is undefined, but is either optimized\nout or has no effect.  That's why the code doesn't crash without ASan and\nUBSan.  Relying on undefined behavior is wrong nevertheless, so let's stop\ndoing it.\n\n> Furthermore, fsck.c:fsck_walk_tree() does the same \"immediately\n> reference the object member in lookup_blob()'s and lookup_tree()'s\n> return value\" thing.  I think those should receive the same treatment\n> as well.\n\nHmm, are put_object_name() and all the walk() implementations ready for\na NULL object handed to them?  Or would we rather need to error out\nright there?  Other suspicious places in this regard are (at least):\nbuiltin/merge-tree.c, cache-tree.c, http-push.c, list-objects.c,\nmerge-recursive.c, and shallow.c. :-/\n\nRené\n"},{"id":"329572","messageId":"54f5877b-a143-11c2-d8f6-ff28ed9e7e38@web.de","threadId":"46869","inReplyTo":"19a08fb1-2fb3-f368-772b-36646a179975@web.de","subject":"[PATCH] fsck: check results of lookup_blob() and lookup_tree() for NULL","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-10-03T13:47:56Z","receivedAt":"2017-10-03T13:48:10Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.10.2017 um 14:51 schrieb René Scharfe:\n> Am 03.10.2017 um 12:22 schrieb SZEDER Gábor:\n>> Furthermore, fsck.c:fsck_walk_tree() does the same \"immediately\n>> reference the object member in lookup_blob()'s and lookup_tree()'s\n>> return value\" thing.  I think those should receive the same treatment\n>> as well.\n> \n> Hmm, are put_object_name() and all the walk() implementations ready for\n> a NULL object handed to them?  Or would we rather need to error out\n> right there?\nHow about this?\n\n-- >8 --\nlookup_blob() and lookup_tree() can return NULL if they find an object\nof an unexpected type.  Error out of fsck_walk_tree() in that case, like\nwe do when encountering a bad file mode.  An error message is already\nshown by object_as_type(), which gets called by the lookup functions.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n fsck.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 2ad00fc4d0..561a13ac27 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -358,14 +358,20 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op\n \t\t\tcontinue;\n \n \t\tif (S_ISDIR(entry.mode)) {\n-\t\t\tobj = &lookup_tree(entry.oid)->object;\n+\t\t\tstruct tree *tree = lookup_tree(entry.oid);\n+\t\t\tif (!tree)\n+\t\t\t\treturn -1;\n+\t\t\tobj = &tree->object;\n \t\t\tif (name)\n \t\t\t\tput_object_name(options, obj, \"%s%s/\", name,\n \t\t\t\t\tentry.path);\n \t\t\tresult = options->walk(obj, OBJ_TREE, data, options);\n \t\t}\n \t\telse if (S_ISREG(entry.mode) || S_ISLNK(entry.mode)) {\n-\t\t\tobj = &lookup_blob(entry.oid)->object;\n+\t\t\tstruct blob *blob = lookup_blob(entry.oid);\n+\t\t\tif (!blob)\n+\t\t\t\treturn -1;\n+\t\t\tobj = &blob->object;\n \t\t\tif (name)\n \t\t\t\tput_object_name(options, obj, \"%s%s\", name,\n \t\t\t\t\tentry.path);\n-- \n2.14.2\n"},{"id":"329638","messageId":"xmqqh8vfftp7.fsf@gitster.mtv.corp.google.com","threadId":"46869","inReplyTo":"54f5877b-a143-11c2-d8f6-ff28ed9e7e38@web.de","subject":"Re: [PATCH] fsck: check results of lookup_blob() and lookup_tree() for NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-04T04:00:36Z","receivedAt":"2017-10-04T04:00:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Am 03.10.2017 um 14:51 schrieb René Scharfe:\n>> Am 03.10.2017 um 12:22 schrieb SZEDER Gábor:\n>>> Furthermore, fsck.c:fsck_walk_tree() does the same \"immediately\n>>> reference the object member in lookup_blob()'s and lookup_tree()'s\n>>> return value\" thing.  I think those should receive the same treatment\n>>> as well.\n>> \n>> Hmm, are put_object_name() and all the walk() implementations ready for\n>> a NULL object handed to them?  Or would we rather need to error out\n>> right there?\n> How about this?\n>\n> -- >8 --\n> lookup_blob() and lookup_tree() can return NULL if they find an object\n> of an unexpected type.  Error out of fsck_walk_tree() in that case, like\n> we do when encountering a bad file mode.  An error message is already\n> shown by object_as_type(), which gets called by the lookup functions.\n\nThe result from options->walk() is checked, and among the callbacks\nthat are assigned to the .walk field:\n\n - mark_object() in builtin/fsck.c gives its own error message to diagnose broken link\n   and returns 1;\n\n - mark_used() in builtin/fsck.c silently returns 1;\n\n - mark_link() in builtin/index-pack.c does the same; and\n\n - check_object() in builtin/unpack-objects.c does the same,\n\nwhen they see a NULL object.\n\nThis patch may avoid the \"unexpected behaviour\" coming from\nexpecting that &((struct tree *)NULL)->object == NULL the current\ncode does, but it also changes the behaviour.  The loop used to\ndiagnose a fishy entry in the tree we are walking, and kept checking\nthe remaining entries in the tree.  You now immediately return, not\nseeing if the later entries in the tree are good and losing objects\nthat are referenced by these entries as dangling.\n\nI am not sure if this is a good change.  I suspect that the \"bad\nmode\" handling should be made less severe instead.\n\n>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  fsck.c | 10 ++++++++--\n>  1 file changed, 8 insertions(+), 2 deletions(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index 2ad00fc4d0..561a13ac27 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -358,14 +358,20 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op\n>  \t\t\tcontinue;\n>  \n>  \t\tif (S_ISDIR(entry.mode)) {\n> -\t\t\tobj = &lookup_tree(entry.oid)->object;\n> +\t\t\tstruct tree *tree = lookup_tree(entry.oid);\n> +\t\t\tif (!tree)\n> +\t\t\t\treturn -1;\n> +\t\t\tobj = &tree->object;\n>  \t\t\tif (name)\n>  \t\t\t\tput_object_name(options, obj, \"%s%s/\", name,\n>  \t\t\t\t\tentry.path);\n>  \t\t\tresult = options->walk(obj, OBJ_TREE, data, options);\n>  \t\t}\n>  \t\telse if (S_ISREG(entry.mode) || S_ISLNK(entry.mode)) {\n> -\t\t\tobj = &lookup_blob(entry.oid)->object;\n> +\t\t\tstruct blob *blob = lookup_blob(entry.oid);\n> +\t\t\tif (!blob)\n> +\t\t\t\treturn -1;\n> +\t\t\tobj = &blob->object;\n>  \t\t\tif (name)\n>  \t\t\t\tput_object_name(options, obj, \"%s%s\", name,\n>  \t\t\t\t\tentry.path);\n"},{"id":"329779","messageId":"1cf044ae-ae3f-4d44-3aac-b5cc6584de1e@web.de","threadId":"46869","inReplyTo":"xmqqh8vfftp7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fsck: check results of lookup_blob() and lookup_tree() for NULL","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-10-05T19:41:10Z","receivedAt":"2017-10-05T19:41:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 04.10.2017 um 06:00 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>> lookup_blob() and lookup_tree() can return NULL if they find an object\n>> of an unexpected type.  Error out of fsck_walk_tree() in that case, like\n>> we do when encountering a bad file mode.  An error message is already\n>> shown by object_as_type(), which gets called by the lookup functions.\n> \n> The result from options->walk() is checked, and among the callbacks\n> that are assigned to the .walk field:\n> \n>   - mark_object() in builtin/fsck.c gives its own error message to diagnose broken link\n>     and returns 1;\n> \n>   - mark_used() in builtin/fsck.c silently returns 1;\n> \n>   - mark_link() in builtin/index-pack.c does the same; and\n> \n>   - check_object() in builtin/unpack-objects.c does the same,\n> \n> when they see a NULL object.\n> \n> This patch may avoid the \"unexpected behaviour\" coming from\n> expecting that &((struct tree *)NULL)->object == NULL the current\n> code does, but it also changes the behaviour.  The loop used to\n> diagnose a fishy entry in the tree we are walking, and kept checking\n> the remaining entries in the tree.  You now immediately return, not\n> seeing if the later entries in the tree are good and losing objects\n> that are referenced by these entries as dangling.\n> \n> I am not sure if this is a good change.  I suspect that the \"bad\n> mode\" handling should be made less severe instead.\n\nMakes sense.  Replacement patch coming up.  I'll pass on the mode\nhandling change, though (at least for now).\n\nRené\n"},{"id":"329780","messageId":"6bbe21c0-024e-ceed-6076-25f2330e598d@web.de","threadId":"46869","inReplyTo":"54f5877b-a143-11c2-d8f6-ff28ed9e7e38@web.de","subject":"[PATCH v2] fsck: handle NULL return of lookup_blob() and lookup_tree()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-10-05T19:41:26Z","receivedAt":"2017-10-05T19:41:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"lookup_blob() and lookup_tree() can return NULL if they find an object\nof an unexpected type.  Accessing the object member is undefined in that\ncase.  Cast the result to a struct object pointer instead; we can do\nthat because object is the first member of all object types.  This trick\nis already used in other places in the code.\n\nAn error message is already shown by object_as_type(), which is called\nby the lookup functions.  The walk callback functions are expected to\nhandle NULL object pointers passed to them, but put_object_name() needs\na valid object, so avoid calling it without one.\n\nSuggested-by: SZEDER Gábor <szeder.dev@gmail.com>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\nChanges from v1:\n- Don't abort on encountering an object with a mismatched type.\n- Added a test for showing the problem with SANITIZE=address,undefined.\n\n fsck.c          |  8 ++++----\n t/t1450-fsck.sh | 22 ++++++++++++++++++++++\n 2 files changed, 26 insertions(+), 4 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 2ad00fc4d0..032699e9ac 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -358,15 +358,15 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op\n \t\t\tcontinue;\n \n \t\tif (S_ISDIR(entry.mode)) {\n-\t\t\tobj = &lookup_tree(entry.oid)->object;\n-\t\t\tif (name)\n+\t\t\tobj = (struct object *)lookup_tree(entry.oid);\n+\t\t\tif (name && obj)\n \t\t\t\tput_object_name(options, obj, \"%s%s/\", name,\n \t\t\t\t\tentry.path);\n \t\t\tresult = options->walk(obj, OBJ_TREE, data, options);\n \t\t}\n \t\telse if (S_ISREG(entry.mode) || S_ISLNK(entry.mode)) {\n-\t\t\tobj = &lookup_blob(entry.oid)->object;\n-\t\t\tif (name)\n+\t\t\tobj = (struct object *)lookup_blob(entry.oid);\n+\t\t\tif (name && obj)\n \t\t\t\tput_object_name(options, obj, \"%s%s\", name,\n \t\t\t\t\tentry.path);\n \t\t\tresult = options->walk(obj, OBJ_BLOB, data, options);\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 4087150db1..cb4b66e29d 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -222,6 +222,28 @@ test_expect_success 'unparseable tree object' '\n \ttest_i18ngrep ! \"fatal: empty filename in tree entry\" out\n '\n \n+hex2oct() {\n+\tperl -ne 'printf \"\\\\%03o\", hex for /../g'\n+}\n+\n+test_expect_success 'tree entry with type mismatch' '\n+\ttest_when_finished \"remove_object \\$blob\" &&\n+\ttest_when_finished \"remove_object \\$tree\" &&\n+\ttest_when_finished \"remove_object \\$commit\" &&\n+\ttest_when_finished \"git update-ref -d refs/heads/type_mismatch\" &&\n+\tblob=$(echo blob | git hash-object -w --stdin) &&\n+\tblob_bin=$(echo $blob | hex2oct) &&\n+\ttree=$(\n+\t\tprintf \"40000 dir\\0${blob_bin}100644 file\\0${blob_bin}\" |\n+\t\tgit hash-object -t tree --stdin -w --literally\n+\t) &&\n+\tcommit=$(git commit-tree $tree) &&\n+\tgit update-ref refs/heads/type_mismatch $commit &&\n+\ttest_must_fail git fsck >out 2>&1 &&\n+\ttest_i18ngrep \"is a blob, not a tree\" out &&\n+\ttest_i18ngrep ! \"dangling blob\" out\n+'\n+\n test_expect_success 'tag pointing to nonexistent' '\n \tcat >invalid-tag <<-\\EOF &&\n \tobject ffffffffffffffffffffffffffffffffffffffff\n-- \n2.14.2\n"},{"id":"329833","messageId":"xmqq8tgpypyl.fsf@gitster.mtv.corp.google.com","threadId":"46869","inReplyTo":"6bbe21c0-024e-ceed-6076-25f2330e598d@web.de","subject":"Re: [PATCH v2] fsck: handle NULL return of lookup_blob() and lookup_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T02:23:14Z","receivedAt":"2017-10-06T02:23:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> An error message is already shown by object_as_type(), which is called\n> by the lookup functions.  The walk callback functions are expected to\n> handle NULL object pointers passed to them, but put_object_name() needs\n> a valid object, so avoid calling it without one.\n\nThanks for getting the details right ;-)\n\n> +\tblob_bin=$(echo $blob | hex2oct) &&\n> +\ttree=$(\n> +\t\tprintf \"40000 dir\\0${blob_bin}100644 file\\0${blob_bin}\" |\n\nWow, that's ... cute.\n\n> +\t\tgit hash-object -t tree --stdin -w --literally\n\nMakes me curious why --literally is here.  Even if we let\ncheck_tree() called from index_mem() by taking the normal path,\nit wouldn't complain the type mismatch, I suspect.  I guess doing it\nthis way is a future-proof against check_tree() getting tightened in\nthe future, in which case I think it makes sense.\n\nAnd for the same reason, hashing \"--literally\" like this patch does\nis a better solution than using \"git mktree\", which would have\nallowed us to avoid the hex2oct and instead feed the tree in a bit\nmore human-readable way.\n\nThanks, will queue.\n\n> +\t) &&\n> +\tcommit=$(git commit-tree $tree) &&\n> +\tgit update-ref refs/heads/type_mismatch $commit &&\n> +\ttest_must_fail git fsck >out 2>&1 &&\n> +\ttest_i18ngrep \"is a blob, not a tree\" out &&\n> +\ttest_i18ngrep ! \"dangling blob\" out\n> +'\n> +\n>  test_expect_success 'tag pointing to nonexistent' '\n>  \tcat >invalid-tag <<-\\EOF &&\n>  \tobject ffffffffffffffffffffffffffffffffffffffff\n"},{"id":"329901","messageId":"954958d2-5d5a-15e0-0e2b-421f859c1227@web.de","threadId":"46869","inReplyTo":"xmqq8tgpypyl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] fsck: handle NULL return of lookup_blob() and lookup_tree()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-10-06T16:21:08Z","receivedAt":"2017-10-06T16:21:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.10.2017 um 04:23 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>> +\tblob_bin=$(echo $blob | hex2oct) &&\n>> +\ttree=$(\n>> +\t\tprintf \"40000 dir\\0${blob_bin}100644 file\\0${blob_bin}\" |\n> \n> Wow, that's ... cute.\n> \n>> +\t\tgit hash-object -t tree --stdin -w --literally\n> \n> Makes me curious why --literally is here.  Even if we let\n> check_tree() called from index_mem() by taking the normal path,\n> it wouldn't complain the type mismatch, I suspect.  I guess doing it\n> this way is a future-proof against check_tree() getting tightened in\n> the future, in which case I think it makes sense.\n> \n> And for the same reason, hashing \"--literally\" like this patch does\n> is a better solution than using \"git mktree\", which would have\n> allowed us to avoid the hex2oct and instead feed the tree in a bit\n> more human-readable way.\n\ngit mktree errors out already, complaining about the object type\nmismatch.  But I added \"--literally\" only accidentally, when I copied\nthe invocation from a few lines up.  The test works fine without that\nflag currently.  The flag captures the intent, however, of knowingly\nbuilding a flawed tree object.\n\nRené\n"}]}