Re: [PATCH] real_path: make real_path thread-safe

From: Stefan Beller <sbeller@google.com>
Date: Dec 5, 2016, 19:57 UTC
Message-ID: <CAGZ79kauPdE1uiFSvBALkNiwXbnV6d6xhwLdWNQwRir_8rTG6Q@mail.gmail.com>
In-Reply-To: <1480964316-99305-2-git-send-email-bmwill@google.com>

On Mon, Dec 5, 2016 at 10:58 AM, Brandon Williams <bmwill@google.com> wrote:

Show 7 quoted lines

> The current implementation of real_path uses chdir() in order to resolve
> symlinks.  Unfortunately this isn't thread-safe as chdir() affects a
> process as a whole and not just an individual thread.  Instead perform
> the symlink resolution by hand so that the calls to chdir() can be
> removed, making real_path reentrant.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>

Thanks for working on this, some comments below:

Show 6 quoted lines

>
> +/* removes the last path component from 'path' except if 'path' is root */
> +static void strip_last_component(struct strbuf *path)
> +{
> +       if (path->len > 1) {
> +               char *last_slash = find_last_dir_sep(path->buf);

What happens when there is no dir_sep?

> +/* gets the next component in 'remaining' and places it in 'next' */
> +static void get_next_component(struct strbuf *next, struct strbuf *remaining)
> +{

It's more than just getting it, it also chops it off of 'remaining' ?

Show 6 quoted lines

> +       strbuf_reset(&resolved);
> +
> +       if (is_absolute_path(path)) {
> +               /* absolute path; start with only root as being resolved */
> +               strbuf_addch(&resolved, '/');
> +               strbuf_addstr(&remaining, path + 1);

This is where we would wait for input of Windows savy people.

Show 5 quoted lines

> +       } else {
> +               /* relative path; can use CWD as the initial resolved path */
> +               if (strbuf_getcwd(&resolved)) {
> +                       if (die_on_error)
> +                               die_errno("Could not get current working directory");

I am looking at xgetcwd, which words it slightly differently.

    if (strbuf_getcwd(&sb))
        die_errno(_("unable to get current working directory"));

Not sure if aligning them would be a good idea?

Going by "git grep die_errno" as well as our Coding guidelines, we don't want to see capitalized error messages.

> +
> +               if (next.len == 0) {
> +                       continue; /* empty component */

which means we resolve over path//with//double//slashes just fine, as well as /./ parts. :)

Show 5 quoted lines

>                 }
>
> -               if (sb.len) {
> -                       if (!cwd.len && strbuf_getcwd(&cwd)) {
> +               /* append the next component and resolve resultant path */

"resultant" indicates you have a math background. :) But I had to look it up, I guess it is fine that way, though "resulting" may cause less mental friction for non native speakers.

Show 9 quoted lines

> +                       if (!(errno == ENOENT && !remaining.len)) {
>                                 if (die_on_error)
> -                                       die_errno("Could not get current working directory");
> +                                       die_errno("Invalid path '%s'",
> +                                                 resolved.buf);
>                                 else
>                                         goto error_out;
>                         }
> +               } else if (S_ISLNK(st.st_mode)) {

As far as I can tell, we could keep the symlink strbuf at a smaller scope here? (I was surprised how many strbufs are declared at the beginning of the function)

> +       //strbuf_release(&resolved);

This is why the cover letter toned down expectations ? (no // as comment, maybe remove that line?)

Thanks, Stefan