threads / patch / 46869

patchtag: avoid NULL pointer arithmetic

Subject: [PATCH] tag: avoid NULL pointer arithmetic

## tl;dr

13 messages between Oct 1, 2017 and Oct 6, 2017. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

René Scharfe· Oct 1, 2017, 14:45 UTC · lore

lookup_blob() etc. can return NULL if the referenced object isn't of the expected type. In theory it's wrong to reference the object member in that case. In practice it's OK because it's located at offset 0 for all types, so the pointer arithmetic (NULL + 0) is optimized out by the compiler. The issue is reported by Clang's AddressSanitizer, though.

Avoid the ASan error by casting the results of the lookup functions to struct object pointers. That works fine with NULL pointers as well. We already rely on the object member being first in all object types in other places in the code.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 tag.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to tag.c +4 −4
diff --git a/tag.c b/tag.c
index 7e10acfb6e..fcbe012f7a 100644
--- a/tag.c
+++ b/tag.c
@@ -142,13 +142,13 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)
 	bufptr = nl + 1;
 
 	if (!strcmp(type, blob_type)) {
-		item->tagged = &lookup_blob(&oid)->object;
+		item->tagged = (struct object *)lookup_blob(&oid);
 	} else if (!strcmp(type, tree_type)) {
-		item->tagged = &lookup_tree(&oid)->object;
+		item->tagged = (struct object *)lookup_tree(&oid);
 	} else if (!strcmp(type, commit_type)) {
-		item->tagged = &lookup_commit(&oid)->object;
+		item->tagged = (struct object *)lookup_commit(&oid);
 	} else if (!strcmp(type, tag_type)) {
-		item->tagged = &lookup_tag(&oid)->object;
+		item->tagged = (struct object *)lookup_tag(&oid);
 	} else {
 		error("Unknown type %s", type);
 		item->tagged = NULL;

-- 
2.14.2
Junio C Hamano· Oct 2, 2017, 04:13 UTC · re: René Scharfe · lore

Re: [PATCH] tag: avoid NULL pointer arithmetic

René Scharfe <l.s.r@web.de> writes:
Show 13 quoted lines
> lookup_blob() etc. can return NULL if the referenced object isn't of the
> expected type.  In theory it's wrong to reference the object member in
> that case.  In practice it's OK because it's located at offset 0 for all
> types, so the pointer arithmetic (NULL + 0) is optimized out by the
> compiler.  The issue is reported by Clang's AddressSanitizer, though.
>
> Avoid the ASan error by casting the results of the lookup functions to
> struct object pointers.  That works fine with NULL pointers as well.  We
> already rely on the object member being first in all object types in
> other places in the code.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
Yikes, that is tricky.

Taking the address of .object field appears to be a lot cleaner and more kosher than casting, but from the compiler's point of view it's quite the opposite. Sigh.

Thanks.  The patch itself looks like the right solution to me.
Show 25 quoted lines
>  tag.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tag.c b/tag.c
> index 7e10acfb6e..fcbe012f7a 100644
> --- a/tag.c
> +++ b/tag.c
> @@ -142,13 +142,13 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)
>  	bufptr = nl + 1;
>  
>  	if (!strcmp(type, blob_type)) {
> -		item->tagged = &lookup_blob(&oid)->object;
> +		item->tagged = (struct object *)lookup_blob(&oid);
>  	} else if (!strcmp(type, tree_type)) {
> -		item->tagged = &lookup_tree(&oid)->object;
> +		item->tagged = (struct object *)lookup_tree(&oid);
>  	} else if (!strcmp(type, commit_type)) {
> -		item->tagged = &lookup_commit(&oid)->object;
> +		item->tagged = (struct object *)lookup_commit(&oid);
>  	} else if (!strcmp(type, tag_type)) {
> -		item->tagged = &lookup_tag(&oid)->object;
> +		item->tagged = (struct object *)lookup_tag(&oid);
>  	} else {
>  		error("Unknown type %s", type);
>  		item->tagged = NULL;
Jeff King· Oct 2, 2017, 05:08 UTC · re: René Scharfe · lore

Re: [PATCH] tag: avoid NULL pointer arithmetic

On Sun, Oct 01, 2017 at 04:45:13PM +0200, René Scharfe wrote:
Show 10 quoted lines
> lookup_blob() etc. can return NULL if the referenced object isn't of the
> expected type.  In theory it's wrong to reference the object member in
> that case.  In practice it's OK because it's located at offset 0 for all
> types, so the pointer arithmetic (NULL + 0) is optimized out by the
> compiler.  The issue is reported by Clang's AddressSanitizer, though.
> 
> Avoid the ASan error by casting the results of the lookup functions to
> struct object pointers.  That works fine with NULL pointers as well.  We
> already rely on the object member being first in all object types in
> other places in the code.

Out of curiosity, did you have to do anything to coax this out of ASan (e.g., a specific version)? I've been running it pretty regularly and didn't see this one (I did switch from clang to gcc a month or two ago, but this code is pretty old, I think).

-Peff
René Scharfe· Oct 2, 2017, 13:06 UTC · re: Jeff King · lore

Re: [PATCH] tag: avoid NULL pointer arithmetic

Am 02.10.2017 um 07:08 schrieb Jeff King:
Show 17 quoted lines
> On Sun, Oct 01, 2017 at 04:45:13PM +0200, René Scharfe wrote:
> 
>> lookup_blob() etc. can return NULL if the referenced object isn't of the
>> expected type.  In theory it's wrong to reference the object member in
>> that case.  In practice it's OK because it's located at offset 0 for all
>> types, so the pointer arithmetic (NULL + 0) is optimized out by the
>> compiler.  The issue is reported by Clang's AddressSanitizer, though.
>>
>> Avoid the ASan error by casting the results of the lookup functions to
>> struct object pointers.  That works fine with NULL pointers as well.  We
>> already rely on the object member being first in all object types in
>> other places in the code.
> 
> Out of curiosity, did you have to do anything to coax this out of ASan
> (e.g., a specific version)?  I've been running it pretty regularly and
> didn't see this one (I did switch from clang to gcc a month or two ago,
> but this code is pretty old, I think).

