Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
On Sun, Jul 20, 2014 at 10:00:41AM +0200, René Scharfe wrote: > -- >8 -- > Subject: [PATCH] unix-socket: remove stale socket before calling chdir() > > unix_stream_listen() is given a path. It calls unix_sockaddr_init(), > which in turn can call chdir(). After that a relative path doesn't > mean the same as before. Any use of the original path should thus > happen before that call. For that reason, unlink the given path > (to get rid of a possibly existing stale socket) right at the > beginning of the function. Thanks, I think this ordering problem was just missed in 1eb10f4 (unix-socket: handle long socket pathnames, 2012-01-09). Your solution looks OK, though I think also just using: unlink(sa.sun_path); would work, too (that is the path we are feeding to bind(), whether we have chdir'd or not, so perhaps it is a little more obviously correct?). I'm OK with either. > diff --git a/unix-socket.c b/unix-socket.c > index 01f119f..91bd6b8 100644 > --- a/unix-socket.c > +++ b/unix-socket.c > @@ -99,11 +99,12 @@ int unix_stream_listen(const char *path) > struct sockaddr_un sa; > struct unix_sockaddr_context ctx; > > + unlink(path); > + > if (unix_sockaddr_init(&sa, path, &ctx) < 0) > return -1; > fd = unix_stream_socket(); > > - unlink(path); I briefly wondered if this should be unlinking only when we get EEXIST, but I don't think it is worth caring about. The only caller is credential-cache, and it always wants to unconditionally replace whatever is there (it will already have tried to contact any existing socket). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
Am 20.07.2014 01:55, schrieb Karsten Blees: Am 18.07.2014 13:32, schrieb René Scharfe: Am 18.07.2014 01:03, schrieb Karsten Blees: Am 17.07.2014 19:05, schrieb René Scharfe: Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy: [...] "These routines have traditionally been used by programs to save the name of a working directory for the purpose of returning to it. A much faster and less error-prone method of accomplishing this is to open the current directory (.) and use the fchdir(2) function to return." fchdir() is part of the POSIX-XSI extension, as is realpath(). So why not use realpath() directly (which would also be thread-safe)? That's a good question; thanks for stepping back and looking at the bigger picture. If there is widespread OS support for a functionality then we should use it and just provide a compatibility implementation for those platforms lacking it. The downside is that compat code gets less testing. I just noticed that in contrast to the POSIX realpath(), our real_path() doesn't require the last path component to exist. I don't know if this property is required by the calling code, though. If it's important then removing the last component on ENOENT, calling realpath() again and appending it should handle that, right? Seeing that readlink() You mean realpath()? We don't have a stub for that yet. is left as a stub in compat/mingw.h that only errors out, would the equivalent function on Windows be PathCanonicalize (http://msdn.microsoft.com/en-us/library/windows/desktop/bb773569%28v=vs.85%29.aspx)? PathCanonicalize() doesn't return an absolute path, the realpath() equivalent would be GetFullPathName() (doesn't resolve symlinks) or GetFinalPathNameByHandle() (requires Vista, resolves symlinks, requires the path to exist). I meant readlink() and assumed its ENOSYS stub in compat/mingw.h means that git doesn't handle symlinks on Windows at all. However, a POSIX conformant getcwd must fail with ERANGE if the buffer is too small. So a better alternative would be to add a strbuf_getcwd() that works similar to strbuf_readlink() (i.e. resize the buffer until its large enough). That's a good idea anyway, will send patches. Side note: the 'hard' 260 limit for the current directory also means that as long as we *simulate* realpath() via chdir()/getcwd(), long paths [1] don't work here. Great, so that motivation for getting a chdir()-free implementation remains. :) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
Am 20.07.2014 02:29, schrieb Karsten Blees: > unix-socket.c: This looks pretty broken. The cd / cd back logic is only > ever used if the socket path is too long. In this case, after cd'ing to > the parent directory of the socket, unix_stream_listen tries to unlink > the *original* socket path, instead of the remaining socket basename in > sockaddr_un.sun_path. I.e. the subsequent bind() will fail on second > invocation of the credential daemon. -- >8 -- Subject: [PATCH] unix-socket: remove stale socket before calling chdir() unix_stream_listen() is given a path. It calls unix_sockaddr_init(), which in turn can call chdir(). After that a relative path doesn't mean the same as before. Any use of the original path should thus happen before that call. For that reason, unlink the given path (to get rid of a possibly existing stale socket) right at the beginning of the function. Noticed-by: Karsten Blees Signed-off-by: Rene Scharfe --- unix-socket.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/unix-socket.c b/unix-socket.c index 01f119f..91bd6b8 100644 --- a/unix-socket.c +++ b/unix-socket.c @@ -99,11 +99,12 @@ int unix_stream_listen(const char *path) struct sockaddr_un sa; struct unix_sockaddr_context ctx; + unlink(path); + if (unix_sockaddr_init(&sa, path, &ctx) < 0) return -1; fd = unix_stream_socket(); - unlink(path); if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) goto fail; -- 2.0.2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
Am 18.07.2014 12:49, schrieb Duy Nguyen: > can be used, else we fall back to chdir. I think there are only four > places that follow this pattern, here, setup.c (.git discovery), git.c > (restore_env) and unix-socket.c. Enough call sites to make it worth > the effort? > real_path(): here we actually don't want to cd / cd back, we just do this because getcwd reolves symlinks. setup.c: AFAICT this sets up the work dir and stays there (no cd back). git.c: Only does a 'preliminary' repo setup so that the alias mechanism reads the correct config files. Then it reverts the setup if the resulting git command doesn't need it. Perhaps it would be better (and faster) to teach the alias code to read the right config file in the first place? unix-socket.c: This looks pretty broken. The cd / cd back logic is only ever used if the socket path is too long. In this case, after cd'ing to the parent directory of the socket, unix_stream_listen tries to unlink the *original* socket path, instead of the remaining socket basename in sockaddr_un.sun_path. I.e. the subsequent bind() will fail on second invocation of the credential daemon. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
Am 18.07.2014 13:32, schrieb René Scharfe:
> Am 18.07.2014 01:03, schrieb Karsten Blees:
>> Am 17.07.2014 19:05, schrieb René Scharfe:
>>> Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:
>> [...]
>>> "These routines have traditionally been used by programs to save the
>>> name of a working directory for the purpose of returning to it. A much
>>> faster and less error-prone method of accomplishing this is to open the
>>> current directory (.) and use the fchdir(2) function to return."
>>>
>>
>> fchdir() is part of the POSIX-XSI extension, as is realpath(). So why not
>> use realpath() directly (which would also be thread-safe)?
>
> That's a good question; thanks for stepping back and looking at the bigger
> picture. If there is widespread OS support for a functionality then we
> should use it and just provide a compatibility implementation for those
> platforms lacking it. The downside is that compat code gets less testing.
>
I just noticed that in contrast to the POSIX realpath(), our real_path()
doesn't require the last path component to exist. I don't know if this property
is required by the calling code, though.
> Seeing that readlink()
You mean realpath()? We don't have a stub for that yet.
> is left as a stub in compat/mingw.h that only errors out, would the
> equivalent function on Windows be PathCanonicalize
> (http://msdn.microsoft.com/en-us/library/windows/desktop/bb773569%28v=vs.85%29.aspx)?
>
PathCanonicalize() doesn't return an absolute path, the realpath() equivalent
would be GetFullPathName() (doesn't resolve symlinks) or
GetFinalPathNameByHandle() (requires Vista, resolves symlinks, requires the
path to exist).
>> For non-XSI-compliant platforms, we could keep the current implementation.
>
> OK, so realpath() for Linux and the BSDs, mingw_realpath() wrapping
> PathCanonicalize() for Windows and the current code for the rest?
>
>> Or re-implement a thread-safe version, e.g. applying resolve_symlink() from
>> lockfile.c to all path components.
>
> Thread safety sounds good. We'd also need something like
> normalize_path_copy() but without the conversion of backslashes to slashes,
> in order to get rid of "." and ".." path components and something like
> absolute_path() that doesn't die on error, no?
>
Windows can handle forward slashes, so normalize_path_copy works just fine.
>> If I may bother you with the Windows point of view:
>>
>> There is no fchdir(), and I'm pretty sure open(".") won't work either.
>
> On Windows, there *is* an absolute path length limit of 260 in the normal
> case and a bit more than 32000 for some functions using the \\?\ namespace.
> So one could get away with using a constant-sized buffer for a "remember the
> place and return later" function here.
>
The current directory is pretty much the only exception to the \\?\ trick [1].
So a fixed buffer for getcwd() would actually be fine on Windows (although it
would have to be 3 * PATH_MAX, as PATH_MAX wide chars will convert to at most 3
* PATH_MAX UTF-8 chars).
However, a POSIX conformant getcwd must fail with ERANGE if the buffer is too
small. So a better alternative would be to add a strbuf_getcwd() that works
similar to strbuf_readlink() (i.e. resize the buffer until its large enough).
Side note: the 'hard' 260 limit for the current directory also means that as
long as we *simulate* realpath() via chdir()/getcwd(), long paths [1] don't
work here.
> Also, _getcwd can be asked to allocate an appropriately-sized buffer for use,
> like GNU's get_current_dir_name, by specifying NULL as its first parameter
> (http://msdn.microsoft.com/en-us/library/sf98bd4y.aspx).
>
We use nedmalloc in the Windows builds, so unfortuately we cannot free memory
allocated by MSVCRT.dll.
[1]
http://msdn.microsoft.com/en-us/library/windows/desktop/aa365530%28v=vs.85%29.aspx
[2] https://github.com/msysgit/git/commit/84393750
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
On Fri, Jul 18, 2014 at 10:08 PM, René Scharfe wrote: >> We could wrap this "get cwd, cd back" pattern as well. So "save_cwd" >> returns an opaque pointer, "go_back" takes the pointer, (f)chdir back >> and free the pointer if needed. On platforms that support fchdir, it >> can be used, else we fall back to chdir. I think there are only four >> places that follow this pattern, here, setup.c (.git discovery), git.c >> (restore_env) and unix-socket.c. Enough call sites to make it worth >> the effort? > > On a cursory look I didn't find any more potential users. Something > like this? I think it looks nice. > Applying it to setup.c looks difficult to impossible, though. Yeah, setup.c needs cwd as a string because it does something else to cwd besides going back. Too bad. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
Karsten Blees writes: > Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy: >> e.g. "git init". Make it static too to reduce stack usage. > > But wouldn't this increase overall memory usage? Stack memory > will be reused by subsequent code, while static memory cannot > be reused (but still increases the process working set). Correct. And this is a function that is not involved in deep recursion, so avoiding stack allocation seems quite misguided. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
Am 18.07.2014 12:49, schrieb Duy Nguyen:
> On Fri, Jul 18, 2014 at 6:03 AM, Karsten Blees
> wrote:
>> Am 17.07.2014 19:05, schrieb René Scharfe:
>>> Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:
>> [...]
>>> "These routines have traditionally been used by programs to save the
>>> name of a working directory for the purpose of returning to it. A much
>>> faster and less error-prone method of accomplishing this is to open the
>>> current directory (.) and use the fchdir(2) function to return."
>>>
>>
>> fchdir() is part of the POSIX-XSI extension, as is realpath(). So why not
>> use realpath() directly (which would also be thread-safe)?
>
> But realpath still needs a given buffer (of PATH_MAX at least again).
> Unless you pass a NULL pointer as "resolved_name", then Linux can
> allocate the buffer but that's implementation specific [1]. I guess we
> can make a wrapper "git_getcwd" that returns a new buffer. The default
> implementation returns one of PATH_MAX chars, certain platforms like
> Linux can use realpath (or something else) to return a longer path?
>
> [1] http://pubs.opengroup.org/onlinepubs/009695399/functions/realpath.html
realpath() can be used to implement the whole of real_path_internal(),
IIUC, so there would be no need to change the current directory anymore.
The realpath(3) manpage for Linux mentions that behaviour of realpath()
with resolved_path being NULL is not standardized by POSIX.1-2001 but
by POSIX.1-2008. At least Linux, OpenBSD and FreeBSD seem to support
that, based on their manpages. Here's the standard doc:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/realpath.html
>> For non-XSI-compliant platforms, we could keep the current implementation.
>> Or re-implement a thread-safe version, e.g. applying resolve_symlink() from
>> lockfile.c to all path components.
>>
>>
>> If I may bother you with the Windows point of view:
>>
>> There is no fchdir(), and I'm pretty sure open(".") won't work either.
>>
>> fchdir() could be emulated using GetFileInformationByHandleEx(FileNameInfo).
>> realpath() is pretty much what GetFinalPathNameByHandle() does. However,
>> both of these APIs require Windows Vista.
>>
>> Opening a handle to a directory can be done using FILE_FLAG_BACKUP_SEMANTICS,
>> which AFAICT MSVCRT.dll's open() implementation does _not_ do (could be
>> emulated in our mingw_open() wrapper, though).
>>
>> ...lots of work for little benefit, I would think.
>>
>
> We could wrap this "get cwd, cd back" pattern as well. So "save_cwd"
> returns an opaque pointer, "go_back" takes the pointer, (f)chdir back
> and free the pointer if needed. On platforms that support fchdir, it
> can be used, else we fall back to chdir. I think there are only four
> places that follow this pattern, here, setup.c (.git discovery), git.c
> (restore_env) and unix-socket.c. Enough call sites to make it worth
> the effort?
On a cursory look I didn't find any more potential users. Something
like this? Applying it to setup.c looks difficult to impossible,
though.
---
Makefile | 8
abspath.c | 10 ++
cache.h | 1 +
git.c | 9 +
save-cwd-fchdir.c | 25 +
save-cwd.c| 22 ++
save-cwd.h| 3 +++
unix-socket.c | 11 ++-
8 files changed, 76 insertions(+), 13 deletions(-)
create mode 100644 save-cwd-fchdir.c
create mode 100644 save-cwd.c
create mode 100644 save-cwd.h
diff --git a/Makefile b/Makefile
index 840057c..ecf3129 100644
--- a/Makefile
+++ b/Makefile
@@ -32,6 +32,8 @@ all::
#
# Define HAVE_ALLOCA_H if you have working alloca(3) defined in that header.
#
+# Define HAVE_FCHDIR if you have fchdir(2).
+#
# Define NO_CURL if you do not have libcurl installed. git-http-fetch and
# git-http-push are not built, and you cannot use http:// and https://
# transports (neither smart nor dumb).
@@ -1118,6 +1120,12 @@ ifdef HAVE_ALLOCA_H
BASIC_CFLAGS += -DHAVE_ALLOCA_H
endif
+ifdef HAVE_FCHDIR
+ COMPAT_OBJS += save-cwd-fchdir.o
+else
+ COMPAT_OBJS += save-cwd.o
+endif
+
ifdef NO_CURL
BASIC_CFLAGS += -DNO_CURL
REMOTE_CURL_PRIMARY =
diff --git a/abspath.c b/abspath.c
index ca33558..f49bacc 100644
--- a/abspath.c
+++ b/abspath.c
@@ -41,7 +41,7 @@ static const char *real_path_internal(const char *path, int
die_on_error)
* here so that we can chdir() back to it at the end of the
* function:
*/
- char cwd[1024] = "";
+ struct saved_cwd *cwd = NULL;
int buf_index = 1;
@@ -80,7 +80,9 @@ static const char *real_path_internal(const char *path, int
die_on_error)
}
if (*buf) {
- if (!*cwd && !getcwd(cwd, sizeof(cwd))) {
+ if (!cwd)
+ cwd = save_cwd();
+ if (!cwd) {
if (die_on_error)
Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
Am 18.07.2014 01:03, schrieb Karsten Blees:
Am 17.07.2014 19:05, schrieb René Scharfe:
Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:
[...]
"These routines have traditionally been used by programs to save the
name of a working directory for the purpose of returning to it. A much
faster and less error-prone method of accomplishing this is to open the
current directory (.) and use the fchdir(2) function to return."
fchdir() is part of the POSIX-XSI extension, as is realpath(). So why not
use realpath() directly (which would also be thread-safe)?
That's a good question; thanks for stepping back and looking at the
bigger picture. If there is widespread OS support for a functionality
then we should use it and just provide a compatibility implementation
for those platforms lacking it. The downside is that compat code gets
less testing.
Seeing that readlink() is left as a stub in compat/mingw.h that only
errors out, would the equivalent function on Windows be PathCanonicalize
(http://msdn.microsoft.com/en-us/library/windows/desktop/bb773569%28v=vs.85%29.aspx)?
For non-XSI-compliant platforms, we could keep the current implementation.
OK, so realpath() for Linux and the BSDs, mingw_realpath() wrapping
PathCanonicalize() for Windows and the current code for the rest?
Or re-implement a thread-safe version, e.g. applying resolve_symlink() from
lockfile.c to all path components.
Thread safety sounds good. We'd also need something like
normalize_path_copy() but without the conversion of backslashes to
slashes, in order to get rid of "." and ".." path components and
something like absolute_path() that doesn't die on error, no?
If I may bother you with the Windows point of view:
There is no fchdir(), and I'm pretty sure open(".") won't work either.
On Windows, there *is* an absolute path length limit of 260 in the
normal case and a bit more than 32000 for some functions using the \\?\
namespace. So one could get away with using a constant-sized buffer for
a "remember the place and return later" function here.
Also, _getcwd can be asked to allocate an appropriately-sized buffer for
use, like GNU's get_current_dir_name, by specifying NULL as its first
parameter (http://msdn.microsoft.com/en-us/library/sf98bd4y.aspx).
Not having to move around at all as mentioned above is still better, of
course.
René
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
On Fri, Jul 18, 2014 at 6:03 AM, Karsten Blees wrote:
> Am 17.07.2014 19:05, schrieb René Scharfe:
>> Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:
> [...]
>> "These routines have traditionally been used by programs to save the
>> name of a working directory for the purpose of returning to it. A much
>> faster and less error-prone method of accomplishing this is to open the
>> current directory (.) and use the fchdir(2) function to return."
>>
>
> fchdir() is part of the POSIX-XSI extension, as is realpath(). So why not
> use realpath() directly (which would also be thread-safe)?
But realpath still needs a given buffer (of PATH_MAX at least again).
Unless you pass a NULL pointer as "resolved_name", then Linux can
allocate the buffer but that's implementation specific [1]. I guess we
can make a wrapper "git_getcwd" that returns a new buffer. The default
implementation returns one of PATH_MAX chars, certain platforms like
Linux can use realpath (or something else) to return a longer path?
[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/realpath.html
> For non-XSI-compliant platforms, we could keep the current implementation.
> Or re-implement a thread-safe version, e.g. applying resolve_symlink() from
> lockfile.c to all path components.
>
>
> If I may bother you with the Windows point of view:
>
> There is no fchdir(), and I'm pretty sure open(".") won't work either.
>
> fchdir() could be emulated using GetFileInformationByHandleEx(FileNameInfo).
> realpath() is pretty much what GetFinalPathNameByHandle() does. However,
> both of these APIs require Windows Vista.
>
> Opening a handle to a directory can be done using FILE_FLAG_BACKUP_SEMANTICS,
> which AFAICT MSVCRT.dll's open() implementation does _not_ do (could be
> emulated in our mingw_open() wrapper, though).
>
> ...lots of work for little benefit, I would think.
>
We could wrap this "get cwd, cd back" pattern as well. So "save_cwd"
returns an opaque pointer, "go_back" takes the pointer, (f)chdir back
and free the pointer if needed. On platforms that support fchdir, it
can be used, else we fall back to chdir. I think there are only four
places that follow this pattern, here, setup.c (.git discovery), git.c
(restore_env) and unix-socket.c. Enough call sites to make it worth
the effort?
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
Am 17.07.2014 19:05, schrieb René Scharfe:
> Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:
[...]
> "These routines have traditionally been used by programs to save the
> name of a working directory for the purpose of returning to it. A much
> faster and less error-prone method of accomplishing this is to open the
> current directory (.) and use the fchdir(2) function to return."
>
fchdir() is part of the POSIX-XSI extension, as is realpath(). So why not
use realpath() directly (which would also be thread-safe)?
For non-XSI-compliant platforms, we could keep the current implementation.
Or re-implement a thread-safe version, e.g. applying resolve_symlink() from
lockfile.c to all path components.
If I may bother you with the Windows point of view:
There is no fchdir(), and I'm pretty sure open(".") won't work either.
fchdir() could be emulated using GetFileInformationByHandleEx(FileNameInfo).
realpath() is pretty much what GetFinalPathNameByHandle() does. However,
both of these APIs require Windows Vista.
Opening a handle to a directory can be done using FILE_FLAG_BACKUP_SEMANTICS,
which AFAICT MSVCRT.dll's open() implementation does _not_ do (could be
emulated in our mingw_open() wrapper, though).
...lots of work for little benefit, I would think.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
Am 17.07.2014 20:03, schrieb Junio C Hamano: > Nguyễn Thái Ngọc Duy writes: > >> This array 'cwd' is used to store the result from getcwd() and chdir() >> back. PATH_MAX is the right constant for the job. On systems with >> longer PATH_MAX (eg. 4096 on Linux), hard coding 1024 fails stuff, >> e.g. "git init". Make it static too to reduce stack usage. >> >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- > > Thanks. It seems that this 1024 has been with us since the > beginning of this piece of code. I briefly wondered if there are > strange platform that will have PATH_MAX shorter than 1024 that will > be hurt by this change, but the result in cwd[] is used to grow the > final result bufs[] that is sized based on PATH_MAX anyway, so it > will not be an issue (besides, the absurdly short one seems to be > a different macro, MAX_PATH, on Windows). > Indeed, there's a strange platform where PATH_MAX is only 259. With Unicode support, the current directory can be that many wide characters, which means up to 3 * PATH_MAX UTF-8 bytes (if all of them are CJK). I don't think this will be a problem, though, as the return buffer is PATH_MAX-bounded as well. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy: > e.g. "git init". Make it static too to reduce stack usage. But wouldn't this increase overall memory usage? Stack memory will be reused by subsequent code, while static memory cannot be reused (but still increases the process working set). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
René Scharfe writes:
> "These routines have traditionally been used by programs to save the
> name of a working directory for the purpose of returning to it. A much
> faster and less error-prone method of accomplishing this is to open the
> current directory (.) and use the fchdir(2) function to return."
>
> So, how about something like this?
Yeah, I've always wanted to see us use fchdir() for coming back
(another thing I wanted to see is to use openat() and friends).
I do not offhand recall if the run of chdir() in gitdir discovery
code has a similar "now we are done, let's go back to the original"
use of chdir() there, but if we do, we should fix it, too.
Looks sensible from a cursory read.
>
> ---
> abspath.c | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/abspath.c b/abspath.c
> index ca33558..7fff13a 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -38,10 +38,10 @@ static const char *real_path_internal(const char *path,
> int die_on_error)
>
> /*
>* If we have to temporarily chdir(), store the original CWD
> - * here so that we can chdir() back to it at the end of the
> + * here so that we can fchdir() back to it at the end of the
>* function:
>*/
> - char cwd[1024] = "";
> + int cwd_fd = -1;
>
> int buf_index = 1;
>
> @@ -80,7 +80,9 @@ static const char *real_path_internal(const char *path, int
> die_on_error)
> }
>
> if (*buf) {
> - if (!*cwd && !getcwd(cwd, sizeof(cwd))) {
> + if (cwd_fd < 0)
> + cwd_fd = open(".", O_RDONLY);
> + if (cwd_fd < 0) {
> if (die_on_error)
> die_errno("Could not get current
> working directory");
> else
> @@ -142,8 +144,11 @@ static const char *real_path_internal(const char *path,
> int die_on_error)
> retval = buf;
> error_out:
> free(last_elem);
> - if (*cwd && chdir(cwd))
> - die_errno("Could not change back to '%s'", cwd);
> + if (cwd_fd >= 0) {
> + if (fchdir(cwd_fd))
> + die_errno("Could not change back to the original
> working directory");
> + close(cwd_fd);
> + }
>
> return retval;
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
Nguyễn Thái Ngọc Duy writes: > This array 'cwd' is used to store the result from getcwd() and chdir() > back. PATH_MAX is the right constant for the job. On systems with > longer PATH_MAX (eg. 4096 on Linux), hard coding 1024 fails stuff, > e.g. "git init". Make it static too to reduce stack usage. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- Thanks. It seems that this 1024 has been with us since the beginning of this piece of code. I briefly wondered if there are strange platform that will have PATH_MAX shorter than 1024 that will be hurt by this change, but the result in cwd[] is used to grow the final result bufs[] that is sized based on PATH_MAX anyway, so it will not be an issue (besides, the absurdly short one seems to be a different macro, MAX_PATH, on Windows). Will queue. > abspath.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/abspath.c b/abspath.c > index ca33558..c0c868f 100644 > --- a/abspath.c > +++ b/abspath.c > @@ -41,7 +41,7 @@ static const char *real_path_internal(const char *path, int > die_on_error) >* here so that we can chdir() back to it at the end of the >* function: >*/ > - char cwd[1024] = ""; > + static char cwd[PATH_MAX]; > > int buf_index = 1; > > @@ -49,6 +49,8 @@ static const char *real_path_internal(const char *path, int > die_on_error) > char *last_elem = NULL; > struct stat st; > > + *cwd = '\0'; > + > /* We've already done it */ > if (path == buf || path == next_buf) > return path; -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()
Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:
> This array 'cwd' is used to store the result from getcwd() and chdir()
> back. PATH_MAX is the right constant for the job.
PATH_MAX may be better than 1024, but there can't really be a correct
constant. The actual limit depends on the file system.
> On systems with
> longer PATH_MAX (eg. 4096 on Linux), hard coding 1024 fails stuff,
> e.g. "git init".
Out of curiosity, I just created a path with over 13 characters on
Linux. It's not actually usable but it shows that 4096 is not a real
limit on Linux. Here's how I created that insane path:
a=1234567890
x=$a$a$a$a$a
y=$x$x$x$x$x
cd /tmp
while true
do
mkdir $y || break
cd $y || break
done
pwd >/tmp/y
cd /tmp
wc -c
#include
#include
#include
int main(int argc, char **argv)
{
char cwd[20];
printf("PATH_MAX = %d\n", PATH_MAX);
if (getcwd(cwd, sizeof(cwd)))
printf("strlen(getcwd()) = %zu\n", strlen(cwd));
return 0;
}
> Make it static too to reduce stack usage.
Why is that needed? Is recursion involved? (Didn't find any in the
function itself after a very brief look.)
There is get_current_dir_name(), a GNU extension that allocates the
necessary memory. Perhaps we can use it instead and provide a
compatibility implementation based on getcwd() for platforms that don't
have it?
But then there's also this advice from the getcwd(3) manpage on OpenBSD:
"These routines have traditionally been used by programs to save the
name of a working directory for the purpose of returning to it. A much
faster and less error-prone method of accomplishing this is to open the
current directory (.) and use the fchdir(2) function to return."
So, how about something like this?
---
abspath.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/abspath.c b/abspath.c
index ca33558..7fff13a 100644
--- a/abspath.c
+++ b/abspath.c
@@ -38,10 +38,10 @@ static const char *real_path_internal(const char *path, int
die_on_error)
/*
* If we have to temporarily chdir(), store the original CWD
-* here so that we can chdir() back to it at the end of the
+* here so that we can fchdir() back to it at the end of the
* function:
*/
- char cwd[1024] = "";
+ int cwd_fd = -1;
int buf_index = 1;
@@ -80,7 +80,9 @@ static const char *real_path_internal(const char *path, int
die_on_error)
}
if (*buf) {
- if (!*cwd && !getcwd(cwd, sizeof(cwd))) {
+ if (cwd_fd < 0)
+ cwd_fd = open(".", O_RDONLY);
+ if (cwd_fd < 0) {
if (die_on_error)
die_errno("Could not get current
working directory");
else
@@ -142,8 +144,11 @@ static const char *real_path_internal(const char *path,
int die_on_error)
retval = buf;
error_out:
free(last_elem);
- if (*cwd && chdir(cwd))
- die_errno("Could not change back to '%s'", cwd);
+ if (cwd_fd >= 0) {
+ if (fchdir(cwd_fd))
+ die_errno("Could not change back to the original
working directory");
+ close(cwd_fd);
+ }
return retval;
}
--
2.0.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
