@matheusbsbs / matheus-bagliano / commits / 7ce4088ab7

parse-options: consistently allocate memory in fix_filename()

When handling OPT_FILENAME(), we have to stick the "prefix" (if any) in front of the filename to make up for the fact that Git has chdir()'d to the top of the repository. We can do this with prefix_filename(), but there are a few special cases we handle ourselves. Unfortunately the memory allocation is inconsistent here; if we do make it to prefix_filename(), we'll allocate a string which the caller must free to avoid a leak. But if we hit our special cases, we'll return the string as-is, and a caller which tries to free it will crash. So there's no way to win. Let's consistently allocate, so that callers can do the right thing. There are now three cases to care about in the function (and hence a three-armed if/else): 1. we got a NULL input (and should leave it as NULL, though arguably this is the sign of a bug; let's keep the status quo for now and we can pick at that scab later) 2. we hit a special case that means we leave the name intact; we should duplicate the string. This includes our special "-" matching. Prior to this patch, it also included empty prefixes and absolute filenames. But we can observe that prefix_filename() already handles these, so we don't need to detect them. 3. everything else goes to prefix_filename() I've dropped the "const" from the "char **file" parameter to indicate that we're allocating, though in practice it's not really important. This is all being shuffled through a void pointer via opt->value before it hits code which ever looks at the string. And it's even a bit weird, because we are really taking _in_ a const string and using the same out-parameter for a non-const string. A better function signature would be: static char *fix_filename(const char *prefix, const char *file); but that would mean the caller dereferences the double-pointer (and the NULL check is currently handled inside this function). So I took the path of least-change here. Note that we have to fix several callers in this commit, too, or we'll break the leak-checking tests. These are "new" leaks in the sense that they are now triggered by the test suite, but these spots have always been leaky when Git is run in a subdirectory of the repository. I fixed all of the cases that trigger with GIT_TEST_PASSING_SANITIZE_LEAK. There may be others in scripts that have other leaks, but we can fix them later along with those other leaks (and again, you _couldn't_ fix them before this patch, so this is the necessary first step). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Mar 4, 2023 at 05:31 UTC 7ce4088ab78e06fc5c4ae42fc75b65a48bf7b3ff

6 files changed +20 -11

builtin/archive.c

+2 -1

@@ -81,7 +81,7 @@ static int run_remote_archiver(int argc, const char **argv,

81 int cmd_archive(int argc, const char **argv, const char *prefix)

82 {

83 const char *exec = "git-upload-archive";

84 - const char *output = NULL;

84 + char *output = NULL;

85 const char *remote = NULL;

86 struct option local_opts[] = {

87 OPT_FILENAME('o', "output", &output,

@@ -106,5 +106,6 @@ int cmd_archive(int argc, const char **argv, const char *prefix)

106

107 setvbuf(stderr, NULL, _IOLBF, BUFSIZ);

108

109 + UNLEAK(output);

110 return write_archive(argc, argv, prefix, the_repository, output, 0);

111 }

builtin/checkout.c

+2 -1

@@ -74,7 +74,7 @@ struct checkout_opts {

74 const char *ignore_unmerged_opt;

75 int ignore_unmerged;

76 int pathspec_file_nul;

77 - const char *pathspec_from_file;

77 + char *pathspec_from_file;

79 const char *new_branch;

80 const char *new_branch_force;

@@ -1872,6 +1872,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)

1872 options, checkout_usage, &new_branch_info);

1873 branch_info_release(&new_branch_info);

1874 clear_pathspec(&opts.pathspec);

1875 + free(opts.pathspec_from_file);

1876 FREE_AND_NULL(options);

1877 return ret;

1878 }

builtin/reset.c

+3 -1

@@ -316,7 +316,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)

316 int reset_type = NONE, update_ref_status = 0, quiet = 0;

317 int no_refresh = 0;

318 int patch_mode = 0, pathspec_file_nul = 0, unborn;

319 - const char *rev, *pathspec_from_file = NULL;

319 + const char *rev;

320 + char *pathspec_from_file = NULL;

321 struct object_id oid;

322 struct pathspec pathspec;

323 int intent_to_add = 0;

@@ -486,5 +487,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)

487 if (!pathspec.nr)

488 remove_branch_state(the_repository, 0);

489

490 + free(pathspec_from_file);

491 return update_ref_status;

492 }

builtin/tag.c

+3 -1

@@ -433,7 +433,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)

433 int create_reflog = 0;

434 int annotate = 0, force = 0;

435 int cmdmode = 0, create_tag_object = 0;

436 - const char *msgfile = NULL, *keyid = NULL;

436 + char *msgfile = NULL;

437 + const char *keyid = NULL;

438 struct msg_arg msg = { .buf = STRBUF_INIT };

439 struct ref_transaction *transaction;

440 struct strbuf err = STRBUF_INIT;

@@ -643,5 +644,6 @@ cleanup:

644 strbuf_release(&reflog_msg);

645 strbuf_release(&msg.buf);

646 strbuf_release(&err);

647 + free(msgfile);

648 return ret;

649 }

parse-options.c

+8 -6

@@ -59,12 +59,14 @@ static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,

59 return 0;

60 }

62 -static void fix_filename(const char *prefix, const char **file)

62 +static void fix_filename(const char *prefix, char **file)

63 {

64 - if (!file || !*file || !prefix || is_absolute_path(*file)

65 - || !strcmp("-", *file))

66 - return;

67 - *file = prefix_filename(prefix, *file);

64 + if (!file || !*file)

65 + ; /* leave as NULL */

66 + else if (!strcmp("-", *file))

67 + *file = xstrdup(*file);

68 + else

69 + *file = prefix_filename(prefix, *file);

70 }

72 static enum parse_opt_result opt_command_mode_error(

@@ -177,7 +179,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,

179 err = get_arg(p, opt, flags, (const char **)opt->value);

180

181 if (!err)

180 - fix_filename(p->prefix, (const char **)opt->value);

182 + fix_filename(p->prefix, (char **)opt->value);

183 return err;

184

185 case OPTION_CALLBACK:

t/helper/test-parse-pathspec-file.c

+2 -1

@@ -6,7 +6,7 @@

6 int cmd__parse_pathspec_file(int argc, const char **argv)

7 {

8 struct pathspec pathspec;

9 - const char *pathspec_from_file = NULL;

9 + char *pathspec_from_file = NULL;

10 int pathspec_file_nul = 0, i;

12 static const char *const usage[] = {

@@ -29,5 +29,6 @@ int cmd__parse_pathspec_file(int argc, const char **argv)

29 printf("%s\n", pathspec.items[i].original);

31 clear_pathspec(&pathspec);

32 + free(pathspec_from_file);

33 return 0;

34 }