[PATCH] list-objects: perform NULL check before the variable is dereferenced

[PATCH] list-objects: perform NULL check before the variable is dereferenced

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 list-objects.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/list-objects.c b/list-objects.c
index 3dd4a96..34044d9 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -15,12 +15,13 @@ static void process_blob(struct rev_info *revs,
 			 const char *name,
 			 void *cb_data)
 {
-	struct object *obj = &blob->object;
+	struct object *obj;
 
 	if (!revs->blob_objects)
 		return;
-	if (!obj)
+	if (!blob)
 		die("bad blob object");
+	obj = &blob->object;
 	if (obj->flags & (UNINTERESTING | SEEN))
 		return;
 	obj->flags |= SEEN;
@@ -67,7 +68,7 @@ static void process_tree(struct rev_info *revs,
 			 const char *name,
 			 void *cb_data)
 {
-	struct object *obj = &tree->object;
+	struct object *obj;
 	struct tree_desc desc;
 	struct name_entry entry;
 	struct name_path me;
@@ -77,8 +78,9 @@ static void process_tree(struct rev_info *revs,
 
 	if (!revs->tree_objects)
 		return;
-	if (!obj)
+	if (!tree)
 		die("bad tree object");
+	obj = &tree->object;
 	if (obj->flags & (UNINTERESTING | SEEN))
 		return;
 	if (parse_tree(tree) < 0)
-- 
1.7.3.1.256.g2539c.dirty

Re: [PATCH] list-objects: perform NULL check before the variable is dereferenced

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  list-objects.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)

Please explain why this is needed?

I can see that process_blob() is called from process_tree() which passes
the return value from lookup_blob(entry.sha1) directly without looking at
it.  lookup_blob() can issue an error message and return NULL if there is
a SHA-1 collision with an object that is not a blob.

> diff --git a/list-objects.c b/list-objects.c
> index 3dd4a96..34044d9 100644
> --- a/list-objects.c
> +++ b/list-objects.c
> @@ -15,12 +15,13 @@ static void process_blob(struct rev_info *revs,
>  			 const char *name,
>  			 void *cb_data)
>  {
> -	struct object *obj = &blob->object;
> +	struct object *obj;
>  
>  	if (!revs->blob_objects)
>  		return;
> -	if (!obj)
> +	if (!blob)
>  		die("bad blob object");
> +	obj = &blob->object;
>  	if (obj->flags & (UNINTERESTING | SEEN))
>  		return;
>  	obj->flags |= SEEN;
> @@ -67,7 +68,7 @@ static void process_tree(struct rev_info *revs,
>  			 const char *name,
>  			 void *cb_data)
>  {
> -	struct object *obj = &tree->object;
> +	struct object *obj;
>  	struct tree_desc desc;
>  	struct name_entry entry;
>  	struct name_path me;
> @@ -77,8 +78,9 @@ static void process_tree(struct rev_info *revs,
>  
>  	if (!revs->tree_objects)
>  		return;
> -	if (!obj)
> +	if (!tree)
>  		die("bad tree object");
> +	obj = &tree->object;
>  	if (obj->flags & (UNINTERESTING | SEEN))
>  		return;
>  	if (parse_tree(tree) < 0)

Re: [PATCH] list-objects: perform NULL check before the variable is dereferenced

[Nguyen Thai Ngoc Duy]

2012/4/10 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  list-objects.c |   10 ++++++----
>>  1 files changed, 6 insertions(+), 4 deletions(-)
>
> Please explain why this is needed?
>
> I can see that process_blob() is called from process_tree() which passes
> the return value from lookup_blob(entry.sha1) directly without looking at
> it.  lookup_blob() can issue an error message and return NULL if there is
> a SHA-1 collision with an object that is not a blob.

to avoid segmentation fault in that case, if "blob" is NULL, it dies
at blob->object anyway and the check "if (!obj)" is useless.

>
>> diff --git a/list-objects.c b/list-objects.c
>> index 3dd4a96..34044d9 100644
>> --- a/list-objects.c
>> +++ b/list-objects.c
>> @@ -15,12 +15,13 @@ static void process_blob(struct rev_info *revs,
>>                        const char *name,
>>                        void *cb_data)
>>  {
>> -     struct object *obj = &blob->object;
>> +     struct object *obj;
>>
>>       if (!revs->blob_objects)
>>               return;
>> -     if (!obj)
>> +     if (!blob)
>>               die("bad blob object");
>> +     obj = &blob->object;
>>       if (obj->flags & (UNINTERESTING | SEEN))
>>               return;
>>       obj->flags |= SEEN;
>> @@ -67,7 +68,7 @@ static void process_tree(struct rev_info *revs,
>>                        const char *name,
>>                        void *cb_data)
>>  {
>> -     struct object *obj = &tree->object;
>> +     struct object *obj;
>>       struct tree_desc desc;
>>       struct name_entry entry;
>>       struct name_path me;
>> @@ -77,8 +78,9 @@ static void process_tree(struct rev_info *revs,
>>
>>       if (!revs->tree_objects)
>>               return;
>> -     if (!obj)
>> +     if (!tree)
>>               die("bad tree object");
>> +     obj = &tree->object;
>>       if (obj->flags & (UNINTERESTING | SEEN))
>>               return;
>>       if (parse_tree(tree) < 0)



-- 
Duy

Re: [PATCH] list-objects: perform NULL check before the variable is dereferenced

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> 2012/4/10 Junio C Hamano <gitster@pobox.com>:
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>>  list-objects.c |   10 ++++++----
>>>  1 files changed, 6 insertions(+), 4 deletions(-)
>>
>> Please explain why this is needed?
>>
>> I can see that process_blob() is called from process_tree() which passes
>> the return value from lookup_blob(entry.sha1) directly without looking at
>> it.  lookup_blob() can issue an error message and return NULL if there is
>> a SHA-1 collision with an object that is not a blob.
>
> to avoid segmentation fault in that case, if "blob" is NULL, it dies
> at blob->object anyway and the check "if (!obj)" is useless.

Well, I didn't mean that you should explain whatever to _me_ as a
response; I meant that the log message should explain that to future
readers. I thought you have been here long enough to know that ;-)

Also "check NULL before dereferencing" means "avoid segmentation fault",
so that is not the primary thing that needs to be explained.  The point is
to explain why and how a NULL could come into the codepath in the first
place.

Please try again when post 1.7.10 cycle opens.

Thanks.

Re: [PATCH] list-objects: perform NULL check before the variable is dereferenced

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> 2012/4/10 Junio C Hamano <gitster@pobox.com>:
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>>  list-objects.c |   10 ++++++----
>>>  1 files changed, 6 insertions(+), 4 deletions(-)
>>
>> Please explain why this is needed?
>>
>> I can see that process_blob() is called from process_tree() which passes
>> the return value from lookup_blob(entry.sha1) directly without looking at
>> it.  lookup_blob() can issue an error message and return NULL if there is
>> a SHA-1 collision with an object that is not a blob.
>
> to avoid segmentation fault in that case, if "blob" is NULL, it dies
> at blob->object anyway and the check "if (!obj)" is useless.

Well, I didn't mean that you should explain whatever to _me_ as a
response; I meant that the log message should explain that to future
readers. I thought you have been here long enough to know that ;-)

Also "check NULL before dereferencing" means "avoid segmentation fault",
so that is not the primary thing that needs to be explained.  The point is
to explain why and how a NULL could come into the codepath in the first
place.

Please try again when the post-1.7.10 cycle opens.

Thanks.