{"thread":{"id":"30187","subject":"[PATCH] list-objects: perform NULL check before the variable is dereferenced","startedAt":"2012-04-09T10:45:11Z","lastAt":"2012-04-10T00:09:12Z","messageCount":5,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"188792","messageId":"1333968311-29889-1-git-send-email-pclouds@gmail.com","threadId":"30187","inReplyTo":null,"subject":"[PATCH] list-objects: perform NULL check before the variable is dereferenced","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-04-09T10:45:11Z","receivedAt":"2012-04-09T10:45:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n list-objects.c | 10 ++++++----\n 1 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex 3dd4a96..34044d9 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -15,12 +15,13 @@ static void process_blob(struct rev_info *revs,\n \t\t\t const char *name,\n \t\t\t void *cb_data)\n {\n-\tstruct object *obj = &blob->object;\n+\tstruct object *obj;\n \n \tif (!revs->blob_objects)\n \t\treturn;\n-\tif (!obj)\n+\tif (!blob)\n \t\tdie(\"bad blob object\");\n+\tobj = &blob->object;\n \tif (obj->flags & (UNINTERESTING | SEEN))\n \t\treturn;\n \tobj->flags |= SEEN;\n@@ -67,7 +68,7 @@ static void process_tree(struct rev_info *revs,\n \t\t\t const char *name,\n \t\t\t void *cb_data)\n {\n-\tstruct object *obj = &tree->object;\n+\tstruct object *obj;\n \tstruct tree_desc desc;\n \tstruct name_entry entry;\n \tstruct name_path me;\n@@ -77,8 +78,9 @@ static void process_tree(struct rev_info *revs,\n \n \tif (!revs->tree_objects)\n \t\treturn;\n-\tif (!obj)\n+\tif (!tree)\n \t\tdie(\"bad tree object\");\n+\tobj = &tree->object;\n \tif (obj->flags & (UNINTERESTING | SEEN))\n \t\treturn;\n \tif (parse_tree(tree) < 0)\n-- \n1.7.3.1.256.g2539c.dirty\n"},{"id":"188807","messageId":"7v7gxowxfh.fsf@alter.siamese.dyndns.org","threadId":"30187","inReplyTo":"1333968311-29889-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] list-objects: perform NULL check before the variable is dereferenced","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-09T18:51:46Z","receivedAt":"2012-04-09T18:51:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> list-objects.c | 10 ++++++----\n> 1 files changed, 6 insertions(+), 4 deletions(-)\n\nPlease explain why this is needed?\n\nI can see that process_blob() is called from process_tree() which passes\nthe return value from lookup_blob(entry.sha1) directly without looking at\nit. lookup_blob() can issue an error message and return NULL if there is\na SHA-1 collision with an object that is not a blob.\n\n> diff --git a/list-objects.c b/list-objects.c\n> index 3dd4a96..34044d9 100644\n> --- a/list-objects.c\n> +++ b/list-objects.c\n> @@ -15,12 +15,13 @@ static void process_blob(struct rev_info *revs,\n> \t\t\t const char *name,\n> \t\t\t void *cb_data)\n> {\n> -\tstruct object *obj = &blob->object;\n> +\tstruct object *obj;\n> \n> \tif (!revs->blob_objects)\n> \t\treturn;\n> -\tif (!obj)\n> +\tif (!blob)\n> \t\tdie(\"bad blob object\");\n> +\tobj = &blob->object;\n> \tif (obj->flags & (UNINTERESTING | SEEN))\n> \t\treturn;\n> \tobj->flags |= SEEN;\n> @@ -67,7 +68,7 @@ static void process_tree(struct rev_info *revs,\n> \t\t\t const char *name,\n> \t\t\t void *cb_data)\n> {\n> -\tstruct object *obj = &tree->object;\n> +\tstruct object *obj;\n> \tstruct tree_desc desc;\n> \tstruct name_entry entry;\n> \tstruct name_path me;\n> @@ -77,8 +78,9 @@ static void process_tree(struct rev_info *revs,\n> \n> \tif (!revs->tree_objects)\n> \t\treturn;\n> -\tif (!obj)\n> +\tif (!tree)\n> \t\tdie(\"bad tree object\");\n> +\tobj = &tree->object;\n> \tif (obj->flags & (UNINTERESTING | SEEN))\n> \t\treturn;\n> \tif (parse_tree(tree) < 0)\n"},{"id":"188825","messageId":"CACsJy8ASo8j+hmJ7Kw2uLcD9cS8SXc6yOoc5GDUcFHYk9W9a_g@mail.gmail.com","threadId":"30187","inReplyTo":"7v7gxowxfh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] list-objects: perform NULL check before the variable is dereferenced","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-04-09T23:47:17Z","receivedAt":"2012-04-09T23:47:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2012/4/10 Junio C Hamano :\n> Nguyễn Thái Ngọc Duy   writes:\n>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>>  list-objects.c |   10 ++++++----\n>>  1 files changed, 6 insertions(+), 4 deletions(-)\n>\n> Please explain why this is needed?\n>\n> I can see that process_blob() is called from process_tree() which passes\n> the return value from lookup_blob(entry.sha1) directly without looking at\n> it.  lookup_blob() can issue an error message and return NULL if there is\n> a SHA-1 collision with an object that is not a blob.\n\nto avoid segmentation fault in that case, if \"blob\" is NULL, it dies\nat blob->object anyway and the check \"if (!obj)\" is useless.\n\n>\n>> diff --git a/list-objects.c b/list-objects.c\n>> index 3dd4a96..34044d9 100644\n>> --- a/list-objects.c\n>> +++ b/list-objects.c\n>> @@ -15,12 +15,13 @@ static void process_blob(struct rev_info *revs,\n>>                        const char *name,\n>>                        void *cb_data)\n>>  {\n>> -     struct object *obj = &blob->object;\n>> +     struct object *obj;\n>>\n>>       if (!revs->blob_objects)\n>>               return;\n>> -     if (!obj)\n>> +     if (!blob)\n>>               die(\"bad blob object\");\n>> +     obj = &blob->object;\n>>       if (obj->flags & (UNINTERESTING | SEEN))\n>>               return;\n>>       obj->flags |= SEEN;\n>> @@ -67,7 +68,7 @@ static void process_tree(struct rev_info *revs,\n>>                        const char *name,\n>>                        void *cb_data)\n>>  {\n>> -     struct object *obj = &tree->object;\n>> +     struct object *obj;\n>>       struct tree_desc desc;\n>>       struct name_entry entry;\n>>       struct name_path me;\n>> @@ -77,8 +78,9 @@ static void process_tree(struct rev_info *revs,\n>>\n>>       if (!revs->tree_objects)\n>>               return;\n>> -     if (!obj)\n>> +     if (!tree)\n>>               die(\"bad tree object\");\n>> +     obj = &tree->object;\n>>       if (obj->flags & (UNINTERESTING | SEEN))\n>>               return;\n>>       if (parse_tree(tree) < 0)\n\n\n\n-- \nDuy\n"},{"id":"188826","messageId":"7vbon0tplt.fsf@alter.siamese.dyndns.org","threadId":"30187","inReplyTo":"CACsJy8ASo8j+hmJ7Kw2uLcD9cS8SXc6yOoc5GDUcFHYk9W9a_g@mail.gmail.com","subject":"Re: [PATCH] list-objects: perform NULL check before the variable is dereferenced","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-10T00:09:02Z","receivedAt":"2012-04-10T00:09:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> 2012/4/10 Junio C Hamano :\n>> Nguyễn Thái Ngọc Duy   writes:\n>>\n>>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>>> ---\n>>>  list-objects.c |   10 ++++++----\n>>>  1 files changed, 6 insertions(+), 4 deletions(-)\n>>\n>> Please explain why this is needed?\n>>\n>> I can see that process_blob() is called from process_tree() which passes\n>> the return value from lookup_blob(entry.sha1) directly without looking at\n>> it.  lookup_blob() can issue an error message and return NULL if there is\n>> a SHA-1 collision with an object that is not a blob.\n>\n> to avoid segmentation fault in that case, if \"blob\" is NULL, it dies\n> at blob->object anyway and the check \"if (!obj)\" is useless.\n\nWell, I didn't mean that you should explain whatever to _me_ as a\nresponse; I meant that the log message should explain that to future\nreaders. I thought you have been here long enough to know that ;-)\n\nAlso \"check NULL before dereferencing\" means \"avoid segmentation fault\",\nso that is not the primary thing that needs to be explained. The point is\nto explain why and how a NULL could come into the codepath in the first\nplace.\n\nPlease try again when post 1.7.10 cycle opens.\n\nThanks.\n"},{"id":"188827","messageId":"7vaa2ktplj.fsf@alter.siamese.dyndns.org","threadId":"30187","inReplyTo":"CACsJy8ASo8j+hmJ7Kw2uLcD9cS8SXc6yOoc5GDUcFHYk9W9a_g@mail.gmail.com","subject":"Re: [PATCH] list-objects: perform NULL check before the variable is dereferenced","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-10T00:09:12Z","receivedAt":"2012-04-10T00:09:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> 2012/4/10 Junio C Hamano :\n>> Nguyễn Thái Ngọc Duy   writes:\n>>\n>>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>>> ---\n>>>  list-objects.c |   10 ++++++----\n>>>  1 files changed, 6 insertions(+), 4 deletions(-)\n>>\n>> Please explain why this is needed?\n>>\n>> I can see that process_blob() is called from process_tree() which passes\n>> the return value from lookup_blob(entry.sha1) directly without looking at\n>> it.  lookup_blob() can issue an error message and return NULL if there is\n>> a SHA-1 collision with an object that is not a blob.\n>\n> to avoid segmentation fault in that case, if \"blob\" is NULL, it dies\n> at blob->object anyway and the check \"if (!obj)\" is useless.\n\nWell, I didn't mean that you should explain whatever to _me_ as a\nresponse; I meant that the log message should explain that to future\nreaders. I thought you have been here long enough to know that ;-)\n\nAlso \"check NULL before dereferencing\" means \"avoid segmentation fault\",\nso that is not the primary thing that needs to be explained. The point is\nto explain why and how a NULL could come into the codepath in the first\nplace.\n\nPlease try again when the post-1.7.10 cycle opens.\n\nThanks.\n"}]}