Continue the work in commit: 3d7747e318532a36a263c61cdf92f2decb6424ff
to remove unsafe shared buffer usage in read_gitfile_gently.
Migrate read_gitfile_gently and resolve_gitfile_gently to take strbuf
out params to allocate their return values into, rather than returning
a view into a shared buffer.
Leave the shared buffer in case a caller passes null for this param (for
cases we haven't cleaned up yet).
Migrate callers of resolve_gitfile to resolve_gitfile_gently.
Signed-off-by: Atneya Nair <atneya@xxxxxxxxxx>
---
Notes:
Open questions:
- Is checking the return value of read_gitfile necessary if on error,
we are supposed to die, or set the error field to a non-zero value?
- Should we clean up the other call-sites of read_gitfile?
builtin/init-db.c | 7 ++++---
builtin/rev-parse.c | 4 +++-
repository.c | 9 +++++----
setup.c | 36 +++++++++++++++++++++++++-----------
setup.h | 7 +++----
submodule.c | 32 +++++++++++++++++++++++---------
worktree.c | 27 +++++++++++++--------------
7 files changed, 76 insertions(+), 46 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 0170469b84..9135d07a0d 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -198,11 +198,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
*/
if (real_git_dir) {
int err;
- const char *p;
struct strbuf sb = STRBUF_INIT;
+ struct strbuf gitfile = STRBUF_INIT;
- p = read_gitfile_gently(git_dir, &err);
- if (p && get_common_dir(&sb, p)) {
+ read_gitfile_gently(git_dir, &err, &gitfile);
+ if (!err && get_common_dir(&sb, gitfile.buf)) {
struct strbuf mainwt = STRBUF_INIT;
strbuf_addbuf(&mainwt, &sb);
@@ -213,6 +213,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
git_dir = strbuf_detach(&sb, NULL);
}
strbuf_release(&sb);
+ strbuf_release(&gitfile);
}
if (is_bare_repository_cfg < 0)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index d08987646a..e1db6b3231 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -728,12 +728,14 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
}
if (!strcmp(arg, "--resolve-git-dir")) {
const char *gitdir = argv[++i];
+ struct strbuf resolved_gitdir = STRBUF_INIT;
if (!gitdir)
die(_("--resolve-git-dir requires an argument"));
- gitdir = resolve_gitdir(gitdir);
+ gitdir = resolve_gitdir_gently(gitdir, NULL, &resolved_gitdir);
if (!gitdir)
die(_("not a gitdir '%s'"), argv[i]);
puts(gitdir);
+ strbuf_release(&resolved_gitdir);
continue;
}
}
diff --git a/repository.c b/repository.c
index 7aacb51b65..3ca6dbcf16 100644
--- a/repository.c
+++ b/repository.c
@@ -118,7 +118,7 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)
int ret = 0;
int error = 0;
char *abspath = NULL;
- const char *resolved_gitdir;
+ struct strbuf resolved_gitdir = STRBUF_INIT;
struct set_gitdir_args args = { NULL };
abspath = real_pathdup(gitdir, 0);
@@ -128,15 +128,16 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)
}
/* 'gitdir' must reference the gitdir directly */
- resolved_gitdir = resolve_gitdir_gently(abspath, &error);
- if (!resolved_gitdir) {
+ resolve_gitdir_gently(abspath, &error, &resolved_gitdir);
+ if (error) {
ret = -1;
goto out;
}
- repo_set_gitdir(repo, resolved_gitdir, &args);
+ repo_set_gitdir(repo, resolved_gitdir.buf, &args);
out:
+ strbuf_release(&resolved_gitdir);
free(abspath);
return ret;
}
diff --git a/setup.c b/setup.c
index b69b1cbc2a..2e118cf216 100644
--- a/setup.c
+++ b/setup.c
@@ -397,14 +397,17 @@ int is_nonbare_repository_dir(struct strbuf *path)
int ret = 0;
int gitfile_error;
size_t orig_path_len = path->len;
+ struct strbuf gitfile = STRBUF_INIT;
+
assert(orig_path_len != 0);
strbuf_complete(path, '/');
strbuf_addstr(path, ".git");
- if (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf))
+ if (read_gitfile_gently(path->buf, &gitfile_error, &gitfile) || is_git_directory(path->buf))
ret = 1;
if (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||
gitfile_error == READ_GITFILE_ERR_READ_FAILED)
ret = 1;
+ strbuf_release(&gitfile);
strbuf_setlen(path, orig_path_len);
return ret;
}
@@ -830,7 +833,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
/*
* Try to read the location of the git directory from the .git file,
- * return path to git directory if found. The return value comes from
+ * return path to git directory if found. If passed a valid strbuf, the return
+ * value is is a ptr to within the buffer. If strbuf is null, the return value comes from
* a shared buffer.
*
* On failure, if return_error_code is not NULL, return_error_code
@@ -838,7 +842,7 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
* return_error_code is NULL the function will die instead (for most
* cases).
*/
-const char *read_gitfile_gently(const char *path, int *return_error_code)
+const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf* result_buf)
{
const int max_file_size = 1 << 20; /* 1MB */
int error_code = 0;
@@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
struct stat st;
int fd;
ssize_t len;
- static struct strbuf realpath = STRBUF_INIT;
+ static struct strbuf shared = STRBUF_INIT;
+ if (!result_buf) {
+ result_buf = &shared;
+ }
if (stat(path, &st)) {
/* NEEDSWORK: discern between ENOENT vs other errors */
@@ -900,8 +907,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
goto cleanup_return;
}
- strbuf_realpath(&realpath, dir, 1);
- path = realpath.buf;
+ strbuf_realpath(result_buf, dir, 1);
+ path = result_buf->buf;
+ // TODO is this valid?
+ if (!path) die(_("Unexpected null from realpath '%s'"), dir);
cleanup_return:
if (return_error_code)
@@ -1316,12 +1325,13 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
int offset = dir->len, error_code = 0;
char *gitdir_path = NULL;
char *gitfile = NULL;
+ struct strbuf gitdirenvbuf = STRBUF_INIT;
if (offset > min_offset)
strbuf_addch(dir, '/');
strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?
- NULL : &error_code);
+ NULL : &error_code, &gitdirenvbuf);
if (!gitdirenv) {
if (die_on_error ||
error_code == READ_GITFILE_ERR_NOT_A_FILE) {
@@ -1330,8 +1340,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
gitdir_path = xstrdup(dir->buf);
}
- } else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
+ } else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {
+ strbuf_release(&gitdirenvbuf);
return GIT_DIR_INVALID_GITFILE;
+ }
} else
gitfile = xstrdup(dir->buf);
/*
@@ -1365,9 +1377,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
*/
free(gitdir_path);
free(gitfile);
-
+ strbuf_release(&gitdirenvbuf);
return ret;
}
+ strbuf_release(&gitdirenvbuf);
if (is_git_directory(dir->buf)) {
trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf);
@@ -1692,11 +1705,12 @@ const char *setup_git_directory(void)
return setup_git_directory_gently(NULL);
}
-const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)
+const char *resolve_gitdir_gently(const char *suspect, int *return_error_code,
+ struct strbuf* return_buf)
{
if (is_git_directory(suspect))
return suspect;
- return read_gitfile_gently(suspect, return_error_code);
+ return read_gitfile_gently(suspect, return_error_code, return_buf);
}
/* if any standard file descriptor is missing open it to /dev/null */
diff --git a/setup.h b/setup.h
index 3599aec93c..cf5a63762b 100644
--- a/setup.h
+++ b/setup.h
@@ -36,10 +36,9 @@ int is_nonbare_repository_dir(struct strbuf *path);
#define READ_GITFILE_ERR_NOT_A_REPO 7
#define READ_GITFILE_ERR_TOO_LARGE 8
void read_gitfile_error_die(int error_code, const char *path, const char *dir);
-const char *read_gitfile_gently(const char *path, int *return_error_code);
-#define read_gitfile(path) read_gitfile_gently((path), NULL)
-const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);
-#define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)
+const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf *buf);
+#define read_gitfile(path) read_gitfile_gently((path), NULL, NULL)
+const char *resolve_gitdir_gently(const char *suspect, int *return_error_code, struct strbuf *buf);
void setup_work_tree(void);
diff --git a/submodule.c b/submodule.c
index 213da79f66..455444321b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -316,9 +316,10 @@ int is_submodule_populated_gently(const char *path, int *return_error_code)
int ret = 0;
char *gitdir = xstrfmt("%s/.git", path);
- if (resolve_gitdir_gently(gitdir, return_error_code))
+ struct strbuf resolved_gitdir_buf = STRBUF_INIT;
+ if (resolve_gitdir_gently(gitdir, return_error_code, &resolved_gitdir_buf))
ret = 1;
-
+ strbuf_release(&resolved_gitdir_buf);
free(gitdir);
return ret;
}
@@ -1879,22 +1880,25 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
{
struct child_process cp = CHILD_PROCESS_INIT;
struct strbuf buf = STRBUF_INIT;
+ struct strbuf gitdirbuf = STRBUF_INIT;
FILE *fp;
unsigned dirty_submodule = 0;
const char *git_dir;
int ignore_cp_exit_code = 0;
strbuf_addf(&buf, "%s/.git", path);
- git_dir = read_gitfile(buf.buf);
+ git_dir = read_gitfile_gently(buf.buf, NULL, &gitdirbuf);
if (!git_dir)
git_dir = buf.buf;
if (!is_git_directory(git_dir)) {
if (is_directory(git_dir))
die(_("'%s' not recognized as a git repository"), git_dir);
strbuf_release(&buf);
+ strbuf_release(&gitdirbuf);
/* The submodule is not checked out, so it is not modified */
return 0;
}
+ strbuf_release(&gitdirbuf);
strbuf_reset(&buf);
strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
@@ -1958,15 +1962,16 @@ int submodule_uses_gitfile(const char *path)
{
struct child_process cp = CHILD_PROCESS_INIT;
struct strbuf buf = STRBUF_INIT;
- const char *git_dir;
+ struct strbuf gitfilebuf = STRBUF_INIT;
strbuf_addf(&buf, "%s/.git", path);
- git_dir = read_gitfile(buf.buf);
- if (!git_dir) {
+ read_gitfile_gently(buf.buf, NULL, &gitfilebuf);
+ if (!gitfilebuf.buf) {
strbuf_release(&buf);
return 0;
}
strbuf_release(&buf);
+ strbuf_release(&gitfilebuf);
/* Now test that all nested submodules use a gitfile too */
strvec_pushl(&cp.args,
@@ -2276,6 +2281,7 @@ static void relocate_single_git_dir_into_superproject(const char *path,
{
char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
struct strbuf new_gitdir = STRBUF_INIT;
+ struct strbuf gitfilebuf = STRBUF_INIT;
const struct submodule *sub;
if (submodule_uses_worktrees(path))
@@ -2283,9 +2289,12 @@ static void relocate_single_git_dir_into_superproject(const char *path,
"more than one worktree not supported"), path);
old_git_dir = xstrfmt("%s/.git", path);
- if (read_gitfile(old_git_dir))
+ if (read_gitfile_gently(old_git_dir, NULL, &gitfilebuf)) {
/* If it is an actual gitfile, it doesn't need migration. */
+ strbuf_release(&gitfilebuf);
return;
+ }
+ strbuf_release(&gitfilebuf);
real_old_git_dir = real_pathdup(old_git_dir, 1);
@@ -2343,8 +2352,9 @@ void absorb_git_dir_into_superproject(const char *path,
int err_code;
const char *sub_git_dir;
struct strbuf gitdir = STRBUF_INIT;
+ struct strbuf resolved_gitdir_buf = STRBUF_INIT;
strbuf_addf(&gitdir, "%s/.git", path);
- sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);
+ sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code, &resolved_gitdir_buf);
/* Not populated? */
if (!sub_git_dir) {
@@ -2385,6 +2395,8 @@ void absorb_git_dir_into_superproject(const char *path,
free(real_sub_git_dir);
free(real_common_git_dir);
}
+
+ strbuf_release(&resolved_gitdir_buf);
strbuf_release(&gitdir);
absorb_git_dir_into_superproject_recurse(path, super_prefix);
@@ -2484,17 +2496,19 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
const struct submodule *sub;
const char *git_dir;
int ret = 0;
+ struct strbuf gitfilebuf = STRBUF_INIT;
strbuf_reset(buf);
strbuf_addstr(buf, submodule);
strbuf_complete(buf, '/');
strbuf_addstr(buf, ".git");
- git_dir = read_gitfile(buf->buf);
+ git_dir = read_gitfile_gently(buf->buf, NULL, &gitfilebuf);
if (git_dir) {
strbuf_reset(buf);
strbuf_addstr(buf, git_dir);
}
+ strbuf_release(&gitfilebuf);
if (!is_git_directory(buf->buf)) {
sub = submodule_from_path(the_repository, null_oid(),
submodule);
diff --git a/worktree.c b/worktree.c
index b02a05a74a..a6f125c8da 100644
--- a/worktree.c
+++ b/worktree.c
@@ -309,7 +309,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
{
struct strbuf wt_path = STRBUF_INIT;
struct strbuf realpath = STRBUF_INIT;
- char *path = NULL;
+ struct strbuf gitfile = STRBUF_INIT;
int err, ret = -1;
strbuf_addf(&wt_path, "%s/.git", wt->path);
@@ -353,21 +353,20 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
goto done;
}
- path = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));
- if (!path) {
+ if (!read_gitfile_gently(wt_path.buf, &err, &gitfile)) {
strbuf_addf_gently(errmsg, _("'%s' is not a .git file, error code %d"),
wt_path.buf, err);
goto done;
}
strbuf_realpath(&realpath, git_common_path("worktrees/%s", wt->id), 1);
- ret = fspathcmp(path, realpath.buf);
+ ret = fspathcmp(gitfile.buf, realpath.buf);
if (ret)
strbuf_addf_gently(errmsg, _("'%s' does not point back to '%s'"),
wt->path, git_common_path("worktrees/%s", wt->id));
done:
- free(path);
+ strbuf_release(&gitfile);
strbuf_release(&wt_path);
strbuf_release(&realpath);
return ret;
@@ -567,7 +566,7 @@ static void repair_gitfile(struct worktree *wt,
{
struct strbuf dotgit = STRBUF_INIT;
struct strbuf repo = STRBUF_INIT;
- char *backlink;
+ struct strbuf backlink = STRBUF_INIT;
const char *repair = NULL;
int err;
@@ -582,13 +581,13 @@ static void repair_gitfile(struct worktree *wt,
strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
strbuf_addf(&dotgit, "%s/.git", wt->path);
- backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
+ read_gitfile_gently(dotgit.buf, &err, &backlink);
if (err == READ_GITFILE_ERR_NOT_A_FILE)
fn(1, wt->path, _(".git is not a file"), cb_data);
else if (err)
repair = _(".git file broken");
- else if (fspathcmp(backlink, repo.buf))
+ else if (fspathcmp(backlink.buf, repo.buf))
repair = _(".git file incorrect");
if (repair) {
@@ -596,7 +595,7 @@ static void repair_gitfile(struct worktree *wt,
write_file(dotgit.buf, "gitdir: %s", repo.buf);
}
- free(backlink);
+ strbuf_release(&backlink);
strbuf_release(&repo);
strbuf_release(&dotgit);
}
@@ -685,7 +684,7 @@ void repair_worktree_at_path(const char *path,
struct strbuf realdotgit = STRBUF_INIT;
struct strbuf gitdir = STRBUF_INIT;
struct strbuf olddotgit = STRBUF_INIT;
- char *backlink = NULL;
+ struct strbuf backlink = STRBUF_INIT;
const char *repair = NULL;
int err;
@@ -701,12 +700,12 @@ void repair_worktree_at_path(const char *path,
goto done;
}
- backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
+ read_gitfile_gently(realdotgit.buf, &err, &backlink);
if (err == READ_GITFILE_ERR_NOT_A_FILE) {
fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
goto done;
} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
- if (!(backlink = infer_backlink(realdotgit.buf))) {
+ if (!(backlink.buf = infer_backlink(realdotgit.buf))) {
fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
goto done;
}
@@ -715,7 +714,7 @@ void repair_worktree_at_path(const char *path,
goto done;
}
- strbuf_addf(&gitdir, "%s/gitdir", backlink);
+ strbuf_addf(&gitdir, "%s/gitdir", backlink.buf);
if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
repair = _("gitdir unreadable");
else {
@@ -729,7 +728,7 @@ void repair_worktree_at_path(const char *path,
write_file(gitdir.buf, "%s", realdotgit.buf);
}
done:
- free(backlink);
+ strbuf_release(&backlink);
strbuf_release(&olddotgit);
strbuf_release(&gitdir);
strbuf_release(&realdotgit);
--
2.44.0.rc1.240.g4c46232300-goog
