{"thread":{"id":"37711","subject":"[PATCH] receive-pack: plug minor memory leak in unpack()","startedAt":"2014-10-11T11:00:16Z","lastAt":"2014-11-09T13:49:54Z","messageCount":10,"participants":["René Scharfe","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"250506","messageId":"54390DC0.8060302@web.de","threadId":"37711","inReplyTo":null,"subject":"[PATCH] receive-pack: plug minor memory leak in unpack()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-10-11T11:00:16Z","receivedAt":"2014-10-11T11:00:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The argv_array used in unpack() is never freed.  Instead of adding\nexplicit calls to argv_array_clear() use the args member of struct\nchild_process and let run_command() and friends clean up for us.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/receive-pack.c | 18 ++++++++----------\n 1 file changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex a51846c..443dd37 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1220,7 +1220,6 @@ static const char *pack_lockfile;\n static const char *unpack(int err_fd, struct shallow_info *si)\n {\n \tstruct pack_header hdr;\n-\tstruct argv_array av = ARGV_ARRAY_INIT;\n \tconst char *hdr_err;\n \tint status;\n \tchar hdr_arg[38];\n@@ -1243,16 +1242,16 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \n \tif (si->nr_ours || si->nr_theirs) {\n \t\talt_shallow_file = setup_temporary_shallow(si->shallow);\n-\t\targv_array_pushl(&av, \"--shallow-file\", alt_shallow_file, NULL);\n+\t\targv_array_push(&child.args, \"--shallow-file\");\n+\t\targv_array_push(&child.args, alt_shallow_file);\n \t}\n \n \tif (ntohl(hdr.hdr_entries) < unpack_limit) {\n-\t\targv_array_pushl(&av, \"unpack-objects\", hdr_arg, NULL);\n+\t\targv_array_pushl(&child.args, \"unpack-objects\", hdr_arg, NULL);\n \t\tif (quiet)\n-\t\t\targv_array_push(&av, \"-q\");\n+\t\t\targv_array_push(&child.args, \"-q\");\n \t\tif (fsck_objects)\n-\t\t\targv_array_push(&av, \"--strict\");\n-\t\tchild.argv = av.argv;\n+\t\t\targv_array_push(&child.args, \"--strict\");\n \t\tchild.no_stdout = 1;\n \t\tchild.err = err_fd;\n \t\tchild.git_cmd = 1;\n@@ -1267,13 +1266,12 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\tif (gethostname(keep_arg + s, sizeof(keep_arg) - s))\n \t\t\tstrcpy(keep_arg + s, \"localhost\");\n \n-\t\targv_array_pushl(&av, \"index-pack\",\n+\t\targv_array_pushl(&child.args, \"index-pack\",\n \t\t\t\t \"--stdin\", hdr_arg, keep_arg, NULL);\n \t\tif (fsck_objects)\n-\t\t\targv_array_push(&av, \"--strict\");\n+\t\t\targv_array_push(&child.args, \"--strict\");\n \t\tif (fix_thin)\n-\t\t\targv_array_push(&av, \"--fix-thin\");\n-\t\tchild.argv = av.argv;\n+\t\t\targv_array_push(&child.args, \"--fix-thin\");\n \t\tchild.out = -1;\n \t\tchild.err = err_fd;\n \t\tchild.git_cmd = 1;\n-- \n2.1.2\n"},{"id":"250520","messageId":"20141012015321.GA15272@peff.net","threadId":"37711","inReplyTo":"54390DC0.8060302@web.de","subject":"Re: [PATCH] receive-pack: plug minor memory leak in unpack()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-12T01:53:21Z","receivedAt":"2014-10-12T01:53:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 11, 2014 at 01:00:16PM +0200, René Scharfe wrote:\n\n> The argv_array used in unpack() is never freed.  Instead of adding\n> explicit calls to argv_array_clear() use the args member of struct\n> child_process and let run_command() and friends clean up for us.\n\nLooks good. I notice that the recently added prepare_push_cert_sha1 uses\nan argv_array to create the child_process.env, and we leak the result. I\nwonder if run-command should provide a managed env array similar to the\n\"args\" array.\n\n-Peff\n"},{"id":"250565","messageId":"xmqq1tqb4wkm.fsf@gitster.dls.corp.google.com","threadId":"37711","inReplyTo":"20141012015321.GA15272@peff.net","subject":"Re: [PATCH] receive-pack: plug minor memory leak in unpack()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-13T19:08:09Z","receivedAt":"2014-10-13T19:08:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Oct 11, 2014 at 01:00:16PM +0200, René Scharfe wrote:\n>\n>> The argv_array used in unpack() is never freed.  Instead of adding\n>> explicit calls to argv_array_clear() use the args member of struct\n>> child_process and let run_command() and friends clean up for us.\n>\n> Looks good. I notice that the recently added prepare_push_cert_sha1 uses\n> an argv_array to create the child_process.env, and we leak the result.\n\nYou're right.  finish_command() runs argv_array_clear() on\ncmd->args, but does not care about cmd->env so anybody that use the\n(optional) \"use these environment variable settings while running\nthe command\" would leak unless the caller takes care of it.\n\n> I wonder if run-command should provide a managed env array similar\n> to the \"args\" array.\n\nI took a look at a few of them:\n\n - setup_pager() uses a static set of environment variables that are\n   not built with argv_array(); should be easy to convert, though.\n\n - wt_status_print_submodule_summary() does use two argv_arrays, for\n   env and argv.  It can use the managed \"args\" today, and with such\n   a change it can also use the managed \"envs\".\n\n - receive-pack.c::prepare_push_cert_sha1() would benefit from\n   managed \"envs\".\n\n - http-backend.c::run_service() would benefit from managed \"envs\".\n\n - daemon.c::handle() uses a static set of environment variables\n   that are not built with argv_array().  Same for argv.\n\nIt shouldn't be too hard to catch and convert all of them.\n"},{"id":"250610","messageId":"20141014091628.GB16686@peff.net","threadId":"37711","inReplyTo":"xmqq1tqb4wkm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] receive-pack: plug minor memory leak in unpack()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-14T09:16:29Z","receivedAt":"2014-10-14T09:16:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 13, 2014 at 12:08:09PM -0700, Junio C Hamano wrote:\n\n> > I wonder if run-command should provide a managed env array similar\n> > to the \"args\" array.\n> \n> I took a look at a few of them:\n\nI took a brief look, too.\n\nI had hoped we could just all it \"env\" and everybody would be happy\nusing it instead of bare pointers. But quite a few callers assign\n\"local_repo_env\" to to child_process.env. We could copy it into an\nargv_array, of course, but that really feels like working around the\ninterface. So I think we would prefer to keep both forms available.\n\nThat raises the question: what should it be called? The \"argv_array\"\nform of \"argv\" is called \"args\". The more I see it, the more I hate that\nname, as the two are easily confused. We could have:\n\n  const char **argv;\n  struct argv_array argv_array;\n  const char **env;\n  struct argv_array env_array;\n\nthough \"argv_array\" is a lot to type when you have a string of\nargv_array_pushf() calls (which are already by themselves kind of\nverbose). Maybe that's not too big a deal, though.\n\nWe could flip it to give the managed version the short name (and calling\nthe unmanaged version \"env_ptr\" or something). That would require\nmunging the existing callers, but the tweak would be simple.\n\n>  - daemon.c::handle() uses a static set of environment variables\n>    that are not built with argv_array().  Same for argv.\n\nMost of the callers you mentioned are good candidates. This one is\ntricky.\n\nThe argv array gets malloc'd and set up by the parent git-daemon\nprocess. Then each time we get a client, we create a new struct\nchild_process that references it. So using the managed argv-array would\nactually be a bit more complicated (and not save any memory; we just\nalways point to the single copy for each child).\n\nFor the environment, we build it in a function-local buffer, point the\nchild_process's env field at it, start the child, and then copy the\nchild_process into our global list of children. When we notice a child\nis dead (by linearly going through the list with waitpid), we free the\nlist entry. So there are a few potentially bad things here:\n\n  1. We memcpy the child_process to put it on the list. Which does work,\n     though it feels a little like we are violating the abstraction\n     barrier.\n\n  2. The child_process in the list points to the local \"env\" buffer that\n     is no longer valid. There's no bug because we don't ever look at\n     it. Moving to a managed env would fix that. But I have to wonder if\n     we even want to be keeping the \"struct child_process\" around in the\n     first place (all we really care about is the pid).\n\n  3. If we do move to a managed env, then we expect it to get cleaned up\n     in finish_command. But we never call that; we just free the list\n     memory containing the child_process. We would want to call\n     finish_command. Except that we will have reaped the process already\n     with our call to waitpid() from check_dead_children. So we'd need a\n     new call to do just the cleanup without the wait, I guess.\n\n  4. For every loop on the listen socket, we call waitpid() for each\n     living child, which is a bit wasteful. We'd probably do better to\n     call waitpid(0, &status, WNOHANG), and then look up the resulting\n     pid in a hashmap or sorted list when we actually see something that\n     died. I don't know that this is a huge problem in practice. We use\n     git-daemon pretty heavily on our backend servers at GitHub, and it\n     seems to use about 5% of a CPU constantly on each machine. Which is\n     kind of lame, given that it isn't doing anything at all, but is\n     hardly earth-shattering.\n\nSo I'm not sure if it is worth converting to a managed env. There's a\nbit of ugliness, but none of it is causing any problems, and doing so\nopens a can of worms. The most interesting thing to fix (to me, anyway)\nis number 4, but that has nothing to do with the env in the first place.\n:)\n\n-Peff\n"},{"id":"250823","messageId":"54439CDA.9070804@web.de","threadId":"37711","inReplyTo":"20141014091628.GB16686@peff.net","subject":"Re: [PATCH] receive-pack: plug minor memory leak in unpack()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-10-19T11:13:30Z","receivedAt":"2014-10-19T11:13:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.10.2014 um 11:16 schrieb Jeff King:\n> On Mon, Oct 13, 2014 at 12:08:09PM -0700, Junio C Hamano wrote:\n>\n>>> I wonder if run-command should provide a managed env array similar\n>>> to the \"args\" array.\n\nThat's a good idea.\n\n>>\n>> I took a look at a few of them:\n>\n> I took a brief look, too.\n>\n> I had hoped we could just all it \"env\" and everybody would be happy\n> using it instead of bare pointers. But quite a few callers assign\n> \"local_repo_env\" to to child_process.env. We could copy it into an\n> argv_array, of course, but that really feels like working around the\n> interface. So I think we would prefer to keep both forms available.\n\nWe could add a flag (clear_local_repo_env?) and reference local_repo_env \nonly in run-command.c for these cases.  But some other cases remain that \nare better off providing their own array, like in daemon.c.\n\n> That raises the question: what should it be called? The \"argv_array\"\n> form of \"argv\" is called \"args\". The more I see it, the more I hate that\n> name, as the two are easily confused. We could have:\n>\n>    const char **argv;\n>    struct argv_array argv_array;\n>    const char **env;\n>    struct argv_array env_array;\n>\n> though \"argv_array\" is a lot to type when you have a string of\n> argv_array_pushf() calls (which are already by themselves kind of\n> verbose). Maybe that's not too big a deal, though.\n\nI actually like args and argv. :)  Mixing them up is noticed by the \ncompiler, so any confusion is cleared up rather quickly.\n\n> We could flip it to give the managed version the short name (and calling\n> the unmanaged version \"env_ptr\" or something). That would require\n> munging the existing callers, but the tweak would be simple.\n\nPerhaps, but I'm a but skeptical of big renames.  Let's start small and \nadd env_array, and see how far we get with that.\n\n>>   - daemon.c::handle() uses a static set of environment variables\n>>     that are not built with argv_array().  Same for argv.\n>\n> Most of the callers you mentioned are good candidates. This one is\n> tricky.\n>\n> The argv array gets malloc'd and set up by the parent git-daemon\n> process. Then each time we get a client, we create a new struct\n> child_process that references it. So using the managed argv-array would\n> actually be a bit more complicated (and not save any memory; we just\n> always point to the single copy for each child).\n>\n> For the environment, we build it in a function-local buffer, point the\n> child_process's env field at it, start the child, and then copy the\n> child_process into our global list of children. When we notice a child\n> is dead (by linearly going through the list with waitpid), we free the\n> list entry. So there are a few potentially bad things here:\n>\n>    1. We memcpy the child_process to put it on the list. Which does work,\n>       though it feels a little like we are violating the abstraction\n>       barrier.\n>\n>    2. The child_process in the list points to the local \"env\" buffer that\n>       is no longer valid. There's no bug because we don't ever look at\n>       it. Moving to a managed env would fix that. But I have to wonder if\n>       we even want to be keeping the \"struct child_process\" around in the\n>       first place (all we really care about is the pid).\n>\n>    3. If we do move to a managed env, then we expect it to get cleaned up\n>       in finish_command. But we never call that; we just free the list\n>       memory containing the child_process. We would want to call\n>       finish_command. Except that we will have reaped the process already\n>       with our call to waitpid() from check_dead_children. So we'd need a\n>       new call to do just the cleanup without the wait, I guess.\n>\n>    4. For every loop on the listen socket, we call waitpid() for each\n>       living child, which is a bit wasteful. We'd probably do better to\n>       call waitpid(0, &status, WNOHANG), and then look up the resulting\n>       pid in a hashmap or sorted list when we actually see something that\n>       died. I don't know that this is a huge problem in practice. We use\n>       git-daemon pretty heavily on our backend servers at GitHub, and it\n>       seems to use about 5% of a CPU constantly on each machine. Which is\n>       kind of lame, given that it isn't doing anything at all, but is\n>       hardly earth-shattering.\n>\n> So I'm not sure if it is worth converting to a managed env. There's a\n> bit of ugliness, but none of it is causing any problems, and doing so\n> opens a can of worms. The most interesting thing to fix (to me, anyway)\n> is number 4, but that has nothing to do with the env in the first place.\n> :)\n\nTrickiness makes me nervous, especially in daemon.c.  And 5% CPU usage \njust for waiting sounds awful.  Using waitpid(0, ...) is not supported \nby the current implementation in compat/mingw.c, however.\n\nI agree that env handling should only be changed after the wait loop has \nbeen improved.\n\nBy the way, does getaddrinfo(3) show up in your profiles much?  Recently \nI looked into calling it only on demand instead of for all incoming \nconnections because doing that unconditional with a user-supplied \n(\"tainted\") hostname just felt wrong.  The resulting patch series turned \nout to be not very pretty and I didn't see any performance improvements \nin my very limited tests, however; not sure if it's worth it.\n\nRené\n"},{"id":"250824","messageId":"54439CF3.8070305@web.de","threadId":"37711","inReplyTo":"xmqq1tqb4wkm.fsf@gitster.dls.corp.google.com","subject":"[PATCH 1/2] run-command: add env_array, an optional argv_array for env","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-10-19T11:13:55Z","receivedAt":"2014-10-19T11:13:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Similar to args, add a struct argv_array member to struct child_process\nthat simplifies specifying the environment for children.  It is freed\nautomatically by finish_command() or if start_command() encounters an\nerror.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n Documentation/technical/api-run-command.txt | 5 +++++\n run-command.c                               | 6 ++++++\n run-command.h                               | 3 ++-\n 3 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt\nindex 842b838..3f12fcd 100644\n--- a/Documentation/technical/api-run-command.txt\n+++ b/Documentation/technical/api-run-command.txt\n@@ -169,6 +169,11 @@ string pointers (NULL terminated) in .env:\n . If the string does not contain '=', it names an environment\n   variable that will be removed from the child process's environment.\n \n+If the .env member is NULL, `start_command` will point it at the\n+.env_array `argv_array` (so you may use one or the other, but not both).\n+The memory in .env_array will be cleaned up automatically during\n+`finish_command` (or during `start_command` when it is unsuccessful).\n+\n To specify a new initial working directory for the sub-process,\n specify it in the .dir member.\n \ndiff --git a/run-command.c b/run-command.c\nindex 761f0fd..46be513 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -12,6 +12,7 @@ void child_process_init(struct child_process *child)\n {\n \tmemset(child, 0, sizeof(*child));\n \targv_array_init(&child->args);\n+\targv_array_init(&child->env_array);\n }\n \n struct child_to_clean {\n@@ -287,6 +288,8 @@ int start_command(struct child_process *cmd)\n \n \tif (!cmd->argv)\n \t\tcmd->argv = cmd->args.argv;\n+\tif (!cmd->env)\n+\t\tcmd->env = cmd->env_array.argv;\n \n \t/*\n \t * In case of errors we must keep the promise to close FDs\n@@ -338,6 +341,7 @@ fail_pipe:\n \t\t\terror(\"cannot create %s pipe for %s: %s\",\n \t\t\t\tstr, cmd->argv[0], strerror(failed_errno));\n \t\t\targv_array_clear(&cmd->args);\n+\t\t\targv_array_clear(&cmd->env_array);\n \t\t\terrno = failed_errno;\n \t\t\treturn -1;\n \t\t}\n@@ -524,6 +528,7 @@ fail_pipe:\n \t\telse if (cmd->err)\n \t\t\tclose(cmd->err);\n \t\targv_array_clear(&cmd->args);\n+\t\targv_array_clear(&cmd->env_array);\n \t\terrno = failed_errno;\n \t\treturn -1;\n \t}\n@@ -550,6 +555,7 @@ int finish_command(struct child_process *cmd)\n {\n \tint ret = wait_or_whine(cmd->pid, cmd->argv[0]);\n \targv_array_clear(&cmd->args);\n+\targv_array_clear(&cmd->env_array);\n \treturn ret;\n }\n \ndiff --git a/run-command.h b/run-command.h\nindex 1b135d1..2137315 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -10,6 +10,7 @@\n struct child_process {\n \tconst char **argv;\n \tstruct argv_array args;\n+\tstruct argv_array env_array;\n \tpid_t pid;\n \t/*\n \t * Using .in, .out, .err:\n@@ -44,7 +45,7 @@ struct child_process {\n \tunsigned clean_on_exit:1;\n };\n \n-#define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT }\n+#define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT }\n void child_process_init(struct child_process *);\n \n int start_command(struct child_process *);\n-- \n2.1.2\n"},{"id":"250825","messageId":"54439D0C.1070308@web.de","threadId":"37711","inReplyTo":"xmqq1tqb4wkm.fsf@gitster.dls.corp.google.com","subject":"[PATCH 2/2] use env_array member of struct child_process","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-10-19T11:14:20Z","receivedAt":"2014-10-19T11:14:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Convert users of struct child_process to using the managed env_array for\nspecifying environment variables instead of supplying an array on the\nstack or bringing their own argv_array.  This shortens and simplifies\nthe code and ensures automatically that the allocated memory is freed\nafter use.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/receive-pack.c | 23 ++++++++++++++---------\n http-backend.c         |  9 +++------\n pager.c                | 15 ++++-----------\n transport-helper.c     | 10 ++--------\n wt-status.c            |  6 ++----\n 5 files changed, 25 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex f2f6c67..7593861 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -453,7 +453,6 @@ leave:\n static void prepare_push_cert_sha1(struct child_process *proc)\n {\n \tstatic int already_done;\n-\tstruct argv_array env = ARGV_ARRAY_INIT;\n \n \tif (!push_cert.len)\n \t\treturn;\n@@ -487,20 +486,26 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n \t\tnonce_status = check_nonce(push_cert.buf, bogs);\n \t}\n \tif (!is_null_sha1(push_cert_sha1)) {\n-\t\targv_array_pushf(&env, \"GIT_PUSH_CERT=%s\", sha1_to_hex(push_cert_sha1));\n-\t\targv_array_pushf(&env, \"GIT_PUSH_CERT_SIGNER=%s\",\n+\t\targv_array_pushf(&proc->env_array, \"GIT_PUSH_CERT=%s\",\n+\t\t\t\t sha1_to_hex(push_cert_sha1));\n+\t\targv_array_pushf(&proc->env_array, \"GIT_PUSH_CERT_SIGNER=%s\",\n \t\t\t\t sigcheck.signer ? sigcheck.signer : \"\");\n-\t\targv_array_pushf(&env, \"GIT_PUSH_CERT_KEY=%s\",\n+\t\targv_array_pushf(&proc->env_array, \"GIT_PUSH_CERT_KEY=%s\",\n \t\t\t\t sigcheck.key ? sigcheck.key : \"\");\n-\t\targv_array_pushf(&env, \"GIT_PUSH_CERT_STATUS=%c\", sigcheck.result);\n+\t\targv_array_pushf(&proc->env_array, \"GIT_PUSH_CERT_STATUS=%c\",\n+\t\t\t\t sigcheck.result);\n \t\tif (push_cert_nonce) {\n-\t\t\targv_array_pushf(&env, \"GIT_PUSH_CERT_NONCE=%s\", push_cert_nonce);\n-\t\t\targv_array_pushf(&env, \"GIT_PUSH_CERT_NONCE_STATUS=%s\", nonce_status);\n+\t\t\targv_array_pushf(&proc->env_array,\n+\t\t\t\t\t \"GIT_PUSH_CERT_NONCE=%s\",\n+\t\t\t\t\t push_cert_nonce);\n+\t\t\targv_array_pushf(&proc->env_array,\n+\t\t\t\t\t \"GIT_PUSH_CERT_NONCE_STATUS=%s\",\n+\t\t\t\t\t nonce_status);\n \t\t\tif (nonce_status == NONCE_SLOP)\n-\t\t\t\targv_array_pushf(&env, \"GIT_PUSH_CERT_NONCE_SLOP=%ld\",\n+\t\t\t\targv_array_pushf(&proc->env_array,\n+\t\t\t\t\t\t \"GIT_PUSH_CERT_NONCE_SLOP=%ld\",\n \t\t\t\t\t\t nonce_stamp_slop);\n \t\t}\n-\t\tproc->env = env.argv;\n \t}\n }\n \ndiff --git a/http-backend.c b/http-backend.c\nindex 404e682..e3e0dda 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -314,7 +314,6 @@ static void run_service(const char **argv)\n \tconst char *encoding = getenv(\"HTTP_CONTENT_ENCODING\");\n \tconst char *user = getenv(\"REMOTE_USER\");\n \tconst char *host = getenv(\"REMOTE_ADDR\");\n-\tstruct argv_array env = ARGV_ARRAY_INIT;\n \tint gzipped_request = 0;\n \tstruct child_process cld = CHILD_PROCESS_INIT;\n \n@@ -329,13 +328,12 @@ static void run_service(const char **argv)\n \t\thost = \"(none)\";\n \n \tif (!getenv(\"GIT_COMMITTER_NAME\"))\n-\t\targv_array_pushf(&env, \"GIT_COMMITTER_NAME=%s\", user);\n+\t\targv_array_pushf(&cld.env_array, \"GIT_COMMITTER_NAME=%s\", user);\n \tif (!getenv(\"GIT_COMMITTER_EMAIL\"))\n-\t\targv_array_pushf(&env, \"GIT_COMMITTER_EMAIL=%s@http.%s\",\n-\t\t\t\t user, host);\n+\t\targv_array_pushf(&cld.env_array,\n+\t\t\t\t \"GIT_COMMITTER_EMAIL=%s@http.%s\", user, host);\n \n \tcld.argv = argv;\n-\tcld.env = env.argv;\n \tif (gzipped_request)\n \t\tcld.in = -1;\n \tcld.git_cmd = 1;\n@@ -350,7 +348,6 @@ static void run_service(const char **argv)\n \n \tif (finish_command(&cld))\n \t\texit(1);\n-\targv_array_clear(&env);\n }\n \n static int show_text_ref(const char *name, const unsigned char *sha1,\ndiff --git a/pager.c b/pager.c\nindex b2b805a..f6e8c33 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -74,17 +74,10 @@ void setup_pager(void)\n \tpager_process.use_shell = 1;\n \tpager_process.argv = pager_argv;\n \tpager_process.in = -1;\n-\tif (!getenv(\"LESS\") || !getenv(\"LV\")) {\n-\t\tstatic const char *env[3];\n-\t\tint i = 0;\n-\n-\t\tif (!getenv(\"LESS\"))\n-\t\t\tenv[i++] = \"LESS=FRX\";\n-\t\tif (!getenv(\"LV\"))\n-\t\t\tenv[i++] = \"LV=-c\";\n-\t\tenv[i] = NULL;\n-\t\tpager_process.env = env;\n-\t}\n+\tif (!getenv(\"LESS\"))\n+\t\targv_array_push(&pager_process.env_array, \"LESS=FRX\");\n+\tif (!getenv(\"LV\"))\n+\t\targv_array_push(&pager_process.env_array, \"LV=-c\");\n \tif (start_command(&pager_process))\n \t\treturn;\n \ndiff --git a/transport-helper.c b/transport-helper.c\nindex 2b24d51..033a228 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -108,12 +108,6 @@ static struct child_process *get_helper(struct transport *transport)\n \tint refspec_alloc = 0;\n \tint duped;\n \tint code;\n-\tchar git_dir_buf[sizeof(GIT_DIR_ENVIRONMENT) + PATH_MAX + 1];\n-\tconst char *helper_env[] = {\n-\t\tgit_dir_buf,\n-\t\tNULL\n-\t};\n-\n \n \tif (data->helper)\n \t\treturn data->helper;\n@@ -129,8 +123,8 @@ static struct child_process *get_helper(struct transport *transport)\n \thelper->git_cmd = 0;\n \thelper->silent_exec_failure = 1;\n \n-\tsnprintf(git_dir_buf, sizeof(git_dir_buf), \"%s=%s\", GIT_DIR_ENVIRONMENT, get_git_dir());\n-\thelper->env = helper_env;\n+\targv_array_pushf(&helper->env_array, \"%s=%s\", GIT_DIR_ENVIRONMENT,\n+\t\t\t get_git_dir());\n \n \tcode = start_command(helper);\n \tif (code < 0 && errno == ENOENT)\ndiff --git a/wt-status.c b/wt-status.c\nindex 1bf5d72..66d267c 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -726,14 +726,14 @@ static void wt_status_print_changed(struct wt_status *s)\n static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitted)\n {\n \tstruct child_process sm_summary = CHILD_PROCESS_INIT;\n-\tstruct argv_array env = ARGV_ARRAY_INIT;\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n \tstruct strbuf cmd_stdout = STRBUF_INIT;\n \tstruct strbuf summary = STRBUF_INIT;\n \tchar *summary_content;\n \tsize_t len;\n \n-\targv_array_pushf(&env, \"GIT_INDEX_FILE=%s\", s->index_file);\n+\targv_array_pushf(&sm_summary.env_array, \"GIT_INDEX_FILE=%s\",\n+\t\t\t s->index_file);\n \n \targv_array_push(&argv, \"submodule\");\n \targv_array_push(&argv, \"summary\");\n@@ -745,14 +745,12 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \t\targv_array_push(&argv, s->amend ? \"HEAD^\" : \"HEAD\");\n \n \tsm_summary.argv = argv.argv;\n-\tsm_summary.env = env.argv;\n \tsm_summary.git_cmd = 1;\n \tsm_summary.no_stdin = 1;\n \tfflush(s->fp);\n \tsm_summary.out = -1;\n \n \trun_command(&sm_summary);\n-\targv_array_clear(&env);\n \targv_array_clear(&argv);\n \n \tlen = strbuf_read(&cmd_stdout, sm_summary.out, 1024);\n-- \n2.1.2\n"},{"id":"250846","messageId":"20141020091916.GA12913@peff.net","threadId":"37711","inReplyTo":"54439CDA.9070804@web.de","subject":"Re: [PATCH] receive-pack: plug minor memory leak in unpack()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-20T09:19:17Z","receivedAt":"2014-10-20T09:19:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 19, 2014 at 01:13:30PM +0200, René Scharfe wrote:\n\n> >We could flip it to give the managed version the short name (and calling\n> >the unmanaged version \"env_ptr\" or something). That would require\n> >munging the existing callers, but the tweak would be simple.\n> \n> Perhaps, but I'm a but skeptical of big renames.  Let's start small and add\n> env_array, and see how far we get with that.\n\nYeah, having basically implemented patches similar to yours, I think\nthat is a good first step. Both of your patches looked good to me.\n\n> Trickiness makes me nervous, especially in daemon.c.  And 5% CPU usage just\n> for waiting sounds awful.  Using waitpid(0, ...) is not supported by the\n> current implementation in compat/mingw.c, however.\n\nI guess you could use wait() and a counter that you increment whenever\nyou get SIGCLD, but that feels a bit hacky. I wonder how bad a real\nwaitpid would be for mingw.\n\n> By the way, does getaddrinfo(3) show up in your profiles much?  Recently I\n> looked into calling it only on demand instead of for all incoming\n> connections because doing that unconditional with a user-supplied\n> (\"tainted\") hostname just felt wrong.  The resulting patch series turned out\n> to be not very pretty and I didn't see any performance improvements in my\n> very limited tests, however; not sure if it's worth it.\n\nIt shows up in the child, not the parent, so it wasn't part of the\nprofiling I did recently. I did look at it just now, and it does\nintroduce some latency into each request (though not a lot of CPU;\nmainly it's the DNS request). Like you, I'm nervous about convincing\ngit-daemon to do lookups on random hosts. By itself it's not horrible\n(except for tying up git-daemon with absurdly long chains of glueless\nreferences), but I worry that it could exacerbate other problems\n(overflows or other bugs in DNS resolvers, as part of a cache-poisoning\nscheme, orbeing used for DoS amplification).\n\nI think doing it on demand would be a lot more sensible. We do not need\nto do a lookup at all unless the %H, %CH, or %IP interpolated path\nfeatures are used. And we do not need to do hostname canonicalization\nunless %CH is used.\n\n-Peff\n"},{"id":"250847","messageId":"20141020091949.GB12913@peff.net","threadId":"37711","inReplyTo":"54439D0C.1070308@web.de","subject":"Re: [PATCH 2/2] use env_array member of struct child_process","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-20T09:19:49Z","receivedAt":"2014-10-20T09:19:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 19, 2014 at 01:14:20PM +0200, René Scharfe wrote:\n\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -726,14 +726,14 @@ static void wt_status_print_changed(struct wt_status *s)\n>  static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitted)\n>  {\n>  \tstruct child_process sm_summary = CHILD_PROCESS_INIT;\n> -\tstruct argv_array env = ARGV_ARRAY_INIT;\n>  \tstruct argv_array argv = ARGV_ARRAY_INIT;\n\nI don't think it belongs in this patch, but a follow-on 3/2 might be to\ngive argv the same treatment.\n\n-Peff\n"},{"id":"251612","messageId":"545F7102.50403@web.de","threadId":"37711","inReplyTo":"20141020091949.GB12913@peff.net","subject":"Re: [PATCH 2/2] use env_array member of struct child_process","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-11-09T13:49:54Z","receivedAt":"2014-11-09T13:49:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.10.2014 um 11:19 schrieb Jeff King:\n> On Sun, Oct 19, 2014 at 01:14:20PM +0200, René Scharfe wrote:\n> \n>> --- a/wt-status.c\n>> +++ b/wt-status.c\n>> @@ -726,14 +726,14 @@ static void wt_status_print_changed(struct wt_status *s)\n>>   static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitted)\n>>   {\n>>   \tstruct child_process sm_summary = CHILD_PROCESS_INIT;\n>> -\tstruct argv_array env = ARGV_ARRAY_INIT;\n>>   \tstruct argv_array argv = ARGV_ARRAY_INIT;\n> \n> I don't think it belongs in this patch, but a follow-on 3/2 might be to\n> give argv the same treatment.\n\nYes, good idea.\n\n-- >8 --\nSubject: [PATCH] use args member of struct child_process\n\nConvert users of struct child_process to using the managed argv_array\nargs instead of providing their own.  This shortens the code a bit and\nensures that the allocated memory is released automatically after use.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/repack.c | 47 ++++++++++++++++++++++-------------------------\n wt-status.c      | 17 +++++++----------\n 2 files changed, 29 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 2845620..83e91c7 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -135,7 +135,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t};\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct string_list_item *item;\n-\tstruct argv_array cmd_args = ARGV_ARRAY_INIT;\n \tstruct string_list names = STRING_LIST_INIT_DUP;\n \tstruct string_list rollback = STRING_LIST_INIT_NODUP;\n \tstruct string_list existing_packs = STRING_LIST_INIT_DUP;\n@@ -202,56 +201,55 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tsigchain_push_common(remove_pack_on_signal);\n \n-\targv_array_push(&cmd_args, \"pack-objects\");\n-\targv_array_push(&cmd_args, \"--keep-true-parents\");\n+\targv_array_push(&cmd.args, \"pack-objects\");\n+\targv_array_push(&cmd.args, \"--keep-true-parents\");\n \tif (!pack_kept_objects)\n-\t\targv_array_push(&cmd_args, \"--honor-pack-keep\");\n-\targv_array_push(&cmd_args, \"--non-empty\");\n-\targv_array_push(&cmd_args, \"--all\");\n-\targv_array_push(&cmd_args, \"--reflog\");\n-\targv_array_push(&cmd_args, \"--indexed-objects\");\n+\t\targv_array_push(&cmd.args, \"--honor-pack-keep\");\n+\targv_array_push(&cmd.args, \"--non-empty\");\n+\targv_array_push(&cmd.args, \"--all\");\n+\targv_array_push(&cmd.args, \"--reflog\");\n+\targv_array_push(&cmd.args, \"--indexed-objects\");\n \tif (window)\n-\t\targv_array_pushf(&cmd_args, \"--window=%s\", window);\n+\t\targv_array_pushf(&cmd.args, \"--window=%s\", window);\n \tif (window_memory)\n-\t\targv_array_pushf(&cmd_args, \"--window-memory=%s\", window_memory);\n+\t\targv_array_pushf(&cmd.args, \"--window-memory=%s\", window_memory);\n \tif (depth)\n-\t\targv_array_pushf(&cmd_args, \"--depth=%s\", depth);\n+\t\targv_array_pushf(&cmd.args, \"--depth=%s\", depth);\n \tif (max_pack_size)\n-\t\targv_array_pushf(&cmd_args, \"--max-pack-size=%s\", max_pack_size);\n+\t\targv_array_pushf(&cmd.args, \"--max-pack-size=%s\", max_pack_size);\n \tif (no_reuse_delta)\n-\t\targv_array_pushf(&cmd_args, \"--no-reuse-delta\");\n+\t\targv_array_pushf(&cmd.args, \"--no-reuse-delta\");\n \tif (no_reuse_object)\n-\t\targv_array_pushf(&cmd_args, \"--no-reuse-object\");\n+\t\targv_array_pushf(&cmd.args, \"--no-reuse-object\");\n \tif (write_bitmaps)\n-\t\targv_array_push(&cmd_args, \"--write-bitmap-index\");\n+\t\targv_array_push(&cmd.args, \"--write-bitmap-index\");\n \n \tif (pack_everything & ALL_INTO_ONE) {\n \t\tget_non_kept_pack_filenames(&existing_packs);\n \n \t\tif (existing_packs.nr && delete_redundant) {\n \t\t\tif (unpack_unreachable)\n-\t\t\t\targv_array_pushf(&cmd_args,\n+\t\t\t\targv_array_pushf(&cmd.args,\n \t\t\t\t\t\t\"--unpack-unreachable=%s\",\n \t\t\t\t\t\tunpack_unreachable);\n \t\t\telse if (pack_everything & LOOSEN_UNREACHABLE)\n-\t\t\t\targv_array_push(&cmd_args,\n+\t\t\t\targv_array_push(&cmd.args,\n \t\t\t\t\t\t\"--unpack-unreachable\");\n \t\t}\n \t} else {\n-\t\targv_array_push(&cmd_args, \"--unpacked\");\n-\t\targv_array_push(&cmd_args, \"--incremental\");\n+\t\targv_array_push(&cmd.args, \"--unpacked\");\n+\t\targv_array_push(&cmd.args, \"--incremental\");\n \t}\n \n \tif (local)\n-\t\targv_array_push(&cmd_args,  \"--local\");\n+\t\targv_array_push(&cmd.args,  \"--local\");\n \tif (quiet)\n-\t\targv_array_push(&cmd_args,  \"--quiet\");\n+\t\targv_array_push(&cmd.args,  \"--quiet\");\n \tif (delta_base_offset)\n-\t\targv_array_push(&cmd_args,  \"--delta-base-offset\");\n+\t\targv_array_push(&cmd.args,  \"--delta-base-offset\");\n \n-\targv_array_push(&cmd_args, packtmp);\n+\targv_array_push(&cmd.args, packtmp);\n \n-\tcmd.argv = cmd_args.argv;\n \tcmd.git_cmd = 1;\n \tcmd.out = -1;\n \tcmd.no_stdin = 1;\n@@ -270,7 +268,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tret = finish_command(&cmd);\n \tif (ret)\n \t\treturn ret;\n-\targv_array_clear(&cmd_args);\n \n \tif (!names.nr && !quiet)\n \t\tprintf(\"Nothing new to pack.\\n\");\ndiff --git a/wt-status.c b/wt-status.c\nindex cdbc8d7..b54eac5 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -726,7 +726,6 @@ static void wt_status_print_changed(struct wt_status *s)\n static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitted)\n {\n \tstruct child_process sm_summary = CHILD_PROCESS_INIT;\n-\tstruct argv_array argv = ARGV_ARRAY_INIT;\n \tstruct strbuf cmd_stdout = STRBUF_INIT;\n \tstruct strbuf summary = STRBUF_INIT;\n \tchar *summary_content;\n@@ -735,23 +734,21 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \targv_array_pushf(&sm_summary.env_array, \"GIT_INDEX_FILE=%s\",\n \t\t\t s->index_file);\n \n-\targv_array_push(&argv, \"submodule\");\n-\targv_array_push(&argv, \"summary\");\n-\targv_array_push(&argv, uncommitted ? \"--files\" : \"--cached\");\n-\targv_array_push(&argv, \"--for-status\");\n-\targv_array_push(&argv, \"--summary-limit\");\n-\targv_array_pushf(&argv, \"%d\", s->submodule_summary);\n+\targv_array_push(&sm_summary.args, \"submodule\");\n+\targv_array_push(&sm_summary.args, \"summary\");\n+\targv_array_push(&sm_summary.args, uncommitted ? \"--files\" : \"--cached\");\n+\targv_array_push(&sm_summary.args, \"--for-status\");\n+\targv_array_push(&sm_summary.args, \"--summary-limit\");\n+\targv_array_pushf(&sm_summary.args, \"%d\", s->submodule_summary);\n \tif (!uncommitted)\n-\t\targv_array_push(&argv, s->amend ? \"HEAD^\" : \"HEAD\");\n+\t\targv_array_push(&sm_summary.args, s->amend ? \"HEAD^\" : \"HEAD\");\n \n-\tsm_summary.argv = argv.argv;\n \tsm_summary.git_cmd = 1;\n \tsm_summary.no_stdin = 1;\n \tfflush(s->fp);\n \tsm_summary.out = -1;\n \n \trun_command(&sm_summary);\n-\targv_array_clear(&argv);\n \n \tlen = strbuf_read(&cmd_stdout, sm_summary.out, 1024);\n \n-- \n2.1.3\n"}]}