{"thread":{"id":"26730","subject":"[PATCH 3/3] clone: Free a few paths","startedAt":"2011-03-14T19:18:35Z","lastAt":"2011-03-21T15:57:05Z","messageCount":48,"participants":["Carlos Martín Nieto","Jonathan Nieder","Jeff King","Junio C Hamano","Nguyen Thai Ngoc Duy","Erik Faye-Lund","Nguyễn Thái Ngọc Duy","Lasse Makholm"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"163331","messageId":"1300130318-11279-1-git-send-email-cmn@elego.de","threadId":"26730","inReplyTo":null,"subject":"[PATCH 0/3] Fix some errors reported by valgrind","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-14T19:18:35Z","receivedAt":"2011-03-14T19:18:35Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"This patch series fixes some errors I found when running the test\nsuite with --valgrind.\n\nCarlos Martín Nieto (3):\n make_absolute_path: Don't try to copy a string to itself\n setup_path(): Free temporary buffer\n clone: Free a few paths\n\n abspath.c | 2 +-\n builtin/clone.c | 9 ++++++---\n exec_cmd.c | 9 ++++++---\n 3 files changed, 13 insertions(+), 7 deletions(-)\n\n-- \n1.7.4.1\n"},{"id":"163334","messageId":"1300130318-11279-2-git-send-email-cmn@elego.de","threadId":"26730","inReplyTo":"1300130318-11279-1-git-send-email-cmn@elego.de","subject":"[PATCH 1/3] make_absolute_path: Don't try to copy a string to itself","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-14T19:18:36Z","receivedAt":"2011-03-14T19:18:36Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"Sometimes (at least in t-0001-init.sh test 12), the return value of\nmake_absolute_path() is passed to it as an argument, making the first\nand second arguments to strlcpy() the same, making the test fail when\nrun under valgrind.\n\nSigned-off-by: Carlos Martín Nieto \n---\n\nThis patch assumes the path returned by make_absolute_path() is never\nlonger than PATH_MAX, which I think is a safe assumption.\n\n abspath.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex 91ca00f..9149a98 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -24,7 +24,7 @@ const char *make_absolute_path(const char *path)\n \tchar *last_elem = NULL;\n \tstruct stat st;\n \n-\tif (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)\n+\tif (buf != path && strlcpy(buf, path, PATH_MAX) >= PATH_MAX)\n \t\tdie (\"Too long path: %.*s\", 60, path);\n \n \twhile (depth--) {\n-- \n1.7.4.1\n"},{"id":"163332","messageId":"1300130318-11279-3-git-send-email-cmn@elego.de","threadId":"26730","inReplyTo":"1300130318-11279-1-git-send-email-cmn@elego.de","subject":"[PATCH 2/3] setup_path(): Free temporary buffer","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-14T19:18:37Z","receivedAt":"2011-03-14T19:18:37Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"Make sure the pointer git_exec_path() returns is in the heap and free\nit after it's no longer needed.\n\nSigned-off-by: Carlos Martín Nieto \n---\n exec_cmd.c | 9 ++++++---\n 1 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 38545e8..c16c3d4 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -73,11 +73,11 @@ const char *git_exec_path(void)\n \tconst char *env;\n \n \tif (argv_exec_path)\n-\t\treturn argv_exec_path;\n+\t\treturn xstrdup(argv_exec_path);\n \n \tenv = getenv(EXEC_PATH_ENVIRONMENT);\n \tif (env && *env) {\n-\t\treturn env;\n+\t\treturn xstrdup(env);\n \t}\n \n \treturn system_path(GIT_EXEC_PATH);\n@@ -99,10 +99,13 @@ void setup_path(void)\n {\n \tconst char *old_path = getenv(\"PATH\");\n \tstruct strbuf new_path = STRBUF_INIT;\n+\tchar *exec_path = (char *) git_exec_path();\n \n-\tadd_path(&new_path, git_exec_path());\n+\tadd_path(&new_path, exec_path);\n \tadd_path(&new_path, argv0_path);\n \n+\tfree(exec_path);\n+\n \tif (old_path)\n \t\tstrbuf_addstr(&new_path, old_path);\n \telse\n-- \n1.7.4.1\n"},{"id":"163330","messageId":"1300130318-11279-4-git-send-email-cmn@elego.de","threadId":"26730","inReplyTo":"1300130318-11279-1-git-send-email-cmn@elego.de","subject":"[PATCH 3/3] clone: Free a few paths","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-14T19:18:38Z","receivedAt":"2011-03-14T19:18:38Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"Free the path, repo, dir buffers\n\nSigned-off-by: Carlos Martín Nieto \n---\n builtin/clone.c | 9 ++++++---\n 1 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 60d9a64..d90770a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -365,8 +365,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n {\n \tint is_bundle = 0, is_local;\n \tstruct stat buf;\n-\tconst char *repo_name, *repo, *work_tree, *git_dir;\n-\tchar *path, *dir;\n+\tconst char *repo_name, *work_tree, *git_dir;\n+\tchar *path, *dir, *repo;\n \tint dest_exists;\n \tconst struct ref *refs, *remote_head;\n \tconst struct ref *remote_head_points_at;\n@@ -415,7 +415,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \telse if (!strchr(repo_name, ':'))\n \t\trepo = xstrdup(make_absolute_path(repo_name));\n \telse\n-\t\trepo = repo_name;\n+\t\trepo = xstrdup(repo_name);\n \tis_local = path && !is_bundle;\n \tif (is_local && option_depth)\n \t\twarning(\"--depth is ignored in local clones; use file:// instead.\");\n@@ -665,6 +665,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\terr = run_command_v_opt(argv_submodule, RUN_GIT_CMD);\n \t}\n \n+\tfree(dir);\n+\tfree(repo);\n+\tfree(path);\n \tstrbuf_release(&reflog_msg);\n \tstrbuf_release(&branch_top);\n \tstrbuf_release(&key);\n-- \n1.7.4.1\n"},{"id":"163337","messageId":"20110314194501.GA30483@elie","threadId":"26730","inReplyTo":"1300130318-11279-4-git-send-email-cmn@elego.de","subject":"Re: [PATCH 3/3] clone: Free a few paths","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-14T19:45:01Z","receivedAt":"2011-03-14T19:45:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nCarlos Martín Nieto wrote:\n\n> Free the path, repo, dir buffers\n> \n> Signed-off-by: Carlos Martín Nieto \n[...]\n> +\tfree(dir);\n> +\tfree(repo);\n> +\tfree(path);\n> \tstrbuf_release(&reflog_msg);\n> \tstrbuf_release(&branch_top);\n> \tstrbuf_release(&key);\n\nThanks. The commit message should probably mention that this is for\nthe sake of valgrind rather a true memory leak, since the memory is\nfreed by _exit at the appropriate time already.\n\nThe patch itself seems sane, since the performance effect should be\nnegligible.\n\nBut it reminds me: does \"valgrind --tool=memcheck\" provide a way to\nannotate allocations like these? In other words, is it be possible to\nhave functions xmalloc_permanent and xstrdup_permanent that\n\n * allocate a buffer that is never meant to be freed;\n * do not cause valgrind to complain;\n * could be reimplemented some day by taking allocations from a large\n contiguous pool, to avoid malloc overhead and to take advantage of\n the knowledge that these allocations never need to be freed\n\n?\n\nCurious,\nJonathan\n"},{"id":"163340","messageId":"20110314200245.GB22602@sigill.intra.peff.net","threadId":"26730","inReplyTo":"1300130318-11279-2-git-send-email-cmn@elego.de","subject":"Re: [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-14T20:02:45Z","receivedAt":"2011-03-14T20:02:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 14, 2011 at 08:18:36PM +0100, Carlos Martín Nieto wrote:\n\n> Sometimes (at least in t-0001-init.sh test 12), the return value of\n> make_absolute_path() is passed to it as an argument, making the first\n> and second arguments to strlcpy() the same, making the test fail when\n> run under valgrind.\n\nThanks, I can reproduce here and your fix looks good. I used to\nperiodically run valgrind on the whole suite, but I haven't for quite a\nwhile. I should probably start doing it again.\n\n-Peff\n"},{"id":"163341","messageId":"20110314200958.GC22602@sigill.intra.peff.net","threadId":"26730","inReplyTo":"1300130318-11279-3-git-send-email-cmn@elego.de","subject":"Re: [PATCH 2/3] setup_path(): Free temporary buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-14T20:09:58Z","receivedAt":"2011-03-14T20:09:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 14, 2011 at 08:18:37PM +0100, Carlos Martín Nieto wrote:\n\n> Make sure the pointer git_exec_path() returns is in the heap and free\n> it after it's no longer needed.\n\nCould you explain the motivation a bit more? From looking at the code,\nit looks like the situation is that git_exec_path sometimes returns a\nnewly allocated string and sometimes not, so the caller may leak in the\nformer case.\n\nThat seems to be the fault of system_path, which sometimes allocates and\nsometimes does not. Should we also be fixing system_path, which is a\nsource of leaks? Or instead converting system_path to use a static\nbuffer?\n\n> @@ -99,10 +99,13 @@ void setup_path(void)\n> {\n> \tconst char *old_path = getenv(\"PATH\");\n> \tstruct strbuf new_path = STRBUF_INIT;\n> +\tchar *exec_path = (char *) git_exec_path();\n\nIck. If it is now returning an allocated buffer that the caller is\nresponsible for free-ing, then its declaration should drop the const in\nthe return value.\n\nWhat about other callers of git_exec_path? Aren't load_command_list and\nlist_commands leaking, too (and possibly worse, since we now always\nallocated memory)?\n\n-Peff\n"},{"id":"163342","messageId":"7v7hc1cvdt.fsf@alter.siamese.dyndns.org","threadId":"26730","inReplyTo":"1300130318-11279-3-git-send-email-cmn@elego.de","subject":"Re: [PATCH 2/3] setup_path(): Free temporary buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-14T20:14:22Z","receivedAt":"2011-03-14T20:14:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> Make sure the pointer git_exec_path() returns is in the heap and free\n> it after it's no longer needed.\n>\n> Signed-off-by: Carlos Martín Nieto \n> ---\n> exec_cmd.c | 9 ++++++---\n> 1 files changed, 6 insertions(+), 3 deletions(-)\n\nThe proposed log message does not explain half of what this patch does, if\nI am reading the patch correctly. Here is what I would consider as a fair\ndescription:\n\n Subject: git_exec_path: always return a free-able string\n\n git_exec_path() returns a string that the callers do not have to free\n in most cases, but when it calls into system_path() and ends up going\n into runtime-prefix codepath, it allocates an extra string on the\n heap, which ends up leaking because the callers are not allowed to\n free it.\n\n In order to allow the callers to clean up in all cases, change the API\n of git_exec_path() to always return a string on heap. This requires\n all the callers to free it.\n\n Update only one caller setup_path() to follow the new API, but leave\n other callers to leak even in normal cases. The caller in git.c exits\n immediately after using it, so we don't care about the leak there\n anyway. Also help.c has a few calls to it but the number of calls to\n the function is small and bounded, so the leak is small and we don't\n care.\n\n\nAnd then reviewers can agree or disagree if the small leaks in git.c (just\none string allocation that immediately is followed by exit after its use)\nand help.c (in list_commands() and load_commands_list(), neither of which\nis called millions of times anyway) are OK to accept.\n\nI tend to think they are Ok, but then I also tend to think one leak of\nexec-path return value in setup_path() is perfectly fine for the same\nreason, so in that sense I don't see a point in this patch...\n\n> diff --git a/exec_cmd.c b/exec_cmd.c\n> index 38545e8..c16c3d4 100644\n> --- a/exec_cmd.c\n> +++ b/exec_cmd.c\n> @@ -73,11 +73,11 @@ const char *git_exec_path(void)\n> \tconst char *env;\n> \n> \tif (argv_exec_path)\n> -\t\treturn argv_exec_path;\n> +\t\treturn xstrdup(argv_exec_path);\n> \n> \tenv = getenv(EXEC_PATH_ENVIRONMENT);\n> \tif (env && *env) {\n> -\t\treturn env;\n> +\t\treturn xstrdup(env);\n> \t}\n> \n> \treturn system_path(GIT_EXEC_PATH);\n> @@ -99,10 +99,13 @@ void setup_path(void)\n> {\n> \tconst char *old_path = getenv(\"PATH\");\n> \tstruct strbuf new_path = STRBUF_INIT;\n> +\tchar *exec_path = (char *) git_exec_path();\n> \n> -\tadd_path(&new_path, git_exec_path());\n> +\tadd_path(&new_path, exec_path);\n> \tadd_path(&new_path, argv0_path);\n> \n> +\tfree(exec_path);\n> +\n> \tif (old_path)\n> \t\tstrbuf_addstr(&new_path, old_path);\n> \telse\n"},{"id":"163343","messageId":"7v39mpcuv9.fsf@alter.siamese.dyndns.org","threadId":"26730","inReplyTo":"1300130318-11279-2-git-send-email-cmn@elego.de","subject":"Re: [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-14T20:25:30Z","receivedAt":"2011-03-14T20:25:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> Sometimes (at least in t-0001-init.sh test 12), the return value of\n> make_absolute_path() is passed to it as an argument, making the first\n\nI don't think it is a bad idea per-se to avoid a copy from the same memory\nlocation into the same memory location, but independent of the necessity\nof fixes at the low-level, shouldn't we fix the callers that do not check\nif what they have is already absolute?\n\n> and second arguments to strlcpy() the same, making the test fail when\n> run under valgrind.\n>\n> Signed-off-by: Carlos Martín Nieto \n> ---\n>\n> This patch assumes the path returned by make_absolute_path() is never\n> longer than PATH_MAX, which I think is a safe assumption.\n\n>\n> abspath.c | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/abspath.c b/abspath.c\n> index 91ca00f..9149a98 100644\n> --- a/abspath.c\n> +++ b/abspath.c\n> @@ -24,7 +24,7 @@ const char *make_absolute_path(const char *path)\n> \tchar *last_elem = NULL;\n> \tstruct stat st;\n> \n> -\tif (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)\n> +\tif (buf != path && strlcpy(buf, path, PATH_MAX) >= PATH_MAX)\n> \t\tdie (\"Too long path: %.*s\", 60, path);\n> \n> \twhile (depth--) {\n"},{"id":"163347","messageId":"1300140119.4320.38.camel@bee.lab.cmartin.tk","threadId":"26730","inReplyTo":"7v7hc1cvdt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] setup_path(): Free temporary buffer","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-14T22:01:53Z","receivedAt":"2011-03-14T22:01:53Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On lun, 2011-03-14 at 13:14 -0700, Junio C Hamano wrote:\n> Carlos Martín Nieto writes:\n> [...]\n> \n> Update only one caller setup_path() to follow the new API, but leave\n> other callers to leak even in normal cases. The caller in git.c exits\n> immediately after using it, so we don't care about the leak there\n> anyway. Also help.c has a few calls to it but the number of calls to\n> the function is small and bounded, so the leak is small and we don't\n> care.\n\n Oops. I blindly tested the path only on the test case where it was\ntriggering an error without it and completely missed the other uses,\nwhich is embarrassing.\n\n> \n> \n> And then reviewers can agree or disagree if the small leaks in git.c (just\n> one string allocation that immediately is followed by exit after its use)\n> and help.c (in list_commands() and load_commands_list(), neither of which\n> is called millions of times anyway) are OK to accept.\n> \n> I tend to think they are Ok, but then I also tend to think one leak of\n> exec-path return value in setup_path() is perfectly fine for the same\n> reason, so in that sense I don't see a point in this patch...\n\n Which brings us to the matter of whether we actually care about memory\nleaks, as the processes are short-lived and the system is going to clean\nup after us. Do we, unless the leaks are huge? As there is built-in\nvalgrind support in the test suite, I went in with the assumption that\nwe did. It seems however that hardly any code paths free their memory,\nother than when using strbuf.\n\n In case we don't, valgrind should be told not to bother reporting leaks\n(and maybe mention in some document that small leaks are not an issue).\n\n> \n> > diff --git a/exec_cmd.c b/exec_cmd.c\n> > index 38545e8..c16c3d4 100644\n> > --- a/exec_cmd.c\n> > +++ b/exec_cmd.c\n> > @@ -73,11 +73,11 @@ const char *git_exec_path(void)\n> > \tconst char *env;\n> > \n> > \tif (argv_exec_path)\n> > -\t\treturn argv_exec_path;\n> > +\t\treturn xstrdup(argv_exec_path);\n> > \n> > \tenv = getenv(EXEC_PATH_ENVIRONMENT);\n> > \tif (env && *env) {\n> > -\t\treturn env;\n> > +\t\treturn xstrdup(env);\n> > \t}\n> > \n> > \treturn system_path(GIT_EXEC_PATH);\n> > @@ -99,10 +99,13 @@ void setup_path(void)\n> > {\n> > \tconst char *old_path = getenv(\"PATH\");\n> > \tstruct strbuf new_path = STRBUF_INIT;\n> > +\tchar *exec_path = (char *) git_exec_path();\n> > \n> > -\tadd_path(&new_path, git_exec_path());\n> > +\tadd_path(&new_path, exec_path);\n> > \tadd_path(&new_path, argv0_path);\n> > \n> > +\tfree(exec_path);\n> > +\n> > \tif (old_path)\n> > \t\tstrbuf_addstr(&new_path, old_path);\n> > \telse\n"},{"id":"163348","messageId":"1300140128.4320.39.camel@bee.lab.cmartin.tk","threadId":"26730","inReplyTo":"7v39mpcuv9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-14T22:02:07Z","receivedAt":"2011-03-14T22:02:07Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On lun, 2011-03-14 at 13:25 -0700, Junio C Hamano wrote:\n> Carlos Martín Nieto writes:\n> \n> > Sometimes (at least in t-0001-init.sh test 12), the return value of\n> > make_absolute_path() is passed to it as an argument, making the first\n> \n> I don't think it is a bad idea per-se to avoid a copy from the same memory\n> location into the same memory location, but independent of the necessity\n> of fixes at the low-level, shouldn't we fix the callers that do not check\n> if what they have is already absolute?\n\n If we'd like the semantics to be \"whatever I had, I now know what the\nabsolute path is\" then we could make the check in the beginning of the\nfunction, to centralise the check. If the semantics should be \"I don't\nhave an absolute path, so I need to figure out what it is\", then there\nshould be a check before calling make_absolute_path() (the name suggests\nthe second).\n\n As there are only ~15-20 uses of make_absolute_path(), we could just\nleave this as it is (as it works under normal conditions and causes\nvalgrind to warn us), and I'll examine the callers tomorrow.\n\n There is however the extra functionality the function offers, namely\nresolving links. It might be good to split it into two functions so each\ncaller can specify what it wants.\n\n There is also this in setup.c:prefix_path()\n\n const char *orig = path;\n char *sanitized;\n if (is_absolute_path(orig)) {\n const char *temp = make_absolute_path(path);\n sanitized = xmalloc(len + strlen(temp) + 1);\n strcpy(sanitized, temp);\n } else {\n sanitized = xmalloc(len + strlen(path) + 1);\n if (len)\n memcpy(sanitized, prefix, len);\n strcpy(sanitized + len, path);\n }\n\nwhich doesn't seem to make sense. Should the call to\nmake_absolute_path() actually be a call to a function resolve_links()\nthat just incorporates that functionality from make_absolute_path()?\n\n\n> \n> > and second arguments to strlcpy() the same, making the test fail when\n> > run under valgrind.\n> >\n> > Signed-off-by: Carlos Martín Nieto \n> > ---\n> >\n> > This patch assumes the path returned by make_absolute_path() is never\n> > longer than PATH_MAX, which I think is a safe assumption.\n> \n> >\n> > abspath.c | 2 +-\n> > 1 files changed, 1 insertions(+), 1 deletions(-)\n> >\n> > diff --git a/abspath.c b/abspath.c\n> > index 91ca00f..9149a98 100644\n> > --- a/abspath.c\n> > +++ b/abspath.c\n> > @@ -24,7 +24,7 @@ const char *make_absolute_path(const char *path)\n> > \tchar *last_elem = NULL;\n> > \tstruct stat st;\n> > \n> > -\tif (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)\n> > +\tif (buf != path && strlcpy(buf, path, PATH_MAX) >= PATH_MAX)\n> > \t\tdie (\"Too long path: %.*s\", 60, path);\n> > \n> > \twhile (depth--) {\n"},{"id":"163349","messageId":"1300141129.4320.48.camel@bee.lab.cmartin.tk","threadId":"26730","inReplyTo":"20110314200958.GC22602@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] setup_path(): Free temporary buffer","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-14T22:18:49Z","receivedAt":"2011-03-14T22:18:49Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On lun, 2011-03-14 at 16:09 -0400, Jeff King wrote:\n> On Mon, Mar 14, 2011 at 08:18:37PM +0100, Carlos Martín Nieto wrote:\n> \n> > Make sure the pointer git_exec_path() returns is in the heap and free\n> > it after it's no longer needed.\n> \n> Could you explain the motivation a bit more? From looking at the code,\n> it looks like the situation is that git_exec_path sometimes returns a\n> newly allocated string and sometimes not, so the caller may leak in the\n> former case.\n\n I mostly wanted valgrind to shut up, but it seems there are a lot of\nsmall leaks we don't really care about.\n\n> \n> That seems to be the fault of system_path, which sometimes allocates and\n> sometimes does not. Should we also be fixing system_path, which is a\n> source of leaks? Or instead converting system_path to use a static\n> buffer?\n\n Converting system_path() to use a static buffer would also work, and\nwould probably be an easier solution. I'll look at that tomorrow and\nresend.\n\n> \n> > @@ -99,10 +99,13 @@ void setup_path(void)\n> > {\n> > \tconst char *old_path = getenv(\"PATH\");\n> > \tstruct strbuf new_path = STRBUF_INIT;\n> > +\tchar *exec_path = (char *) git_exec_path();\n> \n> Ick. If it is now returning an allocated buffer that the caller is\n> responsible for free-ing, then its declaration should drop the const in\n> the return value.\n\n Yeah... I meant to delete that. It seems I've completely botched this\npatch.\n\n> \n> What about other callers of git_exec_path? Aren't load_command_list and\n> list_commands leaking, too (and possibly worse, since we now always\n> allocated memory)?\n\n Test-suite-induced tunnel vision. I only tested what reported a failure\n(under valgrind) without the patch.\n\n cmn\n"},{"id":"163352","messageId":"7vpqptb976.fsf@alter.siamese.dyndns.org","threadId":"26730","inReplyTo":"1300140128.4320.39.camel@bee.lab.cmartin.tk","subject":"Re: [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-14T22:58:53Z","receivedAt":"2011-03-14T22:58:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n>> I don't think it is a bad idea per-se to avoid a copy from the same memory\n>> location into the same memory location, but independent of the necessity\n>> of fixes at the low-level, shouldn't we fix the callers that do not check\n>> if what they have is already absolute?\n>\n> If we'd like the semantics to be \"whatever I had, I now know what the\n> absolute path is\" then we could make the check in the beginning of the\n> function, to centralise the check. If the semantics should be \"I don't\n> have an absolute path, so I need to figure out what it is\", then there\n> should be a check before calling make_absolute_path() (the name suggests\n> the second).\n\nGood thinking, and I think the former semantics would be easier to use.\n\n> There is however the extra functionality the function offers, namely\n> resolving links. It might be good to split it into two functions so each\n> caller can specify what it wants.\n\nProbably.\n"},{"id":"163361","messageId":"20110315011230.GA31865@sigill.intra.peff.net","threadId":"26730","inReplyTo":"1300140119.4320.38.camel@bee.lab.cmartin.tk","subject":"Re: [PATCH 2/3] setup_path(): Free temporary buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-15T01:12:30Z","receivedAt":"2011-03-15T01:12:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 14, 2011 at 11:01:53PM +0100, Carlos Martín Nieto wrote:\n\n> > I tend to think they are Ok, but then I also tend to think one leak of\n> > exec-path return value in setup_path() is perfectly fine for the same\n> > reason, so in that sense I don't see a point in this patch...\n> \n> Which brings us to the matter of whether we actually care about memory\n> leaks, as the processes are short-lived and the system is going to clean\n> up after us. Do we, unless the leaks are huge? As there is built-in\n> valgrind support in the test suite, I went in with the assumption that\n> we did. It seems however that hardly any code paths free their memory,\n> other than when using strbuf.\n> \n> In case we don't, valgrind should be told not to bother reporting leaks\n> (and maybe mention in some document that small leaks are not an issue).\n\nDon't we already use --leak-check=no in our valgrind support for the\ntest suite? The valgrind support is there primarily to find memory\naccess errors, not leaks.\n\nIt would be nice if it could find leaks, too, but there is currently a\nlot of noise due to uninteresting leaks like this. I haven't looked at\nvalgrind's support for suppressing leak reporting, but presumably we\ncould build a suppression file that would drop the uninteresting ones.\nWe could also fix them, of course, but if they are one-time-per-program\nallocations, it might not be worth cluttering the code.\n\n-Peff\n"},{"id":"163374","messageId":"1300181536.19100.7.camel@bee.lab.cmartin.tk","threadId":"26730","inReplyTo":"20110315011230.GA31865@sigill.intra.peff.net","subject":"[PATCH] t/README: Add a note about running commands under valgrind","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-15T09:32:11Z","receivedAt":"2011-03-15T09:32:11Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"The test suite runs valgrind with certain options activated. Add a\nnote saying how to run commands under the same conditions as the test\nsuite does.\n\nSigned-off-by: Carlos Martín Nieto \n---\n\n As Jeff pointed out, the test suite does use --leak-check=no. I was\nusing valgrind manually as I was chasing a different error that does\nshow up. How about adding this to the README?\n\n t/README | 7 +++++++\n 1 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 78c9e65..2a73fc3 100644\n--- a/t/README\n+++ b/t/README\n@@ -98,6 +98,13 @@ appropriately before running \"make\".\n \tnot see any output, this option implies --verbose. For\n \tconvenience, it also implies --tee.\n \n+\t*NOTE*: As the git process is short-lived and some errors are\n+\tnot interesting, valgrind is run with (among others) the\n+\toption --leak-check=no. In order to run a single command under\n+\tthe same conditions manually, you should set GIT_VALGRIND to\n+\tpoint to the 't/valgrind/' directory and use the commands\n+\tunder 't/valgrind/bin/'.\n+\n --tee::\n \tIn addition to printing the test output to the terminal,\n \twrite it to files named 't/test-results/$TEST_NAME.out'.\n-- \n1.7.4.1\n"},{"id":"163382","messageId":"1300190396.19100.31.camel@bee.lab.cmartin.tk","threadId":"26730","inReplyTo":"7vpqptb976.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-15T11:59:49Z","receivedAt":"2011-03-15T11:59:49Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On lun, 2011-03-14 at 15:58 -0700, Junio C Hamano wrote:\n> Carlos Martín Nieto writes:\n> \n> >> I don't think it is a bad idea per-se to avoid a copy from the same memory\n> >> location into the same memory location, but independent of the necessity\n> >> of fixes at the low-level, shouldn't we fix the callers that do not check\n> >> if what they have is already absolute?\n> >\n> > If we'd like the semantics to be \"whatever I had, I now know what the\n> > absolute path is\" then we could make the check in the beginning of the\n> > function, to centralise the check. If the semantics should be \"I don't\n> > have an absolute path, so I need to figure out what it is\", then there\n> > should be a check before calling make_absolute_path() (the name suggests\n> > the second).\n> \n> Good thinking, and I think the former semantics would be easier to use.\n\n I was asking about why we didn't just use realpath, but it seems there\nis no portable way of dealing with it (even without counting NFS over\ndifferent systems).\n\n I'm thinking of renaming make_absolute_path to real_path as it's in\nfact our implementation of realpath (and actually describes what it\ndoes), without a is_absolute_path check, as a path could be absolute but\npoint to a link, adding a comment to say that if you don't mind links,\nyou should use make_nonrelative_path (I'd rename it to absolute_path,\nbut that may be too close to the old make_absolute_path)\n\n> \n> > There is however the extra functionality the function offers, namely\n> > resolving links. It might be good to split it into two functions so each\n> > caller can specify what it wants.\n> \n> Probably.\n\n With the changes mentioned earlier, if you want an absolute pathname,\nyou'd call absolute_path/make_nonrelative_path and if you want to make\nsure you have the real path of the target file, you'd use real_path just\nas you'd use realpath on a sane system, with\n\n cmn\n"},{"id":"163386","messageId":"1300192832.19100.35.camel@bee.lab.cmartin.tk","threadId":"26730","inReplyTo":"1300190396.19100.31.camel@bee.lab.cmartin.tk","subject":"Re: [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-15T12:40:31Z","receivedAt":"2011-03-15T12:40:31Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On mar, 2011-03-15 at 12:59 +0100, Carlos Martín Nieto wrote:\n> On lun, 2011-03-14 at 15:58 -0700, Junio C Hamano wrote:\n> > Carlos Martín Nieto writes:\n[...]\n> > \n> > > There is however the extra functionality the function offers, namely\n> > > resolving links. It might be good to split it into two functions so each\n> > > caller can specify what it wants.\n> > \n> > Probably.\n> \n> With the changes mentioned earlier, if you want an absolute pathname,\n> you'd call absolute_path/make_nonrelative_path and if you want to make\n> sure you have the real path of the target file, you'd use real_path just\n> as you'd use realpath on a sane system, with\n\n ... a comment on the functions and maybe some documentation in\nDocumentation/techncal, as it doesn't seem to exist yet.\n\n cmn\n"},{"id":"163395","messageId":"7vd3lsb9m3.fsf@alter.siamese.dyndns.org","threadId":"26730","inReplyTo":"1300192832.19100.35.camel@bee.lab.cmartin.tk","subject":"Re: [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-15T17:02:12Z","receivedAt":"2011-03-15T17:02:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> On mar, 2011-03-15 at 12:59 +0100, Carlos Martín Nieto wrote:\n>> On lun, 2011-03-14 at 15:58 -0700, Junio C Hamano wrote:\n>> > Carlos Martín Nieto writes:\n> [...]\n>> > \n>> > > There is however the extra functionality the function offers, namely\n>> > > resolving links. It might be good to split it into two functions so each\n>> > > caller can specify what it wants.\n>> > \n>> > Probably.\n>> \n>> With the changes mentioned earlier, if you want an absolute pathname,\n>> you'd call absolute_path/make_nonrelative_path and if you want to make\n>> sure you have the real path of the target file, you'd use real_path just\n>> as you'd use realpath on a sane system, with\n>\n> ... a comment on the functions and maybe some documentation in\n> Documentation/techncal, as it doesn't seem to exist yet.\n\nWe probably should involve Nguyễn in this thread as his fingers are\neverywhere on the codepaths related to setup.\n"},{"id":"163396","messageId":"7v8vwgb9fj.fsf@alter.siamese.dyndns.org","threadId":"26730","inReplyTo":"1300181536.19100.7.camel@bee.lab.cmartin.tk","subject":"Re: [PATCH] t/README: Add a note about running commands under valgrind","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-15T17:06:08Z","receivedAt":"2011-03-15T17:06:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> As Jeff pointed out, the test suite does use --leak-check=no. I was\n> using valgrind manually as I was chasing a different error that does\n> show up. How about adding this to the README?\n\n> diff --git a/t/README b/t/README\n> index 78c9e65..2a73fc3 100644\n> --- a/t/README\n> +++ b/t/README\n> @@ -98,6 +98,13 @@ appropriately before running \"make\".\n> \tnot see any output, this option implies --verbose. For\n> \tconvenience, it also implies --tee.\n> \n> +\t*NOTE*: As the git process is short-lived and some errors are\n> +\tnot interesting, valgrind is run with (among others) the\n> +\toption --leak-check=no. In order to run a single command under\n> +\tthe same conditions manually, you should set GIT_VALGRIND to\n> +\tpoint to the 't/valgrind/' directory and use the commands\n> +\tunder 't/valgrind/bin/'.\n\nI think what the text says is a good addition. If I were writing this\nmyself, I would rephrase \"*NOTE*:\" to say \"Note that ...\", though, as that\nseems to be more in line with the other parts of the document.\n"},{"id":"163397","messageId":"1300208923.19100.39.camel@bee.lab.cmartin.tk","threadId":"26730","inReplyTo":"7v8vwgb9fj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t/README: Add a note about running commands under valgrind","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-15T17:08:42Z","receivedAt":"2011-03-15T17:08:42Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On mar, 2011-03-15 at 10:06 -0700, Junio C Hamano wrote:\n> Carlos Martín Nieto writes:\n> \n> > As Jeff pointed out, the test suite does use --leak-check=no. I was\n> > using valgrind manually as I was chasing a different error that does\n> > show up. How about adding this to the README?\n> \n> > diff --git a/t/README b/t/README\n> > index 78c9e65..2a73fc3 100644\n> > --- a/t/README\n> > +++ b/t/README\n> > @@ -98,6 +98,13 @@ appropriately before running \"make\".\n> > \tnot see any output, this option implies --verbose. For\n> > \tconvenience, it also implies --tee.\n> > \n> > +\t*NOTE*: As the git process is short-lived and some errors are\n> > +\tnot interesting, valgrind is run with (among others) the\n> > +\toption --leak-check=no. In order to run a single command under\n> > +\tthe same conditions manually, you should set GIT_VALGRIND to\n> > +\tpoint to the 't/valgrind/' directory and use the commands\n> > +\tunder 't/valgrind/bin/'.\n> \n> I think what the text says is a good addition. If I were writing this\n> myself, I would rephrase \"*NOTE*:\" to say \"Note that ...\", though, as that\n> seems to be more in line with the other parts of the document.\n\n Good point. Should I resend?\n\n cmn\n"},{"id":"163399","messageId":"1300210062.19100.46.camel@bee.lab.cmartin.tk","threadId":"26730","inReplyTo":"7vd3lsb9m3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-15T17:27:41Z","receivedAt":"2011-03-15T17:27:41Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On mar, 2011-03-15 at 10:02 -0700, Junio C Hamano wrote:\n> Carlos Martín Nieto writes:\n> \n> > On mar, 2011-03-15 at 12:59 +0100, Carlos Martín Nieto wrote:\n> >> On lun, 2011-03-14 at 15:58 -0700, Junio C Hamano wrote:\n> >> > Carlos Martín Nieto writes:\n> > [...]\n> >> > \n> >> > > There is however the extra functionality the function offers, namely\n> >> > > resolving links. It might be good to split it into two functions so each\n> >> > > caller can specify what it wants.\n> >> > \n> >> > Probably.\n> >> \n> >> With the changes mentioned earlier, if you want an absolute pathname,\n> >> you'd call absolute_path/make_nonrelative_path and if you want to make\n> >> sure you have the real path of the target file, you'd use real_path just\n> >> as you'd use realpath on a sane system, with\n> >\n> > ... a comment on the functions and maybe some documentation in\n> > Documentation/techncal, as it doesn't seem to exist yet.\n> \n> We probably should involve Nguyễn in this thread as his fingers are\n> everywhere on the codepaths related to setup.\n> \n\n I've been changing this a bit, trying to make all the paths normalized,\nbut it'll take a bit longer. I'll send a partial patch when I've\nfinished something worth seeing (for the moment, the test fail if there\nis a symlink somewhere in the tree, as I've mixed\nreal_path/make_absolute_path and absolute_path/make_nonrelative_path a\nbit).\n\n Is it a good idea to normalize the paths? Otherwise, everything could\nbe replaced by real_path/make_absolute_path (as most calls already are).\nAs it's transitive and these paths aren't stored permanently (other than\nwith clone), as long as we agree on one representation, it should be\nfine.\n\n Is there a performance hit if we resolve links all the time? If we run\neverything through normalize_path(_copy), is it slower than resolving\nlinks?\n\n cmn\n"},{"id":"163458","messageId":"1300274770-4798-1-git-send-email-cmn@elego.de","threadId":"26730","inReplyTo":"20110314200958.GC22602@sigill.intra.peff.net","subject":"[PATCH] system_path: use a static buffer","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-16T11:26:10Z","receivedAt":"2011-03-16T11:26:10Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"Make system_path behave like the other path functions by using a\nstatic buffer, fixing a memory leak.\n\nAlso make sure the prefix pointer is always initialized to either\nPREFIX or NULL.\n\nSigned-off-by: Carlos Martín Nieto \n---\n\nThis fixes the leak I was trying to fix with my original patch, but\nthis seems much cleaner\n\n exec_cmd.c | 9 ++++-----\n 1 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 38545e8..12ce017 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -9,11 +9,11 @@ static const char *argv0_path;\n const char *system_path(const char *path)\n {\n #ifdef RUNTIME_PREFIX\n-\tstatic const char *prefix;\n+\tstatic const char *prefix = NULL;\n #else\n \tstatic const char *prefix = PREFIX;\n #endif\n-\tstruct strbuf d = STRBUF_INIT;\n+\tstatic char buf[PATH_MAX+1];\n \n \tif (is_absolute_path(path))\n \t\treturn path;\n@@ -33,9 +33,8 @@ const char *system_path(const char *path)\n \t}\n #endif\n \n-\tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n-\tpath = strbuf_detach(&d, NULL);\n-\treturn path;\n+\tsnprintf(buf, PATH_MAX, \"%s/%s\", prefix, path);\n+\treturn buf;\n }\n \n const char *git_extract_argv0_path(const char *argv0)\n-- \n1.7.4.1\n"},{"id":"163471","messageId":"AANLkTimvVd8VrAvcuT9Qww07-Gg1JtyA=+8nr4EeU0_D@mail.gmail.com","threadId":"26730","inReplyTo":"7vd3lsb9m3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-03-16T14:04:11Z","receivedAt":"2011-03-16T14:04:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 16, 2011 at 12:02 AM, Junio C Hamano wrote:\n> Carlos Martín Nieto writes:\n>\n>> On mar, 2011-03-15 at 12:59 +0100, Carlos Martín Nieto wrote:\n>>> On lun, 2011-03-14 at 15:58 -0700, Junio C Hamano wrote:\n>>> > Carlos Martín Nieto writes:\n>> [...]\n>>> >\n>>> > >  There is however the extra functionality the function offers, namely\n>>> > > resolving links. It might be good to split it into two functions so each\n>>> > > caller can specify what it wants.\n>>> >\n>>> > Probably.\n>>>\n>>>  With the changes mentioned earlier, if you want an absolute pathname,\n>>> you'd call absolute_path/make_nonrelative_path and if you want to make\n>>> sure you have the real path of the target file, you'd use real_path just\n>>> as you'd use realpath on a sane system, with\n>>\n>>  ... a comment on the functions and maybe some documentation in\n>> Documentation/techncal, as it doesn't seem to exist yet.\n>\n> We probably should involve Nguyễn in this thread as his fingers are\n> everywhere on the codepaths related to setup.\n\nThanks, my attempt to fix up setup code leaves more traces that I expect.\n\nSplitting functions is fine, but is there any use of\nabsolute_path/make_nonrelative_path alone? Most setup code uses\nmake_absolute_path() for $GIT_{DIR,WORK_TREE}. For GIT_DIR, a\nresolved/normalized path is preferred. For GIT_WORK_TREE, I'm not\nsure. I tend to treat it the same way as GIT_DIR, but you guys may\nhave a special case.\n-- \nDuy\n"},{"id":"163472","messageId":"AANLkTimQ81mwYhWLzGunimQzapEUkMmvKj47PuPWPgm0@mail.gmail.com","threadId":"26730","inReplyTo":"1300210062.19100.46.camel@bee.lab.cmartin.tk","subject":"Re: [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-03-16T14:16:35Z","receivedAt":"2011-03-16T14:16:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/3/16 Carlos Martín Nieto :\n> I've been changing this a bit, trying to make all the paths normalized,\n> but it'll take a bit longer. I'll send a partial patch when I've\n> finished something worth seeing (for the moment, the test fail if there\n> is a symlink somewhere in the tree, as I've mixed\n> real_path/make_absolute_path and absolute_path/make_nonrelative_path a\n> bit).\n>\n>  Is it a good idea to normalize the paths? Otherwise, everything could\n> be replaced by real_path/make_absolute_path (as most calls already are).\n> As it's transitive and these paths aren't stored permanently (other than\n> with clone), as long as we agree on one representation, it should be\n> fine.\n\nI think the question is whether it's _necessary_ to do that. Any gain?\nmake_absolute_path() calls are not in critical path, I don't think we\nshould bother much, unless there are bugs like one you fixed in your\npatch.\n\n>  Is there a performance hit if we resolve links all the time? If we run\n> everything through normalize_path(_copy), is it slower than resolving\n> links?\n\nWhat paths are you talking about? If they are inside $GIT_DIR, we\ntouch them quite often. But there are not many of them (unless you\nspread loose objects all over the place), resolving links should not\nbe an issue.\n\nIf they are inside worktree, maybe. Though I'm not sure if we want to\nnormalize all of them.\n-- \nDuy\n"},{"id":"163478","messageId":"1300286976.7214.27.camel@bee.lab.cmartin.tk","threadId":"26730","inReplyTo":"AANLkTimQ81mwYhWLzGunimQzapEUkMmvKj47PuPWPgm0@mail.gmail.com","subject":"Re: [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-16T14:49:30Z","receivedAt":"2011-03-16T14:49:30Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On mié, 2011-03-16 at 21:16 +0700, Nguyen Thai Ngoc Duy wrote:\n> 2011/3/16 Carlos Martín Nieto :\n> > I've been changing this a bit, trying to make all the paths normalized,\n> > but it'll take a bit longer. I'll send a partial patch when I've\n> > finished something worth seeing (for the moment, the test fail if there\n> > is a symlink somewhere in the tree, as I've mixed\n> > real_path/make_absolute_path and absolute_path/make_nonrelative_path a\n> > bit).\n> >\n> > Is it a good idea to normalize the paths? Otherwise, everything could\n> > be replaced by real_path/make_absolute_path (as most calls already are).\n> > As it's transitive and these paths aren't stored permanently (other than\n> > with clone), as long as we agree on one representation, it should be\n> > fine.\n> \n> I think the question is whether it's _necessary_ to do that. Any gain?\n> make_absolute_path() calls are not in critical path, I don't think we\n> should bother much, unless there are bugs like one you fixed in your\n> patch.\n\n I was under the wrong impression that non-normalized paths were what\nwas causing is_inside_dir not to recognize the paths (this with a patch\nusing non-resolved absolute paths as given by make_nonrelative_path\nrather than make_absolute_path).\n As it turns out, getcwd resolves the links for us, so is_inside_dir\nwould say e.g. that /home/cmn/two/git/t wasn't under /home/cmn/two/git,\nbecause getcwd said the cwd was /home/cmn/one/git (two is a symlink to\none).\n\n At any rate, I think make_absolute_path is mis-named and it should be\ncalled real_path (or make_real_path). The difference between\nmake_nonrelative_path and make_absolute_path is certainly not clear\nwithout looking at the implementation.\n\n> \n> > Is there a performance hit if we resolve links all the time? If we run\n> > everything through normalize_path(_copy), is it slower than resolving\n> > links?\n> \n> What paths are you talking about? If they are inside $GIT_DIR, we\n> touch them quite often. But there are not many of them (unless you\n> spread loose objects all over the place), resolving links should not\n> be an issue.\n\n There aren't in fact that many calls to these functions, so resolving\nshould be fine. More on this as an answer to your other mail.\n"},{"id":"163479","messageId":"AANLkTinUqK=pxTGYZn0oLm5i9cs4=XRPjHXnRp-FP9xh@mail.gmail.com","threadId":"26730","inReplyTo":"1300286976.7214.27.camel@bee.lab.cmartin.tk","subject":"Re: [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-03-16T14:58:03Z","receivedAt":"2011-03-16T14:58:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 16, 2011 at 9:49 PM, Carlos Martín Nieto wrote:\n>  At any rate, I think make_absolute_path is mis-named and it should be\n> called real_path (or make_real_path). The difference between\n> make_nonrelative_path and make_absolute_path is certainly not clear\n> without looking at the implementation.\n\nMore precise, descriptive names are always appreciated :)\n-- \nDuy\n"},{"id":"163480","messageId":"1300288108.7214.37.camel@bee.lab.cmartin.tk","threadId":"26730","inReplyTo":"AANLkTimvVd8VrAvcuT9Qww07-Gg1JtyA=+8nr4EeU0_D@mail.gmail.com","subject":"Re: [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-16T15:08:28Z","receivedAt":"2011-03-16T15:08:28Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On mié, 2011-03-16 at 21:04 +0700, Nguyen Thai Ngoc Duy wrote:\n> On Wed, Mar 16, 2011 at 12:02 AM, Junio C Hamano wrote:\n> > Carlos Martín Nieto writes:\n> >\n> >> On mar, 2011-03-15 at 12:59 +0100, Carlos Martín Nieto wrote:\n> >>> On lun, 2011-03-14 at 15:58 -0700, Junio C Hamano wrote:\n> >>> > Carlos Martín Nieto writes:\n> >> [...]\n> >>> >\n> >>> > > There is however the extra functionality the function offers, namely\n> >>> > > resolving links. It might be good to split it into two functions so each\n> >>> > > caller can specify what it wants.\n> >>> >\n> >>> > Probably.\n> >>>\n> >>> With the changes mentioned earlier, if you want an absolute pathname,\n> >>> you'd call absolute_path/make_nonrelative_path and if you want to make\n> >>> sure you have the real path of the target file, you'd use real_path just\n> >>> as you'd use realpath on a sane system, with\n> >>\n> >> ... a comment on the functions and maybe some documentation in\n> >> Documentation/techncal, as it doesn't seem to exist yet.\n> >\n> > We probably should involve Nguyễn in this thread as his fingers are\n> > everywhere on the codepaths related to setup.\n> \n> Thanks, my attempt to fix up setup code leaves more traces that I expect.\n> \n> Splitting functions is fine, but is there any use of\n> absolute_path/make_nonrelative_path alone? Most setup code uses\n> make_absolute_path() for $GIT_{DIR,WORK_TREE}. For GIT_DIR, a\n> resolved/normalized path is preferred. For GIT_WORK_TREE, I'm not\n> sure. I tend to treat it the same way as GIT_DIR, but you guys may\n> have a special case.\n\n There only real use for absolute_path/make_nonrelative_path would be\nfor paths that are written to disk (as we should respect the user's path\npreferences). For paths we never write out, real_path/make_absolute_path\nmakes sense.\n\n The way I want to split it up is to have real_path use a simplistic\napproach, and use a resolve_link function if it needs to. From logging\nwhat paths make_absolute_path is called with, all I could find were\ndirectories (\".\" is used very often as well). This should make real_path\nvery easy to understand. I'll start a new thread with the renaming\npatches and then split up the function.\n"},{"id":"163485","messageId":"AANLkTinEMzezKdfGUrwKv7sJ+tSK5duYM6XZMvBa-yj3@mail.gmail.com","threadId":"26730","inReplyTo":"1300274770-4798-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH] system_path: use a static buffer","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-03-16T15:58:03Z","receivedAt":"2011-03-16T15:58:03Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Mar 16, 2011 at 12:26 PM, Carlos Martín Nieto wrote:\n> Make system_path behave like the other path functions by using a\n> static buffer, fixing a memory leak.\n>\n> Also make sure the prefix pointer is always initialized to either\n> PREFIX or NULL.\n>\n> Signed-off-by: Carlos Martín Nieto \n> ---\n>\n> This fixes the leak I was trying to fix with my original patch, but\n> this seems much cleaner\n>\n>  exec_cmd.c |    9 ++++-----\n>  1 files changed, 4 insertions(+), 5 deletions(-)\n>\n> diff --git a/exec_cmd.c b/exec_cmd.c\n> index 38545e8..12ce017 100644\n> --- a/exec_cmd.c\n> +++ b/exec_cmd.c\n> @@ -9,11 +9,11 @@ static const char *argv0_path;\n>  const char *system_path(const char *path)\n>  {\n>  #ifdef RUNTIME_PREFIX\n> -       static const char *prefix;\n> +       static const char *prefix = NULL;\n>  #else\n>        static const char *prefix = PREFIX;\n>  #endif\n> -       struct strbuf d = STRBUF_INIT;\n> +       static char buf[PATH_MAX+1];\n\nWhy PATH_MAX + 1? POSIX says that PATH_MAX includes the null-termination...\n\n> @@ -33,9 +33,8 @@ const char *system_path(const char *path)\n>        }\n>  #endif\n>\n> -       strbuf_addf(&d, \"%s/%s\", prefix, path);\n> -       path = strbuf_detach(&d, NULL);\n> -       return path;\n> +       snprintf(buf, PATH_MAX, \"%s/%s\", prefix, path);\n\nPerhaps \"snprintf(buf, sizeof(buf) - 1, \"%s/%s\", prefix, path);\" instead?\n"},{"id":"163486","messageId":"1300292667.7214.43.camel@bee.lab.cmartin.tk","threadId":"26730","inReplyTo":"AANLkTinEMzezKdfGUrwKv7sJ+tSK5duYM6XZMvBa-yj3@mail.gmail.com","subject":"Re: [PATCH] system_path: use a static buffer","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-16T16:24:21Z","receivedAt":"2011-03-16T16:24:21Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On mié, 2011-03-16 at 16:58 +0100, Erik Faye-Lund wrote:\n> On Wed, Mar 16, 2011 at 12:26 PM, Carlos Martín Nieto wrote:\n> > Make system_path behave like the other path functions by using a\n> > static buffer, fixing a memory leak.\n> >\n> > Also make sure the prefix pointer is always initialized to either\n> > PREFIX or NULL.\n> >\n> > Signed-off-by: Carlos Martín Nieto \n> > ---\n> >\n> > This fixes the leak I was trying to fix with my original patch, but\n> > this seems much cleaner\n> >\n> > exec_cmd.c | 9 ++++-----\n> > 1 files changed, 4 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/exec_cmd.c b/exec_cmd.c\n> > index 38545e8..12ce017 100644\n> > --- a/exec_cmd.c\n> > +++ b/exec_cmd.c\n> > @@ -9,11 +9,11 @@ static const char *argv0_path;\n> > const char *system_path(const char *path)\n> > {\n> > #ifdef RUNTIME_PREFIX\n> > - static const char *prefix;\n> > + static const char *prefix = NULL;\n> > #else\n> > static const char *prefix = PREFIX;\n> > #endif\n> > - struct strbuf d = STRBUF_INIT;\n> > + static char buf[PATH_MAX+1];\n> \n> Why PATH_MAX + 1? POSIX says that PATH_MAX includes the null-termination...\n\n A lot of other code I've been looking at uses it. I was not aware it\nincluded the terminator.\n\n> \n> > @@ -33,9 +33,8 @@ const char *system_path(const char *path)\n> > }\n> > #endif\n> >\n> > - strbuf_addf(&d, \"%s/%s\", prefix, path);\n> > - path = strbuf_detach(&d, NULL);\n> > - return path;\n> > + snprintf(buf, PATH_MAX, \"%s/%s\", prefix, path);\n> \n> Perhaps \"snprintf(buf, sizeof(buf) - 1, \"%s/%s\", prefix, path);\" instead?\n\n The manpage states that the size parameter includes the\nnull-terminator, so sizeof(buf) would be better, I think. I'll send out\na new mail with the updated patch.\n\n Shouldn't we check to see if there was truncation (i.e. return value >=\nsizeof(buf)) and die in that case?\n"},{"id":"163488","messageId":"1300293183-26354-1-git-send-email-cmn@elego.de","threadId":"26730","inReplyTo":"AANLkTinEMzezKdfGUrwKv7sJ+tSK5duYM6XZMvBa-yj3@mail.gmail.com","subject":"[PATCH] system_path: use a static buffer","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-16T16:33:03Z","receivedAt":"2011-03-16T16:33:03Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"Make system_path behave like the other path functions by using a\nstatic buffer, fixing a memory leak.\n\nAlso make sure the prefix pointer is always initialized to either\nPREFIX or NULL.\n\nSigned-off-by: Carlos Martín Nieto \n---\n exec_cmd.c | 11 ++++++-----\n 1 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 38545e8..5686952 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -9,11 +9,11 @@ static const char *argv0_path;\n const char *system_path(const char *path)\n {\n #ifdef RUNTIME_PREFIX\n-\tstatic const char *prefix;\n+\tstatic const char *prefix = NULL;\n #else\n \tstatic const char *prefix = PREFIX;\n #endif\n-\tstruct strbuf d = STRBUF_INIT;\n+\tstatic char buf[PATH_MAX];\n \n \tif (is_absolute_path(path))\n \t\treturn path;\n@@ -33,9 +33,10 @@ const char *system_path(const char *path)\n \t}\n #endif\n \n-\tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n-\tpath = strbuf_detach(&d, NULL);\n-\treturn path;\n+\tif (snprintf(buf, sizeof(buf), \"%s/%s\", prefix, path) >= sizeof(buf))\n+\t\tdie(\"system path too long for %s\", path);\n+\n+\treturn buf;\n }\n \n const char *git_extract_argv0_path(const char *argv0)\n-- \n1.7.4.1\n"},{"id":"163507","messageId":"7vipvi7q5g.fsf@alter.siamese.dyndns.org","threadId":"26730","inReplyTo":"1300293183-26354-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH] system_path: use a static buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-16T20:43:07Z","receivedAt":"2011-03-16T20:43:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> Make system_path behave like the other path functions by using a\n> static buffer, fixing a memory leak.\n>\n> Also make sure the prefix pointer is always initialized to either\n> PREFIX or NULL.\n>\n> Signed-off-by: Carlos Martín Nieto \n> ---\n\nHave you made sure all the callers are Ok with this change?\n\nIf somebody called system_path(GIT_EXEC_PATH), saved the result in a\nvariable without copying, and then called system_path(ETC_GITATTRIBUTES),\nhis variable may now have a value unrelated to GIT_EXEC_PATH, and you\nwould fix all such callers to save the value away with strdup().\n"},{"id":"163550","messageId":"1300359664-6230-1-git-send-email-cmn@elego.de","threadId":"26730","inReplyTo":"7vipvi7q5g.fsf@alter.siamese.dyndns.org","subject":"[PATCH] system_path: use a static buffer","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-17T11:01:04Z","receivedAt":"2011-03-17T11:01:04Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"Make system_path behave like the other path functions by using a\nstatic buffer, fixing a memory leak.\n\nAlso make sure the prefix pointer is always initialized to either\nPREFIX or NULL.\n\ngit_etc_gitattributes and git_etc_gitconfig are the only users who are\naffected by this change. Make them use a static buffer, which fits\ntheir use better as well.\n\nSigned-off-by: Carlos Martín Nieto \n---\n\nOn mié, 2011-03-16 at 13:43 -0700, Junio C Hamano wrote:\nCarlos Martín Nieto writes:\n> \n> > Make system_path behave like the other path functions by using a\n> > static buffer, fixing a memory leak.\n> >\n> > Also make sure the prefix pointer is always initialized to either\n> > PREFIX or NULL.\n> >\n> > Signed-off-by: Carlos Martín Nieto \n> > ---\n> \n> Have you made sure all the callers are Ok with this change?\n> \n> If somebody called system_path(GIT_EXEC_PATH), saved the result in a\n> variable without copying, and then called system_path(ETC_GITATTRIBUTES),\n> his variable may now have a value unrelated to GIT_EXEC_PATH, and you\n> would fix all such callers to save the value away with strdup().\n\n\nI checked again, and except for the ones changed in this patch, the\nrest copy it to their own buffer or pass it to puts, setenv or\nstrbuf_addstr.\n\nThe way these functions are used suggest the caller expects them to\ndeal with their own memory, so that's what I've done.\n\nTBH, valgrind only reports a win of ~6-7kB when doing git log on\ngit.git, but it's a step in the right direction (and adds consistency\nto system_path, which is the main win).\n\n attr.c | 6 +++---\n config.c | 6 +++---\n exec_cmd.c | 11 ++++++-----\n 3 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 6aff695..64d803f 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -467,9 +467,9 @@ static void drop_attr_stack(void)\n \n const char *git_etc_gitattributes(void)\n {\n-\tstatic const char *system_wide;\n-\tif (!system_wide)\n-\t\tsystem_wide = system_path(ETC_GITATTRIBUTES);\n+\tstatic char system_wide[PATH_MAX];\n+\tif (!system_wide[0])\n+\t\tstrlcpy(system_wide, system_path(ETC_GITATTRIBUTES), PATH_MAX);\n \treturn system_wide;\n }\n \ndiff --git a/config.c b/config.c\nindex 822ef83..cd1c295 100644\n--- a/config.c\n+++ b/config.c\n@@ -808,9 +808,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \n const char *git_etc_gitconfig(void)\n {\n-\tstatic const char *system_wide;\n-\tif (!system_wide)\n-\t\tsystem_wide = system_path(ETC_GITCONFIG);\n+\tstatic char system_wide[PATH_MAX];\n+\tif (!system_wide[0])\n+\t\tstrlcpy(system_wide, system_path(ETC_GITCONFIG), PATH_MAX);\n \treturn system_wide;\n }\n \ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 38545e8..5686952 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -9,11 +9,11 @@ static const char *argv0_path;\n const char *system_path(const char *path)\n {\n #ifdef RUNTIME_PREFIX\n-\tstatic const char *prefix;\n+\tstatic const char *prefix = NULL;\n #else\n \tstatic const char *prefix = PREFIX;\n #endif\n-\tstruct strbuf d = STRBUF_INIT;\n+\tstatic char buf[PATH_MAX];\n \n \tif (is_absolute_path(path))\n \t\treturn path;\n@@ -33,9 +33,10 @@ const char *system_path(const char *path)\n \t}\n #endif\n \n-\tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n-\tpath = strbuf_detach(&d, NULL);\n-\treturn path;\n+\tif (snprintf(buf, sizeof(buf), \"%s/%s\", prefix, path) >= sizeof(buf))\n+\t\tdie(\"system path too long for %s\", path);\n+\n+\treturn buf;\n }\n \n const char *git_extract_argv0_path(const char *argv0)\n-- \n1.7.4.1\n"},{"id":"163562","messageId":"1300371853-8965-1-git-send-email-cmn@elego.de","threadId":"26730","inReplyTo":"1300359664-6230-1-git-send-email-cmn@elego.de","subject":"[PATCH] system_path: use a static buffer","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-17T14:24:13Z","receivedAt":"2011-03-17T14:24:13Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"Make system_path behave like the other path functions by using a\nstatic buffer, fixing a memory leak.\n\nAlso make sure the prefix pointer is always initialized to either\nPREFIX or NULL.\n\ngit_etc_gitattributes and git_etc_gitconfig are the only users who are\naffected by this change. Make them use a static buffer, which fits\ntheir use better as well.\n\nSigned-off-by: Carlos Martín Nieto \n---\n\nIt was pointed out that one should always check for an encoding error\n(-1) with the printf family. I'm not sure how likely this is to\nhappen, but this should make the code extra portable :)\n\n attr.c | 6 +++---\n config.c | 6 +++---\n exec_cmd.c | 15 ++++++++++-----\n 3 files changed, 16 insertions(+), 11 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 6aff695..64d803f 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -467,9 +467,9 @@ static void drop_attr_stack(void)\n \n const char *git_etc_gitattributes(void)\n {\n-\tstatic const char *system_wide;\n-\tif (!system_wide)\n-\t\tsystem_wide = system_path(ETC_GITATTRIBUTES);\n+\tstatic char system_wide[PATH_MAX];\n+\tif (!system_wide[0])\n+\t\tstrlcpy(system_wide, system_path(ETC_GITATTRIBUTES), PATH_MAX);\n \treturn system_wide;\n }\n \ndiff --git a/config.c b/config.c\nindex 822ef83..cd1c295 100644\n--- a/config.c\n+++ b/config.c\n@@ -808,9 +808,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \n const char *git_etc_gitconfig(void)\n {\n-\tstatic const char *system_wide;\n-\tif (!system_wide)\n-\t\tsystem_wide = system_path(ETC_GITCONFIG);\n+\tstatic char system_wide[PATH_MAX];\n+\tif (!system_wide[0])\n+\t\tstrlcpy(system_wide, system_path(ETC_GITCONFIG), PATH_MAX);\n \treturn system_wide;\n }\n \ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 38545e8..35d5cd8 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -9,11 +9,12 @@ static const char *argv0_path;\n const char *system_path(const char *path)\n {\n #ifdef RUNTIME_PREFIX\n-\tstatic const char *prefix;\n+\tstatic const char *prefix = NULL;\n #else\n \tstatic const char *prefix = PREFIX;\n #endif\n-\tstruct strbuf d = STRBUF_INIT;\n+\tstatic char buf[PATH_MAX];\n+\tint ret;\n \n \tif (is_absolute_path(path))\n \t\treturn path;\n@@ -33,9 +34,13 @@ const char *system_path(const char *path)\n \t}\n #endif\n \n-\tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n-\tpath = strbuf_detach(&d, NULL);\n-\treturn path;\n+\tret = snprintf(buf, sizeof(buf), \"%s/%s\", prefix, path);\n+\tif (ret >= sizeof(buf))\n+\t\tdie(\"system path too long for %s\", path);\n+\telse if (ret < 0)\n+\t\tdie_errno(\"encoding error\");\n+\n+\treturn buf;\n }\n \n const char *git_extract_argv0_path(const char *argv0)\n-- \n1.7.4.1\n"},{"id":"163611","messageId":"7v4o70zy96.fsf@alter.siamese.dyndns.org","threadId":"26730","inReplyTo":"20110314194501.GA30483@elie","subject":"Re: [PATCH 3/3] clone: Free a few paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-18T07:25:09Z","receivedAt":"2011-03-18T07:25:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Thanks. The commit message should probably mention that this is for\n> the sake of valgrind rather a true memory leak, since the memory is\n> freed by _exit at the appropriate time already.\n\nOr perhaps you are planning to split larger later half of cmd_clone() into\na helper function, so that other people can call it after doing their own\ncommand line processing.\n\n> The patch itself seems sane, since the performance effect should be\n> negligible.\n\nPerhaps and yes.\n\nI would have preferred a patch to free path and dir without doing anything\nelse, as path is coming from get_repo_path() that always return a string\non heap, and dir comes either directly from xstrdup() or guess_dir_name()\nthat gives strbuf_detach() or xstrndup() result to us, and they are\nclearly leaking and are safely freed, though.\n"},{"id":"163613","messageId":"7vpqpoyjo3.fsf@alter.siamese.dyndns.org","threadId":"26730","inReplyTo":"1300371853-8965-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH] system_path: use a static buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-18T07:25:32Z","receivedAt":"2011-03-18T07:25:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> +\tret = snprintf(buf, sizeof(buf), \"%s/%s\", prefix, path);\n> +\tif (ret >= sizeof(buf))\n> +\t\tdie(\"system path too long for %s\", path);\n> +\telse if (ret < 0)\n> +\t\tdie_errno(\"encoding error\");\n\nPOSIX says snprintf() should set errno in this case, and your use of\ndie_errno() would show that information, but what is \"encoding error\"?\n\nJust being curious, as I suspect that \"snprintf() returned an error\" may\nbe more appropriate, if the answer is \"I don't know what kind of error it\nis, but snprintf() found something faulty while encoding so I chose to\ncall it encoding error\".\n\nBy the way, thanks for checking all the callers.\n"},{"id":"163625","messageId":"AANLkTikms3Ek1CgQHg+gT8Eqs2K6PCvOB-kWZ-aqGawg@mail.gmail.com","threadId":"26730","inReplyTo":"1300371853-8965-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH] system_path: use a static buffer","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-03-18T10:34:07Z","receivedAt":"2011-03-18T10:34:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 17, 2011 at 9:24 PM, Carlos Martín Nieto wrote:\n> -       static const char *system_wide;\n> -       if (!system_wide)\n> -               system_wide = system_path(ETC_GITATTRIBUTES);\n> +       static char system_wide[PATH_MAX];\n\n...\n\n> -       static const char *system_wide;\n> -       if (!system_wide)\n> -               system_wide = system_path(ETC_GITCONFIG);\n> +       static char system_wide[PATH_MAX];\n\n...\n\n>  #ifdef RUNTIME_PREFIX\n> -       static const char *prefix;\n> +       static const char *prefix = NULL;\n>  #else\n>        static const char *prefix = PREFIX;\n>  #endif\n> -       struct strbuf d = STRBUF_INIT;\n> +       static char buf[PATH_MAX];\n> +       int ret;\n\nIt was pointed out elsewhere [1] that PATH_MAX only specifies max\nlength of a path element, not full path. I think we'd need to stay\naway from preallocated PATH_MAX-sized arrays.\n\n[1] http://mid.gmane.org/AANLkTikXvx7-Q8B_dqG5mMHGK_Rw-dFaeQdXi0zW98SD@mail.gmail.com\n-- \nDuy\n"},{"id":"163626","messageId":"20110318113818.GA6049@elie","threadId":"26730","inReplyTo":"AANLkTikms3Ek1CgQHg+gT8Eqs2K6PCvOB-kWZ-aqGawg@mail.gmail.com","subject":"PATH_MAX (Re: [PATCH] system_path: use a static buffer)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-18T11:38:18Z","receivedAt":"2011-03-18T11:38:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nNguyen Thai Ngoc Duy wrote:\n\n> It was pointed out elsewhere [1] that PATH_MAX only specifies max\n> length of a path element, not full path. I think we'd need to stay\n> away from preallocated PATH_MAX-sized arrays.\n\nNo, PATH_MAX is actually the maximum length of a path, and when you\nuse, say, open(2), it will fail if your path is longer than that. The\nmaximum length of a path component on most filesytems is 255 or 256;\nPATH_MAX on Linux is 4096.\n\nIt is indeed possible to have paths with length longer than that. The\nway to support that is to use relative paths wherever possible, which\ndoes sound to me like an interesting long-term goal (mostly because I\nsuspect the result would be easier to read and, especially, to reason\nabout with respect to race conditions).\n\nHope that helps,\nJonathan\n"},{"id":"163627","messageId":"1300448369-5896-1-git-send-email-pclouds@gmail.com","threadId":"26730","inReplyTo":"AANLkTikms3Ek1CgQHg+gT8Eqs2K6PCvOB-kWZ-aqGawg@mail.gmail.com","subject":"[PATCH 1/2] wrapper.c: add xgetcwd()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-03-18T11:39:28Z","receivedAt":"2011-03-18T11:39:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"getcwd() requires the input buffer must be large enough to contain\ncurrent working directory, or it will return ERANGE. We currently\ntreat ERANGE critical and die out.\n\nxgetcwd() handles ERANGE and grows the buffer if necessary. Like other\nfunctions in x* family, it will die() if fails.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n git-compat-util.h | 2 ++\n wrapper.c | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 0 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 49b50ee..e8b1398 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -433,6 +433,8 @@ extern int xmkstemp(char *template);\n extern int xmkstemp_mode(char *template, int mode);\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+struct strbuf;\n+extern void xgetcwd(struct strbuf *);\n \n static inline size_t xsize_t(off_t len)\n {\ndiff --git a/wrapper.c b/wrapper.c\nindex 2829000..5c8cbdf 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -217,6 +217,20 @@ int xmkstemp(char *template)\n \treturn fd;\n }\n \n+void xgetcwd(struct strbuf *sb)\n+{\n+\tconst char *ret;\n+\tif (sb->alloc - sb->len < PATH_MAX)\n+\t\tstrbuf_grow(sb, PATH_MAX);\n+\twhile ((ret = getcwd(sb->buf + sb->len,\n+\t\t\t sb->alloc - sb->len - 1)) == NULL &&\n+\t errno == ERANGE)\n+\t\tstrbuf_grow(sb, PATH_MAX);\n+\tif (!ret)\n+\t\tdie_errno(\"Unable to read current working directory\");\n+\tsb->len += strlen(sb->buf + sb->len);\n+}\n+\n /* git_mkstemp() - create tmp file honoring TMPDIR variable */\n int git_mkstemp(char *path, size_t len, const char *template)\n {\n-- \n1.7.4.74.g639db\n"},{"id":"163628","messageId":"1300448369-5896-2-git-send-email-pclouds@gmail.com","threadId":"26730","inReplyTo":"1300448369-5896-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/2] setup_gently: use xgetcwd()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-03-18T11:39:29Z","receivedAt":"2011-03-18T11:39:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n On Fri, Mar 18, 2011 at 5:34 PM, Nguyen Thai Ngoc Duy wrote:\n > It was pointed out elsewhere [1] that PATH_MAX only specifies max\n > length of a path element, not full path. I think we'd need to stay\n > away from preallocated PATH_MAX-sized arrays.\n >\n > [1] http://mid.gmane.org/AANLkTikXvx7-Q8B_dqG5mMHGK_Rw-dFaeQdXi0zW98SD@mail.gmail.com\n \n Perhaps this as a start? I don't intend to kill all PATH_MAX by myself\n though until I finish all other topics I'm working on.\n\n setup.c | 9 +++++----\n 1 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 03cd84f..f7b3556 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -506,7 +506,8 @@ static dev_t get_device_or_die(const char *path, const char *prefix)\n static const char *setup_git_directory_gently_1(int *nongit_ok)\n {\n \tconst char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);\n-\tstatic char cwd[PATH_MAX+1];\n+\tstatic struct strbuf sb_cwd = STRBUF_INIT;\n+\tchar *cwd;\n \tconst char *gitdirenv, *ret;\n \tchar *gitfile;\n \tint len, offset, ceil_offset;\n@@ -521,9 +522,9 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)\n \tif (nongit_ok)\n \t\t*nongit_ok = 0;\n \n-\tif (!getcwd(cwd, sizeof(cwd)-1))\n-\t\tdie_errno(\"Unable to read current working directory\");\n-\toffset = len = strlen(cwd);\n+\txgetcwd(&sb_cwd);\n+\toffset = len = sb_cwd.len;\n+\tcwd = sb_cwd.buf;\n \n \t/*\n \t * If GIT_DIR is set explicitly, we're not going\n-- \n1.7.4.74.g639db\n"},{"id":"163629","messageId":"AANLkTimW9TofyEqZUQwkAkVqA_Sr5kgV_rm6dJ6LTVbb@mail.gmail.com","threadId":"26730","inReplyTo":"20110318113818.GA6049@elie","subject":"Re: PATH_MAX (Re: [PATCH] system_path: use a static buffer)","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-03-18T11:54:00Z","receivedAt":"2011-03-18T11:54:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 18, 2011 at 6:38 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Nguyen Thai Ngoc Duy wrote:\n>\n>> It was pointed out elsewhere [1] that PATH_MAX only specifies max\n>> length of a path element, not full path. I think we'd need to stay\n>> away from preallocated PATH_MAX-sized arrays.\n>\n> No, PATH_MAX is actually the maximum length of a path, and when you\n> use, say, open(2), it will fail if your path is longer than that.  The\n> maximum length of a path component on most filesytems is 255 or 256;\n> PATH_MAX on Linux is 4096.\n\nHmm.. too late I sent two patches before reading this :) Cloning\nlinux-2.6.git for verifying. But getcwd() can return a path longer\nthan PATH_MAX, right?\n-- \nDuy\n"},{"id":"163883","messageId":"1300700875.2583.8.camel@bee.lab.cmartin.tk","threadId":"26730","inReplyTo":"20110318113818.GA6049@elie","subject":"Re: PATH_MAX (Re: [PATCH] system_path: use a static buffer)","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-21T09:47:52Z","receivedAt":"2011-03-21T09:47:52Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On vie, 2011-03-18 at 06:38 -0500, Jonathan Nieder wrote:\n> Hi,\n> \n> Nguyen Thai Ngoc Duy wrote:\n> \n> > It was pointed out elsewhere [1] that PATH_MAX only specifies max\n> > length of a path element, not full path. I think we'd need to stay\n> > away from preallocated PATH_MAX-sized arrays.\n> \n> No, PATH_MAX is actually the maximum length of a path, and when you\n> use, say, open(2), it will fail if your path is longer than that. The\n> maximum length of a path component on most filesytems is 255 or 256;\n> PATH_MAX on Linux is 4096.\n> \n> It is indeed possible to have paths with length longer than that. The\n> way to support that is to use relative paths wherever possible, which\n\n So what PATH_MAX describes is the maximum length of a string\nrepresenting a path, but not necessarily the length of the path itself.\n\n> does sound to me like an interesting long-term goal (mostly because I\n> suspect the result would be easier to read and, especially, to reason\n> about with respect to race conditions).\n\n There is also the following effect with git\n\n carlos@bee:~/apps$ mkdir one\n carlos@bee:~/apps$ ln -s one two\n carlos@bee:~/apps$ ln -s two three\n carlos@bee:~/apps$ cd three\n carlos@bee:~/apps/three$ git init\n Initialized empty Git repository in /home/carlos/apps/one/.git/\n\n which is at best a bit annoying. Many instances of real_path (formerly\nmake_absolute_path) could be skipped and is_inside_dir could do the\ntransformation to physical path if it needs to.\n\n There may be a few edge cases, but most of the transformation should be\nfairly straightforward (he says as he steps off the cliff...)\n\n cmn\n"},{"id":"163884","messageId":"1300701380.2583.13.camel@bee.lab.cmartin.tk","threadId":"26730","inReplyTo":"7vpqpoyjo3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] system_path: use a static buffer","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-21T09:56:19Z","receivedAt":"2011-03-21T09:56:19Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On vie, 2011-03-18 at 00:25 -0700, Junio C Hamano wrote:\n> Carlos Martín Nieto writes:\n> \n> > +\tret = snprintf(buf, sizeof(buf), \"%s/%s\", prefix, path);\n> > +\tif (ret >= sizeof(buf))\n> > +\t\tdie(\"system path too long for %s\", path);\n> > +\telse if (ret < 0)\n> > +\t\tdie_errno(\"encoding error\");\n> \n> POSIX says snprintf() should set errno in this case, and your use of\n> die_errno() would show that information, but what is \"encoding error\"?\n> \n> Just being curious, as I suspect that \"snprintf() returned an error\" may\n> be more appropriate, if the answer is \"I don't know what kind of error it\n> is, but snprintf() found something faulty while encoding so I chose to\n> call it encoding error\".\n\n\n My manpage says snprintf returns -1 if there was an output or encoding\nerror. As there couldn't be an output error because it's writing to\nmemory and we can't output what snprintf chocked on because whatever\ndie_errno uses will also choke on it, I just put \"encoding error\". I'd\nput \"error assembling system path\" as the actual error message, I guess.\n\n cmn\n"},{"id":"163895","messageId":"20110321111414.GD16334@sigill.intra.peff.net","threadId":"26730","inReplyTo":"1300701380.2583.13.camel@bee.lab.cmartin.tk","subject":"Re: [PATCH] system_path: use a static buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-21T11:14:14Z","receivedAt":"2011-03-21T11:14:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 21, 2011 at 10:56:19AM +0100, Carlos Martín Nieto wrote:\n\n> On vie, 2011-03-18 at 00:25 -0700, Junio C Hamano wrote:\n> > Carlos Martín Nieto writes:\n> > \n> > > +\tret = snprintf(buf, sizeof(buf), \"%s/%s\", prefix, path);\n> > > +\tif (ret >= sizeof(buf))\n> > > +\t\tdie(\"system path too long for %s\", path);\n> > > +\telse if (ret < 0)\n> > > +\t\tdie_errno(\"encoding error\");\n> > \n> > POSIX says snprintf() should set errno in this case, and your use of\n> > die_errno() would show that information, but what is \"encoding error\"?\n> > \n> > Just being curious, as I suspect that \"snprintf() returned an error\" may\n> > be more appropriate, if the answer is \"I don't know what kind of error it\n> > is, but snprintf() found something faulty while encoding so I chose to\n> > call it encoding error\".\n> \n> My manpage says snprintf returns -1 if there was an output or encoding\n> error. As there couldn't be an output error because it's writing to\n> memory and we can't output what snprintf chocked on because whatever\n> die_errno uses will also choke on it, I just put \"encoding error\". I'd\n> put \"error assembling system path\" as the actual error message, I guess.\n\nFWIW, we don't catch snprintf failures in 99% of the calls in git. Most\ncalls just ignore the return value, and some even directly use the\nreturn value to add to a length. The one place that actually does check\nfor the error is strbuf_vaddf, which just says \"your vsnprintf is\nbroken\" and dies.\n\nSo I'm not sure how much we really care about this error code path. If\nanything, we should be replacing all of the calls with something like:\n\n static const char buggy_sprintf_msg[] =\n \"BUG: vsnprintf returned %d; either we fed it a bogus format string\\n\"\n \"(our bug) or your libc is buggy and returns an error when it should\\n\"\n \"tell us how much space is needed. The format string was:\\n\"\n \"%s\\n\";\n int xsnprintf(char *out, size_t size, const char *fmt, ...)\n {\n va_list ap;\n int r;\n\n va_start(ap, fmt);\n r = vsnprintf(out, size, fmt, ap);\n va_end(ap);\n\n if (r < 0)\n die(buggy_sprintf_msg, r, fmt);\n return r;\n }\n\n-Peff\n"},{"id":"163897","messageId":"AANLkTimAD9BurpAZnAjMLvr_wCV-ScS2tsBFY_fs9Jen@mail.gmail.com","threadId":"26730","inReplyTo":"20110318113818.GA6049@elie","subject":"Re: PATH_MAX (Re: [PATCH] system_path: use a static buffer)","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-03-21T11:19:09Z","receivedAt":"2011-03-21T11:19:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 18, 2011 at 6:38 PM, Jonathan Nieder wrote:\n> It is indeed possible to have paths with length longer than that.  The\n> way to support that is to use relative paths wherever possible, which\n> does sound to me like an interesting long-term goal (mostly because I\n> suspect the result would be easier to read and, especially, to reason\n> about with respect to race conditions).\n\nA shorter-term goal is to set a path limit config on trees inside the\nrepo, so people on Linux won't accidentally make paths longer than the\nlimit their Windows fellows have. I think it would be easy to do the\ncheck around index (old trees in repo are left alone, inspecting\nhistory does not have such a limit).\n-- \nDuy\n"},{"id":"163904","messageId":"AANLkTinh4K0Crhs39wuZpKhmxcZkSGhKdZNarmHoimoH@mail.gmail.com","threadId":"26730","inReplyTo":"1300700875.2583.8.camel@bee.lab.cmartin.tk","subject":"Re: PATH_MAX (Re: [PATCH] system_path: use a static buffer)","fromName":"Lasse Makholm","fromEmail":"lasse.makholm@gmail.com","sentAt":"2011-03-21T12:37:20Z","receivedAt":"2011-03-21T12:37:20Z","isPatch":true,"sender":{"key":"lasse.makholm@gmail.com","avatar":"https://gravatar.com/avatar/5bc3a34e4fa8ba26b38c333f370b15c034351e18e09b056a2608f3904ae1a4c1?d=mp&s=160"},"body":"On 21 March 2011 10:47, Carlos Martín Nieto wrote:\n> On vie, 2011-03-18 at 06:38 -0500, Jonathan Nieder wrote:\n>> Hi,\n>>\n>> Nguyen Thai Ngoc Duy wrote:\n>>\n>> > It was pointed out elsewhere [1] that PATH_MAX only specifies max\n>> > length of a path element, not full path. I think we'd need to stay\n>> > away from preallocated PATH_MAX-sized arrays.\n>>\n>> No, PATH_MAX is actually the maximum length of a path, and when you\n>> use, say, open(2), it will fail if your path is longer than that.  The\n>> maximum length of a path component on most filesytems is 255 or 256;\n>> PATH_MAX on Linux is 4096.\n>>\n>> It is indeed possible to have paths with length longer than that.  The\n>> way to support that is to use relative paths wherever possible, which\n>\n>  So what PATH_MAX describes is the maximum length of a string\n> representing a path, but not necessarily the length of the path itself.\n\nAccording to this at least, PATH_MAX is bogus:\nhttp://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html\n\nI think the sane thing would be to never rely on a fixed max path length.\n\n--\n/Lasse\n"},{"id":"163914","messageId":"1300721194.2583.22.camel@bee.lab.cmartin.tk","threadId":"26730","inReplyTo":"20110321111414.GD16334@sigill.intra.peff.net","subject":"Re: [PATCH] system_path: use a static buffer","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-21T15:26:29Z","receivedAt":"2011-03-21T15:26:29Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On lun, 2011-03-21 at 07:14 -0400, Jeff King wrote:\n> On Mon, Mar 21, 2011 at 10:56:19AM +0100, Carlos Martín Nieto wrote:\n> \n> > On vie, 2011-03-18 at 00:25 -0700, Junio C Hamano wrote:\n> > > Carlos Martín Nieto writes:\n> > > \n> > > > +\tret = snprintf(buf, sizeof(buf), \"%s/%s\", prefix, path);\n> > > > +\tif (ret >= sizeof(buf))\n> > > > +\t\tdie(\"system path too long for %s\", path);\n> > > > +\telse if (ret < 0)\n> > > > +\t\tdie_errno(\"encoding error\");\n> > > \n> > > POSIX says snprintf() should set errno in this case, and your use of\n> > > die_errno() would show that information, but what is \"encoding error\"?\n> > > \n> > > Just being curious, as I suspect that \"snprintf() returned an error\" may\n> > > be more appropriate, if the answer is \"I don't know what kind of error it\n> > > is, but snprintf() found something faulty while encoding so I chose to\n> > > call it encoding error\".\n> > \n> > My manpage says snprintf returns -1 if there was an output or encoding\n> > error. As there couldn't be an output error because it's writing to\n> > memory and we can't output what snprintf chocked on because whatever\n> > die_errno uses will also choke on it, I just put \"encoding error\". I'd\n> > put \"error assembling system path\" as the actual error message, I guess.\n> \n> FWIW, we don't catch snprintf failures in 99% of the calls in git. Most\n> calls just ignore the return value, and some even directly use the\n> return value to add to a length. The one place that actually does check\n> for the error is strbuf_vaddf, which just says \"your vsnprintf is\n> broken\" and dies.\n\n It's not actually likely we'll ever meet this error if the only one\nallowed to set the format string is the programmer (and to do otherwise\nis a security risk).\n\n> \n> So I'm not sure how much we really care about this error code path. If\n> anything, we should be replacing all of the calls with something like:\n> \n> static const char buggy_sprintf_msg[] =\n> \"BUG: vsnprintf returned %d; either we fed it a bogus format string\\n\"\n> \"(our bug) or your libc is buggy and returns an error when it should\\n\"\n> \"tell us how much space is needed. The format string was:\\n\"\n> \"%s\\n\";\n> int xsnprintf(char *out, size_t size, const char *fmt, ...)\n> {\n> va_list ap;\n> int r;\n> \n> va_start(ap, fmt);\n> r = vsnprintf(out, size, fmt, ap);\n> va_end(ap);\n> \n> if (r < 0)\n> die(buggy_sprintf_msg, r, fmt);\n> return r;\n> }\n\n Or we could overload (#define) snprintf and replace it with the\nparanoid. It'd go nicely with the vsnprintf that tries to work around\nthe Windows implementation.\n\n I don't feel that strongly we should have the extra check there, seeing\nhow it's rare and not checked anywhere else.\n\n cmn\n"},{"id":"163916","messageId":"20110321155108.GA27662@sigill.intra.peff.net","threadId":"26730","inReplyTo":"1300721194.2583.22.camel@bee.lab.cmartin.tk","subject":"Re: [PATCH] system_path: use a static buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-21T15:51:08Z","receivedAt":"2011-03-21T15:51:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 21, 2011 at 04:26:29PM +0100, Carlos Martín Nieto wrote:\n\n> > FWIW, we don't catch snprintf failures in 99% of the calls in git. Most\n> > calls just ignore the return value, and some even directly use the\n> > return value to add to a length. The one place that actually does check\n> > for the error is strbuf_vaddf, which just says \"your vsnprintf is\n> > broken\" and dies.\n> \n> It's not actually likely we'll ever meet this error if the only one\n> allowed to set the format string is the programmer (and to do otherwise\n> is a security risk).\n\nAgreed.\n\n> Or we could overload (#define) snprintf and replace it with the\n> paranoid. It'd go nicely with the vsnprintf that tries to work around\n> the Windows implementation.\n> \n> I don't feel that strongly we should have the extra check there, seeing\n> how it's rare and not checked anywhere else.\n\nYeah, I am happy to just drop it. AFAICT, an error return from snprintf\nis a bug in your program[1] or a bug in libc. In either case, it should\nfail or produce bogus output on the first run, and we don't need to\nworry about checking errors all the time. Noting the error helps a\nlittle with detection, but since we already install our own vsnprintf on\nknown-buggy platforms, I haven't seen a single complaint.\n\n-Peff\n\n[1] The only error I managed to get out of eglibc was trying to format\n\"%\" (i.e., percent followed by NUL). Skimming the eglibc code (gaah, my\neyes!) it looks like under some conditions it can allocate small work\nbuffers, and return an error if malloc fails. So yeah, I guess it can\nfail randomly, but it seems pretty unlikely.\n"},{"id":"163917","messageId":"1300723030.2583.24.camel@bee.lab.cmartin.tk","threadId":"26730","inReplyTo":"20110321155108.GA27662@sigill.intra.peff.net","subject":"Re: [PATCH] system_path: use a static buffer","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-03-21T15:57:05Z","receivedAt":"2011-03-21T15:57:05Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On lun, 2011-03-21 at 11:51 -0400, Jeff King wrote:\n> Yeah, I am happy to just drop it. AFAICT, an error return from snprintf\n> is a bug in your program[1] or a bug in libc. In either case, it should\n> fail or produce bogus output on the first run, and we don't need to\n> worry about checking errors all the time. Noting the error helps a\n> little with detection, but since we already install our own vsnprintf on\n> known-buggy platforms, I haven't seen a single complaint.\n\n Then the patch in <1300359664-6230-1-git-send-email-cmn@elego.de>\nshould do the trick.\n\n cmn\n"}]}