prefix_filename: return newly allocated string

The prefix_filename() function returns a pointer to static storage, which makes it easy to use dangerously. We already fixed one buggy caller in hash-object recently, and the calls in apply.c are suspicious (I didn't dig in enough to confirm that there is a bug, but we call the function once in apply_all_patches() and then again indirectly from parse_chunk()). Let's make it harder to get wrong by allocating the return value. For simplicity, we'll do this even when the prefix is empty (and we could just return the original file pointer). That will cause us to allocate sometimes when we wouldn't otherwise need to, but this function isn't called in performance critical code-paths (and it already _might_ allocate on any given call, so a caller that cares about performance is questionable anyway). The downside is that the callers need to remember to free() the result to avoid leaking. Most of them already used xstrdup() on the result, so we know they are OK. The remainder have been converted to use free() as appropriate. I considered retaining a prefix_filename_unsafe() for cases where we know the static lifetime is OK (and handling the cleanup is awkward). This is only a handful of cases, though, and it's not worth the mental energy in worrying about whether the "unsafe" variant is OK to use in any situation. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Mar 20, 2017 at 21:28 UTC e4da43b1f063d227b5f7d2922d27458748763a2d
15 files changed +51 -39
abspath.c
+4 -6
@@ -246,20 +246,18 @@ char *absolute_pathdup(const char *path)
246 return strbuf_detach(&sb, NULL);
247 }
248
249 -const char *prefix_filename(const char *pfx, const char *arg)
249 +char *prefix_filename(const char *pfx, const char *arg)
250 {
251 - static struct strbuf path = STRBUF_INIT;
251 + struct strbuf path = STRBUF_INIT;
252 size_t pfx_len = pfx ? strlen(pfx) : 0;
253
254 #ifndef GIT_WINDOWS_NATIVE
255 if (!pfx_len || is_absolute_path(arg))
256 - return arg;
257 - strbuf_reset(&path);
256 + return xstrdup(arg);
257 strbuf_add(&path, pfx, pfx_len);
258 strbuf_addstr(&path, arg);
259 #else
260 /* don't add prefix to absolute paths, but still replace '\' by '/' */
262 - strbuf_reset(&path);
261 if (is_absolute_path(arg))
262 pfx_len = 0;
263 else if (pfx_len)
@@ -267,5 +265,5 @@ const char *prefix_filename(const char *pfx, const char *arg)
265 strbuf_addstr(&path, arg);
266 convert_slashes(path.buf + pfx_len);
267 #endif
270 - return path.buf;
268 + return strbuf_detach(&path, NULL);
269 }
apply.c
+6 -3
@@ -2046,7 +2046,7 @@ static void prefix_one(struct apply_state *state, char **name)
2046 char *old_name = *name;
2047 if (!old_name)
2048 return;
2049 - *name = xstrdup(prefix_filename(state->prefix, *name));
2049 + *name = prefix_filename(state->prefix, *name);
2050 free(old_name);
2051 }
2052
@@ -4805,6 +4805,7 @@ int apply_all_patches(struct apply_state *state,
4805
4806 for (i = 0; i < argc; i++) {
4807 const char *arg = argv[i];
4808 + char *to_free = NULL;
4809 int fd;
4810
4811 if (!strcmp(arg, "-")) {
@@ -4814,19 +4815,21 @@ int apply_all_patches(struct apply_state *state,
4815 errs |= res;
4816 read_stdin = 0;
4817 continue;
4817 - } else if (0 < state->prefix_length)
4818 - arg = prefix_filename(state->prefix, arg);
4818 + } else
4819 + arg = to_free = prefix_filename(state->prefix, arg);
4820
4821 fd = open(arg, O_RDONLY);
4822 if (fd < 0) {
4823 error(_("can't open patch '%s': %s"), arg, strerror(errno));
4824 res = -128;
4825 + free(to_free);
4826 goto end;
4827 }
4828 read_stdin = 0;
4829 set_default_whitespace_mode(state);
4830 res = apply_patch(state, fd, arg, options);
4831 close(fd);
4832 + free(to_free);
4833 if (res < 0)
4834 goto end;
4835 errs |= res;
builtin/config.c
+1 -2
@@ -527,8 +527,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
527 else if (given_config_source.file) {
528 if (!is_absolute_path(given_config_source.file) && prefix)
529 given_config_source.file =
530 - xstrdup(prefix_filename(prefix,
531 - given_config_source.file));
530 + prefix_filename(prefix, given_config_source.file);
531 }
532
533 if (respect_includes == -1)
builtin/hash-object.c
+1 -1
@@ -145,7 +145,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)
145 char *to_free = NULL;
146
147 if (prefix)
148 - arg = to_free = xstrdup(prefix_filename(prefix, arg));
148 + arg = to_free = prefix_filename(prefix, arg);
149 hash_object(arg, type, no_filters ? NULL : vpath ? vpath : arg,
150 flags, literally);
151 free(to_free);
builtin/log.c
+1 -1
@@ -1084,7 +1084,7 @@ static const char *set_outdir(const char *prefix, const char *output_directory)
1084 if (!output_directory)
1085 return prefix;
1086
1087 - return xstrdup(prefix_filename(prefix, output_directory));
1087 + return prefix_filename(prefix, output_directory);
1088 }
1089
1090 static const char * const builtin_format_patch_usage[] = {
builtin/mailinfo.c
+2 -9
@@ -11,13 +11,6 @@
11 static const char mailinfo_usage[] =
12 "git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding=<encoding> | -n] [--scissors | --no-scissors] <msg> <patch> < mail >info";
13
14 -static char *prefix_copy(const char *prefix, const char *filename)
15 -{
16 - if (!prefix || is_absolute_path(filename))
17 - return xstrdup(filename);
18 - return xstrdup(prefix_filename(prefix, filename));
19 -}
20 -
14 int cmd_mailinfo(int argc, const char **argv, const char *prefix)
15 {
16 const char *def_charset;
@@ -60,8 +53,8 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)
53 mi.input = stdin;
54 mi.output = stdout;
55
63 - msgfile = prefix_copy(prefix, argv[1]);
64 - patchfile = prefix_copy(prefix, argv[2]);
56 + msgfile = prefix_filename(prefix, argv[1]);
57 + patchfile = prefix_filename(prefix, argv[2]);
58
59 status = !!mailinfo(&mi, msgfile, patchfile);
60 clear_mailinfo(&mi);
builtin/merge-file.c
+11 -3
@@ -65,11 +65,18 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
65 }
66
67 for (i = 0; i < 3; i++) {
68 - const char *fname = prefix_filename(prefix, argv[i]);
68 + char *fname;
69 + int ret;
70 +
71 if (!names[i])
72 names[i] = argv[i];
71 - if (read_mmfile(mmfs + i, fname))
73 +
74 + fname = prefix_filename(prefix, argv[i]);
75 + ret = read_mmfile(mmfs + i, fname);
76 + free(fname);
77 + if (ret)
78 return -1;
79 +
80 if (mmfs[i].size > MAX_XDIFF_SIZE ||
81 buffer_is_binary(mmfs[i].ptr, mmfs[i].size))
82 return error("Cannot merge binary files: %s",
@@ -86,7 +93,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
93
94 if (ret >= 0) {
95 const char *filename = argv[0];
89 - const char *fpath = prefix_filename(prefix, argv[0]);
96 + char *fpath = prefix_filename(prefix, argv[0]);
97 FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
98
99 if (!f)
@@ -98,6 +105,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
105 else if (fclose(f))
106 ret = error_errno("Could not close %s", filename);
107 free(result.ptr);
108 + free(fpath);
109 }
110
111 if (ret > 127)
builtin/rev-parse.c
+3 -1
@@ -228,7 +228,9 @@ static int show_file(const char *arg, int output_prefix)
228 if ((filter & (DO_NONFLAGS|DO_NOREV)) == (DO_NONFLAGS|DO_NOREV)) {
229 if (output_prefix) {
230 const char *prefix = startup_info->prefix;
231 - show(prefix_filename(prefix, arg));
231 + char *fname = prefix_filename(prefix, arg);
232 + show(fname);
233 + free(fname);
234 } else
235 show(arg);
236 return 1;
builtin/worktree.c
+2 -1
@@ -318,7 +318,8 @@ static int add(int ac, const char **av, const char *prefix)
318 {
319 struct add_opts opts;
320 const char *new_branch_force = NULL;
321 - const char *path, *branch;
321 + char *path;
322 + const char *branch;
323 struct option options[] = {
324 OPT__FORCE(&opts.force, N_("checkout <branch> even if already checked out in other worktree")),
325 OPT_STRING('b', NULL, &opts.new_branch, N_("branch"),
cache.h
+3 -3
@@ -537,10 +537,10 @@ extern char *prefix_path_gently(const char *prefix, int len, int *remaining, con
537 * not have to interact with index entry; i.e. name of a random file
538 * on the filesystem.
539 *
540 - * The return value may point to static storage which will be overwritten by
541 - * further calls.
540 + * The return value is always a newly allocated string (even if the
541 + * prefix was empty).
542 */
543 -extern const char *prefix_filename(const char *prefix, const char *path);
543 +extern char *prefix_filename(const char *prefix, const char *path);
544
545 extern int check_filename(const char *prefix, const char *name);
546 extern void verify_filename(const char *prefix,
diff-no-index.c
+1 -1
@@ -266,7 +266,7 @@ void diff_no_index(struct rev_info *revs,
266 */
267 p = file_from_standard_input;
268 else if (prefix)
269 - p = xstrdup(prefix_filename(prefix, p));
269 + p = prefix_filename(prefix, p);
270 paths[i] = p;
271 }
272
diff.c
+3 -3
@@ -4023,8 +4023,7 @@ int diff_opt_parse(struct diff_options *options,
4023 else if (!strcmp(arg, "--pickaxe-regex"))
4024 options->pickaxe_opts |= DIFF_PICKAXE_REGEX;
4025 else if ((argcount = short_opt('O', av, &optarg))) {
4026 - const char *path = prefix_filename(prefix, optarg);
4027 - options->orderfile = xstrdup(path);
4026 + options->orderfile = prefix_filename(prefix, optarg);
4027 return argcount;
4028 }
4029 else if ((argcount = parse_long_opt("diff-filter", av, &optarg))) {
@@ -4071,13 +4070,14 @@ int diff_opt_parse(struct diff_options *options,
4070 else if (!strcmp(arg, "--no-function-context"))
4071 DIFF_OPT_CLR(options, FUNCCONTEXT);
4072 else if ((argcount = parse_long_opt("output", av, &optarg))) {
4074 - const char *path = prefix_filename(prefix, optarg);
4073 + char *path = prefix_filename(prefix, optarg);
4074 options->file = fopen(path, "w");
4075 if (!options->file)
4076 die_errno("Could not open '%s'", path);
4077 options->close_file = 1;
4078 if (options->use_color != GIT_COLOR_ALWAYS)
4079 options->use_color = GIT_COLOR_NEVER;
4080 + free(path);
4081 return argcount;
4082 } else
4083 return 0;
parse-options.c
+1 -1
@@ -40,7 +40,7 @@ static void fix_filename(const char *prefix, const char **file)
40 if (!file || !*file || !prefix || is_absolute_path(*file)
41 || !strcmp("-", *file))
42 return;
43 - *file = xstrdup(prefix_filename(prefix, *file));
43 + *file = prefix_filename(prefix, *file);
44 }
45
46 static int opt_command_mode_error(const struct option *opt,
setup.c
+8 -3
@@ -135,6 +135,7 @@ int path_inside_repo(const char *prefix, const char *path)
135 int check_filename(const char *prefix, const char *arg)
136 {
137 const char *name;
138 + char *to_free = NULL;
139 struct stat st;
140
141 if (starts_with(arg, ":/")) {
@@ -142,13 +143,17 @@ int check_filename(const char *prefix, const char *arg)
143 return 1;
144 name = arg + 2;
145 } else if (prefix)
145 - name = prefix_filename(prefix, arg);
146 + name = to_free = prefix_filename(prefix, arg);
147 else
148 name = arg;
148 - if (!lstat(name, &st))
149 + if (!lstat(name, &st)) {
150 + free(to_free);
151 return 1; /* file exists */
150 - if (errno == ENOENT || errno == ENOTDIR)
152 + }
153 + if (errno == ENOENT || errno == ENOTDIR) {
154 + free(to_free);
155 return 0; /* file does not exist */
156 + }
157 die_errno("failed to stat '%s'", arg);
158 }
159
worktree.c
+4 -1
@@ -250,16 +250,19 @@ struct worktree *find_worktree(struct worktree **list,
250 {
251 struct worktree *wt;
252 char *path;
253 + char *to_free = NULL;
254
255 if ((wt = find_worktree_by_suffix(list, arg)))
256 return wt;
257
257 - arg = prefix_filename(prefix, arg);
258 + if (prefix)
259 + arg = to_free = prefix_filename(prefix, arg);
260 path = real_pathdup(arg, 1);
261 for (; *list; list++)
262 if (!fspathcmp(path, real_path((*list)->path)))
263 break;
264 free(path);
265 + free(to_free);
266 return *list;
267 }
268