threads / patch / 42035

patchExtend runtime prefix computation

Subject: [PATCH] Extend runtime prefix computation

## tl;dr

4 messages between Apr 15, 2016 and Apr 20, 2016. Diffs are folded; open one to read it.

replies: 3people: 4as markdown or json

Michael Weiser· Apr 15, 2016, 14:30 UTC · lore

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 <David.Abdurachmanov@cern.ch>
    Pull-Request: https://github.com/git/git/pull/224
 exec_cmd.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 58 insertions(+), 14 deletions(-)
Show changes to exec_cmd.c +58 −14
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 <mach-o/dyld.h>
+#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· Apr 15, 2016, 16:43 UTC · re: Michael Weiser · lore

Re: [PATCH] Extend runtime prefix computation

Michael Weiser <michael.weiser@gmx.de> writes:
Show 5 quoted lines
> 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.
Show 11 quoted lines
> 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 <mach-o/dyld.h>
> +#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?

Show 15 quoted lines
>  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?
Show 81 quoted lines
> -	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· Apr 18, 2016, 07:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] Extend runtime prefix computation

Hi Junio,
On Fri, 15 Apr 2016, Junio C Hamano wrote:
Show 12 quoted lines
> Michael Weiser <michael.weiser@gmx.de> 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.

Show 7 quoted lines
> >  #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· Apr 20, 2016, 17:52 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Extend runtime prefix computation

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

← back to recent threads