{"thread":{"id":"29163","subject":"[PATCH v2 1/4] compat/setenv.c: update errno when erroring out","startedAt":"2011-12-14T14:07:07Z","lastAt":"2011-12-15T18:25:41Z","messageCount":9,"participants":["Erik Faye-Lund","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"181130","messageId":"1323871631-2872-1-git-send-email-kusmabite@gmail.com","threadId":"29163","inReplyTo":null,"subject":"[PATCH v2 0/4] setenv/putenv errors","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-12-14T14:07:07Z","receivedAt":"2011-12-14T14:07:07Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Here's v2 of this series. It grew a bit since the last round, as\nI figured fixing the error-paths in compat/setenv.c made sense to\ndo while I was at it, considering how Windows (which is one of\nthe few likely platforms to be able to actually trigger this\nissue) use it.\n\nErik Faye-Lund (4):\n  compat/setenv.c: update errno when erroring out\n  compat/setenv.c: error if name contains '='\n  wrapper: supply xsetenv and xputenv\n  use wrapper for unchecked setenv/putenv calls\n\n builtin/clone.c      |    2 +-\n builtin/commit.c     |    6 +++---\n builtin/help.c       |    4 ++--\n builtin/init-db.c    |    2 +-\n builtin/merge.c      |    4 ++--\n builtin/notes.c      |    2 +-\n builtin/remote-ext.c |    4 ++--\n builtin/revert.c     |    2 +-\n compat/setenv.c      |   10 ++++++++--\n config.c             |    2 +-\n exec_cmd.c           |    4 ++--\n git-compat-util.h    |    2 ++\n git.c                |   18 +++++++++---------\n pager.c              |    2 +-\n run-command.c        |    2 +-\n setup.c              |    6 +++---\n wrapper.c            |   14 ++++++++++++++\n 17 files changed, 54 insertions(+), 32 deletions(-)\n\n-- \n1.7.7.1.msysgit.0.272.g9e47e\n"},{"id":"181129","messageId":"1323871631-2872-2-git-send-email-kusmabite@gmail.com","threadId":"29163","inReplyTo":"1323871631-2872-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH v2 1/4] compat/setenv.c: update errno when erroring out","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-12-14T14:07:08Z","receivedAt":"2011-12-14T14:07:08Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Previously, gitsetenv didn't update errno as it should when\nerroring out. Fix this.\n\nSigned-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\n compat/setenv.c |   10 ++++++++--\n 1 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/setenv.c b/compat/setenv.c\nindex 3a22ea7..89947b7 100644\n--- a/compat/setenv.c\n+++ b/compat/setenv.c\n@@ -6,7 +6,10 @@ int gitsetenv(const char *name, const char *value, int replace)\n \tsize_t namelen, valuelen;\n \tchar *envstr;\n \n-\tif (!name || !value) return -1;\n+\tif (!name || !value) {\n+\t\terrno = EINVAL;\n+\t\treturn -1;\n+\t}\n \tif (!replace) {\n \t\tchar *oldval = NULL;\n \t\toldval = getenv(name);\n@@ -16,7 +19,10 @@ int gitsetenv(const char *name, const char *value, int replace)\n \tnamelen = strlen(name);\n \tvaluelen = strlen(value);\n \tenvstr = malloc((namelen + valuelen + 2));\n-\tif (!envstr) return -1;\n+\tif (!envstr) {\n+\t\terrno = ENOMEM;\n+\t\treturn -1;\n+\t}\n \n \tmemcpy(envstr, name, namelen);\n \tenvstr[namelen] = '=';\n-- \n1.7.7.1.msysgit.0.272.g9e47e\n"},{"id":"181131","messageId":"1323871631-2872-3-git-send-email-kusmabite@gmail.com","threadId":"29163","inReplyTo":"1323871631-2872-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH v2 2/4] compat/setenv.c: error if name contains '='","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-12-14T14:07:09Z","receivedAt":"2011-12-14T14:07:09Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"According to POSIX, setenv should error out with EINVAL if it's\nasked to set an environment variable whose name contains an equals\nsign. Implement this detail in our compatibility-fallback.\n\nSigned-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\n compat/setenv.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/compat/setenv.c b/compat/setenv.c\nindex 89947b7..fc1439a 100644\n--- a/compat/setenv.c\n+++ b/compat/setenv.c\n@@ -6,7 +6,7 @@ int gitsetenv(const char *name, const char *value, int replace)\n \tsize_t namelen, valuelen;\n \tchar *envstr;\n \n-\tif (!name || !value) {\n+\tif (!name || strchr(name, '=') || !value) {\n \t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n-- \n1.7.7.1.msysgit.0.272.g9e47e\n"},{"id":"181132","messageId":"1323871631-2872-4-git-send-email-kusmabite@gmail.com","threadId":"29163","inReplyTo":"1323871631-2872-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH v2 3/4] wrapper: supply xsetenv and xputenv","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-12-14T14:07:10Z","receivedAt":"2011-12-14T14:07:10Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\n git-compat-util.h |    2 ++\n wrapper.c         |   14 ++++++++++++++\n 2 files changed, 16 insertions(+), 0 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 77062ed..ab17d53 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -439,6 +439,8 @@ extern int xdup(int fd);\n extern FILE *xfdopen(int fd, const char *mode);\n extern int xmkstemp(char *template);\n extern int xmkstemp_mode(char *template, int mode);\n+extern int xsetenv(const char *name, const char *val, int override);\n+extern int xputenv(const char *string);\n extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n extern int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1);\n \ndiff --git a/wrapper.c b/wrapper.c\nindex 85f09df..8d172ac 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -381,3 +381,17 @@ int remove_or_warn(unsigned int mode, const char *file)\n {\n \treturn S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);\n }\n+\n+int xsetenv(const char *name, const char *val, int overwrite)\n+{\n+\tif (setenv(name, val, overwrite))\n+\t\tdie_errno(\"setenv failed to set '%s' to '%s'\", name, val);\n+\treturn 0;\n+}\n+\n+int xputenv(const char *string)\n+{\n+\tif (putenv(string))\n+\t\tdie_errno(\"putenv failed to set '%s' \", string);\n+\treturn 0;\n+}\n-- \n1.7.7.1.msysgit.0.272.g9e47e\n"},{"id":"181133","messageId":"1323871631-2872-5-git-send-email-kusmabite@gmail.com","threadId":"29163","inReplyTo":"1323871631-2872-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH v2 4/4] use wrapper for unchecked setenv/putenv calls","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-12-14T14:07:11Z","receivedAt":"2011-12-14T14:07:11Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"This avoids us from accidentally dropping state, possibly leading\nto unexpected behaviour.\n\nThis is especially important on Windows, where the maximum size of\nthe environment is 32 kB.\n\nSigned-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\n builtin/clone.c      |    2 +-\n builtin/commit.c     |    6 +++---\n builtin/help.c       |    4 ++--\n builtin/init-db.c    |    2 +-\n builtin/merge.c      |    4 ++--\n builtin/notes.c      |    2 +-\n builtin/remote-ext.c |    4 ++--\n builtin/revert.c     |    2 +-\n config.c             |    2 +-\n exec_cmd.c           |    4 ++--\n git.c                |   18 +++++++++---------\n pager.c              |    2 +-\n run-command.c        |    2 +-\n setup.c              |    6 +++---\n 14 files changed, 30 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex efe8b6c..8d81c29 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -566,7 +566,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tatexit(remove_junk);\n \tsigchain_push_common(remove_junk_on_signal);\n \n-\tsetenv(CONFIG_ENVIRONMENT, mkpath(\"%s/config\", git_dir), 1);\n+\txsetenv(CONFIG_ENVIRONMENT, mkpath(\"%s/config\", git_dir), 1);\n \n \tif (safe_create_leading_directories_const(git_dir) < 0)\n \t\tdie(_(\"could not create leading directories of '%s'\"), git_dir);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e36e9ad..2b87da9 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -361,13 +361,13 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \t\t\tdie(_(\"unable to create temporary index\"));\n \n \t\told_index_env = getenv(INDEX_ENVIRONMENT);\n-\t\tsetenv(INDEX_ENVIRONMENT, index_lock.filename, 1);\n+\t\txsetenv(INDEX_ENVIRONMENT, index_lock.filename, 1);\n \n \t\tif (interactive_add(argc, argv, prefix, patch_interactive) != 0)\n \t\t\tdie(_(\"interactive add failed\"));\n \n \t\tif (old_index_env && *old_index_env)\n-\t\t\tsetenv(INDEX_ENVIRONMENT, old_index_env, 1);\n+\t\t\txsetenv(INDEX_ENVIRONMENT, old_index_env, 1);\n \t\telse\n \t\t\tunsetenv(INDEX_ENVIRONMENT);\n \n@@ -1023,7 +1023,7 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \tif (edit_flag)\n \t\tuse_editor = 1;\n \tif (!use_editor)\n-\t\tsetenv(\"GIT_EDITOR\", \":\", 1);\n+\t\txsetenv(\"GIT_EDITOR\", \":\", 1);\n \n \t/* Sanity check options */\n \tif (amend && !current_head)\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 61ff798..e7dc15b 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -330,7 +330,7 @@ static void setup_man_path(void)\n \tif (old_path)\n \t\tstrbuf_addstr(&new_path, old_path);\n \n-\tsetenv(\"MANPATH\", new_path.buf, 1);\n+\txsetenv(\"MANPATH\", new_path.buf, 1);\n \n \tstrbuf_release(&new_path);\n }\n@@ -371,7 +371,7 @@ static void show_man_page(const char *git_cmd)\n static void show_info_page(const char *git_cmd)\n {\n \tconst char *page = cmd_to_page(git_cmd);\n-\tsetenv(\"INFOPATH\", system_path(GIT_INFO_PATH), 1);\n+\txsetenv(\"INFOPATH\", system_path(GIT_INFO_PATH), 1);\n \texeclp(\"info\", \"info\", \"gitman\", page, (char *)NULL);\n \tdie(\"no info viewer handled the request\");\n }\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex d07554c..21ff09e 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -537,7 +537,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \tif (is_bare_repository_cfg == 1) {\n \t\tstatic char git_dir[PATH_MAX+1];\n \n-\t\tsetenv(GIT_DIR_ENVIRONMENT,\n+\t\txsetenv(GIT_DIR_ENVIRONMENT,\n \t\t\tgetcwd(git_dir, sizeof(git_dir)), argc > 0);\n \t}\n \ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a1c8534..a4ae473 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1257,7 +1257,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tstrbuf_addstr(&buf, \"merge\");\n \tfor (i = 0; i < argc; i++)\n \t\tstrbuf_addf(&buf, \" %s\", argv[i]);\n-\tsetenv(\"GIT_REFLOG_ACTION\", buf.buf, 0);\n+\txsetenv(\"GIT_REFLOG_ACTION\", buf.buf, 0);\n \tstrbuf_reset(&buf);\n \n \tfor (i = 0; i < argc; i++) {\n@@ -1267,7 +1267,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tremotes = &commit_list_insert(commit, remotes)->next;\n \t\tstrbuf_addf(&buf, \"GITHEAD_%s\",\n \t\t\t    sha1_to_hex(commit->object.sha1));\n-\t\tsetenv(buf.buf, argv[i], 1);\n+\t\txsetenv(buf.buf, argv[i], 1);\n \t\tstrbuf_reset(&buf);\n \t\tif (merge_remote_util(commit) &&\n \t\t    merge_remote_util(commit)->obj &&\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 10b8bc7..7b53c69 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -1076,7 +1076,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstrbuf_addstr(&sb, override_notes_ref);\n \t\texpand_notes_ref(&sb);\n-\t\tsetenv(\"GIT_NOTES_REF\", sb.buf, 1);\n+\t\txsetenv(\"GIT_NOTES_REF\", sb.buf, 1);\n \t\tstrbuf_release(&sb);\n \t}\n \ndiff --git a/builtin/remote-ext.c b/builtin/remote-ext.c\nindex 692c834..0b2169a 100644\n--- a/builtin/remote-ext.c\n+++ b/builtin/remote-ext.c\n@@ -38,8 +38,8 @@ static char *strip_escapes(const char *str, const char *service,\n \t\tpsoff = 4;\n \n \t/* Pass the service to command. */\n-\tsetenv(\"GIT_EXT_SERVICE\", service, 1);\n-\tsetenv(\"GIT_EXT_SERVICE_NOPREFIX\", service + psoff, 1);\n+\txsetenv(\"GIT_EXT_SERVICE\", service, 1);\n+\txsetenv(\"GIT_EXT_SERVICE_NOPREFIX\", service + psoff, 1);\n \n \t/* Scan the length of argument. */\n \twhile (str[rpos] && (escape || str[rpos] != ' ')) {\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 1ea525c..955a99f 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -1007,7 +1007,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \tstruct commit_list *cur;\n \tint res;\n \n-\tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n+\txsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n \tif (opts->allow_ff)\n \t\tassert(!(opts->signoff || opts->no_commit ||\n \t\t\t\topts->record_origin || opts->edit));\ndiff --git a/config.c b/config.c\nindex 5ea101f..f461a62 100644\n--- a/config.c\n+++ b/config.c\n@@ -43,7 +43,7 @@ void git_config_push_parameter(const char *text)\n \t\tstrbuf_addch(&env, ' ');\n \t}\n \tsq_quote_buf(&env, text);\n-\tsetenv(CONFIG_DATA_ENVIRONMENT, env.buf, 1);\n+\txsetenv(CONFIG_DATA_ENVIRONMENT, env.buf, 1);\n \tstrbuf_release(&env);\n }\n \ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 171e841..a5a92dd 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -63,7 +63,7 @@ void git_set_argv_exec_path(const char *exec_path)\n \t/*\n \t * Propagate this setting to external programs.\n \t */\n-\tsetenv(EXEC_PATH_ENVIRONMENT, exec_path, 1);\n+\txsetenv(EXEC_PATH_ENVIRONMENT, exec_path, 1);\n }\n \n \n@@ -108,7 +108,7 @@ void setup_path(void)\n \telse\n \t\tstrbuf_addstr(&new_path, _PATH_DEFPATH);\n \n-\tsetenv(\"PATH\", new_path.buf, 1);\n+\txsetenv(\"PATH\", new_path.buf, 1);\n \n \tstrbuf_release(&new_path);\n }\ndiff --git a/git.c b/git.c\nindex 8e34903..cb80de2 100644\n--- a/git.c\n+++ b/git.c\n@@ -54,7 +54,7 @@ int check_pager_config(const char *cmd)\n static void commit_pager_choice(void) {\n \tswitch (use_pager) {\n \tcase 0:\n-\t\tsetenv(\"GIT_PAGER\", \"cat\", 1);\n+\t\txsetenv(\"GIT_PAGER\", \"cat\", 1);\n \t\tbreak;\n \tcase 1:\n \t\tsetup_pager();\n@@ -109,7 +109,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--no-replace-objects\")) {\n \t\t\tread_replace_refs = 0;\n-\t\t\tsetenv(NO_REPLACE_OBJECTS_ENVIRONMENT, \"1\", 1);\n+\t\t\txsetenv(NO_REPLACE_OBJECTS_ENVIRONMENT, \"1\", 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--git-dir\")) {\n@@ -117,13 +117,13 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\tfprintf(stderr, \"No directory given for --git-dir.\\n\" );\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n-\t\t\tsetenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1);\n+\t\t\txsetenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n \t\t} else if (!prefixcmp(cmd, \"--git-dir=\")) {\n-\t\t\tsetenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);\n+\t\t\txsetenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--namespace\")) {\n@@ -131,13 +131,13 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\tfprintf(stderr, \"No namespace given for --namespace.\\n\" );\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n-\t\t\tsetenv(GIT_NAMESPACE_ENVIRONMENT, (*argv)[1], 1);\n+\t\t\txsetenv(GIT_NAMESPACE_ENVIRONMENT, (*argv)[1], 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n \t\t} else if (!prefixcmp(cmd, \"--namespace=\")) {\n-\t\t\tsetenv(GIT_NAMESPACE_ENVIRONMENT, cmd + 12, 1);\n+\t\t\txsetenv(GIT_NAMESPACE_ENVIRONMENT, cmd + 12, 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--work-tree\")) {\n@@ -145,19 +145,19 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\tfprintf(stderr, \"No directory given for --work-tree.\\n\" );\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n-\t\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1);\n+\t\t\txsetenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n \t\t} else if (!prefixcmp(cmd, \"--work-tree=\")) {\n-\t\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, cmd + 12, 1);\n+\t\t\txsetenv(GIT_WORK_TREE_ENVIRONMENT, cmd + 12, 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--bare\")) {\n \t\t\tstatic char git_dir[PATH_MAX+1];\n \t\t\tis_bare_repository_cfg = 1;\n-\t\t\tsetenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);\n+\t\t\txsetenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"-c\")) {\ndiff --git a/pager.c b/pager.c\nindex 975955b..d3a1299 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -76,7 +76,7 @@ void setup_pager(void)\n \tif (!pager)\n \t\treturn;\n \n-\tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n+\txsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n \n \t/* spawn the pager */\n \tpager_argv[0] = pager;\ndiff --git a/run-command.c b/run-command.c\nindex 1c51043..37abbde 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -258,7 +258,7 @@ fail_pipe:\n \t\tif (cmd->env) {\n \t\t\tfor (; *cmd->env; cmd->env++) {\n \t\t\t\tif (strchr(*cmd->env, '='))\n-\t\t\t\t\tputenv((char *)*cmd->env);\n+\t\t\t\t\txputenv((char *)*cmd->env);\n \t\t\t\telse\n \t\t\t\t\tunsetenv(*cmd->env);\n \t\t\t}\ndiff --git a/setup.c b/setup.c\nindex 61c22e6..06f38d0 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -309,7 +309,7 @@ void setup_work_tree(void)\n \t * if $GIT_WORK_TREE is set relative\n \t */\n \tif (getenv(GIT_WORK_TREE_ENVIRONMENT))\n-\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, \".\", 1);\n+\t\txsetenv(GIT_WORK_TREE_ENVIRONMENT, \".\", 1);\n \n \tset_git_dir(relative_path(git_dir, work_tree));\n \tinitialized = 1;\n@@ -683,9 +683,9 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \n \tprefix = setup_git_directory_gently_1(nongit_ok);\n \tif (prefix)\n-\t\tsetenv(\"GIT_PREFIX\", prefix, 1);\n+\t\txsetenv(\"GIT_PREFIX\", prefix, 1);\n \telse\n-\t\tsetenv(\"GIT_PREFIX\", \"\", 1);\n+\t\txsetenv(\"GIT_PREFIX\", \"\", 1);\n \n \tif (startup_info) {\n \t\tstartup_info->have_repository = !nongit_ok || !*nongit_ok;\n-- \n1.7.7.1.msysgit.0.272.g9e47e\n"},{"id":"181179","messageId":"20111214181658.GA1691@sigill.intra.peff.net","threadId":"29163","inReplyTo":"1323871631-2872-5-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH v2 4/4] use wrapper for unchecked setenv/putenv calls","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-14T18:16:58Z","receivedAt":"2011-12-14T18:16:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 14, 2011 at 03:07:11PM +0100, Erik Faye-Lund wrote:\n\n> This avoids us from accidentally dropping state, possibly leading\n> to unexpected behaviour.\n\nI do think this is fine in a \"be extra cautious\" kind of way.\n\n> This is especially important on Windows, where the maximum size of\n> the environment is 32 kB.\n\nBut does your patch actually detect that? As Andreas pointed out, these\nlimits don't typically come into play at setenv time. Instead, the\nenvironment is allocated on the heap, and then the result is passed to\nexec/spawn, which will fail.\n\nSo your patch is really detecting a failure to malloc, not an overflow\nof the environment size, and Windows is just as (un)likely to run out of\nheap as any other platform.\n\nYou can check how your platform behaves by applying this patch:\n\ndiff --git a/git.c b/git.c\nindex f10e434..57f6b12 100644\n--- a/git.c\n+++ b/git.c\n@@ -223,6 +223,16 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\t\talias_argv[i] = (*argv)[i];\n \t\t\talias_argv[argc] = NULL;\n \n+\t\t\t/* make gigantic environment */\n+\t\t\t{\n+\t\t\t\tint len = 256 * 1024;\n+\t\t\t\tchar *buf = xmalloc(len);\n+\t\t\t\tmemset(buf, 'z', len);\n+\t\t\t\tbuf[len-1] = '\\0';\n+\t\t\t\tif (setenv(\"FOO\", buf, 1))\n+\t\t\t\t\tdie(\"setenv failed\");\n+\t\t\t}\n+\n \t\t\tret = run_command_v_opt(alias_argv, RUN_USING_SHELL);\n \t\t\tif (ret >= 0)   /* normal exit */\n \t\t\t\texit(ret);\n\nand then running:\n\n  git -c alias.foo='!echo ok' foo\n\nwhich yields:\n\n  fatal: cannot exec 'echo ok': Argument list too long\n\non Linux.\n\n-Peff\n\nPS I tried to come up with an invocation of git that would demonstrate\n   this, but it turns out it's really hard. The contents of environment\n   variables we set are either constants, come from the environment (so\n   they can't be too big already!), or come from filesystem paths. So\n   it's possible to overflow now, but you have to have a nearly-full\n   environment in the first place, and then have a long path that tips\n   it over the limit.\n"},{"id":"181232","messageId":"7vzketohvg.fsf@alter.siamese.dyndns.org","threadId":"29163","inReplyTo":"1323871631-2872-4-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH v2 3/4] wrapper: supply xsetenv and xputenv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-15T17:38:11Z","receivedAt":"2011-12-15T17:38:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund <kusmabite@gmail.com> writes:\n\n> +extern int xsetenv(const char *name, const char *val, int override);\n> +extern int xputenv(const char *string);\n\nActually, putenv(3) takes \"char *string\".\n\nI adjusted 3 & 4 locally before queuing, so there is no need for resend,\nand judging from the later part of the discussion, it seems that it may be\nbetter to use only the first two patches from this series after all.\n"},{"id":"181235","messageId":"CABPQNSY+Q=Dt8oKodJMqRRb+ADdfDgQTxWKDfOpHvb5L9J=vvw@mail.gmail.com","threadId":"29163","inReplyTo":"20111214181658.GA1691@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] use wrapper for unchecked setenv/putenv calls","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-12-15T18:04:21Z","receivedAt":"2011-12-15T18:04:21Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Dec 14, 2011 at 7:16 PM, Jeff King <peff@peff.net> wrote:\n> On Wed, Dec 14, 2011 at 03:07:11PM +0100, Erik Faye-Lund wrote:\n>\n>> This avoids us from accidentally dropping state, possibly leading\n>> to unexpected behaviour.\n>\n> I do think this is fine in a \"be extra cautious\" kind of way.\n>\n>> This is especially important on Windows, where the maximum size of\n>> the environment is 32 kB.\n>\n> But does your patch actually detect that? As Andreas pointed out, these\n> limits don't typically come into play at setenv time. Instead, the\n> environment is allocated on the heap, and then the result is passed to\n> exec/spawn, which will fail.\n>\n> So your patch is really detecting a failure to malloc, not an overflow\n> of the environment size, and Windows is just as (un)likely to run out of\n> heap as any other platform.\n>\n\nYou are right; I just assumed that our setenv was implemented on top\nof SetEnvironmentVariable, which does impose max-limits on setting (if\nI read the documentation correctly):\nhttp://msdn.microsoft.com/en-us/library/windows/desktop/ms686206(v=vs.85).aspx\n\nBut we don't. We have mingw_setenv, which simply modifies environ.\n\n> You can check how your platform behaves by applying this patch:\n>\n> diff --git a/git.c b/git.c\n> index f10e434..57f6b12 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -223,6 +223,16 @@ static int handle_alias(int *argcp, const char ***argv)\n>                                alias_argv[i] = (*argv)[i];\n>                        alias_argv[argc] = NULL;\n>\n> +                       /* make gigantic environment */\n> +                       {\n> +                               int len = 256 * 1024;\n> +                               char *buf = xmalloc(len);\n> +                               memset(buf, 'z', len);\n> +                               buf[len-1] = '\\0';\n> +                               if (setenv(\"FOO\", buf, 1))\n> +                                       die(\"setenv failed\");\n> +                       }\n> +\n>                        ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);\n>                        if (ret >= 0)   /* normal exit */\n>                                exit(ret);\n>\n> and then running:\n>\n>  git -c alias.foo='!echo ok' foo\n>\n> which yields:\n>\n>  fatal: cannot exec 'echo ok': Argument list too long\n>\n> on Linux.\n>\n\nYeah, but I'm getting:\n$ git -c alias.foo='!echo ok' foo\nok\n\nWhich is strange to me.\n\nBut if I do:\n$ git -c alias.foo='!echo $FOO' foo\n\nit does echo a bunch of 'z's. And we get the expected amount:\n$ git -c alias.foo='!echo $FOO' foo | wc -c\n 262144\n\nThis strikes me as very strange, because we end up calling the ANSI\nversion of CreateProcess with an environment-block beyond 32767 chars,\nwhich MSDN says should fail:\n\n\"The ANSI version of this function, CreateProcessA fails if the total\nsize of the environment block for the process exceeds 32,767\ncharacters.\"\n\nhttp://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx\n\nOh well, this is strange behavior in a good way; I'm not going to complain :P\n"},{"id":"181239","messageId":"CABPQNSZVRfFk=Y6JnnDkvcbPwXgm1MMeevDyrjFF=D4ubGPaTg@mail.gmail.com","threadId":"29163","inReplyTo":"7vzketohvg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/4] wrapper: supply xsetenv and xputenv","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-12-15T18:25:41Z","receivedAt":"2011-12-15T18:25:41Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Dec 15, 2011 at 6:38 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Erik Faye-Lund <kusmabite@gmail.com> writes:\n>\n>> +extern int xsetenv(const char *name, const char *val, int override);\n>> +extern int xputenv(const char *string);\n>\n> Actually, putenv(3) takes \"char *string\".\n\nAh, thanks for spotting.\n\n> I adjusted 3 & 4 locally before queuing, so there is no need for resend,\n> and judging from the later part of the discussion, it seems that it may be\n> better to use only the first two patches from this series after all.\n\nYeah, I agree; patch 1 and 2 are the only ones that makes sense.\n"}]}