I did "make -j4 SANITIZE=undefined,address BLK_SHA1=1 test" with clang version 4.0.1-1 (tags/RELEASE_401/final), and t1450-fsck.sh failed.

René
Jeff King· Oct 2, 2017, 19:23 UTC · re: René Scharfe · lore

Re: [PATCH] tag: avoid NULL pointer arithmetic

On Mon, Oct 02, 2017 at 03:06:57PM +0200, René Scharfe wrote:
Show 12 quoted lines
> >> Avoid the ASan error by casting the results of the lookup functions to
> >> struct object pointers.  That works fine with NULL pointers as well.  We
> >> already rely on the object member being first in all object types in
> >> other places in the code.
> > 
> > Out of curiosity, did you have to do anything to coax this out of ASan
> > (e.g., a specific version)?  I've been running it pretty regularly and
> > didn't see this one (I did switch from clang to gcc a month or two ago,
> > but this code is pretty old, I think).
> 
> I did "make -j4 SANITIZE=undefined,address BLK_SHA1=1 test" with
> clang version 4.0.1-1 (tags/RELEASE_401/final), and t1450-fsck.sh failed.
Interesting. I can trigger it with the same setup, but not if:
  1. I use gcc instead of clang.
  2. If I only use one of UBSan and ASan. It's the combination that
     triggers it.
-Peff
SZEDER Gábor· Oct 3, 2017, 10:22 UTC · re: René Scharfe · lore

Re: [PATCH] tag: avoid NULL pointer arithmetic

Show 10 quoted lines
> lookup_blob() etc. can return NULL if the referenced object isn't of the
> expected type.  In theory it's wrong to reference the object member in
> that case.  In practice it's OK because it's located at offset 0 for all
> types, so the pointer arithmetic (NULL + 0) is optimized out by the
> compiler.  The issue is reported by Clang's AddressSanitizer, though.
> 
> Avoid the ASan error by casting the results of the lookup functions to
> struct object pointers.  That works fine with NULL pointers as well.  We
> already rely on the object member being first in all object types in
> other places in the code.

