# [PATCH] Extend runtime prefix computation 4 messages from 2016-04-15 to 2016-04-20. Participants: Michael Weiser, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/42035 ## Michael Weiser, 2016-04-15 14:30 Subject: [PATCH] Extend runtime prefix computation Message-ID: <20160415143001.GA67437@dinsnail.net> URL: https://gitlist.dev/e/20160415143001.GA67437%40dinsnail.net ``` Make git fully relocatable at runtime extending the runtime prefix calculation. Handle absolute and relative paths in argv0. Handle no path at all in argv0 in a system-specific manner. Replace assertions with initialised variables and checks that lead to fallback to the static prefix. --- Notes: Tested-by: David Abdurachmanov Pull-Request: https://github.com/git/git/pull/224 exec_cmd.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 58 insertions(+), 14 deletions(-) diff --git a/exec_cmd.c b/exec_cmd.c index 9d5703a..f0db070 100644 --- a/exec_cmd.c +++ b/exec_cmd.c @@ -3,30 +3,27 @@ #include "quote.h" #include "argv-array.h" #define MAX_ARGS 32 +#if defined(__APPLE__) +#include +#endif static const char *argv_exec_path; static const char *argv0_path; char *system_path(const char *path) { -#ifdef RUNTIME_PREFIX - static const char *prefix; -#else static const char *prefix = PREFIX; -#endif struct strbuf d = STRBUF_INIT; if (is_absolute_path(path)) return xstrdup(path); #ifdef RUNTIME_PREFIX - assert(argv0_path); - assert(is_absolute_path(argv0_path)); - - if (!prefix && - !(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) && - !(prefix = strip_path_suffix(argv0_path, BINDIR)) && - !(prefix = strip_path_suffix(argv0_path, "git"))) { + if (!argv0_path || + !is_absolute_path(argv0_path) || + (!(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) && + !(prefix = strip_path_suffix(argv0_path, BINDIR)) && + !(prefix = strip_path_suffix(argv0_path, "git")))) { prefix = PREFIX; trace_printf("RUNTIME_PREFIX requested, " "but prefix computation failed. " @@ -41,6 +38,8 @@ char *system_path(const char *path) const char *git_extract_argv0_path(const char *argv0) { const char *slash; + char *to_resolve = NULL; + const char *resolved; if (!argv0 || !*argv0) return NULL; @@ -48,11 +47,56 @@ const char *git_extract_argv0_path(const char *argv0) slash = find_last_dir_sep(argv0); if (slash) { - argv0_path = xstrndup(argv0, slash - argv0); - return slash + 1; + /* ... it's either an absolute path */ + if (is_absolute_path(argv0)) { + argv0_path = xstrndup(argv0, slash - argv0); + return slash + 1; + } + + /* ... or a relative path, in which case we have to make it + * absolute first and do the whole thing again */ + to_resolve = xstrdup(argv0); + } else { + /* argv0 is no path at all, just a name. Resolve it into a + * path. Unfortunately, this gets system specific. */ +#if defined(__linux__) + struct stat st; + if (!stat("/proc/self/exe", &st)) + to_resolve = xstrdup("/proc/self/exe"); +#elif defined(__APPLE__) + /* call with len == 0 queries the necessary buffer size */ + uint32_t len = 0; + if(_NSGetExecutablePath(NULL, &len) != 0) { + to_resolve = malloc(len); + if (to_resolve) { + /* get the executable path now we have a buffer + * of proper size */ + if(_NSGetExecutablePath(to_resolve, &len) != 0) { + free(to_resolve); + return argv0; + } + } + } +#endif + + /* if to_resolve is still NULL here, something failed above or + * we are on an unsupported system. system_path() will warn + * and fall back to the static prefix */ + if (!to_resolve) + return argv0; } - return argv0; + /* resolve path from absolute to canonical */ + resolved = real_path(to_resolve); + free(to_resolve); + + slash = find_last_dir_sep(resolved); + if (!slash) + return argv0; + + argv0_path = xstrndup(resolved, slash - resolved); + slash = xstrdup(slash + 1); + return slash; } void git_set_argv_exec_path(const char *exec_path) -- 2.6.4 (Apple Git-63) ``` ## Junio C Hamano, 2016-04-15 16:43 Subject: Re: [PATCH] Extend runtime prefix computation Message-ID: URL: https://gitlist.dev/e/xmqqfuumddqm.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160415143001.GA67437@dinsnail.net> ``` Michael Weiser writes: > Make git fully relocatable at runtime extending the runtime prefix > calculation. Handle absolute and relative paths in argv0. Handle no path > at all in argv0 in a system-specific manner. Replace assertions with > initialised variables and checks that lead to fallback to the static > prefix. That's a dense description of "what" without saying much about "why". Hint: start by describing what case(s) the current code fails to find the correct runtime prefix. That would give readers a better understanding of what problem you are trying to solve. Missing sign-off. > diff --git a/exec_cmd.c b/exec_cmd.c > index 9d5703a..f0db070 100644 > --- a/exec_cmd.c > +++ b/exec_cmd.c > @@ -3,30 +3,27 @@ > #include "quote.h" > #include "argv-array.h" > #define MAX_ARGS 32 > +#if defined(__APPLE__) > +#include > +#endif We tend to avoid system specific includes in individual *.c files. Perhaps implement platform specific bits in compat/? E.g. each platform specific file in compat/ may implement and export the same git_extract_argv_path() function, perhaps, so that this file does not even need to know what platforms it supports? > char *system_path(const char *path) > { > -#ifdef RUNTIME_PREFIX > - static const char *prefix; > -#else > static const char *prefix = PREFIX; > -#endif > struct strbuf d = STRBUF_INIT; > > if (is_absolute_path(path)) > return xstrdup(path); > > #ifdef RUNTIME_PREFIX > - assert(argv0_path); > - assert(is_absolute_path(argv0_path)); Aren't these protecting against future and careless change that forgets to call extract-argv0-path or make that function return something that is not an absolute path? Is it a good idea to drop these checks, and if so why? > - if (!prefix && > - !(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) && > - !(prefix = strip_path_suffix(argv0_path, BINDIR)) && > - !(prefix = strip_path_suffix(argv0_path, "git"))) { > + if (!argv0_path || > + !is_absolute_path(argv0_path) || > + (!(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) && > + !(prefix = strip_path_suffix(argv0_path, BINDIR)) && > + !(prefix = strip_path_suffix(argv0_path, "git")))) { > prefix = PREFIX; > trace_printf("RUNTIME_PREFIX requested, " > "but prefix computation failed. " > @@ -41,6 +38,8 @@ char *system_path(const char *path) > const char *git_extract_argv0_path(const char *argv0) > { > const char *slash; > + char *to_resolve = NULL; > + const char *resolved; > > if (!argv0 || !*argv0) > return NULL; > @@ -48,11 +47,56 @@ const char *git_extract_argv0_path(const char *argv0) > slash = find_last_dir_sep(argv0); > > if (slash) { > - argv0_path = xstrndup(argv0, slash - argv0); > - return slash + 1; > + /* ... it's either an absolute path */ > + if (is_absolute_path(argv0)) { > + argv0_path = xstrndup(argv0, slash - argv0); > + return slash + 1; > + } > + > + /* ... or a relative path, in which case we have to make it > + * absolute first and do the whole thing again */ > + to_resolve = xstrdup(argv0); > + } else { > + /* argv0 is no path at all, just a name. Resolve it into a > + * path. Unfortunately, this gets system specific. */ > +#if defined(__linux__) > + struct stat st; > + if (!stat("/proc/self/exe", &st)) > + to_resolve = xstrdup("/proc/self/exe"); > +#elif defined(__APPLE__) > + /* call with len == 0 queries the necessary buffer size */ > + uint32_t len = 0; > + if(_NSGetExecutablePath(NULL, &len) != 0) { > + to_resolve = malloc(len); > + if (to_resolve) { > + /* get the executable path now we have a buffer > + * of proper size */ > + if(_NSGetExecutablePath(to_resolve, &len) != 0) { > + free(to_resolve); > + return argv0; > + } > + } > + } > +#endif > + > + /* if to_resolve is still NULL here, something failed above or > + * we are on an unsupported system. system_path() will warn > + * and fall back to the static prefix */ > + if (!to_resolve) > + return argv0; > } > > - return argv0; > + /* resolve path from absolute to canonical */ > + resolved = real_path(to_resolve); > + free(to_resolve); > + > + slash = find_last_dir_sep(resolved); > + if (!slash) > + return argv0; > + > + argv0_path = xstrndup(resolved, slash - resolved); > + slash = xstrdup(slash + 1); > + return slash; > } > > void git_set_argv_exec_path(const char *exec_path) ``` ## Johannes Schindelin, 2016-04-18 07:20 Subject: Re: [PATCH] Extend runtime prefix computation Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1604180918500.2967%40virtualbox In-Reply-To: ``` Hi Junio, On Fri, 15 Apr 2016, Junio C Hamano wrote: > Michael Weiser writes: > > > Make git fully relocatable at runtime extending the runtime prefix > > calculation. Handle absolute and relative paths in argv0. Handle no path > > at all in argv0 in a system-specific manner. Replace assertions with > > initialised variables and checks that lead to fallback to the static > > prefix. > > That's a dense description of "what" without saying much about > "why". Hint: start by describing what case(s) the current code > fails to find the correct runtime prefix. That would give readers a > better understanding of what problem you are trying to solve. I have to admit that I am really, *really* skeptical. To me, it looks like this patch opens the door very wide to unintended consequences. > > #ifdef RUNTIME_PREFIX > > - assert(argv0_path); > > - assert(is_absolute_path(argv0_path)); > > Aren't these protecting against future and careless change that > forgets to call extract-argv0-path or make that function return > something that is not an absolute path? This (first) assert() indeed saved me a couple of times from hunting for bugs in the wrong place. Let's keep it. Ciao, Dscho ``` ## Michael Weiser, 2016-04-20 17:52 Subject: Re: [PATCH] Extend runtime prefix computation Message-ID: <20160420175207.GA28640@weiser.dinsnail.net> URL: https://gitlist.dev/e/20160420175207.GA28640%40weiser.dinsnail.net In-Reply-To: ``` Hi, On Fri, Apr 15, 2016 at 09:43:29AM -0700, Junio C Hamano wrote: > We tend to avoid system specific includes in individual *.c files. > Perhaps implement platform specific bits in compat/? E.g. each > platform specific file in compat/ may implement and export the same > git_extract_argv_path() function, perhaps, so that this file does > not even need to know what platforms it supports? On Mon, Apr 18, 2016 at 09:20:25AM +0200, Johannes Schindelin wrote: > I have to admit that I am really, *really* skeptical. To me, it looks like > this patch opens the door very wide to unintended consequences. To make it short: I had received interest from another user for relocatable git installations and an updated patch against current git (based on my last post of the code to this list in 2012). This confirmed to me that the use case is still valid. However, I do not see myself in a position to iterate the code through the review and improvement process that starting to unfold now. So if someone sometime picks this up and carries on, I'd be happy. Otherwise please disregard. -- Thanks, Michael ```