{"thread":{"id":"41793","subject":"[PATCH v3/GSoC 4/5] test-lib.sh: unset all environment variables defined in xdg base dir spec[1]","startedAt":"2016-03-23T10:13:21Z","lastAt":"2016-03-29T02:40:06Z","messageCount":33,"participants":["Hui Yiqun","惠轶群","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":5},"messages":[{"id":"281555","messageId":"1458728005-22555-1-git-send-email-huiyiqun@gmail.com","threadId":"41793","inReplyTo":null,"subject":"[PATCH v3/GSoC 1/5] path.c: implement strbuf_mkpath()","fromName":"Hui Yiqun","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-23T10:13:21Z","receivedAt":"2016-03-23T10:13:21Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"There were already `mkpath`, `mkpathdup` and `mksnpath` for build\nfilename, but lacked a version of `strbuf_` just like `strbuf_git_path`.\n\nIt is convenient to build a path and manipulate the result later with\nstrbuf.\n\nSigned-off-by: Hui Yiqun \n---\n cache.h | 2 ++\n path.c | 21 +++++++++++++++++----\n 2 files changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex b829410..ef843c1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -781,6 +781,8 @@ extern char *git_pathdup(const char *fmt, ...)\n \t__attribute__((format (printf, 1, 2)));\n extern char *mkpathdup(const char *fmt, ...)\n \t__attribute__((format (printf, 1, 2)));\n+extern void strbuf_mkpath(struct strbuf *sb, const char *fmt, ...)\n+\t__attribute__((format (printf, 2, 3)));\n extern char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n \t__attribute__((format (printf, 2, 3)));\n \ndiff --git a/path.c b/path.c\nindex 8b7e168..699af68 100644\n--- a/path.c\n+++ b/path.c\n@@ -433,14 +433,19 @@ char *git_pathdup(const char *fmt, ...)\n \treturn strbuf_detach(&path, NULL);\n }\n \n+static void do_mkpath(struct strbuf *buf, const char *fmt, va_list args)\n+{\n+\tstrbuf_vaddf(buf, fmt, args);\n+\tstrbuf_cleanup_path(buf);\n+}\n+\n char *mkpathdup(const char *fmt, ...)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tva_list args;\n \tva_start(args, fmt);\n-\tstrbuf_vaddf(&sb, fmt, args);\n+\tdo_mkpath(&sb, fmt, args);\n \tva_end(args);\n-\tstrbuf_cleanup_path(&sb);\n \treturn strbuf_detach(&sb, NULL);\n }\n \n@@ -449,9 +454,17 @@ const char *mkpath(const char *fmt, ...)\n \tva_list args;\n \tstruct strbuf *pathname = get_pathname();\n \tva_start(args, fmt);\n-\tstrbuf_vaddf(pathname, fmt, args);\n+\tdo_mkpath(pathname, fmt, args);\n+\tva_end(args);\n+\treturn pathname->buf;\n+}\n+\n+void strbuf_mkpath(struct strbuf *buf, const char *fmt, ...)\n+{\n+\tva_list args;\n+\tva_start(args, fmt);\n+\tdo_mkpath(buf, fmt, args);\n \tva_end(args);\n-\treturn cleanup_path(pathname->buf);\n }\n \n static void do_submodule_path(struct strbuf *buf, const char *path,\n-- \n2.7.4\n"},{"id":"281552","messageId":"1458728005-22555-2-git-send-email-huiyiqun@gmail.com","threadId":"41793","inReplyTo":"1458728005-22555-1-git-send-email-huiyiqun@gmail.com","subject":"[PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()","fromName":"Hui Yiqun","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-23T10:13:22Z","receivedAt":"2016-03-23T10:13:22Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"This function is aimed to provide an uniform location to put\nruntime files according to the xdg base dir spec[1] and stop using\n$HOME. On the other hand, the safety is considered(with directory\npermission).\n\nThis function will use `$XDG_RUNTIME_DIR/git` if XDG_RUNTIME_DIR exists,\notherwise `/tmp/git-$uid`.\n\nThe existence and the permission of the directory is ensured. However,\nif the directory or its parents cannot be created or the directory exists\nbut have wrong permission, this function will give a warning and return NULL\nfor security.\n\n[1] https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html\n\nSigned-off-by: Hui Yiqun \n---\n cache.h | 23 +++++++++++++++++++++++\n path.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 79 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex ef843c1..f8b649b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1001,6 +1001,29 @@ extern int is_ntfs_dotgit(const char *name);\n */\n extern char *xdg_config_home(const char *filename);\n \n+/**\n+ * this function does the following:\n+ *\n+ * 1. if $XDG_RUNTIME_DIR is non-empty, `$XDG_RUNTIME_DIR/git` is used in next\n+ * step, otherwise `/tmp/git-$uid` is taken.\n+ * 2. ensure that above directory does exist. what's more, it must has correct\n+ * permission and ownership.\n+ * 3. a newly allocated string consisting of the path of above directory and\n+ * $filename is returned.\n+ *\n+ * Under following situation, NULL will be returned:\n+ *\n+ * + the directory mentioned in step 1 exists but have wrong permission or\n+ * ownership.\n+ * + the directory or its parent cannot be created.\n+ *\n+ * Notice:\n+ *\n+ * + the caller is responsible for deallocating the returned string.\n+ *\n+ */\n+extern char *xdg_runtime_dir(const char *filename);\n+\n /* object replacement */\n #define LOOKUP_REPLACE_OBJECT 1\n #define LOOKUP_UNKNOWN_OBJECT 2\ndiff --git a/path.c b/path.c\nindex 699af68..2886e59 100644\n--- a/path.c\n+++ b/path.c\n@@ -5,6 +5,7 @@\n #include \"strbuf.h\"\n #include \"string-list.h\"\n #include \"dir.h\"\n+#include \"git-compat-util.h\"\n \n static int get_st_mode_bits(const char *path, int *mode)\n {\n@@ -1206,6 +1207,61 @@ char *xdg_config_home(const char *filename)\n \treturn NULL;\n }\n \n+char *xdg_runtime_dir(const char *filename)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tchar *runtime_dir;\n+\tstruct stat st;\n+\tuid_t uid = getuid();\n+\n+\tassert(filename);\n+\truntime_dir = getenv(\"XDG_RUNTIME_DIR\");\n+\tif (runtime_dir && *runtime_dir)\n+\t\tstrbuf_mkpath(&sb, \"%s/git/\", runtime_dir);\n+\telse\n+\t\tstrbuf_mkpath(&sb, \"/tmp/git-%d\", uid);\n+\n+\tif (!lstat(sb.buf, &st)) {\n+\t\t/*\n+\t\t * As described in XDG base dir spec[1], the subdirectory\n+\t\t * under $XDG_RUNTIME_DIR or its fallback MUST be owned by\n+\t\t * the user, and its unix access mode MUST be 0700.\n+\t\t *\n+\t\t * Calling chmod or chown silently may cause security\n+\t\t * problem if somebody chdir to it, sleep, and then, try\n+\t\t * to open our protected runtime cache or socket.\n+\t\t * So we just put warning and left it to user to solve.\n+\t\t *\n+\t\t * [1]https://specifications.freedesktop.org/basedir-spec/\n+\t\t * basedir-spec-latest.html\n+\t\t */\n+\t\tif ((st.st_mode & 0777) != S_IRWXU) {\n+\t\t\twarning(\"permission of runtime directory '%s' \"\n+\t\t\t\t\t\"MUST be 0700 instead of 0%o\\n\",\n+\t\t\t\t\tsb.buf, (st.st_mode & 0777));\n+\t\t\treturn NULL;\n+\t\t} else if (st.st_uid != uid) {\n+\t\t\twarning(\"owner of runtime directory '%s' \"\n+\t\t\t\t\t\"MUST be %d instead of %d\\n\",\n+\t\t\t\t\tsb.buf, uid, st.st_uid);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\t/* TODO: check whether st.buf is an directory */\n+\t} else {\n+\t\tif (safe_create_leading_directories_const(sb.buf) < 0) {\n+\t\t\twarning(\"unable to create directories for '%s'\\n\",\n+\t\t\t\t\tsb.buf);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tif (mkdir(sb.buf, 0700) < 0) {\n+\t\t\twarning(\"unable to mkdir '%s'\\n\", sb.buf);\n+\t\t\treturn NULL;\n+\t\t}\n+\t}\n+\tstrbuf_addf(&sb, \"/%s\", filename);\n+\treturn strbuf_detach(&sb, NULL);\n+}\n+\n GIT_PATH_FUNC(git_path_cherry_pick_head, \"CHERRY_PICK_HEAD\")\n GIT_PATH_FUNC(git_path_revert_head, \"REVERT_HEAD\")\n GIT_PATH_FUNC(git_path_squash_msg, \"SQUASH_MSG\")\n-- \n2.7.4\n"},{"id":"281546","messageId":"1458728005-22555-3-git-send-email-huiyiqun@gmail.com","threadId":"41793","inReplyTo":"1458728005-22555-1-git-send-email-huiyiqun@gmail.com","subject":"[PATCH v3/GSoC 3/5] git-credential-cache: put socket to xdg-compatible path","fromName":"Hui Yiqun","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-23T10:13:23Z","receivedAt":"2016-03-23T10:13:23Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"move .git-credential-cache/socket to xdg_runtime_dir(\"credential-cache.sock\")\n\nSigned-off-by: Hui Yiqun \n---\n credential-cache.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/credential-cache.c b/credential-cache.c\nindex f4afdc6..40d838b 100644\n--- a/credential-cache.c\n+++ b/credential-cache.c\n@@ -105,7 +105,7 @@ int main(int argc, const char **argv)\n \top = argv[0];\n \n \tif (!socket_path)\n-\t\tsocket_path = expand_user_path(\"~/.git-credential-cache/socket\");\n+\t\tsocket_path = xdg_runtime_dir(\"credential-cache.sock\");\n \tif (!socket_path)\n \t\tdie(\"unable to find a suitable socket path; use --socket\");\n \n-- \n2.7.4\n"},{"id":"281545","messageId":"1458728005-22555-4-git-send-email-huiyiqun@gmail.com","threadId":"41793","inReplyTo":"1458728005-22555-1-git-send-email-huiyiqun@gmail.com","subject":"[PATCH v3/GSoC 4/5] test-lib.sh: unset all environment variables defined in xdg base dir spec[1]","fromName":"Hui Yiqun","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-23T10:13:24Z","receivedAt":"2016-03-23T10:13:24Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"Otherwise, on environments where these variables and set, an assignment\nto one of these variables will cause the variable being implicitly exported.\n\nFor example:\n\n $ XDG_RUNTIME_DIR=/run/user/2000 bash\n $ XDG_RUNTIME_DIR=/tmp/whatever # it should not be exported!\n $ bash\n $ echo $XDG_RUNTIME_DIR\n /tmp/whatever # instead of empty\n\n[1] https://specifications.freedesktop.org/basedir-spec\n /basedir-spec-latest.html\n\nHelped-by: Jeff King \nSigned-off-by: Hui Yiqun \n---\n t/test-lib.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 0b47eb6..60a837a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -93,7 +93,17 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $(\"$PERL_PATH\" -e '\n \tmy @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);\n \tprint join(\"\\n\", @vars);\n ')\n+# Unset all environment variables defined in xdg base dir spec[1]\n+# to make sure that the test are running with a known state.\n+#\n+# [1] https://specifications.freedesktop.org/basedir-spec\n+# /basedir-spec-latest.html\n+unset XDG_DATA_HOME\n unset XDG_CONFIG_HOME\n+unset XDG_DATA_DIRS\n+unset XDG_CONFIG_DIRS\n+unset XDG_CACHE_HOME\n+unset XDG_RUNTIME_DIR\n unset GITPERLLIB\n GIT_AUTHOR_EMAIL=author@example.com\n GIT_AUTHOR_NAME='A U Thor'\n-- \n2.7.4\n"},{"id":"281558","messageId":"1458728005-22555-5-git-send-email-huiyiqun@gmail.com","threadId":"41793","inReplyTo":"1458728005-22555-1-git-send-email-huiyiqun@gmail.com","subject":"[PATCH v3/GSoC 5/5] t0301: test credential-cache support of XDG_RUNTIME_DIR","fromName":"Hui Yiqun","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-23T10:13:25Z","receivedAt":"2016-03-23T10:13:25Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"t0301 now tests git-credential-cache support for XDG user-specific\nruntime path. Specifically:\n\n* if $XDG_RUNTIME_DIR exists, use socket at\n `$XDG_RUNTIME_DIR/git/credential-cache.sock`.\n\n* otherwise, `/tmp/git-$uid/credential-cache.sock` is taken.\n\nHelped-by: Jeff King \nSigned-off-by: Hui Yiqun \n---\n t/t0301-credential-cache.sh | 29 +++++++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh\nindex 82c8411..264c394 100755\n--- a/t/t0301-credential-cache.sh\n+++ b/t/t0301-credential-cache.sh\n@@ -12,7 +12,36 @@ test -z \"$NO_UNIX_SOCKETS\" || {\n # don't leave a stale daemon running\n trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT\n \n+test_expect_success 'set $XDG_RUNTIME_DIR' '\n+\tXDG_RUNTIME_DIR=$HOME/xdg_runtime/ &&\n+\texport XDG_RUNTIME_DIR\n+'\n+\n+helper_test cache\n+\n+test_expect_success 'when $XDG_RUNTIME_DIR is set, `$XDG_RUNTIME_DIR/git` are used' '\n+\ttest_path_is_missing \"/tmp/git-$(id -u)/credential-cache.sock\" &&\n+\ttest -S \"$HOME/xdg_runtime/git/credential-cache.sock\"\n+'\n+\n+test_expect_success 'force git-credential-cache to exit so that socket disappear' '\n+\tgit credential-cache exit &&\n+\ttest_path_is_missing \"$HOME/xdg_runtime/git/credential-cache.sock\" &&\n+\tunset XDG_RUNTIME_DIR\n+'\n+\n helper_test cache\n+\n+test_expect_success 'when $XDG_RUNTIME_DIR is not set, `/tmp/git-$(id -u) is used' '\n+\ttest_path_is_missing \"$HOME/xdg_runtime/git/credential-cache.sock\" &&\n+\ttest -S \"/tmp/git-$(id -u)/credential-cache.sock\"\n+'\n+\n+# TODO: if $XDG_RUNTIME_DIR/git/ exists, but has wrong permission and ownership,\n+# `helper_test cache` must fail.\n+\n+# TODO: check whether `--socket` works\n+\n helper_test_timeout cache --timeout=1\n \n # we can't rely on our \"trap\" above working after test_done,\n-- \n2.7.4\n"},{"id":"281782","messageId":"CAKqreuzQgnL8G-UpXs_UOgPCOc1gVxxUgmVzY_LbpYpsB+pS4Q@mail.gmail.com","threadId":"41793","inReplyTo":"1458728005-22555-1-git-send-email-huiyiqun@gmail.com","subject":"Re: [PATCH v3/GSoC 1/5] path.c: implement strbuf_mkpath()","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-25T07:13:41Z","receivedAt":"2016-03-25T07:13:41Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"So, are there some additional points I could improve?\n\n2016-03-23 18:13 GMT+08:00 Hui Yiqun :\n> There were already `mkpath`, `mkpathdup` and `mksnpath` for build\n> filename, but lacked a version of `strbuf_` just like `strbuf_git_path`.\n>\n> It is convenient to build a path and manipulate the result later with\n> strbuf.\n>\n> Signed-off-by: Hui Yiqun \n> ---\n> cache.h | 2 ++\n> path.c | 21 +++++++++++++++++----\n> 2 files changed, 19 insertions(+), 4 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index b829410..ef843c1 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -781,6 +781,8 @@ extern char *git_pathdup(const char *fmt, ...)\n> __attribute__((format (printf, 1, 2)));\n> extern char *mkpathdup(const char *fmt, ...)\n> __attribute__((format (printf, 1, 2)));\n> +extern void strbuf_mkpath(struct strbuf *sb, const char *fmt, ...)\n> + __attribute__((format (printf, 2, 3)));\n> extern char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n> __attribute__((format (printf, 2, 3)));\n>\n> diff --git a/path.c b/path.c\n> index 8b7e168..699af68 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -433,14 +433,19 @@ char *git_pathdup(const char *fmt, ...)\n> return strbuf_detach(&path, NULL);\n> }\n>\n> +static void do_mkpath(struct strbuf *buf, const char *fmt, va_list args)\n> +{\n> + strbuf_vaddf(buf, fmt, args);\n> + strbuf_cleanup_path(buf);\n> +}\n> +\n> char *mkpathdup(const char *fmt, ...)\n> {\n> struct strbuf sb = STRBUF_INIT;\n> va_list args;\n> va_start(args, fmt);\n> - strbuf_vaddf(&sb, fmt, args);\n> + do_mkpath(&sb, fmt, args);\n> va_end(args);\n> - strbuf_cleanup_path(&sb);\n> return strbuf_detach(&sb, NULL);\n> }\n>\n> @@ -449,9 +454,17 @@ const char *mkpath(const char *fmt, ...)\n> va_list args;\n> struct strbuf *pathname = get_pathname();\n> va_start(args, fmt);\n> - strbuf_vaddf(pathname, fmt, args);\n> + do_mkpath(pathname, fmt, args);\n> + va_end(args);\n> + return pathname->buf;\n> +}\n> +\n> +void strbuf_mkpath(struct strbuf *buf, const char *fmt, ...)\n> +{\n> + va_list args;\n> + va_start(args, fmt);\n> + do_mkpath(buf, fmt, args);\n> va_end(args);\n> - return cleanup_path(pathname->buf);\n> }\n>\n> static void do_submodule_path(struct strbuf *buf, const char *path,\n> --\n> 2.7.4\n>\n"},{"id":"281792","messageId":"20160325095110.GA8880@sigill.intra.peff.net","threadId":"41793","inReplyTo":"1458728005-22555-1-git-send-email-huiyiqun@gmail.com","subject":"Re: [PATCH v3/GSoC 1/5] path.c: implement strbuf_mkpath()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-25T09:51:10Z","receivedAt":"2016-03-25T09:51:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 23, 2016 at 06:13:21PM +0800, Hui Yiqun wrote:\n\n> There were already `mkpath`, `mkpathdup` and `mksnpath` for build\n> filename, but lacked a version of `strbuf_` just like `strbuf_git_path`.\n> \n> It is convenient to build a path and manipulate the result later with\n> strbuf.\n\nMakes sense, and the patch looks fine to me.\n\n-Peff\n"},{"id":"281794","messageId":"20160325095923.GB8880@sigill.intra.peff.net","threadId":"41793","inReplyTo":"1458728005-22555-2-git-send-email-huiyiqun@gmail.com","subject":"Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-25T09:59:23Z","receivedAt":"2016-03-25T09:59:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 23, 2016 at 06:13:22PM +0800, Hui Yiqun wrote:\n\n> +/**\n> + * this function does the following:\n> + *\n> + * 1. if $XDG_RUNTIME_DIR is non-empty, `$XDG_RUNTIME_DIR/git` is used in next\n> + * step, otherwise `/tmp/git-$uid` is taken.\n> + * 2. ensure that above directory does exist. what's more, it must has correct\n> + * permission and ownership.\n> + * 3. a newly allocated string consisting of the path of above directory and\n> + * $filename is returned.\n> + *\n> + * Under following situation, NULL will be returned:\n> + *\n> + * + the directory mentioned in step 1 exists but have wrong permission or\n> + * ownership.\n> + * + the directory or its parent cannot be created.\n> + *\n> + * Notice:\n> + *\n> + * + the caller is responsible for deallocating the returned string.\n> + *\n> + */\n> +extern char *xdg_runtime_dir(const char *filename);\n\nThere's a lot of \"what\" here that the caller doesn't really care about,\nand which may go stale with respect to the implementation over time. Can\nwe make something more succinct like:\n\n /*\n * Return a path suitable for writing run-time files related to git,\n * or NULL if no such path can be established. The resulting string\n * should be freed by the caller.\n */\n\n?\n\n> --- a/path.c\n> +++ b/path.c\n> @@ -5,6 +5,7 @@\n> #include \"strbuf.h\"\n> #include \"string-list.h\"\n> #include \"dir.h\"\n> +#include \"git-compat-util.h\"\n\nWhy do we need this? It should generally be the first file included, as\nit sets up defines used by other header files. It looks like we include\n\"cache.h\" in this file, which is enough (it explicitly includes\ngit-compat-util.h first to cover this case).\n\n> +char *xdg_runtime_dir(const char *filename)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tchar *runtime_dir;\n> +\tstruct stat st;\n> +\tuid_t uid = getuid();\n> +\n> +\tassert(filename);\n> +\truntime_dir = getenv(\"XDG_RUNTIME_DIR\");\n> +\tif (runtime_dir && *runtime_dir)\n> +\t\tstrbuf_mkpath(&sb, \"%s/git/\", runtime_dir);\n> +\telse\n> +\t\tstrbuf_mkpath(&sb, \"/tmp/git-%d\", uid);\n> +\n> +\tif (!lstat(sb.buf, &st)) {\n> +\t\t/*\n> +\t\t * As described in XDG base dir spec[1], the subdirectory\n> +\t\t * under $XDG_RUNTIME_DIR or its fallback MUST be owned by\n> +\t\t * the user, and its unix access mode MUST be 0700.\n> +\t\t *\n> +\t\t * Calling chmod or chown silently may cause security\n> +\t\t * problem if somebody chdir to it, sleep, and then, try\n> +\t\t * to open our protected runtime cache or socket.\n> +\t\t * So we just put warning and left it to user to solve.\n> +\t\t *\n\nThere are some minor English problems here (and elsewhere). E.g., you\nprobably want \"So we just issue a warning and leave it to the user to\nsolve.\".\n\n> +\t\tif ((st.st_mode & 0777) != S_IRWXU) {\n> +\t\t\twarning(\"permission of runtime directory '%s' \"\n> +\t\t\t\t\t\"MUST be 0700 instead of 0%o\\n\",\n> +\t\t\t\t\tsb.buf, (st.st_mode & 0777));\n> +\t\t\treturn NULL;\n> +\t\t} else if (st.st_uid != uid) {\n> +\t\t\twarning(\"owner of runtime directory '%s' \"\n> +\t\t\t\t\t\"MUST be %d instead of %d\\n\",\n> +\t\t\t\t\tsb.buf, uid, st.st_uid);\n> +\t\t\treturn NULL;\n> +\t\t}\n\nThese cases still leak \"sb\", I think.\n\n> +\t\t/* TODO: check whether st.buf is an directory */\n\nShould we complete this todo? It's should just be S_ISDIR(st.st_mode).\n\n> +\t} else {\n> +\t\tif (safe_create_leading_directories_const(sb.buf) < 0) {\n> +\t\t\twarning(\"unable to create directories for '%s'\\n\",\n> +\t\t\t\t\tsb.buf);\n> +\t\t\treturn NULL;\n> +\t\t}\n> +\t\tif (mkdir(sb.buf, 0700) < 0) {\n> +\t\t\twarning(\"unable to mkdir '%s'\\n\", sb.buf);\n> +\t\t\treturn NULL;\n> +\t\t}\n\nThese ones leak, too.\n\n-Peff\n"},{"id":"281795","messageId":"20160325100041.GC8880@sigill.intra.peff.net","threadId":"41793","inReplyTo":"1458728005-22555-3-git-send-email-huiyiqun@gmail.com","subject":"Re: [PATCH v3/GSoC 3/5] git-credential-cache: put socket to xdg-compatible path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-25T10:00:42Z","receivedAt":"2016-03-25T10:00:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 23, 2016 at 06:13:23PM +0800, Hui Yiqun wrote:\n\n> move .git-credential-cache/socket to xdg_runtime_dir(\"credential-cache.sock\")\n\nMotivation?\n\n> diff --git a/credential-cache.c b/credential-cache.c\n> index f4afdc6..40d838b 100644\n> --- a/credential-cache.c\n> +++ b/credential-cache.c\n> @@ -105,7 +105,7 @@ int main(int argc, const char **argv)\n> \top = argv[0];\n> \n> \tif (!socket_path)\n> -\t\tsocket_path = expand_user_path(\"~/.git-credential-cache/socket\");\n> +\t\tsocket_path = xdg_runtime_dir(\"credential-cache.sock\");\n> \tif (!socket_path)\n> \t\tdie(\"unable to find a suitable socket path; use --socket\");\n\nWe do our own mkdir and chmod in credential-cache; this should be\nredundant with what xdg_runtime_dir() does, and can be removed, right?\n\n-Peff\n"},{"id":"281796","messageId":"20160325100535.GD8880@sigill.intra.peff.net","threadId":"41793","inReplyTo":"1458728005-22555-4-git-send-email-huiyiqun@gmail.com","subject":"Re: [PATCH v3/GSoC 4/5] test-lib.sh: unset all environment variables defined in xdg base dir spec[1]","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-25T10:05:35Z","receivedAt":"2016-03-25T10:05:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 23, 2016 at 06:13:24PM +0800, Hui Yiqun wrote:\n\n> Otherwise, on environments where these variables and set, an assignment\n> to one of these variables will cause the variable being implicitly exported.\n\nYes, that's one problem, though I think the more general problem is\nsimply that they pollute the test environment (so git might actually be\ntouching your _real_ credential-cache socket, and not a fake test one).\n\nI'd probably write:\n\n We try to clean the test environment of variables that may affect the\n outcome, so that the tests always start from a known state. We already\n clean XDG_CONFIG_HOME, but now that we support XDG_RUNTIME_DIR, we\n must clean that, too. While we're at it, let's simply cover all\n variables mentioned in the xdg spec[1] to future-proof and to cover\n any places where they might unexpectedly have an impact.\n\nFeel free to use or adapt that as you see fit.\n\n> +# Unset all environment variables defined in xdg base dir spec[1]\n> +# to make sure that the test are running with a known state.\n> +#\n> +# [1] https://specifications.freedesktop.org/basedir-spec\n> +# /basedir-spec-latest.html\n> +unset XDG_DATA_HOME\n> unset XDG_CONFIG_HOME\n> +unset XDG_DATA_DIRS\n> +unset XDG_CONFIG_DIRS\n> +unset XDG_CACHE_HOME\n> +unset XDG_RUNTIME_DIR\n\nThanks for being thorough here. This is much nicer than just adding\nXDG_RUNTIME_DIR.\n\n-Peff\n"},{"id":"281813","messageId":"CAKqreux8FHdJoKDishjQkbi9g1oUc265EUK4nOJ_sgeFivGSNA@mail.gmail.com","threadId":"41793","inReplyTo":"20160325095923.GB8880@sigill.intra.peff.net","subject":"Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-25T14:21:48Z","receivedAt":"2016-03-25T14:21:48Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-25 17:59 GMT+08:00 Jeff King :\n> On Wed, Mar 23, 2016 at 06:13:22PM +0800, Hui Yiqun wrote:\n>\n>> +/**\n>> + * this function does the following:\n>> + *\n>> + * 1. if $XDG_RUNTIME_DIR is non-empty, `$XDG_RUNTIME_DIR/git` is used in next\n>> + * step, otherwise `/tmp/git-$uid` is taken.\n>> + * 2. ensure that above directory does exist. what's more, it must has correct\n>> + * permission and ownership.\n>> + * 3. a newly allocated string consisting of the path of above directory and\n>> + * $filename is returned.\n>> + *\n>> + * Under following situation, NULL will be returned:\n>> + *\n>> + * + the directory mentioned in step 1 exists but have wrong permission or\n>> + * ownership.\n>> + * + the directory or its parent cannot be created.\n>> + *\n>> + * Notice:\n>> + *\n>> + * + the caller is responsible for deallocating the returned string.\n>> + *\n>> + */\n>> +extern char *xdg_runtime_dir(const char *filename);\n>\n> There's a lot of \"what\" here that the caller doesn't really care about,\n> and which may go stale with respect to the implementation over time. Can\n> we make something more succinct like:\n>\n> /*\n> * Return a path suitable for writing run-time files related to git,\n> * or NULL if no such path can be established. The resulting string\n> * should be freed by the caller.\n> */\n>\n> ?\n\nThat's clearer, but if I were the caller, I would worry about the\nsecurity of the path.\nHow about adding:\n\nThe security of the path is ensured by file permission.\n\n>\n>> --- a/path.c\n>> +++ b/path.c\n>> @@ -5,6 +5,7 @@\n>> #include \"strbuf.h\"\n>> #include \"string-list.h\"\n>> #include \"dir.h\"\n>> +#include \"git-compat-util.h\"\n>\n> Why do we need this? It should generally be the first file included, as\n> it sets up defines used by other header files. It looks like we include\n> \"cache.h\" in this file, which is enough (it explicitly includes\n> git-compat-util.h first to cover this case).\n\nI include this header for `getuid` and `stat`. Now that there is an indirect\nincluding, I will delete this one.\n\n>\n>> +char *xdg_runtime_dir(const char *filename)\n>> +{\n>> + struct strbuf sb = STRBUF_INIT;\n>> + char *runtime_dir;\n>> + struct stat st;\n>> + uid_t uid = getuid();\n>> +\n>> + assert(filename);\n>> + runtime_dir = getenv(\"XDG_RUNTIME_DIR\");\n>> + if (runtime_dir && *runtime_dir)\n>> + strbuf_mkpath(&sb, \"%s/git/\", runtime_dir);\n>> + else\n>> + strbuf_mkpath(&sb, \"/tmp/git-%d\", uid);\n>> +\n>> + if (!lstat(sb.buf, &st)) {\n>> + /*\n>> + * As described in XDG base dir spec[1], the subdirectory\n>> + * under $XDG_RUNTIME_DIR or its fallback MUST be owned by\n>> + * the user, and its unix access mode MUST be 0700.\n>> + *\n>> + * Calling chmod or chown silently may cause security\n>> + * problem if somebody chdir to it, sleep, and then, try\n>> + * to open our protected runtime cache or socket.\n>> + * So we just put warning and left it to user to solve.\n>> + *\n>\n> There are some minor English problems here (and elsewhere). E.g., you\n> probably want \"So we just issue a warning and leave it to the user to\n> solve.\".\n\nSorry for my English.\n\n>> + if ((st.st_mode & 0777) != S_IRWXU) {\n>> + warning(\"permission of runtime directory '%s' \"\n>> + \"MUST be 0700 instead of 0%o\\n\",\n>> + sb.buf, (st.st_mode & 0777));\n>> + return NULL;\n>> + } else if (st.st_uid != uid) {\n>> + warning(\"owner of runtime directory '%s' \"\n>> + \"MUST be %d instead of %d\\n\",\n>> + sb.buf, uid, st.st_uid);\n>> + return NULL;\n>> + }\n>\n> These cases still leak \"sb\", I think.\n>\n>> + /* TODO: check whether st.buf is an directory */\n>\n> Should we complete this todo? It's should just be S_ISDIR(st.st_mode).\n>\n>> + } else {\n>> + if (safe_create_leading_directories_const(sb.buf) < 0) {\n>> + warning(\"unable to create directories for '%s'\\n\",\n>> + sb.buf);\n>> + return NULL;\n>> + }\n>> + if (mkdir(sb.buf, 0700) < 0) {\n>> + warning(\"unable to mkdir '%s'\\n\", sb.buf);\n>> + return NULL;\n>> + }\n>\n> These ones leak, too.\n\nI will deal with it.\n\nI find there are some similar leakage in this file. I'll fix them in\nanother patch.\n\nDo you think we need some additional comments for the release of strbuf?\n>\n> -Peff\n"},{"id":"281812","messageId":"CAKqreuxFGXZuoBT=N48KKBkwft6aDQ-z7wOR4uUyK=3fbs-yRg@mail.gmail.com","threadId":"41793","inReplyTo":"CAKqreux8FHdJoKDishjQkbi9g1oUc265EUK4nOJ_sgeFivGSNA@mail.gmail.com","subject":"Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-25T14:23:04Z","receivedAt":"2016-03-25T14:23:04Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"> I find there are some similar leakage in this file. I'll fix them in\n> another patch.\n>\n> Do you think we need some additional comments for the release of strbuf?\n\nFor example, in strbuf.h.\n"},{"id":"281814","messageId":"CAKqreuzwa+ztwsF3CRb++J-iqW=_evfBQ7Q7veyYU2ydJXnpcg@mail.gmail.com","threadId":"41793","inReplyTo":"20160325100041.GC8880@sigill.intra.peff.net","subject":"Re: [PATCH v3/GSoC 3/5] git-credential-cache: put socket to xdg-compatible path","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-25T14:28:55Z","receivedAt":"2016-03-25T14:28:55Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-25 18:00 GMT+08:00 Jeff King :\n> On Wed, Mar 23, 2016 at 06:13:23PM +0800, Hui Yiqun wrote:\n>\n>> move .git-credential-cache/socket to xdg_runtime_dir(\"credential-cache.sock\")\n>\n> Motivation?\n\nI will improve my commit message.\n\n>\n>> diff --git a/credential-cache.c b/credential-cache.c\n>> index f4afdc6..40d838b 100644\n>> --- a/credential-cache.c\n>> +++ b/credential-cache.c\n>> @@ -105,7 +105,7 @@ int main(int argc, const char **argv)\n>> op = argv[0];\n>>\n>> if (!socket_path)\n>> - socket_path = expand_user_path(\"~/.git-credential-cache/socket\");\n>> + socket_path = xdg_runtime_dir(\"credential-cache.sock\");\n>> if (!socket_path)\n>> die(\"unable to find a suitable socket path; use --socket\");\n>\n> We do our own mkdir and chmod in credential-cache; this should be\n> redundant with what xdg_runtime_dir() does, and can be removed, right?\n\nBut user may specify another path via --socket , this path may have\nwrong permission. I'm considering how to handle this situation.\n\n>\n> -Peff\n"},{"id":"281830","messageId":"xmqqpoui4huo.fsf@gitster.mtv.corp.google.com","threadId":"41793","inReplyTo":"CAKqreux8FHdJoKDishjQkbi9g1oUc265EUK4nOJ_sgeFivGSNA@mail.gmail.com","subject":"Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T16:55:59Z","receivedAt":"2016-03-25T16:55:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"惠轶群 writes:\n\n>> There's a lot of \"what\" here that the caller doesn't really care about,\n>> and which may go stale with respect to the implementation over time. Can\n>> we make something more succinct like:\n>>\n>> /*\n>> * Return a path suitable for writing run-time files related to git,\n>> * or NULL if no such path can be established. The resulting string\n>> * should be freed by the caller.\n>> */\n>>\n>> ?\n>\n> That's clearer, but if I were the caller, I would worry about the\n> security of the path.\n> How about adding:\n>\n> The security of the path is ensured by file permission.\n\nIs \"by file permission\" descriptive enough?\n\nTo protect /a/b/c/socket, what filesystem entities have the right\npermission bits set? If the parent directory is writable by an\nattacker, the permission bits on 'socket' itself may not matter as\nthe attacker can rename it away and create new one herself, for\nexample.\n\n> I will deal with it.\n>\n> I find there are some similar leakage in this file. I'll fix them in\n> another patch.\n>\n> Do you think we need some additional comments for the release of strbuf?\n\nAs Documentation/technical/api-strbuf.txt has this, I think we are\nalready OK.\n\n`strbuf_release`::\n\n\tRelease a string buffer and the memory it used. You should not use the\n\tstring buffer after using this function, unless you initialize it again.\n"},{"id":"281840","messageId":"20160325175510.GA10563@sigill.intra.peff.net","threadId":"41793","inReplyTo":"xmqqpoui4huo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-25T17:55:10Z","receivedAt":"2016-03-25T17:55:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 25, 2016 at 09:55:59AM -0700, Junio C Hamano wrote:\n\n> 惠轶群 writes:\n> \n> >> There's a lot of \"what\" here that the caller doesn't really care about,\n> >> and which may go stale with respect to the implementation over time. Can\n> >> we make something more succinct like:\n> >>\n> >> /*\n> >> * Return a path suitable for writing run-time files related to git,\n> >> * or NULL if no such path can be established. The resulting string\n> >> * should be freed by the caller.\n> >> */\n> >>\n> >> ?\n> >\n> > That's clearer, but if I were the caller, I would worry about the\n> > security of the path.\n> > How about adding:\n> >\n> > The security of the path is ensured by file permission.\n> \n> Is \"by file permission\" descriptive enough?\n> \n> To protect /a/b/c/socket, what filesystem entities have the right\n> permission bits set? If the parent directory is writable by an\n> attacker, the permission bits on 'socket' itself may not matter as\n> the attacker can rename it away and create new one herself, for\n> example.\n\nI think that is discussed elsewhere, and referring to the xdg document\nis enough. My main point is that the docstring about a function should\ntell a potential caller what they need to know to use it, but if it gets\noverly long, that information is lost in the noise.\n\n-Peff\n"},{"id":"281841","messageId":"20160325175635.GB10563@sigill.intra.peff.net","threadId":"41793","inReplyTo":"CAKqreuzwa+ztwsF3CRb++J-iqW=_evfBQ7Q7veyYU2ydJXnpcg@mail.gmail.com","subject":"Re: [PATCH v3/GSoC 3/5] git-credential-cache: put socket to xdg-compatible path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-25T17:56:35Z","receivedAt":"2016-03-25T17:56:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 25, 2016 at 10:28:55PM +0800, 惠轶群 wrote:\n\n> >> diff --git a/credential-cache.c b/credential-cache.c\n> >> index f4afdc6..40d838b 100644\n> >> --- a/credential-cache.c\n> >> +++ b/credential-cache.c\n> >> @@ -105,7 +105,7 @@ int main(int argc, const char **argv)\n> >> op = argv[0];\n> >>\n> >> if (!socket_path)\n> >> - socket_path = expand_user_path(\"~/.git-credential-cache/socket\");\n> >> + socket_path = xdg_runtime_dir(\"credential-cache.sock\");\n> >> if (!socket_path)\n> >> die(\"unable to find a suitable socket path; use --socket\");\n> >\n> > We do our own mkdir and chmod in credential-cache; this should be\n> > redundant with what xdg_runtime_dir() does, and can be removed, right?\n> \n> But user may specify another path via --socket , this path may have\n> wrong permission. I'm considering how to handle this situation.\n\nGood point, we do need to cover that case.\n\nPerhaps the work done by xdg_runtime_dir() needs to be split into two\nfucntions: one to just provide the path, and the second to securely\ncreate a given path.\n\n-Peff\n"},{"id":"281843","messageId":"20160325175947.GC10563@sigill.intra.peff.net","threadId":"41793","inReplyTo":"CAKqreux8FHdJoKDishjQkbi9g1oUc265EUK4nOJ_sgeFivGSNA@mail.gmail.com","subject":"Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-25T17:59:47Z","receivedAt":"2016-03-25T17:59:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 25, 2016 at 10:21:48PM +0800, 惠轶群 wrote:\n\n> > There are some minor English problems here (and elsewhere). E.g., you\n> > probably want \"So we just issue a warning and leave it to the user to\n> > solve.\".\n> \n> Sorry for my English.\n\nThanks. And sorry if that sounded too harsh. I know that working in a\nnon-native language is tough. Usually in a review I'll try to provide\nspecific English fixes, but in this case, I think a lot of these\nmessages are still in flux, so I I didn't want to waste either of our\ntime going over specifics if the content is just going to change later.\n\n> > These ones leak, too.\n> \n> I will deal with it.\n> \n> I find there are some similar leakage in this file. I'll fix them in\n> another patch.\n\nGreat, thanks.\n\n-Peff\n"},{"id":"281844","messageId":"xmqq4mbu4evm.fsf@gitster.mtv.corp.google.com","threadId":"41793","inReplyTo":"20160325175510.GA10563@sigill.intra.peff.net","subject":"Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T18:00:13Z","receivedAt":"2016-03-25T18:00:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > That's clearer, but if I were the caller, I would worry about the\n>> > security of the path.\n>> > How about adding:\n>> >\n>> > The security of the path is ensured by file permission.\n>> \n>> Is \"by file permission\" descriptive enough?\n>> \n>> To protect /a/b/c/socket, what filesystem entities have the right\n>> permission bits set? If the parent directory is writable by an\n>> attacker, the permission bits on 'socket' itself may not matter as\n>> the attacker can rename it away and create new one herself, for\n>> example.\n>\n> I think that is discussed elsewhere, and referring to the xdg document\n> is enough. My main point is that the docstring about a function should\n> tell a potential caller what they need to know to use it, but if it gets\n> overly long, that information is lost in the noise.\n\nI agree with your main point, and I was wondering if \"by file\npermission\" is merely adding yet another noise if there is\ndiscussion elsewhere already, and/or if it does not refer to an\nexternal document that has a fuller discussion, because it lacks any\nuseful information by itself.\n"},{"id":"281845","messageId":"CAKqreuyOGk=iZes_bgiBpnO5Ht+9VzZG3_j5H33i6qtBx1R5nQ@mail.gmail.com","threadId":"41793","inReplyTo":"20160325175635.GB10563@sigill.intra.peff.net","subject":"Re: [PATCH v3/GSoC 3/5] git-credential-cache: put socket to xdg-compatible path","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-25T18:00:30Z","receivedAt":"2016-03-25T18:00:30Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-26 1:56 GMT+08:00 Jeff King :\n> On Fri, Mar 25, 2016 at 10:28:55PM +0800, 惠轶群 wrote:\n>\n>> >> diff --git a/credential-cache.c b/credential-cache.c\n>> >> index f4afdc6..40d838b 100644\n>> >> --- a/credential-cache.c\n>> >> +++ b/credential-cache.c\n>> >> @@ -105,7 +105,7 @@ int main(int argc, const char **argv)\n>> >> op = argv[0];\n>> >>\n>> >> if (!socket_path)\n>> >> - socket_path = expand_user_path(\"~/.git-credential-cache/socket\");\n>> >> + socket_path = xdg_runtime_dir(\"credential-cache.sock\");\n>> >> if (!socket_path)\n>> >> die(\"unable to find a suitable socket path; use --socket\");\n>> >\n>> > We do our own mkdir and chmod in credential-cache; this should be\n>> > redundant with what xdg_runtime_dir() does, and can be removed, right?\n>>\n>> But user may specify another path via --socket , this path may have\n>> wrong permission. I'm considering how to handle this situation.\n>\n> Good point, we do need to cover that case.\n>\n> Perhaps the work done by xdg_runtime_dir() needs to be split into two\n> fucntions: one to just provide the path, and the second to securely\n> create a given path.\n\nGood, I will implement it like that.\n\n>\n> -Peff\n"},{"id":"281956","messageId":"CAKqreuyrENgmB+a7eAHNx9fA4RsQu2PwjLV1wc2z7=kzNTseUw@mail.gmail.com","threadId":"41793","inReplyTo":"xmqqpoui4huo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-28T13:37:55Z","receivedAt":"2016-03-28T13:37:55Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-26 0:55 GMT+08:00 Junio C Hamano :\n> 惠轶群 writes:\n>\n>>> There's a lot of \"what\" here that the caller doesn't really care about,\n>>> and which may go stale with respect to the implementation over time. Can\n>>> we make something more succinct like:\n>>>\n>>> /*\n>>> * Return a path suitable for writing run-time files related to git,\n>>> * or NULL if no such path can be established. The resulting string\n>>> * should be freed by the caller.\n>>> */\n>>>\n>>> ?\n>>\n>> That's clearer, but if I were the caller, I would worry about the\n>> security of the path.\n>> How about adding:\n>>\n>> The security of the path is ensured by file permission.\n>\n> Is \"by file permission\" descriptive enough?\n>\n> To protect /a/b/c/socket, what filesystem entities have the right\n> permission bits set? If the parent directory is writable by an\n> attacker, the permission bits on 'socket' itself may not matter as\n> the attacker can rename it away and create new one herself, for\n> example.\n>\n>> I will deal with it.\n>>\n>> I find there are some similar leakage in this file. I'll fix them in\n>> another patch.\n>>\n>> Do you think we need some additional comments for the release of strbuf?\n>\n> As Documentation/technical/api-strbuf.txt has this, I think we are\n> already OK.\n>\n> `strbuf_release`::\n>\n> Release a string buffer and the memory it used. You should not use the\n> string buffer after using this function, unless you initialize it again.\n>\n\nExcuse me, but I could not find `Documentation/technical/api-strbuf.txt` in\nmaster branch. Do you refer to the header of `strbuf.h`? In which, I learnt how\nto initialize the strbuf and how to take use of it when I began to use\nit. If there\nis also a note about whether I should release it and how do it, such as:\n\nFor every strbuf that has been initialized and buffer of it has not\nbeen detached\nwith strbuf_detach, you should release the resource by strbuf_release.\n\nIt will save me (maybe others) much time to explore the entire method list.\n"},{"id":"281958","messageId":"CAKqreuzNeY7HryZvMgLKpPAbXOQ_qLfX63YOj57Wo+KrHJbtMA@mail.gmail.com","threadId":"41793","inReplyTo":"20160325175947.GC10563@sigill.intra.peff.net","subject":"Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-28T14:12:09Z","receivedAt":"2016-03-28T14:12:09Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-26 1:59 GMT+08:00 Jeff King :\n> On Fri, Mar 25, 2016 at 10:21:48PM +0800, 惠轶群 wrote:\n>\n>> > There are some minor English problems here (and elsewhere). E.g., you\n>> > probably want \"So we just issue a warning and leave it to the user to\n>> > solve.\".\n>>\n>> Sorry for my English.\n>\n> Thanks. And sorry if that sounded too harsh. I know that working in a\n> non-native language is tough. Usually in a review I'll try to provide\n> specific English fixes, but in this case, I think a lot of these\n> messages are still in flux, so I I didn't want to waste either of our\n> time going over specifics if the content is just going to change later.\n>\n>> > These ones leak, too.\n>>\n>> I will deal with it.\n>>\n>> I find there are some similar leakage in this file. I'll fix them in\n>> another patch.\n>\n> Great, thanks.\n\nAfter read the source code of strbuf more carefully, I get the conclusion\nthat if a strbuf is initialized with STRBUF_INIT but is not used, there is\nno need to release it. Is it true?\n\n> -Peff\n"},{"id":"281959","messageId":"xmqqa8li1xgw.fsf@gitster.mtv.corp.google.com","threadId":"41793","inReplyTo":"CAKqreuyrENgmB+a7eAHNx9fA4RsQu2PwjLV1wc2z7=kzNTseUw@mail.gmail.com","subject":"Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-28T14:35:59Z","receivedAt":"2016-03-28T14:35:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"惠轶群 writes:\n\n> Excuse me, but I could not find\n> `Documentation/technical/api-strbuf.txt` in master branch. Do you\n> refer to the header of `strbuf.h`? In which, I learnt how to\n> initialize the strbuf and how to take use of it when I began to\n> use it.\n\nAh, yes, it was a relatively recent development that usage text from\nthe former was moved to the latter.\n\n> If there is also a note about whether I should release it\n> and how do it, such as:\n>\n> For every strbuf that has been initialized and buffer of it has\n> not been detached with strbuf_detach, you should release the\n> resource by strbuf_release.\n>\n> It will save me (maybe others) much time to explore the entire method list.\n\nWhile it may not hurt, it sounds somewhat extreme and funny me, as\nit sounds as if you are saying \"for every pointer variable that\npoints at a piece of memory allocated with malloc(3) and friends,\nyou must free(3) them after you are done\".\n\nThe way _release() refers to its internal resource usage could be\nimproved, I guess. \"Release ... the memory it used\" may not click\nas freeing the memory to some readers.\n\n strbuf.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex f72fd14..a783f09 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -82,7 +82,7 @@ extern char strbuf_slopbuf[];\n extern void strbuf_init(struct strbuf *, size_t);\n \n /**\n- * Release a string buffer and the memory it used. You should not use the\n+ * Release a string buffer and free the memory it used. You should not use the\n * string buffer after using this function, unless you initialize it again.\n */\n extern void strbuf_release(struct strbuf *);\n"},{"id":"281960","messageId":"xmqq60w61wsp.fsf@gitster.mtv.corp.google.com","threadId":"41793","inReplyTo":"CAKqreuzNeY7HryZvMgLKpPAbXOQ_qLfX63YOj57Wo+KrHJbtMA@mail.gmail.com","subject":"Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-28T14:50:30Z","receivedAt":"2016-03-28T14:50:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"惠轶群 writes:\n\n> After read the source code of strbuf more carefully, I get the conclusion\n> that if a strbuf is initialized with STRBUF_INIT but is not used, there is\n> no need to release it. Is it true?\n\nIf it is initialized with STRBUF_INIT and never used, there is no\nreason for the variable to exist ;-)\n\nLeaving the variable in the code, and not calling release on it at\nthe end, would be OK (i.e. there is no leak) today, but may invite\nfuture bugs (e.g. people may use the variable to tentatively build\ntheir string before the function returns to leave the scope of the\nvariable without adding _release() themselves).\n"},{"id":"281962","messageId":"CAKqreuxJ7Rz2G_hmnAOH7W4rBBKr=CX3pjbzJkkK1ZhN=3_o-w@mail.gmail.com","threadId":"41793","inReplyTo":"xmqq60w61wsp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-28T15:00:53Z","receivedAt":"2016-03-28T15:00:53Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-28 22:50 GMT+08:00 Junio C Hamano :\n> 惠轶群 writes:\n>\n>> After read the source code of strbuf more carefully, I get the conclusion\n>> that if a strbuf is initialized with STRBUF_INIT but is not used, there is\n>> no need to release it. Is it true?\n>\n> If it is initialized with STRBUF_INIT and never used, there is no\n> reason for the variable to exist ;-)\n\nI mean, if some developer return before the strbuf is used, there seems\nno need to release the strbuf according to the current implementation.\n\nBut I'm not sure whether this is suitable for the abstraction of strbuf.\n\nFor example:\n\n const char *enter_repo(const char *path, int strict)\n {\n static struct strbuf validated_path = STRBUF_INIT;\n static struct strbuf used_path = STRBUF_INIT;\n\n if (!path)\n return NULL; // no need to release, right?\n ...\n }\n\n\n> Leaving the variable in the code, and not calling release on it at\n> the end, would be OK (i.e. there is no leak) today, but may invite\n> future bugs (e.g. people may use the variable to tentatively build\n> their string before the function returns to leave the scope of the\n> variable without adding _release() themselves).\n"},{"id":"281964","messageId":"1459180292-3475-1-git-send-email-huiyiqun@gmail.com","threadId":"41793","inReplyTo":"20160325175947.GC10563@sigill.intra.peff.net","subject":"[PATCH] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage","fromName":"Hui Yiqun","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-28T15:51:32Z","receivedAt":"2016-03-28T15:51:32Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"According to strbuf.h, strbuf_detach is the sole supported method\nto unwrap a memory buffer from its strbuf shell.\n\nSo we should not return the pointer of strbuf.buf directly.\n\nWhat's more, some memory leakages are solved.\n---\n path.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 969b494..b07e5a7 100644\n--- a/path.c\n+++ b/path.c\n@@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)\n {\n \tstatic struct strbuf validated_path = STRBUF_INIT;\n \tstatic struct strbuf used_path = STRBUF_INIT;\n+\tchar * dbuf = NULL;\n \n \tif (!path)\n \t\treturn NULL;\n@@ -654,7 +655,7 @@ const char *enter_repo(const char *path, int strict)\n \t\tif (used_path.buf[0] == '~') {\n \t\t\tchar *newpath = expand_user_path(used_path.buf);\n \t\t\tif (!newpath)\n-\t\t\t\treturn NULL;\n+\t\t\t\tgoto return_null;\n \t\t\tstrbuf_attach(&used_path, newpath, strlen(newpath),\n \t\t\t\t strlen(newpath));\n \t\t}\n@@ -671,22 +672,22 @@ const char *enter_repo(const char *path, int strict)\n \t\t\tstrbuf_setlen(&used_path, baselen);\n \t\t}\n \t\tif (!suffix[i])\n-\t\t\treturn NULL;\n+\t\t\tgoto return_null;\n \t\tgitfile = read_gitfile(used_path.buf);\n \t\tif (gitfile) {\n \t\t\tstrbuf_reset(&used_path);\n \t\t\tstrbuf_addstr(&used_path, gitfile);\n \t\t}\n \t\tif (chdir(used_path.buf))\n-\t\t\treturn NULL;\n-\t\tpath = validated_path.buf;\n+\t\t\tgoto return_null;\n+\t\tpath = dbuf = strbuf_detach(&validated_path, NULL);\n \t}\n \telse {\n \t\tconst char *gitfile = read_gitfile(path);\n \t\tif (gitfile)\n \t\t\tpath = gitfile;\n \t\tif (chdir(path))\n-\t\t\treturn NULL;\n+\t\t\tgoto return_null;\n \t}\n \n \tif (is_git_directory(\".\")) {\n@@ -695,6 +696,10 @@ const char *enter_repo(const char *path, int strict)\n \t\treturn path;\n \t}\n \n+return_null:\n+ free(dbuf);\n+\tstrbuf_release(&used_path);\n+\tstrbuf_release(&validated_path);\n \treturn NULL;\n }\n \n-- \n2.7.4\n"},{"id":"281965","messageId":"1459180570-5521-1-git-send-email-huiyiqun@gmail.com","threadId":"41793","inReplyTo":"20160325175947.GC10563@sigill.intra.peff.net","subject":"[PATCH v2] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage","fromName":"Hui Yiqun","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-28T15:56:10Z","receivedAt":"2016-03-28T15:56:10Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"According to strbuf.h, strbuf_detach is the sole supported method\nto unwrap a memory buffer from its strbuf shell.\n\nSo we should not return the pointer of strbuf.buf directly.\n\nWhat's more, some memory leakages are solved.\n---\n path.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 969b494..b07e5a7 100644\n--- a/path.c\n+++ b/path.c\n@@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)\n {\n \tstatic struct strbuf validated_path = STRBUF_INIT;\n \tstatic struct strbuf used_path = STRBUF_INIT;\n+\tchar * dbuf = NULL;\n \n \tif (!path)\n \t\treturn NULL;\n@@ -654,7 +655,7 @@ const char *enter_repo(const char *path, int strict)\n \t\tif (used_path.buf[0] == '~') {\n \t\t\tchar *newpath = expand_user_path(used_path.buf);\n \t\t\tif (!newpath)\n-\t\t\t\treturn NULL;\n+\t\t\t\tgoto return_null;\n \t\t\tstrbuf_attach(&used_path, newpath, strlen(newpath),\n \t\t\t\t strlen(newpath));\n \t\t}\n@@ -671,22 +672,22 @@ const char *enter_repo(const char *path, int strict)\n \t\t\tstrbuf_setlen(&used_path, baselen);\n \t\t}\n \t\tif (!suffix[i])\n-\t\t\treturn NULL;\n+\t\t\tgoto return_null;\n \t\tgitfile = read_gitfile(used_path.buf);\n \t\tif (gitfile) {\n \t\t\tstrbuf_reset(&used_path);\n \t\t\tstrbuf_addstr(&used_path, gitfile);\n \t\t}\n \t\tif (chdir(used_path.buf))\n-\t\t\treturn NULL;\n-\t\tpath = validated_path.buf;\n+\t\t\tgoto return_null;\n+\t\tpath = dbuf = strbuf_detach(&validated_path, NULL);\n \t}\n \telse {\n \t\tconst char *gitfile = read_gitfile(path);\n \t\tif (gitfile)\n \t\t\tpath = gitfile;\n \t\tif (chdir(path))\n-\t\t\treturn NULL;\n+\t\t\tgoto return_null;\n \t}\n \n \tif (is_git_directory(\".\")) {\n@@ -695,6 +696,10 @@ const char *enter_repo(const char *path, int strict)\n \t\treturn path;\n \t}\n \n+return_null:\n+ free(dbuf);\n+\tstrbuf_release(&used_path);\n+\tstrbuf_release(&validated_path);\n \treturn NULL;\n }\n \n-- \n2.7.4\n"},{"id":"281966","messageId":"1459180638-6034-1-git-send-email-huiyiqun@gmail.com","threadId":"41793","inReplyTo":"20160325175947.GC10563@sigill.intra.peff.net","subject":"[PATCH v3] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage","fromName":"Hui Yiqun","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-28T15:57:18Z","receivedAt":"2016-03-28T15:57:18Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"According to strbuf.h, strbuf_detach is the sole supported method\nto unwrap a memory buffer from its strbuf shell.\n\nSo we should not return the pointer of strbuf.buf directly.\n\nWhat's more, some memory leakages are solved.\n---\n path.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 969b494..9801617 100644\n--- a/path.c\n+++ b/path.c\n@@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)\n {\n \tstatic struct strbuf validated_path = STRBUF_INIT;\n \tstatic struct strbuf used_path = STRBUF_INIT;\n+\tchar * dbuf = NULL;\n \n \tif (!path)\n \t\treturn NULL;\n@@ -654,7 +655,7 @@ const char *enter_repo(const char *path, int strict)\n \t\tif (used_path.buf[0] == '~') {\n \t\t\tchar *newpath = expand_user_path(used_path.buf);\n \t\t\tif (!newpath)\n-\t\t\t\treturn NULL;\n+\t\t\t\tgoto return_null;\n \t\t\tstrbuf_attach(&used_path, newpath, strlen(newpath),\n \t\t\t\t strlen(newpath));\n \t\t}\n@@ -671,22 +672,22 @@ const char *enter_repo(const char *path, int strict)\n \t\t\tstrbuf_setlen(&used_path, baselen);\n \t\t}\n \t\tif (!suffix[i])\n-\t\t\treturn NULL;\n+\t\t\tgoto return_null;\n \t\tgitfile = read_gitfile(used_path.buf);\n \t\tif (gitfile) {\n \t\t\tstrbuf_reset(&used_path);\n \t\t\tstrbuf_addstr(&used_path, gitfile);\n \t\t}\n \t\tif (chdir(used_path.buf))\n-\t\t\treturn NULL;\n-\t\tpath = validated_path.buf;\n+\t\t\tgoto return_null;\n+\t\tpath = dbuf = strbuf_detach(&validated_path, NULL);\n \t}\n \telse {\n \t\tconst char *gitfile = read_gitfile(path);\n \t\tif (gitfile)\n \t\t\tpath = gitfile;\n \t\tif (chdir(path))\n-\t\t\treturn NULL;\n+\t\t\tgoto return_null;\n \t}\n \n \tif (is_git_directory(\".\")) {\n@@ -695,6 +696,10 @@ const char *enter_repo(const char *path, int strict)\n \t\treturn path;\n \t}\n \n+return_null:\n+\tfree(dbuf);\n+\tstrbuf_release(&used_path);\n+\tstrbuf_release(&validated_path);\n \treturn NULL;\n }\n \n-- \n2.7.4\n"},{"id":"281967","messageId":"CAKqreuzoQmkUwyYiAt_GhoHyaCz=WoNshOkhH=D3orNZEG2ayQ@mail.gmail.com","threadId":"41793","inReplyTo":"1459180638-6034-1-git-send-email-huiyiqun@gmail.com","subject":"Re: [PATCH v3] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-28T15:59:54Z","receivedAt":"2016-03-28T15:59:54Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"Sorry, I sent the patch repeatedly to fix a wrongly indent with space.\n\n2016-03-28 23:57 GMT+08:00 Hui Yiqun :\n> According to strbuf.h, strbuf_detach is the sole supported method\n> to unwrap a memory buffer from its strbuf shell.\n>\n> So we should not return the pointer of strbuf.buf directly.\n>\n> What's more, some memory leakages are solved.\n> ---\n> path.c | 15 ++++++++++-----\n> 1 file changed, 10 insertions(+), 5 deletions(-)\n>\n> diff --git a/path.c b/path.c\n> index 969b494..9801617 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)\n> {\n> static struct strbuf validated_path = STRBUF_INIT;\n> static struct strbuf used_path = STRBUF_INIT;\n> + char * dbuf = NULL;\n>\n> if (!path)\n> return NULL;\n> @@ -654,7 +655,7 @@ const char *enter_repo(const char *path, int strict)\n> if (used_path.buf[0] == '~') {\n> char *newpath = expand_user_path(used_path.buf);\n> if (!newpath)\n> - return NULL;\n> + goto return_null;\n> strbuf_attach(&used_path, newpath, strlen(newpath),\n> strlen(newpath));\n> }\n> @@ -671,22 +672,22 @@ const char *enter_repo(const char *path, int strict)\n> strbuf_setlen(&used_path, baselen);\n> }\n> if (!suffix[i])\n> - return NULL;\n> + goto return_null;\n> gitfile = read_gitfile(used_path.buf);\n> if (gitfile) {\n> strbuf_reset(&used_path);\n> strbuf_addstr(&used_path, gitfile);\n> }\n> if (chdir(used_path.buf))\n> - return NULL;\n> - path = validated_path.buf;\n> + goto return_null;\n> + path = dbuf = strbuf_detach(&validated_path, NULL);\n> }\n> else {\n> const char *gitfile = read_gitfile(path);\n> if (gitfile)\n> path = gitfile;\n> if (chdir(path))\n> - return NULL;\n> + goto return_null;\n> }\n>\n> if (is_git_directory(\".\")) {\n> @@ -695,6 +696,10 @@ const char *enter_repo(const char *path, int strict)\n> return path;\n> }\n>\n> +return_null:\n> + free(dbuf);\n> + strbuf_release(&used_path);\n> + strbuf_release(&validated_path);\n> return NULL;\n> }\n>\n> --\n> 2.7.4\n>\n"},{"id":"281972","messageId":"xmqqd1qezga6.fsf@gitster.mtv.corp.google.com","threadId":"41793","inReplyTo":"CAKqreuxJ7Rz2G_hmnAOH7W4rBBKr=CX3pjbzJkkK1ZhN=3_o-w@mail.gmail.com","subject":"Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-28T17:03:13Z","receivedAt":"2016-03-28T17:03:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"惠轶群 writes:\n\n> For example:\n>\n> const char *enter_repo(const char *path, int strict)\n> {\n> static struct strbuf validated_path = STRBUF_INIT;\n> static struct strbuf used_path = STRBUF_INIT;\n>\n> if (!path)\n> return NULL; // no need to release, right?\n> ...\n> }\n\nCorrect.\n"},{"id":"281975","messageId":"20160328175528.GB20028@sigill.intra.peff.net","threadId":"41793","inReplyTo":"1459180570-5521-1-git-send-email-huiyiqun@gmail.com","subject":"Re: [PATCH v2] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-28T17:55:28Z","receivedAt":"2016-03-28T17:55:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 28, 2016 at 11:56:10PM +0800, Hui Yiqun wrote:\n\n> According to strbuf.h, strbuf_detach is the sole supported method\n> to unwrap a memory buffer from its strbuf shell.\n> \n> So we should not return the pointer of strbuf.buf directly.\n> \n> What's more, some memory leakages are solved.\n\nThere is something else going on here, which makes this case different\nthan some others. Note that the function returns a const string:\n\n> diff --git a/path.c b/path.c\n> index 969b494..b07e5a7 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)\n\nBy convention, that means that the result is not owned by the caller,\nand should not be freed. We implement that by:\n\n> {\n> \tstatic struct strbuf validated_path = STRBUF_INIT;\n> \tstatic struct strbuf used_path = STRBUF_INIT;\n\n...using static variables which persist after the call returns. So this\nfunction retains ownership of the memory, and it remains valid until the\nnext call to enter_repo().\n\nThere's no leak when we return NULL, because the function retains\ncontrol of the memory (though it will hold onto it until the end of the\nprogram if nobody calls enter_repo() again). And thus we shouldn't use\nstrbuf_detach(), which loses that reference to the memory (and since the\ncallers don't take ownership, it actually _creates_ a leak).\n\nWe could release the memory when returning, but I don't think it's a big\ndeal to do so.\n\n-Peff\n"},{"id":"281976","messageId":"xmqqwpomxz4v.fsf@gitster.mtv.corp.google.com","threadId":"41793","inReplyTo":"1459180638-6034-1-git-send-email-huiyiqun@gmail.com","subject":"Re: [PATCH v3] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-28T17:58:56Z","receivedAt":"2016-03-28T17:58:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hui Yiqun writes:\n\n> According to strbuf.h, strbuf_detach is the sole supported method\n> to unwrap a memory buffer from its strbuf shell.\n> ...\n> diff --git a/path.c b/path.c\n> index 969b494..9801617 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)\n> {\n> \tstatic struct strbuf validated_path = STRBUF_INIT;\n> \tstatic struct strbuf used_path = STRBUF_INIT;\n> ...\n> +return_null:\n> +\tfree(dbuf);\n> +\tstrbuf_release(&used_path);\n> +\tstrbuf_release(&validated_path);\n> \treturn NULL;\n> }\n\nI see these strbuf's are \"static\" storage class, so that they do not\nhave to get freed.\n"},{"id":"282020","messageId":"CAKqreux6XLMGN151fn6xN5PeNURLLWjdBVNjKHLzWOjAm45YOg@mail.gmail.com","threadId":"41793","inReplyTo":"xmqqwpomxz4v.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-29T02:38:41Z","receivedAt":"2016-03-29T02:38:41Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-29 1:58 GMT+08:00 Junio C Hamano :\n> Hui Yiqun writes:\n>\n>> According to strbuf.h, strbuf_detach is the sole supported method\n>> to unwrap a memory buffer from its strbuf shell.\n>> ...\n>> diff --git a/path.c b/path.c\n>> index 969b494..9801617 100644\n>> --- a/path.c\n>> +++ b/path.c\n>> @@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)\n>> {\n>> static struct strbuf validated_path = STRBUF_INIT;\n>> static struct strbuf used_path = STRBUF_INIT;\n>> ...\n>> +return_null:\n>> + free(dbuf);\n>> + strbuf_release(&used_path);\n>> + strbuf_release(&validated_path);\n>> return NULL;\n>> }\n>\n> I see these strbuf's are \"static\" storage class, so that they do not\n> have to get freed.\n\nI see, thanks.\n"},{"id":"282021","messageId":"CAKqreuxRbZ6VKNVbv+cNCJdguT3mt0Zy53vXZ+8dg0e1J3x59g@mail.gmail.com","threadId":"41793","inReplyTo":"20160328175528.GB20028@sigill.intra.peff.net","subject":"Re: [PATCH v2] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-29T02:40:06Z","receivedAt":"2016-03-29T02:40:06Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-29 1:55 GMT+08:00 Jeff King :\n> On Mon, Mar 28, 2016 at 11:56:10PM +0800, Hui Yiqun wrote:\n>\n>> According to strbuf.h, strbuf_detach is the sole supported method\n>> to unwrap a memory buffer from its strbuf shell.\n>>\n>> So we should not return the pointer of strbuf.buf directly.\n>>\n>> What's more, some memory leakages are solved.\n>\n> There is something else going on here, which makes this case different\n> than some others. Note that the function returns a const string:\n>\n>> diff --git a/path.c b/path.c\n>> index 969b494..b07e5a7 100644\n>> --- a/path.c\n>> +++ b/path.c\n>> @@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)\n>\n> By convention, that means that the result is not owned by the caller,\n> and should not be freed. We implement that by:\n>\n>> {\n>> static struct strbuf validated_path = STRBUF_INIT;\n>> static struct strbuf used_path = STRBUF_INIT;\n>\n> ...using static variables which persist after the call returns. So this\n> function retains ownership of the memory, and it remains valid until the\n> next call to enter_repo().\n\nSorry for my carelessness. I didn't notice it.\n\n> There's no leak when we return NULL, because the function retains\n> control of the memory (though it will hold onto it until the end of the\n> program if nobody calls enter_repo() again). And thus we shouldn't use\n> strbuf_detach(), which loses that reference to the memory (and since the\n> callers don't take ownership, it actually _creates_ a leak).\n>\n> We could release the memory when returning, but I don't think it's a big\n> deal to do so.\n>\n> -Peff\n"}]}