This sounds like the main goal of the patch is to avoid an ASan error, but I think it's more important to avoid (and to be more explicit about avoiding) the undefined behavior. I.e. along the lines of s/In theory it's wrong/It's undefined behavior/ and s/ASan error/undefined behavior/

Furthermore, fsck.c:fsck_walk_tree() does the same "immediately reference the object member in lookup_blob()'s and lookup_tree()'s return value" thing. I think those should receive the same treatment as well.

Show 30 quoted lines
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  tag.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tag.c b/tag.c
> index 7e10acfb6e..fcbe012f7a 100644
> --- a/tag.c
> +++ b/tag.c
> @@ -142,13 +142,13 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)
>  	bufptr = nl + 1;
>  
>  	if (!strcmp(type, blob_type)) {
> -		item->tagged = &lookup_blob(&oid)->object;
> +		item->tagged = (struct object *)lookup_blob(&oid);
>  	} else if (!strcmp(type, tree_type)) {
> -		item->tagged = &lookup_tree(&oid)->object;
> +		item->tagged = (struct object *)lookup_tree(&oid);
>  	} else if (!strcmp(type, commit_type)) {
> -		item->tagged = &lookup_commit(&oid)->object;
> +		item->tagged = (struct object *)lookup_commit(&oid);
>  	} else if (!strcmp(type, tag_type)) {
> -		item->tagged = &lookup_tag(&oid)->object;
> +		item->tagged = (struct object *)lookup_tag(&oid);
>  	} else {
>  		error("Unknown type %s", type);
>  		item->tagged = NULL;
> -- 
> 2.14.2
>
René Scharfe· Oct 3, 2017, 12:51 UTC · re: SZEDER Gábor · lore

Re: [PATCH] tag: avoid NULL pointer arithmetic

Am 03.10.2017 um 12:22 schrieb SZEDER Gábor:
Show 16 quoted lines
>> lookup_blob() etc. can return NULL if the referenced object isn't of the
>> expected type.  In theory it's wrong to reference the object member in
>> that case.  In practice it's OK because it's located at offset 0 for all
>> types, so the pointer arithmetic (NULL + 0) is optimized out by the
>> compiler.  The issue is reported by Clang's AddressSanitizer, though.
>>
>> Avoid the ASan error by casting the results of the lookup functions to
>> struct object pointers.  That works fine with NULL pointers as well.  We
>> already rely on the object member being first in all object types in
>> other places in the code.
> 
> This sounds like the main goal of the patch is to avoid an ASan error,
> but I think it's more important to avoid (and to be more explicit
> about avoiding) the undefined behavior.  I.e. along the lines of
> s/In theory it's wrong/It's undefined behavior/ and
> s/ASan error/undefined behavior/

You are probably right, but I struggle to pin down the difference. Another try: Adding zero to a NULL pointer is undefined, but is either optimized out or has no effect. That's why the code doesn't crash without ASan and UBSan. Relying on undefined behavior is wrong nevertheless, so let's stop doing it.

> Furthermore, fsck.c:fsck_walk_tree() does the same "immediately
> reference the object member in lookup_blob()'s and lookup_tree()'s
> return value" thing.  I think those should receive the same treatment
> as well.

Hmm, are put_object_name() and all the walk() implementations ready for a NULL object handed to them? Or would we rather need to error out right there? Other suspicious places in this regard are (at least): builtin/merge-tree.c, cache-tree.c, http-push.c, list-objects.c, merge-recursive.c, and shallow.c. :-/

René
René Scharfe· Oct 3, 2017, 13:47 UTC · re: René Scharfe · lore

[PATCH] fsck: check results of lookup_blob() and lookup_tree() for NULL

Am 03.10.2017 um 14:51 schrieb René Scharfe:
Show 9 quoted lines
> Am 03.10.2017 um 12:22 schrieb SZEDER Gábor:
>> Furthermore, fsck.c:fsck_walk_tree() does the same "immediately
>> reference the object member in lookup_blob()'s and lookup_tree()'s
>> return value" thing.  I think those should receive the same treatment
>> as well.
> 
> Hmm, are put_object_name() and all the walk() implementations ready for
> a NULL object handed to them?  Or would we rather need to error out
> right there?
How about this?

