{"thread":{"id":"47845","subject":"[PATCH] CodeAI fixes 1 Allocator sizeof() operand mismatch, 2 Null Pointer Dereference, and 2 Dead Code","startedAt":"2018-02-14T15:50:18Z","lastAt":"2018-02-14T16:46:36Z","messageCount":2,"participants":["Code AI","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"339287","messageId":"CAGm8dMApDdLEzeKU-h16G0NSpnuk9LMTWA29t4MxO1qcNpUvhA@mail.gmail.com","threadId":"47845","inReplyTo":null,"subject":"[PATCH] CodeAI fixes 1 Allocator sizeof() operand mismatch, 2 Null Pointer Dereference, and 2 Dead Code","fromName":"Code AI","fromEmail":"techsupport@mycode.ai","sentAt":"2018-02-14T15:50:12Z","receivedAt":"2018-02-14T15:50:18Z","isPatch":true,"sender":{"key":"techsupport@mycode.ai","avatar":null},"body":"Hi my name is Benjamin Bales.\n\nI am the founder and creator of CodeAI,\nthe first non-human contributor to your software project. CodeAI finds\nand fixes security defects for you. It fixed 18. It wants to merge 5\ncommits - 1 Allocator sizeof() operand mismatch, 2 Null Pointer\nDereference issues and 2 Dead Code issues in git. To view all 18 fixed\nissues from the run claim your free open source account at mycode.ai\nand the Dockerfile used to build and run your project in CodeAI, here-\nhttps://drive.google.com/open?id=12d2poeHabdc0DSShDcekSU5bI0Il6Qv- .\nIt is always free for open source projects.\n\nIf you have any questions about these results or have general\ninquiries about CodeAI, please send an email to techsupport@mycode.ai\n\nAllocator sizeof() mismatch:\ndiff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\nindex 1145d51..c3ea5c1 100644\n--- a/t/helper/test-hashmap.c\n+++ b/t/helper/test-hashmap.c\n@@ -86,7 +86,7 @@ static void perf_hashmap(unsigned int method,\nunsigned int rounds)\n        unsigned int i, j;\n\n        entries = malloc(TEST_SIZE * sizeof(struct test_entry *));\n-       hashes = malloc(TEST_SIZE * sizeof(int));\n+       hashes = malloc(TEST_SIZE * sizeof(unsigned));\n        for (i = 0; i < TEST_SIZE; i++) {\n                snprintf(buf, sizeof(buf), \"%i\", i);\n                entries[i] = alloc_test_entry(0, buf, strlen(buf), \"\", 0);\n\n\nNull dereference fixes:\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 4c51aec..f26858a 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1604,7 +1604,7 @@ static void show_pack_info(int stat_only)\n                             \"non delta: %d objects\",\n                             baseobjects),\n                          baseobjects);\n-       for (i = 0; i < deepest_delta; i++) {\n+       for (i = 0; chain_histogram && (i < deepest_delta); i++) {\n                if (!chain_histogram[i])\n                        continue;\n                printf_ln(Q_(\"chain length = %d: %lu object\",\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 96c3327..fcd9332 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1721,7 +1721,7 @@ static int verify_absent(const struct cache_entry *ce,\n                         enum unpack_trees_error_types error_type,\n                         struct unpack_trees_options *o)\n {\n-       if (!o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE))\n+       if (ce && (!o->skip_sparse_checkout && (ce->ce_flags &\nCE_NEW_SKIP_WORKTREE)))\n                return 0;\n        return verify_absent_1(ce, error_type, o);\n }\n\n\nDead code fixes:\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -235,7 +235,6 @@ static int edit_patch(int argc, const char **argv,\nconst char *prefix)\n        init_revisions(&rev, prefix);\n        rev.diffopt.context = 7;\n\n-       argc = setup_revisions(argc, argv, &rev, NULL);\n        rev.diffopt.output_format = DIFF_FORMAT_PATCH;\n        rev.diffopt.use_color = 0;\n        rev.diffopt.flags.ignore_dirty_submodules = 1;\n\ndiff --git a/fsck.c b/fsck.c\nindex 032699e..78563c3 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -704,7 +704,6 @@ static int fsck_ident(const char **ident, struct\nobject *obj, struct fsck_option\n            !isdigit(p[4]) ||\n            (p[5] != '\\n'))\n                return report(options, obj, FSCK_MSG_BAD_TIMEZONE,\n\"invalid author/committer line - bad time zone\");\n-       p += 6;\n        return 0;\n }\n\n-- \nSincerely,\n\nCodeAI Tech Support Team\n"},{"id":"339288","messageId":"20180214164628.GA907@sigill.intra.peff.net","threadId":"47845","inReplyTo":"CAGm8dMApDdLEzeKU-h16G0NSpnuk9LMTWA29t4MxO1qcNpUvhA@mail.gmail.com","subject":"Re: [PATCH] CodeAI fixes 1 Allocator sizeof() operand mismatch, 2 Null Pointer Dereference, and 2 Dead Code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-14T16:46:29Z","receivedAt":"2018-02-14T16:46:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 14, 2018 at 10:50:12AM -0500, Code AI wrote:\n\n> Hi my name is Benjamin Bales.\n> \n> I am the founder and creator of CodeAI,\n> the first non-human contributor to your software project. CodeAI finds\n> and fixes security defects for you. It fixed 18. It wants to merge 5\n> commits - 1 Allocator sizeof() operand mismatch, 2 Null Pointer\n> Dereference issues and 2 Dead Code issues in git. To view all 18 fixed\n> issues from the run claim your free open source account at mycode.ai\n> and the Dockerfile used to build and run your project in CodeAI, here-\n> https://drive.google.com/open?id=12d2poeHabdc0DSShDcekSU5bI0Il6Qv- .\n> It is always free for open source projects.\n> \n> If you have any questions about these results or have general\n> inquiries about CodeAI, please send an email to techsupport@mycode.ai\n\nToo bad the AI cannot follow SubmittingPatches. :)\n\nWe've often seen the results of static analyzers on the list. In general\nwe welcome fixes from static analyzers, and even fixes to silence false\npositives from static analyzers if they're not too onerous (and if they\nget the analyzer to a point where it generates only useful results).\n\nBut we prefer to see some analysis done on the call-sites to determine\nif they are actual problems, and if the fix is appropriate.\n\nLet's look at these ones.\n\n> Allocator sizeof() mismatch:\n> diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\n> index 1145d51..c3ea5c1 100644\n> --- a/t/helper/test-hashmap.c\n> +++ b/t/helper/test-hashmap.c\n> @@ -86,7 +86,7 @@ static void perf_hashmap(unsigned int method,\n> unsigned int rounds)\n>         unsigned int i, j;\n> \n>         entries = malloc(TEST_SIZE * sizeof(struct test_entry *));\n> -       hashes = malloc(TEST_SIZE * sizeof(int));\n> +       hashes = malloc(TEST_SIZE * sizeof(unsigned));\n\nI agree this ought to be \"unsigned\", though I don't know if there is any\nplatform in practice where the sizes of \"int\" and \"unsigned int\" differ.\nHowever, an even better solution is \"sizeof(*hashes)\", which eliminates\nthe need to keep it in sync. Or even ALLOC_ARRAY(hashes), which does\nthis for us.\n\n> Null dereference fixes:\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 4c51aec..f26858a 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -1604,7 +1604,7 @@ static void show_pack_info(int stat_only)\n>                              \"non delta: %d objects\",\n>                              baseobjects),\n>                           baseobjects);\n> -       for (i = 0; i < deepest_delta; i++) {\n> +       for (i = 0; chain_histogram && (i < deepest_delta); i++) {\n>                 if (!chain_histogram[i])\n>                         continue;\n>                 printf_ln(Q_(\"chain length = %d: %lu object\",\n\nThis one looks like a false positive. At the beginning of the function,\nwe allocate chain_histogram if deepest_delta is non-zero. And if it's\nzero, we'll never enter this loop.\n\nCuriously, the tool did not flag the reference to chain_histogram in the\nearlier loop. Which is also correct, but in a much less obvious way. It\ndoes:\n\n  if (is_delta_type(obj->type))\n          chain_histogram[obj_stat[i].delta_depth - 1]++;\n\nSo there the assumption is that if we saw any delta types, we would\npreviously have incremented deepest_delta to be non-zero. Which I think\nholds, but seems way less likely for a static analysis tool to have\nrealized.\n\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 96c3327..fcd9332 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1721,7 +1721,7 @@ static int verify_absent(const struct cache_entry *ce,\n>                          enum unpack_trees_error_types error_type,\n>                          struct unpack_trees_options *o)\n>  {\n> -       if (!o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE))\n> +       if (ce && (!o->skip_sparse_checkout && (ce->ce_flags &\n> CE_NEW_SKIP_WORKTREE)))\n>                 return 0;\n>         return verify_absent_1(ce, error_type, o);\n>  }\n\nThis one is hard to evaluate. It seems to suggest that somebody could\npass a NULL ce to verify_absent(). But without knowing how the tool came\nto that conclusion, it's hard to know if that's true of any callsites\n(though just grepping the callers, most seem to otherwise dereference\n\"ce\").\n\nIf there is such a callsite, though, this patch isn't sufficient. We'd\npass the NULL down to verify_absent_1(), which may dereference it, too\nThough it returns early in some cases, so it's _possible_ that the one\ncode path that passes a NULL never sets those flags (again, hard to tell\nwithout the tool reporting which execution path it found with the NULL).\nI'd argue that it's still the wrong fix, though, as the result would be\nvery brittle.\n\n> Dead code fixes:\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -235,7 +235,6 @@ static int edit_patch(int argc, const char **argv,\n> const char *prefix)\n>         init_revisions(&rev, prefix);\n>         rev.diffopt.context = 7;\n> \n> -       argc = setup_revisions(argc, argv, &rev, NULL);\n>         rev.diffopt.output_format = DIFF_FORMAT_PATCH;\n>         rev.diffopt.use_color = 0;\n>         rev.diffopt.flags.ignore_dirty_submodules = 1;\n\nThis is a classic with a twist.  The dead assignments to argc are common\nto many static analysis tools, but usually they suggest:\n\n   -       argc = setup_revisions(argc, argv, &rev, NULL);\n   +       setup_revisions(argc, argv, &rev, NULL);\n\nAs I've argued before for these cases, the assignment is important to\nretain from a maintenance perspective. Since otherwise you leave \"argc\"\nwith a bogus value, which is a trap for somebody who later adds code to\nlook at it (which may not even be near this setup_revisions call, but\nmuch later in the function).\n\nThe twist here is that the tool suggests removing the setup_revision()\ncall entirely. Which seems like it cannot possibly be correct. It's\nsetting up values in \"rev\" based on the incoming arguments.\n\n> diff --git a/fsck.c b/fsck.c\n> index 032699e..78563c3 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -704,7 +704,6 @@ static int fsck_ident(const char **ident, struct\n> object *obj, struct fsck_option\n>             !isdigit(p[4]) ||\n>             (p[5] != '\\n'))\n>                 return report(options, obj, FSCK_MSG_BAD_TIMEZONE,\n> \"invalid author/committer line - bad time zone\");\n> -       p += 6;\n>         return 0;\n>  }\n\nAnother classic. And similar to the case above, I think this should\nremain for maintainability. Even though we do not currently parse\nfurther into the buffer, it would be a surprise for anybody adding code\nafter this is \"p\" had not been updated.\n\nThough I could accept that we're very unlikely to add code here, and\nthat it would go _directly_ after the missing increment of \"p\", so the\nwriter of the new code is likely to notice.\n\n-Peff\n"}]}