{"thread":{"id":"21959","subject":"potential null dereference","startedAt":"2009-12-15T12:41:01Z","lastAt":"2009-12-17T12:30:55Z","messageCount":2,"participants":["Jiri Slaby","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"129945","messageId":"4B2783DD.5060301@gmail.com","threadId":"21959","inReplyTo":null,"subject":"potential null dereference","fromName":"Jiri Slaby","fromEmail":"jirislaby@gmail.com","sentAt":"2009-12-15T12:41:01Z","receivedAt":"2009-12-15T12:41:01Z","isPatch":false,"sender":{"key":"jirislaby@gmail.com","avatar":null},"body":"Hi,\n\nStanse found the following error in unpack-trees.c:\ndereferencing NULL pointer here.[. * o src_index]\n\nint unpack_trees(unsigned len, struct tree_desc *t, struct\nunpack_trees_options *o)\n{\n int ret;\n static struct cache_entry *dfc;\n...\n if (o->src_index) {                   <-- loc0\n  o->result.timestamp.sec = o->src_index->timestamp.sec;\n  o->result.timestamp.nsec = o->src_index->timestamp.nsec;\n }\n o->merge_size = len;\n\n if (!dfc)\n  dfc = xcalloc(1, ((1 + (0) + 8) & ~7));\n o->df_conflict_entry = dfc;\n\n if (len) {\n...\n }\n\n if (o->merge) {\n  while (o->pos < o->src_index->cache_nr) { <-- here\n\nIt triggers, because there is a test for o->src_index being NULL at\nloc0, but here, it is dereferenced without a check. Can this happen\n(e.g. does o->merge != NULL imply o->src_index != NULL)?\n\n\n\n\n\n\nFurther, there is a warning in log-tree.c:\npointer always points to valid memory here, but checking for not\nNULL.[parents]\n\nstatic int log_tree_diff(struct rev_info *opt, struct commit *commit,\nstruct log_info *log)\n{\n int showed_log;\n struct commit_list *parents;\n unsigned const char *sha1 = commit->object.sha1;\n\n if (!opt->diff && !((&opt->diffopt)->flags & (1 << 14)))\n  return 0;\n\n\n parents = commit->parents;\n if (!parents) {            <-- loc0\n  if (opt->show_root_diff) {\n   diff_root_tree_sha1(sha1, \"\", &opt->diffopt);\n   log_tree_diff_flush(opt);\n  }\n  return !opt->loginfo;     <-- loc1\n }\n\n if (parents && parents->next) { <-- here\n\nI.e. if parents was NULL at loc0, we escaped at loc1. But we check\nparents against NULL here again.\n\nthanks,\n-- \njs\n"},{"id":"130028","messageId":"4B2A247F.4070705@lsrfire.ath.cx","threadId":"21959","inReplyTo":"4B2783DD.5060301@gmail.com","subject":"Re: potential null dereference","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-12-17T12:30:55Z","receivedAt":"2009-12-17T12:30:55Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.12.2009 13:41, schrieb Jiri Slaby:\n> Hi,\n> \n> Stanse found the following error in unpack-trees.c:\n> dereferencing NULL pointer here.[. * o src_index]\n> \n> int unpack_trees(unsigned len, struct tree_desc *t, struct\n> unpack_trees_options *o)\n> {\n>  int ret;\n>  static struct cache_entry *dfc;\n> ...\n>  if (o->src_index) {                   <-- loc0\n>   o->result.timestamp.sec = o->src_index->timestamp.sec;\n>   o->result.timestamp.nsec = o->src_index->timestamp.nsec;\n>  }\n>  o->merge_size = len;\n> \n>  if (!dfc)\n>   dfc = xcalloc(1, ((1 + (0) + 8) & ~7));\n>  o->df_conflict_entry = dfc;\n> \n>  if (len) {\n> ...\n>  }\n> \n>  if (o->merge) {\n>   while (o->pos < o->src_index->cache_nr) { <-- here\n> \n> It triggers, because there is a test for o->src_index being NULL at\n> loc0, but here, it is dereferenced without a check. Can this happen\n> (e.g. does o->merge != NULL imply o->src_index != NULL)?\n\nRunning \"git grep -w -B70 unpack_trees\" and looking for \"src_index\"\nusing less' search command showed me that src_index is never NULL when\nunpack_trees() is called.\n\n> Further, there is a warning in log-tree.c:\n> pointer always points to valid memory here, but checking for not\n> NULL.[parents]\n> \n> static int log_tree_diff(struct rev_info *opt, struct commit *commit,\n> struct log_info *log)\n> {\n>  int showed_log;\n>  struct commit_list *parents;\n>  unsigned const char *sha1 = commit->object.sha1;\n> \n>  if (!opt->diff && !((&opt->diffopt)->flags & (1 << 14)))\n>   return 0;\n> \n> \n>  parents = commit->parents;\n>  if (!parents) {            <-- loc0\n>   if (opt->show_root_diff) {\n>    diff_root_tree_sha1(sha1, \"\", &opt->diffopt);\n>    log_tree_diff_flush(opt);\n>   }\n>   return !opt->loginfo;     <-- loc1\n>  }\n> \n>  if (parents && parents->next) { <-- here\n> \n> I.e. if parents was NULL at loc0, we escaped at loc1. But we check\n> parents against NULL here again.\n\nThe check may be duplicate, but I suspect removing it won't change the\nresulting object code -- the compiler should be smart enough to come to\nthe same conclusion.\n\nThanks,\nRené\n"}]}