potential null dereference

potential null dereference

[Jiri Slaby]

Hi,

Stanse found the following error in unpack-trees.c:
dereferencing NULL pointer here.[. * o src_index]

int unpack_trees(unsigned len, struct tree_desc *t, struct
unpack_trees_options *o)
{
 int ret;
 static struct cache_entry *dfc;
...
 if (o->src_index) {                   <-- loc0
  o->result.timestamp.sec = o->src_index->timestamp.sec;
  o->result.timestamp.nsec = o->src_index->timestamp.nsec;
 }
 o->merge_size = len;

 if (!dfc)
  dfc = xcalloc(1, ((1 + (0) + 8) & ~7));
 o->df_conflict_entry = dfc;

 if (len) {
...
 }

 if (o->merge) {
  while (o->pos < o->src_index->cache_nr) { <-- here

It triggers, because there is a test for o->src_index being NULL at
loc0, but here, it is dereferenced without a check. Can this happen
(e.g. does o->merge != NULL imply o->src_index != NULL)?






Further, there is a warning in log-tree.c:
pointer always points to valid memory here, but checking for not
NULL.[parents]

static int log_tree_diff(struct rev_info *opt, struct commit *commit,
struct log_info *log)
{
 int showed_log;
 struct commit_list *parents;
 unsigned const char *sha1 = commit->object.sha1;

 if (!opt->diff && !((&opt->diffopt)->flags & (1 << 14)))
  return 0;


 parents = commit->parents;
 if (!parents) {            <-- loc0
  if (opt->show_root_diff) {
   diff_root_tree_sha1(sha1, "", &opt->diffopt);
   log_tree_diff_flush(opt);
  }
  return !opt->loginfo;     <-- loc1
 }

 if (parents && parents->next) { <-- here

I.e. if parents was NULL at loc0, we escaped at loc1. But we check
parents against NULL here again.

thanks,
-- 
js

Re: potential null dereference

[René Scharfe]

Am 15.12.2009 13:41, schrieb Jiri Slaby:
> Hi,
> 
> Stanse found the following error in unpack-trees.c:
> dereferencing NULL pointer here.[. * o src_index]
> 
> int unpack_trees(unsigned len, struct tree_desc *t, struct
> unpack_trees_options *o)
> {
>  int ret;
>  static struct cache_entry *dfc;
> ...
>  if (o->src_index) {                   <-- loc0
>   o->result.timestamp.sec = o->src_index->timestamp.sec;
>   o->result.timestamp.nsec = o->src_index->timestamp.nsec;
>  }
>  o->merge_size = len;
> 
>  if (!dfc)
>   dfc = xcalloc(1, ((1 + (0) + 8) & ~7));
>  o->df_conflict_entry = dfc;
> 
>  if (len) {
> ...
>  }
> 
>  if (o->merge) {
>   while (o->pos < o->src_index->cache_nr) { <-- here
> 
> It triggers, because there is a test for o->src_index being NULL at
> loc0, but here, it is dereferenced without a check. Can this happen
> (e.g. does o->merge != NULL imply o->src_index != NULL)?

Running "git grep -w -B70 unpack_trees" and looking for "src_index"
using less' search command showed me that src_index is never NULL when
unpack_trees() is called.

> Further, there is a warning in log-tree.c:
> pointer always points to valid memory here, but checking for not
> NULL.[parents]
> 
> static int log_tree_diff(struct rev_info *opt, struct commit *commit,
> struct log_info *log)
> {
>  int showed_log;
>  struct commit_list *parents;
>  unsigned const char *sha1 = commit->object.sha1;
> 
>  if (!opt->diff && !((&opt->diffopt)->flags & (1 << 14)))
>   return 0;
> 
> 
>  parents = commit->parents;
>  if (!parents) {            <-- loc0
>   if (opt->show_root_diff) {
>    diff_root_tree_sha1(sha1, "", &opt->diffopt);
>    log_tree_diff_flush(opt);
>   }
>   return !opt->loginfo;     <-- loc1
>  }
> 
>  if (parents && parents->next) { <-- here
> 
> I.e. if parents was NULL at loc0, we escaped at loc1. But we check
> parents against NULL here again.

The check may be duplicate, but I suspect removing it won't change the
resulting object code -- the compiler should be smart enough to come to
the same conclusion.

Thanks,
René