Re: [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Mar 4, 2024 at 8:22 PM Atneya Nair <atneya@google.com> wrote:
> 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.

> 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.

> @@ -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.

> @@ -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.

> @@ -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.

> @@ -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.

>                 }
> +               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.

> 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.

> @@ -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?

> @@ -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.

> 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.