[PATCH] tag: avoid NULL pointer arithmetic

[PATCH] tag: avoid NULL pointer arithmetic

[René Scharfe]

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(-)

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

Re: [PATCH] tag: avoid NULL pointer arithmetic

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> 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.

>  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;

Re: [PATCH] tag: avoid NULL pointer arithmetic

[Jeff King]

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).

-Peff

Re: [PATCH] tag: avoid NULL pointer arithmetic

[René Scharfe]

Am 02.10.2017 um 07:08 schrieb Jeff King:
> 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é

Re: [PATCH] tag: avoid NULL pointer arithmetic

[Jeff King]

On Mon, Oct 02, 2017 at 03:06:57PM +0200, René Scharfe wrote:

> >> 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

Re: [PATCH] tag: avoid NULL pointer arithmetic

[SZEDER Gábor]

> 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.

> 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
> 

Re: [PATCH] tag: avoid NULL pointer arithmetic

[René Scharfe]

Am 03.10.2017 um 12:22 schrieb SZEDER Gábor:
>> 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é

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

[René Scharfe]

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.

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);
-- 
2.14.2

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> 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.

>
> 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);

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

[René Scharfe]

Am 04.10.2017 um 06:00 schrieb Junio C Hamano:
> 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é

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

[René Scharfe]

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(-)

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

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

[Junio C Hamano]

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.

> +	) &&
> +	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

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

[René Scharfe]

Am 06.10.2017 um 04:23 schrieb Junio C Hamano:
> 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é