Re: [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Mar 5, 2024, 04:29 UTC
- Message-ID
- <CAPig+cQvyScjiWe1ghFrv9B=v2+JxkkErdCyYFSA_8dTrWu60g@mail.gmail.com>
- In-Reply-To
- <20240305012112.1598053-3-atneya@google.com>
On Mon, Mar 4, 2024 at 8:22 PM Atneya Nair <atneya@google.com> wrote:
Show 24 quoted lines
> 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@google.com>
> ---
> diff --git 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)
> - const char *p;
> + 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;If you're going to adopt this idiom of checking `err` rather than the return code of read_gitfile_gently(), then you should document that `err` will be set to zero in the success case. Presently, the documentation for read_gitfile_gently() only talks about the failure case and doesn't mention that zero indicates success.
Show 8 quoted lines
> diff --git a/setup.c b/setup.c > @@ -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.
What is "a valid strbuf"? Perhaps say instead "if `result_buf` is not NULL, ...". The "is not NULL" wording is consistent with the existing wording used below...
Also... s/is is/is/ s/ptr/pointer/
> * On failure, if return_error_code is not NULL, return_error_code
... "is not NULL" wording is already used here.
Show 6 quoted lines
> @@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
> - static struct strbuf realpath = STRBUF_INIT;
> + static struct strbuf shared = STRBUF_INIT;
> + if (!result_buf) {
> + result_buf = &shared;
> + }Junio mentioned style violations in his response. Omit braces around one line `if` bodies.
Show 5 quoted lines
> @@ -900,8 +907,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code) > - strbuf_realpath(&realpath, dir, 1); > - path = realpath.buf; > + strbuf_realpath(result_buf, dir, 1); > + path = result_buf->buf;
It's a minor thing, but if you name the function argument `realpath`, then the diff becomes less noisy since changes such as these do not need to be made. On the other hand, if `realpath` isn't a good output variable name, then by all means choose a better name.
Show 12 quoted lines
> @@ -1316,12 +1325,13 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
> + struct strbuf gitdirenvbuf = STRBUF_INIT;
> gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?
> - NULL : &error_code);
> + NULL : &error_code, &gitdirenvbuf);
> if (!gitdirenv) {
> @@ -1330,8 +1340,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
> - } 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;
> + }Releasing the strbuf before `return`. Good.
Show 5 quoted lines
> @@ -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;
Likewise. Good.
Show 5 quoted lines
> }
> + strbuf_release(&gitdirenvbuf);
>
> if (is_git_directory(dir->buf)) {
> trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf);There are additional `return` statements (not shown in the context) following this code, but you make this final strbuf_release() call before any of those other `return` statements can be taken. Good.
Show 9 quoted lines
> diff --git 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;Style: Declare `resolved_gitdir_buf` along with `ret` and `gitdir`, then have a blank line before the actual code.
Show 8 quoted lines
> @@ -1879,22 +1880,25 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked) > + struct strbuf gitdirbuf = STRBUF_INIT; > + strbuf_release(&gitdirbuf); > /* The submodule is not checked out, so it is not modified */ > return 0; > } > + strbuf_release(&gitdirbuf); > strbuf_reset(&buf);
Style: Strange indentation?
Show 12 quoted lines
> @@ -1958,15 +1962,16 @@ int submodule_uses_gitfile(const char *path)
> - 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;
> }Not sure what you're trying to do here. strbuf guarantees that its `buf` member will never be NULL, so the new `if (!gitfilebuf.buf)` conditional seems to be dead code. If you really want to check whether an error occurred, pass non-NULL for the second argument and check the return value of read_gitfile_gently() or check the error code.
Show 13 quoted lines
> diff --git a/worktree.c b/worktree.c
> @@ -685,7 +684,7 @@ void repair_worktree_at_path(const char *path,
> - char *backlink = NULL;
> + struct strbuf backlink = STRBUF_INIT;
> @@ -701,12 +700,12 @@ void repair_worktree_at_path(const char *path,
> - 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))) {Don't do this. Never modify the internal state of strbuf directly; consider the state read-only. Modifications should only be made via the API. You'll need to rewrite this code a bit to make it work correctly with the changes proposed by this patch.