diff mbox series

[v4] setup: don't die if realpath(3) fails on getcwd(3)

Message ID 8b20840014d214023c50ee62439147f798e6f9cc.1653419993.git.kevin@kevinlocke.name (mailing list archive)
State Accepted
Commit c37c6dc6a79a1ca7b9d4fa4efd788d8f5ec6369a
Headers show
Series [v4] setup: don't die if realpath(3) fails on getcwd(3) | expand

Commit Message

Kevin Locke May 24, 2022, 7:20 p.m. UTC
Prior to Git 2.35.0, git could be run from an inaccessible working
directory so long as the git repository specified by options and/or
environment variables was accessible.  For example:

    git init repo
    mkdir -p a/b
    cd a/b
    chmod u-x ..
    git -C "${PWD%/a/b}/repo" status

If this example seems a bit contrived, consider running with the
repository owner as a substitute UID (e.g. with runuser(1) or sudo(8))
without ensuring the working directory is accessible by that user.

The code added by e6f8861bd4 ("setup: introduce
startup_info->original_cwd") to preserve the working directory attempts
to normalize the path using strbuf_realpath().  If that fails, as in the
case above, it is treated as a fatal error.

This commit treats strbuf_realpath() errors as non-fatal.  If an error
occurs, setup_original_cwd() will continue without applying removal
prevention for cwd, resulting in the pre-2.35.0 behavior.  The risk
should be minimal, since git will not operate on a repository with
inaccessible ancestors, this behavior is only known to occur when cwd is
a descendant of the repository, an ancestor of cwd is inaccessible, and
no ancestors of the repository are inaccessible.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
---

Changes since v3:
 * Free tmp_original_cwd in both codepaths.
 * Return after strbuf_realpath() fails, rather than jumping to
   no_prevention_needed, to avoid unnecessary free(NULL) and NULL
   reassignment.
 * Invert the condition and remove the else block to match the
   return-on-error code style for better readability.
 * Stop adding "Try" to comment, since strbuf_realpath() hasn't
   been optional since v1.

Changes since v2:
 * Use trace2_data_string(), rather than trace_printf(), to report
   realpath failure.

Changes since v1:
 * Set startup_info->original_cwd = NULL when strbuf_realpath() fails,
   rather than setting it to the un-normalized path.
 * Add a trace message when realpath fails to aid debugging.
 * Remove potential realpath failure cause from comment before it.
 * Improve format for reference to e6f8861bd4 in commit message.
 * Clarify when the pre-2.35.0 behavior may occur as a result of this
   commit in the commit message.
 * Remove 'Fixes:' tag from commit message.

 setup.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Derrick Stolee May 24, 2022, 8:40 p.m. UTC | #1
On 5/24/2022 3:20 PM, Kevin Locke wrote:
> Changes since v3:
>  * Free tmp_original_cwd in both codepaths.
>  * Return after strbuf_realpath() fails, rather than jumping to
>    no_prevention_needed, to avoid unnecessary free(NULL) and NULL
>    reassignment.
>  * Invert the condition and remove the else block to match the
>    return-on-error code style for better readability.
>  * Stop adding "Try" to comment, since strbuf_realpath() hasn't
>    been optional since v1.

...

>  	/* Normalize the directory */
> -	strbuf_realpath(&tmp, tmp_original_cwd, 1);
> +	if (!strbuf_realpath(&tmp, tmp_original_cwd, 0)) {
> +		trace2_data_string("setup", the_repository,
> +				   "realpath-path", tmp_original_cwd);
> +		trace2_data_string("setup", the_repository,
> +				   "realpath-failure", strerror(errno));
> +		free((char*)tmp_original_cwd);
> +		tmp_original_cwd = NULL;
> +		return;
> +	}

This is much easier to reason about.

>  	free((char*)tmp_original_cwd);
>  	tmp_original_cwd = NULL;
>  	startup_info->original_cwd = strbuf_detach(&tmp, NULL);
I had considered trying to remove this duplicate code freeing
temp_original_cwd. It requires adding a variable storing the
return from strbuf_realpath() _or_ knowing that tmp will have
zero length if strbuf_realpath() fails. It would look gross,
though:

	strbuf_realpath(&tmp, tmp_original_cwd, 0);

	if (!tmp->len) {
		trace2_data_string("setup", the_repository,
				   "realpath-path", tmp_original_cwd);
		trace2_data_string("setup", the_repository,
				   "realpath-failure", strerror(errno));
	}
	free((char*)tmp_original_cwd);
	tmp_original_cwd = NULL;
	if (!tmp->len)
		return;

	startup_info->original_cwd = strbuf_detach(&tmp, NULL);

...and that doesn't look very good at all. Thus, I think your v4
is ready to merge. Thanks for working on it!

Thanks,
-Stolee
Junio C Hamano May 24, 2022, 9:25 p.m. UTC | #2
Kevin Locke <kevin@kevinlocke.name> writes:

>  	/* Normalize the directory */
> -	strbuf_realpath(&tmp, tmp_original_cwd, 1);
> +	if (!strbuf_realpath(&tmp, tmp_original_cwd, 0)) {
> +		trace2_data_string("setup", the_repository,
> +				   "realpath-path", tmp_original_cwd);
> +		trace2_data_string("setup", the_repository,
> +				   "realpath-failure", strerror(errno));
> +		free((char*)tmp_original_cwd);
> +		tmp_original_cwd = NULL;
> +		return;
> +	}
> +
>  	free((char*)tmp_original_cwd);
>  	tmp_original_cwd = NULL;
>  	startup_info->original_cwd = strbuf_detach(&tmp, NULL);

It is somewhat sad that we cannot readily use FREE_AND_NULL() here.
If it casted away the constness (see the attached at the end), we
could have saved two lines from the above snippet.

The startup_info->original_cwd member is initialized to NULL, and
I think it is a safe assumption that it still is so when the control
reaches here.  Otherwise, the assignment of strbuf_detach() to it
without first freeing the current value we see in the post context
is leaking already.

So, overall this looks good to me.  Anybody else who have already
spent cycles to review this want to add Reviewed-by: to it?

Thanks.

diff --git i/git-compat-util.h w/git-compat-util.h
index 58fd813bd0..56c6c48461 100644
--- i/git-compat-util.h
+++ w/git-compat-util.h
@@ -976,7 +976,7 @@ int xstrncmpz(const char *s, const char *t, size_t len);
  * FREE_AND_NULL(ptr) is like free(ptr) followed by ptr = NULL. Note
  * that ptr is used twice, so don't pass e.g. ptr++.
  */
-#define FREE_AND_NULL(p) do { free(p); (p) = NULL; } while (0)
+#define FREE_AND_NULL(p) do { free((void*)p); (p) = NULL; } while (0)
 
 #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
 #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x)))
Elijah Newren May 25, 2022, 3:51 a.m. UTC | #3
On Tue, May 24, 2022 at 2:25 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Kevin Locke <kevin@kevinlocke.name> writes:
>
> >       /* Normalize the directory */
> > -     strbuf_realpath(&tmp, tmp_original_cwd, 1);
> > +     if (!strbuf_realpath(&tmp, tmp_original_cwd, 0)) {
> > +             trace2_data_string("setup", the_repository,
> > +                                "realpath-path", tmp_original_cwd);
> > +             trace2_data_string("setup", the_repository,
> > +                                "realpath-failure", strerror(errno));
> > +             free((char*)tmp_original_cwd);
> > +             tmp_original_cwd = NULL;
> > +             return;
> > +     }
> > +
> >       free((char*)tmp_original_cwd);
> >       tmp_original_cwd = NULL;
> >       startup_info->original_cwd = strbuf_detach(&tmp, NULL);
>
> It is somewhat sad that we cannot readily use FREE_AND_NULL() here.
> If it casted away the constness (see the attached at the end), we
> could have saved two lines from the above snippet.
>
> The startup_info->original_cwd member is initialized to NULL, and
> I think it is a safe assumption that it still is so when the control
> reaches here.  Otherwise, the assignment of strbuf_detach() to it
> without first freeing the current value we see in the post context
> is leaking already.
>
> So, overall this looks good to me.  Anybody else who have already
> spent cycles to review this want to add Reviewed-by: to it?

Well, I added my Reviewed-by to v3 and apparently missed a few things
which were fixed up in v4.  So, my review apparently wasn't careful
enough.  But I am happy with this version, so here it is again:

Reviewed-by: Elijah Newren <newren@gmail.com>

(Now someone is free to spot more problems and embarrass me even more...)

>
> Thanks.
>
> diff --git i/git-compat-util.h w/git-compat-util.h
> index 58fd813bd0..56c6c48461 100644
> --- i/git-compat-util.h
> +++ w/git-compat-util.h
> @@ -976,7 +976,7 @@ int xstrncmpz(const char *s, const char *t, size_t len);
>   * FREE_AND_NULL(ptr) is like free(ptr) followed by ptr = NULL. Note
>   * that ptr is used twice, so don't pass e.g. ptr++.
>   */
> -#define FREE_AND_NULL(p) do { free(p); (p) = NULL; } while (0)
> +#define FREE_AND_NULL(p) do { free((void*)p); (p) = NULL; } while (0)
>
>  #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
>  #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x)))

I also like this change, even if it feels like it should be part of a
separate patch.
Junio C Hamano May 25, 2022, 5:11 a.m. UTC | #4
Elijah Newren <newren@gmail.com> writes:

>> diff --git i/git-compat-util.h w/git-compat-util.h
>> index 58fd813bd0..56c6c48461 100644
>> --- i/git-compat-util.h
>> +++ w/git-compat-util.h
>> @@ -976,7 +976,7 @@ int xstrncmpz(const char *s, const char *t, size_t len);
>>   * FREE_AND_NULL(ptr) is like free(ptr) followed by ptr = NULL. Note
>>   * that ptr is used twice, so don't pass e.g. ptr++.
>>   */
>> -#define FREE_AND_NULL(p) do { free(p); (p) = NULL; } while (0)
>> +#define FREE_AND_NULL(p) do { free((void*)p); (p) = NULL; } while (0)
>>
>>  #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
>>  #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x)))
>
> I also like this change, even if it feels like it should be part of a
> separate patch.

Sorry, but I did fail to make it clear that I didn't mean the above
to be anything related to Kevin's patch.  It was "if FREE_AND_NULL()
were defined like this, then we could have used it".  I didn't mean
to say "let's update FREE_AND_NULL() this way so that we can use
it".

Thanks.
diff mbox series

Patch

diff --git a/setup.c b/setup.c
index a7b36f3ffbf..e0a99df512f 100644
--- a/setup.c
+++ b/setup.c
@@ -459,7 +459,16 @@  static void setup_original_cwd(void)
 	 */
 
 	/* Normalize the directory */
-	strbuf_realpath(&tmp, tmp_original_cwd, 1);
+	if (!strbuf_realpath(&tmp, tmp_original_cwd, 0)) {
+		trace2_data_string("setup", the_repository,
+				   "realpath-path", tmp_original_cwd);
+		trace2_data_string("setup", the_repository,
+				   "realpath-failure", strerror(errno));
+		free((char*)tmp_original_cwd);
+		tmp_original_cwd = NULL;
+		return;
+	}
+
 	free((char*)tmp_original_cwd);
 	tmp_original_cwd = NULL;
 	startup_info->original_cwd = strbuf_detach(&tmp, NULL);