Re: [PATCH 2/2] Remember and use GIT_EXEC_PATH on exec()'s
From: Michal Ostrowski <hidden>
Date: 2016-06-15 22:42:16
Subsystem:
kernel build + files below scripts/ (unless maintained elsewhere), the rest · Maintainers:
Nathan Chancellor, Nicolas Schier, Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH 2/2] Remember and use GIT_EXEC_PATH on exec()'s · Michal Ostrowski <hidden>
- 2016-06-15 · Re: [PATCH 2/2] Remember and use GIT_EXEC_PATH on exec()'s · Michal Ostrowski <hidden>
- 2016-06-15 · Re: [PATCH 2/2] Remember and use GIT_EXEC_PATH on exec()'s · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH 2/2] Remember and use GIT_EXEC_PATH on exec()'s · Andreas Ericsson <hidden>
- 2016-06-15 · Re: [PATCH 2/2] Remember and use GIT_EXEC_PATH on exec()'s · Junio C Hamano <hidden>
On Mon, 2006-01-09 at 18:52 -0800, Junio C Hamano wrote:
Michal Ostrowski [off-list ref] writes:quoted
Calls to git_setup_exec_path() are inserted on paths that will execute other git programs. git_setup_exec_path() will ensure that the git installation directories are in the path.About fetch-clone.c (which is shared by fetch-pack and clone-pack), it runs "git-index-pack" from finish_pack and "git-unpack-objects" from unpack_pack, so spelling these exec with execlp("git", "git", "index-pack", ...) might be cleaner, since "git" is required to be in users' PATH even though git-* may be moved out of the PATH in later versions of git. I dunno... In send-pack.c, I wonder why you didn't do a setup_exec_path() at the beginning of main() instead of having two calls close to exec*() call site. The same comment applies for run-command.c; you do it once for each child, but calling it once at the beginning of receive-pack would be good enough. The same thing for daemon.c. I suspect you are trying to limit the extent of damage, but I do not think of a downside if we just call setup_exec_path() once at the beginning of main(). $GIT_EXEC_PATH _could_ have a private copy of broken "diff" to confuse diff-* family, but you cannot say "git diff" in such a setup anyway because "git" does the PATH prefixing already, so it would be a moot point.
I'm not actually happy with the idea of mucking around with PATH, even within git.c. Hence I tried to only change PATH if the code had already committed to an exec. An approach that I think is better is to require all exec's of git programs from within git programs to use a specific git interface, rather than letting each one set up it's own exec parameters. Once you have that implemented, we can have a separate discussion of how the executable is to be found; - should we use PATH? - should we change PATH? - should we always exec using an absolute file name? (my preference) If a user invokes /home/user/bin/git-foo, and git-foo wants to call git-bar, is it legitimate for git-foo to call /usr/local/bin/git-bar, or should it require /home/user/bin/git-bar? Should the same rules be applied to the shell scripts? (In which case we'd want to do something like s:git-:$(GIT_EXEC_PATH)/git-:g.) Anyways, it may just be easier to show this in C (patch below). -- Michal Ostrowski [off-list ref]
diff --git a/Makefile b/Makefile
index c9c15b5..db57858 100644
--- a/Makefile
+++ b/Makefile@@ -173,7 +173,7 @@ DIFF_OBJS = \ LIB_OBJS = \ blob.o commit.o connect.o count-delta.o csum-file.o \ - date.o diff-delta.o entry.o ident.o index.o \ + date.o diff-delta.o entry.o exec_cmd.o ident.o index.o \ object.o pack-check.o patch-delta.o path.o pkt-line.o \ quote.o read-cache.o refs.o run-command.o \ server-info.o setup.o sha1_file.o sha1_name.o strbuf.o \
@@ -184,6 +184,16 @@ LIB_OBJS = \ LIBS = $(LIB_FILE) LIBS += -lz + +# .exec_cmd.bindir stores $(bindir) used to compile exec_cmd.o +# If it has changed, store the new value and force exec_cmd.o to be
rebuilt +ifneq ($(shell cat .exec_cmd.bindir 2>/dev/null),$(bindir)) +.PHONY: exec_cmd.c +$(shell echo $(bindir) > .exec_cmd.bindir) +endif + +exec_cmd.o: CFLAGS+=-DGIT_EXEC_PATH=\"$(bindir)\" + # Shell quote; # Result of this needs to be placed inside '' shq = $(subst ','\'',$(1))
diff --git a/daemon.c b/daemon.c
index 3bd1426..ab793bd 100644
--- a/daemon.c
+++ b/daemon.c@@ -9,6 +9,7 @@ #include <syslog.h> #include "pkt-line.h" #include "cache.h" +#include "exec_cmd.h" static int log_syslog; static int verbose;
@@ -227,7 +228,7 @@ static int upload(char *dir) snprintf(timeout_buf, sizeof timeout_buf, "--timeout=%u", timeout); /* git-upload-pack only ever reads stuff, so this is safe */ - execlp("git-upload-pack", "git-upload-pack", "--strict", timeout_buf,
".", NULL);
+ exec_git_cmd("upload-pack", "--strict", timeout_buf, ".");
return -1;
}
diff --git a/fetch-clone.c b/fetch-clone.c
index f46fe6e..5ae9bda 100644
--- a/fetch-clone.c
+++ b/fetch-clone.c@@ -1,4 +1,5 @@ #include "cache.h" +#include "exec_cmd.h" #include <sys/wait.h> static int finish_pack(const char *pack_tmp_name, const char *me)
@@ -27,8 +28,7 @@ static int finish_pack(const char *pack_ dup2(pipe_fd[1], 1); close(pipe_fd[0]); close(pipe_fd[1]); - execlp("git-index-pack","git-index-pack", - "-o", idx, pack_tmp_name, NULL); + exec_git_cmd("index-pack", "-o", idx, pack_tmp_name); error("cannot exec git-index-pack <%s> <%s>", idx, pack_tmp_name); exit(1);
@@ -105,8 +105,7 @@ int receive_unpack_pack(int fd[2], const dup2(fd[0], 0); close(fd[0]); close(fd[1]); - execlp("git-unpack-objects", "git-unpack-objects", - quiet ? "-q" : NULL, NULL); + exec_git_cmd("unpack-objects", quiet ? "-q" : NULL); die("git-unpack-objects exec failed"); } close(fd[0]);
diff --git a/git.c b/git.c
index 5e7da74..a45dc54 100644
--- a/git.c
+++ b/git.c@@ -10,6 +10,7 @@ #include <stdarg.h> #include <sys/ioctl.h> #include "git-compat-util.h" +#include "exec_cmd.h" #ifndef PATH_MAX # define PATH_MAX 4096
@@ -192,25 +193,6 @@ static void cmd_usage(const char *exec_p exit(1); } -static void prepend_to_path(const char *dir, int len) -{ - char *path, *old_path = getenv("PATH"); - int path_len = len; - - if (!old_path) - old_path = "/usr/local/bin:/usr/bin:/bin"; - - path_len = len + strlen(old_path) + 1; - - path = malloc(path_len + 1); - - memcpy(path, dir, len); - path[len] = ':'; - memcpy(path + len + 1, old_path, path_len - len); - - setenv("PATH", path, 1); -} - static void show_man_page(char *git_cmd) { char *page;
@@ -233,14 +215,10 @@ int main(int argc, char **argv, char **e { char git_command[PATH_MAX + 1]; char wd[PATH_MAX + 1]; - int i, len, show_help = 0; - char *exec_path = getenv("GIT_EXEC_PATH"); + int i, show_help = 0; getcwd(wd, PATH_MAX); - if (!exec_path) - exec_path = GIT_EXEC_PATH; - for (i = 1; i < argc; i++) { char *arg = argv[i];
@@ -257,9 +235,9 @@ int main(int argc, char **argv, char **e if (!strncmp(arg, "exec-path", 9)) { arg += 9; if (*arg == '=') - exec_path = arg + 1; + git_set_exec_path(arg + 1); else { - puts(exec_path); + puts(git_exec_path()); exit(0); } }
@@ -275,48 +253,18 @@ int main(int argc, char **argv, char **e if (i >= argc || show_help) { if (i >= argc) - cmd_usage(exec_path, NULL); + cmd_usage(git_exec_path(), NULL); show_man_page(argv[i]); } - if (*exec_path != '/') { - if (!getcwd(git_command, sizeof(git_command))) { - fprintf(stderr, - "git: cannot determine current directory\n"); - exit(1); - } - len = strlen(git_command); - - /* Trivial cleanup */ - while (!strncmp(exec_path, "./", 2)) { - exec_path += 2; - while (*exec_path == '/') - exec_path++; - } - snprintf(git_command + len, sizeof(git_command) - len, - "/%s", exec_path); - } - else - strcpy(git_command, exec_path); - len = strlen(git_command); - prepend_to_path(git_command, len); - - len += snprintf(git_command + len, sizeof(git_command) - len, - "/git-%s", argv[i]); - if (sizeof(git_command) <= len) { - fprintf(stderr, "git: command name given is too long.\n"); - exit(1); - } - - /* execve() can only ever return if it fails */ - execve(git_command, &argv[i], envp); + execv_git_cmd(argv + i); if (errno == ENOENT) - cmd_usage(exec_path, "'%s' is not a git-command", argv[i]); + cmd_usage(git_exec_path(), "'%s' is not a git-command", + argv[i]); fprintf(stderr, "Failed to run command '%s': %s\n", git_command, strerror(errno)); - return 1; }
diff --git a/receive-pack.c b/receive-pack.c
index f847ec2..8e78e32 100644
--- a/receive-pack.c
+++ b/receive-pack.c@@ -257,7 +257,7 @@ static void read_head_info(void) static const char *unpack(int *error_code) { - int code = run_command(unpacker, NULL); + int code = run_command_v_opt(1, &unpacker, RUN_GIT_CMD); *error_code = 0; switch (code) {
diff --git a/run-command.c b/run-command.c
index 8bf5922..b3d287e 100644
--- a/run-command.c
+++ b/run-command.c@@ -1,6 +1,7 @@ #include "cache.h" #include "run-command.h" #include <sys/wait.h> +#include "exec_cmd.h" int run_command_v_opt(int argc, char **argv, int flags) {
@@ -13,9 +14,13 @@ int run_command_v_opt(int argc, char **a int fd = open("/dev/null", O_RDWR); dup2(fd, 0); dup2(fd, 1); - close(fd); + close(fd); + } + if (flags & RUN_GIT_CMD) { + execv_git_cmd(argv); + } else { + execvp(argv[0], (char *const*) argv); } - execvp(argv[0], (char *const*) argv); die("exec %s failed.", argv[0]); } for (;;) {
diff --git a/run-command.h b/run-command.h
index 2469eea..ef3ee05 100644
--- a/run-command.h
+++ b/run-command.h@@ -12,7 +12,7 @@ enum { }; #define RUN_COMMAND_NO_STDIO 1 - +#define RUN_GIT_CMD 2 /*If this is to be git sub-command */ int run_command_v_opt(int argc, char **argv, int opt); int run_command_v(int argc, char **argv); int run_command(const char *cmd, ...);
diff --git a/shell.c b/shell.c
index cd31618..0d4891f 100644
--- a/shell.c
+++ b/shell.c@@ -12,7 +12,7 @@ static int do_generic_cmd(const char *me my_argv[1] = arg; my_argv[2] = NULL; - return execvp(me, (char**) my_argv); + return execv_git_cmd((char**) my_argv); } static struct commands {
diff --git a/upload-pack.c b/upload-pack.c
index 1834b6b..6602d68 100644
--- a/upload-pack.c
+++ b/upload-pack.c@@ -4,6 +4,7 @@ #include "tag.h" #include "object.h" #include "commit.h" +#include "exec_cmd.h" static const char upload_pack_usage[] = "git-upload-pack [--strict]
[--timeout=nn] <dir>";
@@ -60,7 +61,7 @@ static void create_pack_file(void) close(0); close(fd[0]); close(fd[1]); - *p++ = "git-rev-list"; + *p++ = "rev-list"; *p++ = "--objects"; if (create_full_pack || MAX_NEEDS <= nr_needs) *p++ = "--all";
@@ -79,13 +80,13 @@ static void create_pack_file(void) buf += 41; } *p++ = NULL; - execvp("git-rev-list", argv); + execv_git_cmd(argv); die("git-upload-pack: unable to exec git-rev-list"); } dup2(fd[0], 0); close(fd[0]); close(fd[1]); - execlp("git-pack-objects", "git-pack-objects", "--stdout", NULL); + exec_git_cmd("pack-objects", "--stdout"); die("git-upload-pack: unable to exec git-pack-objects"); }