Skip to content

Commit

Permalink
prefer git_pathdup to git_path in some possibly-dangerous cases
Browse files Browse the repository at this point in the history
Because git_path uses a static buffer that is shared with
calls to git_path, mkpath, etc, it can be dangerous to
assign the result to a variable or pass it to a non-trivial
function. The value may change unexpectedly due to other
calls.

None of the cases changed here has a known bug, but they're
worth converting away from git_path because:

  1. It's easy to use git_pathdup in these cases.

  2. They use constructs (like assignment) that make it
     hard to tell whether they're safe or not.

The extra malloc overhead should be trivial, as an
allocation should be an order of magnitude cheaper than a
system call (which we are clearly about to make, since we
are constructing a filename). The real cost is that we must
remember to free the result.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
Jeff King authored and Junio C Hamano committed Aug 10, 2015
1 parent 77b9b1d commit fcd12db
Show file tree
Hide file tree
Showing 6 changed files with 21 additions and 11 deletions.
4 changes: 3 additions & 1 deletion builtin/fsck.c
Original file line number Diff line number Diff line change
Expand Up @@ -243,13 +243,14 @@ static void check_unreachable_object(struct object *obj)
printf("dangling %s %s\n", typename(obj->type),
sha1_to_hex(obj->sha1));
if (write_lost_and_found) {
const char *filename = git_path("lost-found/%s/%s",
char *filename = git_pathdup("lost-found/%s/%s",
obj->type == OBJ_COMMIT ? "commit" : "other",
sha1_to_hex(obj->sha1));
FILE *f;

if (safe_create_leading_directories_const(filename)) {
error("Could not create lost-found");
free(filename);
return;
}
if (!(f = fopen(filename, "w")))
Expand All @@ -262,6 +263,7 @@ static void check_unreachable_object(struct object *obj)
if (fclose(f))
die_errno("Could not finish '%s'",
filename);
free(filename);
}
return;
}
Expand Down
4 changes: 3 additions & 1 deletion fast-import.c
Original file line number Diff line number Diff line change
Expand Up @@ -407,14 +407,15 @@ static void dump_marks_helper(FILE *, uintmax_t, struct mark_set *);

static void write_crash_report(const char *err)
{
const char *loc = git_path("fast_import_crash_%"PRIuMAX, (uintmax_t) getpid());
char *loc = git_pathdup("fast_import_crash_%"PRIuMAX, (uintmax_t) getpid());
FILE *rpt = fopen(loc, "w");
struct branch *b;
unsigned long lu;
struct recent_command *rc;

if (!rpt) {
error("can't write crash report %s: %s", loc, strerror(errno));
free(loc);
return;
}

Expand Down Expand Up @@ -488,6 +489,7 @@ static void write_crash_report(const char *err)
fputs("-------------------\n", rpt);
fputs("END OF CRASH REPORT\n", rpt);
fclose(rpt);
free(loc);
}

static void end_packfile(void);
Expand Down
3 changes: 2 additions & 1 deletion http-backend.c
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)

static void send_local_file(const char *the_type, const char *name)
{
const char *p = git_path("%s", name);
char *p = git_pathdup("%s", name);
size_t buf_alloc = 8192;
char *buf = xmalloc(buf_alloc);
int fd;
Expand All @@ -191,6 +191,7 @@ static void send_local_file(const char *the_type, const char *name)
}
close(fd);
free(buf);
free(p);
}

static void get_text_file(char *name)
Expand Down
3 changes: 2 additions & 1 deletion notes-merge.c
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ static void write_buf_to_worktree(const unsigned char *obj,
const char *buf, unsigned long size)
{
int fd;
const char *path = git_path(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj));
char *path = git_pathdup(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj));
if (safe_create_leading_directories_const(path))
die_errno("unable to create directory for '%s'", path);
if (file_exists(path))
Expand All @@ -320,6 +320,7 @@ static void write_buf_to_worktree(const unsigned char *obj,
}

close(fd);
free(path);
}

static void write_note_to_worktree(const unsigned char *obj,
Expand Down
14 changes: 8 additions & 6 deletions refs.c
Original file line number Diff line number Diff line change
Expand Up @@ -1288,12 +1288,12 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
*/
static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)
{
const char *packed_refs_file;
char *packed_refs_file;

if (*refs->name)
packed_refs_file = git_path_submodule(refs->name, "packed-refs");
packed_refs_file = git_pathdup_submodule(refs->name, "packed-refs");
else
packed_refs_file = git_path("packed-refs");
packed_refs_file = git_pathdup("packed-refs");

if (refs->packed &&
!stat_validity_check(&refs->packed->validity, packed_refs_file))
Expand All @@ -1312,6 +1312,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)
fclose(f);
}
}
free(packed_refs_file);
return refs->packed;
}

Expand Down Expand Up @@ -1481,14 +1482,15 @@ static int resolve_gitlink_ref_recursive(struct ref_cache *refs,
{
int fd, len;
char buffer[128], *p;
const char *path;
char *path;

if (recursion > MAXDEPTH || strlen(refname) > MAXREFLEN)
return -1;
path = *refs->name
? git_path_submodule(refs->name, "%s", refname)
: git_path("%s", refname);
? git_pathdup_submodule(refs->name, "%s", refname)
: git_pathdup("%s", refname);
fd = open(path, O_RDONLY);
free(path);
if (fd < 0)
return resolve_gitlink_packed_ref(refs, refname, sha1);

Expand Down
4 changes: 3 additions & 1 deletion unpack-trees.c
Original file line number Diff line number Diff line change
Expand Up @@ -1029,10 +1029,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
if (!core_apply_sparse_checkout || !o->update)
o->skip_sparse_checkout = 1;
if (!o->skip_sparse_checkout) {
if (add_excludes_from_file_to_list(git_path("info/sparse-checkout"), "", 0, &el, 0) < 0)
char *sparse = git_pathdup("info/sparse-checkout");
if (add_excludes_from_file_to_list(sparse, "", 0, &el, 0) < 0)
o->skip_sparse_checkout = 1;
else
o->el = &el;
free(sparse);
}

memset(&o->result, 0, sizeof(o->result));
Expand Down

0 comments on commit fcd12db

Please sign in to comment.