# [PATCH] list-objects: perform NULL check before the variable is dereferenced 5 messages from 2012-04-09 to 2012-04-10. Participants: Nguyễn Thái Ngọc Duy, Junio C Hamano, Nguyen Thai Ngoc Duy. Thread: https://gitlist.dev/t/30187 ## Nguyễn Thái Ngọc Duy, 2012-04-09 10:45 Subject: [PATCH] list-objects: perform NULL check before the variable is dereferenced Message-ID: <1333968311-29889-1-git-send-email-pclouds@gmail.com> URL: https://gitlist.dev/e/1333968311-29889-1-git-send-email-pclouds%40gmail.com ``` Signed-off-by: Nguyễn Thái Ngọc Duy --- 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 ``` ## Junio C Hamano, 2012-04-09 18:51 Subject: Re: [PATCH] list-objects: perform NULL check before the variable is dereferenced Message-ID: <7v7gxowxfh.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v7gxowxfh.fsf%40alter.siamese.dyndns.org In-Reply-To: <1333968311-29889-1-git-send-email-pclouds@gmail.com> ``` Nguyễn Thái Ngọc Duy writes: > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > 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) ``` ## Nguyen Thai Ngoc Duy, 2012-04-09 23:47 Subject: Re: [PATCH] list-objects: perform NULL check before the variable is dereferenced Message-ID: URL: https://gitlist.dev/e/CACsJy8ASo8j%2BhmJ7Kw2uLcD9cS8SXc6yOoc5GDUcFHYk9W9a_g%40mail.gmail.com In-Reply-To: <7v7gxowxfh.fsf@alter.siamese.dyndns.org> ``` 2012/4/10 Junio C Hamano : > Nguyễn Thái Ngọc Duy   writes: > >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- >>  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 ``` ## Junio C Hamano, 2012-04-10 00:09 Subject: Re: [PATCH] list-objects: perform NULL check before the variable is dereferenced Message-ID: <7vbon0tplt.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vbon0tplt.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Nguyen Thai Ngoc Duy writes: > 2012/4/10 Junio C Hamano : >> Nguyễn Thái Ngọc Duy   writes: >> >>> Signed-off-by: Nguyễn Thái Ngọc Duy >>> --- >>>  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. ``` ## Junio C Hamano, 2012-04-10 00:09 Subject: Re: [PATCH] list-objects: perform NULL check before the variable is dereferenced Message-ID: <7vaa2ktplj.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vaa2ktplj.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Nguyen Thai Ngoc Duy writes: > 2012/4/10 Junio C Hamano : >> Nguyễn Thái Ngọc Duy   writes: >> >>> Signed-off-by: Nguyễn Thái Ngọc Duy >>> --- >>>  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. ```