-- >8 -- lookup_blob() and lookup_tree() can return NULL if they find an object of an unexpected type. Error out of fsck_walk_tree() in that case, like we do when encountering a bad file mode. An error message is already shown by object_as_type(), which gets called by the lookup functions.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 fsck.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
Show changes to fsck.c +8 −2
diff --git a/fsck.c b/fsck.c
index 2ad00fc4d0..561a13ac27 100644
--- a/fsck.c
+++ b/fsck.c
@@ -358,14 +358,20 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op
 			continue;
 
 		if (S_ISDIR(entry.mode)) {
-			obj = &lookup_tree(entry.oid)->object;
+			struct tree *tree = lookup_tree(entry.oid);
+			if (!tree)
+				return -1;
+			obj = &tree->object;
 			if (name)
 				put_object_name(options, obj, "%s%s/", name,
 					entry.path);
 			result = options->walk(obj, OBJ_TREE, data, options);
 		}
 		else if (S_ISREG(entry.mode) || S_ISLNK(entry.mode)) {
-			obj = &lookup_blob(entry.oid)->object;
+			struct blob *blob = lookup_blob(entry.oid);
+			if (!blob)
+				return -1;
+			obj = &blob->object;
 			if (name)
 				put_object_name(options, obj, "%s%s", name,
 					entry.path);

-- 
2.14.2
Junio C Hamano· Oct 4, 2017, 04:00 UTC · re: René Scharfe · lore

Re: [PATCH] fsck: check results of lookup_blob() and lookup_tree() for NULL

René Scharfe <l.s.r@web.de> writes:
Show 17 quoted lines
> Am 03.10.2017 um 14:51 schrieb René Scharfe:
>> Am 03.10.2017 um 12:22 schrieb SZEDER Gábor:
>>> Furthermore, fsck.c:fsck_walk_tree() does the same "immediately
>>> reference the object member in lookup_blob()'s and lookup_tree()'s
>>> return value" thing.  I think those should receive the same treatment
>>> as well.
>> 
>> Hmm, are put_object_name() and all the walk() implementations ready for
>> a NULL object handed to them?  Or would we rather need to error out
>> right there?
> How about this?
>
> -- >8 --
> lookup_blob() and lookup_tree() can return NULL if they find an object
> of an unexpected type.  Error out of fsck_walk_tree() in that case, like
> we do when encountering a bad file mode.  An error message is already
> shown by object_as_type(), which gets called by the lookup functions.

The result from options->walk() is checked, and among the callbacks that are assigned to the .walk field:

 - mark_object() in builtin/fsck.c gives its own error message to diagnose broken link
   and returns 1;
 - mark_used() in builtin/fsck.c silently returns 1;
 - mark_link() in builtin/index-pack.c does the same; and
 - check_object() in builtin/unpack-objects.c does the same,
when they see a NULL object.

This patch may avoid the "unexpected behaviour" coming from expecting that &((struct tree *)NULL)->object == NULL the current code does, but it also changes the behaviour. The loop used to diagnose a fishy entry in the tree we are walking, and kept checking the remaining entries in the tree. You now immediately return, not seeing if the later entries in the tree are good and losing objects that are referenced by these entries as dangling.

I am not sure if this is a good change. I suspect that the "bad mode" handling should be made less severe instead.

Show 33 quoted lines
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  fsck.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index 2ad00fc4d0..561a13ac27 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -358,14 +358,20 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op
>  			continue;
>  
>  		if (S_ISDIR(entry.mode)) {
> -			obj = &lookup_tree(entry.oid)->object;
> +			struct tree *tree = lookup_tree(entry.oid);
> +			if (!tree)
> +				return -1;
> +			obj = &tree->object;
>  			if (name)
>  				put_object_name(options, obj, "%s%s/", name,
>  					entry.path);
>  			result = options->walk(obj, OBJ_TREE, data, options);
>  		}
>  		else if (S_ISREG(entry.mode) || S_ISLNK(entry.mode)) {
> -			obj = &lookup_blob(entry.oid)->object;
> +			struct blob *blob = lookup_blob(entry.oid);
> +			if (!blob)
> +				return -1;
> +			obj = &blob->object;
>  			if (name)
>  				put_object_name(options, obj, "%s%s", name,
>  					entry.path);
René Scharfe· Oct 5, 2017, 19:41 UTC · re: Junio C Hamano · lore

Re: [PATCH] fsck: check results of lookup_blob() and lookup_tree() for NULL

Am 04.10.2017 um 06:00 schrieb Junio C Hamano:
Show 30 quoted lines
> René Scharfe <l.s.r@web.de> writes:
>> lookup_blob() and lookup_tree() can return NULL if they find an object
>> of an unexpected type.  Error out of fsck_walk_tree() in that case, like
>> we do when encountering a bad file mode.  An error message is already
>> shown by object_as_type(), which gets called by the lookup functions.
> 
> The result from options->walk() is checked, and among the callbacks
> that are assigned to the .walk field:
> 
>   - mark_object() in builtin/fsck.c gives its own error message to diagnose broken link
>     and returns 1;
> 
>   - mark_used() in builtin/fsck.c silently returns 1;
> 
>   - mark_link() in builtin/index-pack.c does the same; and
> 
>   - check_object() in builtin/unpack-objects.c does the same,
> 
> when they see a NULL object.
> 
> This patch may avoid the "unexpected behaviour" coming from
> expecting that &((struct tree *)NULL)->object == NULL the current
> code does, but it also changes the behaviour.  The loop used to
> diagnose a fishy entry in the tree we are walking, and kept checking
> the remaining entries in the tree.  You now immediately return, not
> seeing if the later entries in the tree are good and losing objects
> that are referenced by these entries as dangling.
> 
> I am not sure if this is a good change.  I suspect that the "bad
> mode" handling should be made less severe instead.

Makes sense. Replacement patch coming up. I'll pass on the mode handling change, though (at least for now).

René
René Scharfe· Oct 5, 2017, 19:41 UTC · re: René Scharfe · lore

[PATCH v2] fsck: handle NULL return of lookup_blob() and lookup_tree()

lookup_blob() and lookup_tree() can return NULL if they find an object of an unexpected type. Accessing the object member is undefined in that case. Cast the result to a struct object pointer instead; we can do that because object is the first member of all object types. This trick is already used in other places in the code.

An error message is already shown by object_as_type(), which is called by the lookup functions. The walk callback functions are expected to handle NULL object pointers passed to them, but put_object_name() needs a valid object, so avoid calling it without one.

Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Changes from v1:
- Don't abort on encountering an object with a mismatched type.
- Added a test for showing the problem with SANITIZE=address,undefined.
 fsck.c          |  8 ++++----
 t/t1450-fsck.sh | 22 ++++++++++++++++++++++
 2 files changed, 26 insertions(+), 4 deletions(-)
Show changes to 2 files +26 −4

fsck.c, t/t1450-fsck.sh

diff --git a/fsck.c b/fsck.c
index 2ad00fc4d0..032699e9ac 100644
--- a/fsck.c
+++ b/fsck.c
@@ -358,15 +358,15 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op
 			continue;
 
 		if (S_ISDIR(entry.mode)) {
-			obj = &lookup_tree(entry.oid)->object;
-			if (name)
+			obj = (struct object *)lookup_tree(entry.oid);
+			if (name && obj)
 				put_object_name(options, obj, "%s%s/", name,
 					entry.path);
 			result = options->walk(obj, OBJ_TREE, data, options);
 		}
 		else if (S_ISREG(entry.mode) || S_ISLNK(entry.mode)) {
-			obj = &lookup_blob(entry.oid)->object;
-			if (name)
+			obj = (struct object *)lookup_blob(entry.oid);
+			if (name && obj)
 				put_object_name(options, obj, "%s%s", name,
 					entry.path);
 			result = options->walk(obj, OBJ_BLOB, data, options);
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 4087150db1..cb4b66e29d 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -222,6 +222,28 @@ test_expect_success 'unparseable tree object' '
 	test_i18ngrep ! "fatal: empty filename in tree entry" out
 '
 
+hex2oct() {
+	perl -ne 'printf "\\%03o", hex for /../g'
+}
+
+test_expect_success 'tree entry with type mismatch' '
+	test_when_finished "remove_object \$blob" &&
+	test_when_finished "remove_object \$tree" &&
+	test_when_finished "remove_object \$commit" &&
+	test_when_finished "git update-ref -d refs/heads/type_mismatch" &&
+	blob=$(echo blob | git hash-object -w --stdin) &&
+	blob_bin=$(echo $blob | hex2oct) &&
+	tree=$(
+		printf "40000 dir\0${blob_bin}100644 file\0${blob_bin}" |
+		git hash-object -t tree --stdin -w --literally
+	) &&
+	commit=$(git commit-tree $tree) &&
+	git update-ref refs/heads/type_mismatch $commit &&
+	test_must_fail git fsck >out 2>&1 &&
+	test_i18ngrep "is a blob, not a tree" out &&
+	test_i18ngrep ! "dangling blob" out
+'
+
 test_expect_success 'tag pointing to nonexistent' '
 	cat >invalid-tag <<-\EOF &&
 	object ffffffffffffffffffffffffffffffffffffffff

-- 
2.14.2
Junio C Hamano· Oct 6, 2017, 02:23 UTC · re: René Scharfe · lore

Re: [PATCH v2] fsck: handle NULL return of lookup_blob() and lookup_tree()

René Scharfe <l.s.r@web.de> writes:
> An error message is already shown by object_as_type(), which is called
> by the lookup functions.  The walk callback functions are expected to
> handle NULL object pointers passed to them, but put_object_name() needs
> a valid object, so avoid calling it without one.
Thanks for getting the details right ;-)
> +	blob_bin=$(echo $blob | hex2oct) &&
> +	tree=$(
> +		printf "40000 dir\0${blob_bin}100644 file\0${blob_bin}" |
Wow, that's ... cute.
> +		git hash-object -t tree --stdin -w --literally

Makes me curious why --literally is here. Even if we let check_tree() called from index_mem() by taking the normal path, it wouldn't complain the type mismatch, I suspect. I guess doing it this way is a future-proof against check_tree() getting tightened in the future, in which case I think it makes sense.

And for the same reason, hashing "--literally" like this patch does is a better solution than using "git mktree", which would have allowed us to avoid the hex2oct and instead feed the tree in a bit more human-readable way.

Thanks, will queue.
Show 11 quoted lines
> +	) &&
> +	commit=$(git commit-tree $tree) &&
> +	git update-ref refs/heads/type_mismatch $commit &&
> +	test_must_fail git fsck >out 2>&1 &&
> +	test_i18ngrep "is a blob, not a tree" out &&
> +	test_i18ngrep ! "dangling blob" out
> +'
> +
>  test_expect_success 'tag pointing to nonexistent' '
>  	cat >invalid-tag <<-\EOF &&
>  	object ffffffffffffffffffffffffffffffffffffffff
René Scharfe· Oct 6, 2017, 16:21 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] fsck: handle NULL return of lookup_blob() and lookup_tree()

Am 06.10.2017 um 04:23 schrieb Junio C Hamano:
Show 19 quoted lines
> René Scharfe <l.s.r@web.de> writes:
>> +	blob_bin=$(echo $blob | hex2oct) &&
>> +	tree=$(
>> +		printf "40000 dir\0${blob_bin}100644 file\0${blob_bin}" |
> 
> Wow, that's ... cute.
> 
>> +		git hash-object -t tree --stdin -w --literally
> 
> Makes me curious why --literally is here.  Even if we let
> check_tree() called from index_mem() by taking the normal path,
> it wouldn't complain the type mismatch, I suspect.  I guess doing it
> this way is a future-proof against check_tree() getting tightened in
> the future, in which case I think it makes sense.
> 
> And for the same reason, hashing "--literally" like this patch does
> is a better solution than using "git mktree", which would have
> allowed us to avoid the hex2oct and instead feed the tree in a bit
> more human-readable way.

git mktree errors out already, complaining about the object type mismatch. But I added "--literally" only accidentally, when I copied the invocation from a few lines up. The test works fine without that flag currently. The flag captures the intent, however, of knowingly building a flawed tree object.

René

← back to recent threads