Skip to content

Last preparations before upstreaming Git for Windows' symlink support #2017

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
dscho wants to merge 5 commits into
base: js/test-symlink-windows
Choose a base branch
from
+18 −20
Open

Last preparations before upstreaming Git for Windows' symlink support #2017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eb95a74
mingw: do resolve symlinks in `getcwd()`
dscho Jan 1, 2020
9fee7bd
init: do parse _all_ core.* settings early
dscho Sep 8, 2021
7fe463d
strbuf_readlink(): avoid calling `readlink()` twice in corner-cases
kblees May 11, 2015
accb6d5
strbuf_readlink(): support link targets that exceed 2*PATH_MAX
dscho Jan 9, 2026
9823cbb
trim_last_path_component(): avoid hard-coding the directory separator
kblees May 11, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 7 additions & 11 deletions compat/mingw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1251,18 +1251,16 @@ char *mingw_getcwd(char *pointer, int len)
{
Copy link

@gitgitgadget gitgitgadget bot Dec 17, 2025

Choose a reason for hiding this comment

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

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Tue, Dec 16, 2025 at 03:33:45PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/compat/mingw.c b/compat/mingw.c
> index ba1b7b6dd1..7215b127cc 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1251,18 +1251,16 @@ char *mingw_getcwd(char *pointer, int len)
>  {
>  	wchar_t cwd[MAX_PATH], wpointer[MAX_PATH];
>  	DWORD ret = GetCurrentDirectoryW(ARRAY_SIZE(cwd), cwd);
> +	HANDLE hnd;
>  
>  	if (!ret || ret >= ARRAY_SIZE(cwd)) {
>  		errno = ret ? ENAMETOOLONG : err_win_to_posix(GetLastError());
>  		return NULL;
>  	}
> -	ret = GetLongPathNameW(cwd, wpointer, ARRAY_SIZE(wpointer));
> -	if (!ret && GetLastError() == ERROR_ACCESS_DENIED) {
> -		HANDLE hnd = CreateFileW(cwd, 0,
> -			FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL,
> -			OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
> -		if (hnd == INVALID_HANDLE_VALUE)
> -			return NULL;
> +	hnd = CreateFileW(cwd, 0,
> +			  FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL,
> +			  OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
> +	if (hnd != INVALID_HANDLE_VALUE) {
>  		ret = GetFinalPathNameByHandleW(hnd, wpointer, ARRAY_SIZE(wpointer), 0);
>  		CloseHandle(hnd);
>  		if (!ret || ret >= ARRAY_SIZE(wpointer))

Okay. Due to the change we now also try calling `GetFileAttributesW()`
in case `CreateFileW()` fails, which wasn't the case before. But I'd
consider that to be a win -- if we cannot figure out the final path
name, then we can at least return the unresolved current working
directory.

Patrick

> @@ -1271,13 +1269,11 @@ char *mingw_getcwd(char *pointer, int len)
>  			return NULL;
>  		return pointer;
>  	}
> -	if (!ret || ret >= ARRAY_SIZE(wpointer))
> -		return NULL;
> -	if (GetFileAttributesW(wpointer) == INVALID_FILE_ATTRIBUTES) {
> +	if (GetFileAttributesW(cwd) == INVALID_FILE_ATTRIBUTES) {
>  		errno = ENOENT;
>  		return NULL;
>  	}
> -	if (xwcstoutf(pointer, wpointer, len) < 0)
> +	if (xwcstoutf(pointer, cwd, len) < 0)
>  		return NULL;
>  	convert_slashes(pointer);
>  	return pointer;

wchar_t cwd[MAX_PATH], wpointer[MAX_PATH];
DWORD ret = GetCurrentDirectoryW(ARRAY_SIZE(cwd), cwd);
HANDLE hnd;

if (!ret || ret >= ARRAY_SIZE(cwd)) {
errno = ret ? ENAMETOOLONG : err_win_to_posix(GetLastError());
return NULL;
}
ret = GetLongPathNameW(cwd, wpointer, ARRAY_SIZE(wpointer));
if (!ret && GetLastError() == ERROR_ACCESS_DENIED) {
HANDLE hnd = CreateFileW(cwd, 0,
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL,
OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
if (hnd == INVALID_HANDLE_VALUE)
return NULL;
hnd = CreateFileW(cwd, 0,
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL,
OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
if (hnd != INVALID_HANDLE_VALUE) {
ret = GetFinalPathNameByHandleW(hnd, wpointer, ARRAY_SIZE(wpointer), 0);
CloseHandle(hnd);
if (!ret || ret >= ARRAY_SIZE(wpointer))
Expand All @@ -1271,13 +1269,11 @@ char *mingw_getcwd(char *pointer, int len)
return NULL;
return pointer;
}
if (!ret || ret >= ARRAY_SIZE(wpointer))
return NULL;
if (GetFileAttributesW(wpointer) == INVALID_FILE_ATTRIBUTES) {
if (GetFileAttributesW(cwd) == INVALID_FILE_ATTRIBUTES) {
errno = ENOENT;
return NULL;
}
if (xwcstoutf(pointer, wpointer, len) < 0)
if (xwcstoutf(pointer, cwd, len) < 0)
return NULL;
convert_slashes(pointer);
return pointer;
Expand Down
4 changes: 2 additions & 2 deletions environment.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,8 +324,8 @@ static enum fsync_component parse_fsync_components(const char *var, const char *
return (current & ~negative) | positive;
Copy link

@gitgitgadget gitgitgadget bot Dec 17, 2025

Choose a reason for hiding this comment

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

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Tue, Dec 16, 2025 at 03:33:46PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/setup.c b/setup.c
> index 7086741e6c..42e4e7a690 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -2611,7 +2611,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
>  	 * have set up the repository format such that we can evaluate
>  	 * includeIf conditions correctly in the case of re-initialization.
>  	 */
> -	repo_config(the_repository, platform_core_config, NULL);
> +	repo_config(the_repository, git_default_core_config, NULL);
>  
>  	safe_create_dir(the_repository, git_dir, 0);

Two lines further down we call `create_default_files()`, and there we
end up calling `repo_config(the_repository, git_default_config, NULL)`
as one of the first things. We do so after copying templates though, so
indeed this comes too late.

We also cannot really merge these two calls: we need to re-parse the
configuration after having copied over the template, as the template may
contain a gitconfig file itself.

Furthermore, `git_default_core_config()` already knows to call
`platform_core_config()`, as well. So we're not losing any of that
information, either.

All to say that this change makes sense to me and should be safe, as we
don't end up parsing _more_ configuration keys, we only parse a subset
of it a bit earlier.

Patrick

}

static int git_default_core_config(const char *var, const char *value,
const struct config_context *ctx, void *cb)
int git_default_core_config(const char *var, const char *value,
const struct config_context *ctx, void *cb)
{
/* This needs a better name */
if (!strcmp(var, "core.filemode")) {
Expand Down
2 changes: 2 additions & 0 deletions environment.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ const char *strip_namespace(const char *namespaced_ref);

int git_default_config(const char *, const char *,
const struct config_context *, void *);
int git_default_core_config(const char *var, const char *value,
const struct config_context *ctx, void *cb);

/*
* TODO: All the below state either explicitly or implicitly relies on
Expand Down
4 changes: 2 additions & 2 deletions lockfile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ static void trim_last_path_component(struct strbuf *path)
int i = path->len;

/* back up past trailing slashes, if any */
while (i && path->buf[i - 1] == '/')
while (i && is_dir_sep(path->buf[i - 1]))
i--;

/*
* then go backwards until a slash, or the beginning of the
* string
*/
while (i && path->buf[i - 1] != '/')
while (i && !is_dir_sep(path->buf[i - 1]))
i--;

strbuf_setlen(path, i);
Expand Down
2 changes: 1 addition & 1 deletion setup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2611,7 +2611,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
* have set up the repository format such that we can evaluate
* includeIf conditions correctly in the case of re-initialization.
*/
repo_config(the_repository, platform_core_config, NULL);
repo_config(the_repository, git_default_core_config, NULL);

safe_create_dir(the_repository, git_dir, 0);

Expand Down
8 changes: 4 additions & 4 deletions strbuf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f)
return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
Copy link

@gitgitgadget gitgitgadget bot Dec 17, 2025

Choose a reason for hiding this comment

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

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Tue, Dec 16, 2025 at 03:33:48PM +0000, Karsten Blees via GitGitGadget wrote:
> diff --git a/strbuf.c b/strbuf.c
> index 44a8f6a554..fa4e30f112 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -566,8 +566,6 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f)
>  	return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
>  }
>  
> -#define STRBUF_MAXLINK (2*PATH_MAX)
> -
>  int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
>  {
>  	size_t oldalloc = sb->alloc;
> @@ -575,7 +573,7 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
>  	if (hint < 32)
>  		hint = 32;
>  
> -	while (hint < STRBUF_MAXLINK) {
> +	for (;;) {
>  		ssize_t len;
>  
>  		strbuf_grow(sb, hint + 1);

This makes me wonder whether we have a better way to figure out the
actual size of the buffer that we ultimately need to allocate. But
reading through readlink(3p) doesn't indicate anything, and I'm not sure
whether we can always rely on lstat(3p) to return the correct size for
symlink contents on all platforms.

One thing that _is_ noted though is that calling the function with a
buffer size larger than SSIZE_MAX is implementation-defined. It does
make me a bit uneasy in that light to grow indefinitely.

Which makes me wonder whether Windows has a limit for the symlink
contents that we could enforce in theory so that we can reasonably turn
this into a bounded loop again?

Patrick

Copy link

@gitgitgadget gitgitgadget bot Dec 19, 2025

Choose a reason for hiding this comment

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

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Patrick,

On Wed, 17 Dec 2025, Patrick Steinhardt wrote:

> On Tue, Dec 16, 2025 at 03:33:48PM +0000, Karsten Blees via GitGitGadget wrote:
> > diff --git a/strbuf.c b/strbuf.c
> > index 44a8f6a554..fa4e30f112 100644
> > --- a/strbuf.c
> > +++ b/strbuf.c
> > @@ -566,8 +566,6 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f)
> >  	return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
> >  }
> >  
> > -#define STRBUF_MAXLINK (2*PATH_MAX)
> > -
> >  int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
> >  {
> >  	size_t oldalloc = sb->alloc;
> > @@ -575,7 +573,7 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
> >  	if (hint < 32)
> >  		hint = 32;
> >  
> > -	while (hint < STRBUF_MAXLINK) {
> > +	for (;;) {
> >  		ssize_t len;
> >  
> >  		strbuf_grow(sb, hint + 1);
> 
> This makes me wonder whether we have a better way to figure out the
> actual size of the buffer that we ultimately need to allocate. But
> reading through readlink(3p) doesn't indicate anything, and I'm not sure
> whether we can always rely on lstat(3p) to return the correct size for
> symlink contents on all platforms.
> 
> One thing that _is_ noted though is that calling the function with a
> buffer size larger than SSIZE_MAX is implementation-defined. It does
> make me a bit uneasy in that light to grow indefinitely.
> 
> Which makes me wonder whether Windows has a limit for the symlink
> contents that we could enforce in theory so that we can reasonably turn
> this into a bounded loop again?

https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
suggests that the maximum permissible target path should be 32,768. But
that's not _quite_ correct, as
`../t/../Documentation/RelNotes/../../README.md` is a perfectly valid (if
awkward) symlink target.

Still, I would say that 32,768 would make for a fine (still insanely high,
but not so high as to allow malicious symlinks to cause memory problems)
limit.

Sound good?
Johannes

Copy link

@gitgitgadget gitgitgadget bot Dec 19, 2025

Choose a reason for hiding this comment

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

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Fri, Dec 19, 2025 at 09:50:15AM +0100, Johannes Schindelin wrote:
> Hi Patrick,
> 
> On Wed, 17 Dec 2025, Patrick Steinhardt wrote:
> 
> > On Tue, Dec 16, 2025 at 03:33:48PM +0000, Karsten Blees via GitGitGadget wrote:
> > > diff --git a/strbuf.c b/strbuf.c
> > > index 44a8f6a554..fa4e30f112 100644
> > > --- a/strbuf.c
> > > +++ b/strbuf.c
> > > @@ -566,8 +566,6 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f)
> > >  	return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
> > >  }
> > >  
> > > -#define STRBUF_MAXLINK (2*PATH_MAX)
> > > -
> > >  int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
> > >  {
> > >  	size_t oldalloc = sb->alloc;
> > > @@ -575,7 +573,7 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
> > >  	if (hint < 32)
> > >  		hint = 32;
> > >  
> > > -	while (hint < STRBUF_MAXLINK) {
> > > +	for (;;) {
> > >  		ssize_t len;
> > >  
> > >  		strbuf_grow(sb, hint + 1);
> > 
> > This makes me wonder whether we have a better way to figure out the
> > actual size of the buffer that we ultimately need to allocate. But
> > reading through readlink(3p) doesn't indicate anything, and I'm not sure
> > whether we can always rely on lstat(3p) to return the correct size for
> > symlink contents on all platforms.
> > 
> > One thing that _is_ noted though is that calling the function with a
> > buffer size larger than SSIZE_MAX is implementation-defined. It does
> > make me a bit uneasy in that light to grow indefinitely.
> > 
> > Which makes me wonder whether Windows has a limit for the symlink
> > contents that we could enforce in theory so that we can reasonably turn
> > this into a bounded loop again?
> 
> https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
> suggests that the maximum permissible target path should be 32,768. But
> that's not _quite_ correct, as
> `../t/../Documentation/RelNotes/../../README.md` is a perfectly valid (if
> awkward) symlink target.
> 
> Still, I would say that 32,768 would make for a fine (still insanely high,
> but not so high as to allow malicious symlinks to cause memory problems)
> limit.
> 
> Sound good?
> Johannes

Sounds good to me, thanks!

Patrick

Copy link

@gitgitgadget gitgitgadget bot Dec 30, 2025

Choose a reason for hiding this comment

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

On the Git mailing list, Junio C Hamano wrote (reply to this):

Patrick Steinhardt <ps@pks.im> writes:

>> > This makes me wonder whether we have a better way to figure out the
>> > actual size of the buffer that we ultimately need to allocate. But
>> > reading through readlink(3p) doesn't indicate anything, and I'm not sure
>> > whether we can always rely on lstat(3p) to return the correct size for
>> > symlink contents on all platforms.
>> > 
>> > One thing that _is_ noted though is that calling the function with a
>> > buffer size larger than SSIZE_MAX is implementation-defined. It does
>> > make me a bit uneasy in that light to grow indefinitely.
>> > 
>> > Which makes me wonder whether Windows has a limit for the symlink
>> > contents that we could enforce in theory so that we can reasonably turn
>> > this into a bounded loop again?
>> 
>> https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
>> suggests that the maximum permissible target path should be 32,768. But
>> that's not _quite_ correct, as
>> `../t/../Documentation/RelNotes/../../README.md` is a perfectly valid (if
>> awkward) symlink target.
>> 
>> Still, I would say that 32,768 would make for a fine (still insanely high,
>> but not so high as to allow malicious symlinks to cause memory problems)
>> limit.
>> 
>> Sound good?
>> Johannes
>
> Sounds good to me, thanks!

As this is a generic codepath in strbuf.c, platforms that do not
honor Microsoft's promise cited above can break the assumption made
here by going beyond 32k, no?

I am OK if this infinite loop had our own "we are growing the buffer
very long and still getting not-enough-buf error; let's give up"
termination condition.

IOW, a simpler alternative may be

---- >8 ----
Subject: strbuf_readlink(): do not trust PATH_MAX

We have been bitten before by platforms that sets PATH_MAX way too
low, far below the length of paths they comfortably support.  The
strbuf_readlink() limits the link targets to PATH_MAX, which is a
code path that is broken by such platforms.

Raise the limit to 32kB, which matches the limit of a
platform with such a problem [*].

 * https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation

 strbuf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git c/strbuf.c w/strbuf.c
index 7fb7d12ac0..1c7659bcd2 100644
--- c/strbuf.c
+++ w/strbuf.c
@@ -566,7 +566,11 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f)
 	return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
 }
 
-#define STRBUF_MAXLINK (2*PATH_MAX)
+/*
+ * Do not use PATH_MAX, as some platforms sets it too low;
+ * 32kB matches what Windows has as the real limit for a pathnname.
+ */
+#define STRBUF_MAXLINK (2 * (1 << 15))
 
 int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
 {

Copy link

@gitgitgadget gitgitgadget bot Dec 17, 2025

Choose a reason for hiding this comment

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

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Karsten Blees via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Karsten Blees <blees@dcon.de>
>
> The `strbuf_readlink()` function refuses to read link targets that
> exceed PATH_MAX (even if a sufficient size was specified by the caller).
>
> As some platforms (*cough* Windows *cough*) support longer paths, remove
> this restriction (similar to `strbuf_getcwd()`).
>
> Signed-off-by: Karsten Blees <blees@dcon.de>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  strbuf.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

We've been bitten before by platforms that sets PATH_MAX too low
(i.e., lower than what they comfortably support), so this is a
welcome change.

> diff --git a/strbuf.c b/strbuf.c
> index 44a8f6a554..fa4e30f112 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -566,8 +566,6 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f)
>  	return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
>  }
>  
> -#define STRBUF_MAXLINK (2*PATH_MAX)
> -
>  int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
>  {
>  	size_t oldalloc = sb->alloc;
> @@ -575,7 +573,7 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
>  	if (hint < 32)
>  		hint = 32;
>  
> -	while (hint < STRBUF_MAXLINK) {
> +	for (;;) {
>  		ssize_t len;
>  
>  		strbuf_grow(sb, hint + 1);

I briefly wondered if this would cause us loop infinitely on a truly
broken platform, where readlink() somehow keeps returning negative,
but we only retry when we got ERANGE (which can be seen several
lines below the postimage of hte patch), so we should be safe.

Thanks.

}

#define STRBUF_MAXLINK (2*PATH_MAX)
#define STRBUF_MAXLINK (32767)

int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
{
Expand All @@ -578,12 +578,12 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
while (hint < STRBUF_MAXLINK) {
ssize_t len;

strbuf_grow(sb, hint);
len = readlink(path, sb->buf, hint);
strbuf_grow(sb, hint + 1);
len = readlink(path, sb->buf, hint + 1);
if (len < 0) {
if (errno != ERANGE)
break;
} else if (len < hint) {
} else if (len <= hint) {
strbuf_setlen(sb, len);
return 0;
}
Expand Down
Loading