NotificationsYou must be signed in to change notification settings
Fork2.5k
Star10k

Do not normalize safe.directory paths#6668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

ethomson merged 2 commits intolibgit2:mainfromcsware:safe.directory

Jan 14, 2024

Merged

Do not normalize safe.directory paths#6668

ethomson merged 2 commits intolibgit2:mainfromcsware:safe.directory

Jan 14, 2024

Conversation

Copy link

Contributor

csware commentedDec 11, 2023•
edited
Loading

Vanilla Git does not do it either (cf.https://github.com/gitster/git/blob/master/setup.c#L1150) and calling git_fs_path_prettify_dir can cause performance issues (cf. issue#6649).

Still needed: expansion of "~/"... (but not this PR)

csware force-pushed thesafe.directory branch from8c5ed29 to9a2df4dCompare

December 11, 2023 11:45

ethomson reviewed

Dec 13, 2023

View reviewed changes

src/libgit2/repository.c Outdated


		if (git_fs_path_prettify_dir(&data->tmp, test_path, NULL) == 0 &&
		strcmp(data->tmp.ptr, data->repo_path) == 0)
		git_str_sets(&tmp, data->repo_path);

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We were passing thegit_str here to avoid re-allocing agit_str on every call. We can just re-use the one we have. No need to undo that, I don't think.

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Also, you're looking atdata->repo_path, nottest_path, which we've mutated to handle WSL paths. We should keep that.

ethomson reviewed

Dec 13, 2023

View reviewed changes

src/libgit2/repository.c

		@@ -582,10 +582,14 @@ static int validate_ownership_cb(const git_config_entry entry, void payload)
		strncmp(test_path, "//wsl.localhost/", strlen("//wsl.localhost/")) != 0)
		test_path++;
		#endif

		if (git_fs_path_prettify_dir(&data->tmp, test_path, NULL) == 0 &&

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think that instead ofprettify, we need only callpath_to_dir.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I don't think that usingpath_to_dir is the right way. This would acceptsafe.directory entries ending with a slash which vanilla Git does not accept.

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Is that actually a problem, though? We also accept//wsl.localhost/... prefixed paths which Git for Windows doesn't accept.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

From my point of view, I would keep it as consistent as possible.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Let's do one fix at the time. It's ok for me if we take your suggested patch.

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

From my point of view, I would keep it as consistent as possible.

OK. Let's do that. This feels like a (minor) bug in git that it doesn't allow trailing slashes, but I've often said that libgit2's goal is to be "bug for bug compatible".

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@ethomson
I updated the branch and hope that I did not forget anything.

Copy link

Member

ethomson commentedDec 13, 2023

Agree that we don't need to hit the filesystem on every excluded path. I think that the simplest fix is:

diff --git a/src/libgit2/repository.c b/src/libgit2/repository.cindex 05ece6efc..9fa616fb0 100644--- a/src/libgit2/repository.c+++ b/src/libgit2/repository.c@@ -583,8 +583,11 @@ static int validate_ownership_cb(const git_config_entry *entry, void *payload)                        test_path++; #endif-               if (git_fs_path_prettify_dir(&data->tmp, test_path, NULL) == 0 &&-                   strcmp(data->tmp.ptr, data->repo_path) == 0)+               if (git_str_sets(&data->tmp, test_path) < 0 ||+                   git_fs_path_to_dir(&data->tmp) < 0)+                       return -1;++               if (strcmp(data->tmp.ptr, data->repo_path) == 0)                        *data->is_safe = true;        }

This keeps thegit_str that is used between invocations of the callback to avoid re-allocs, but switches fromprettify_path topath_to_dir, which should avoid touching the filesystem and merely append a/ since that's our (sort of dubious) pattern for directories.

This does not add the trailing/ check on inputs. I don't know how valuable that is - I think that specifying a folder with a trailing slash is perfectly legitimate and that git should allow it. But if you're bullish on exact compatibility here, I think that's a reasonable opinion as well.

Copy link

Member

ethomson commentedDec 13, 2023

Thanks@csware for digging in here to understand the perf problem - it's slow for me to develop / test on Windows - and apologies for causing it in the first place.

csware force-pushed thesafe.directory branch from9a2df4d to9586445Compare

December 13, 2023 14:55

Copy link

ContributorAuthor

csware commentedDec 13, 2023

@ethomson I'd like to talk to you about libgit2, some open PRs and TortoiseGit. Would be nice if we could meet next year.

csware force-pushed thesafe.directory branch from9586445 to4e5a980Compare

December 13, 2023 18:53

csware added2 commits

December 13, 2023 20:16

Do not normalize safe.directory paths

779263b

Vanilla Git does not do it either (cf.https://github.com/gitster/git/blob/master/setup.c#L1150)and calling git_fs_path_prettify_dir can cause performance issues (cf. issuelibgit2#6649).Signed-off-by: Sven Strickroth <email@cs-ware.de>

Improve error message

73f034c

Signed-off-by: Sven Strickroth <email@cs-ware.de>

csware force-pushed thesafe.directory branch from4e5a980 to73f034cCompare

December 13, 2023 19:16

Copy link

ContributorAuthor

csware commentedDec 18, 2023

@ethomson
Is there an open point here?

ethomson merged commit73f034c intolibgit2:main

Jan 14, 2024

Copy link

Member

ethomson commentedJan 14, 2024

Manually merged with a tweak - thanks again!

Copy link

ContributorAuthor

csware commentedJan 14, 2024

This is also relevant for 1.7, right?!

Copy link

ContributorAuthor

csware commentedJan 14, 2024•
edited
Loading

@ethomson
Is there a reason why you ,merged both commits and then reverted the second one which improves the error message? Particularly, the hint regarding%(prefix)/"is missing now.

I don't quite understand your tweak either. Now more copies are needed, right? Particularly, I don't understand the comment regarding the backslash.

csware deleted the safe.directory branch

January 14, 2024 22:15

csware mentioned this pull request

Feb 14, 2024

Repository ownership validation does not skip%(prefix)/ on Windows#6737

Closed

csware commented

Feb 14, 2024

View reviewed changes

src/libgit2/repository.c

		@@ -582,9 +581,7 @@ static int validate_ownership_cb(const git_config_entry entry, void payload)
		strncmp(test_path, "//wsl.localhost/", strlen("//wsl.localhost/")) != 0)

Copy link

ContributorAuthor