[PATCH v3/GSoC 1/5] path.c: implement strbuf_mkpath()

[PATCH v3/GSoC 1/5] path.c: implement strbuf_mkpath()

[Hui Yiqun]

There were already `mkpath`, `mkpathdup` and `mksnpath` for build
filename, but lacked a version of `strbuf_` just like `strbuf_git_path`.

It is convenient to build a path and manipulate the result later with
strbuf.

Signed-off-by: Hui Yiqun <huiyiqun@gmail.com>
---
 cache.h |  2 ++
 path.c  | 21 +++++++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index b829410..ef843c1 100644
--- a/cache.h
+++ b/cache.h
@@ -781,6 +781,8 @@ extern char *git_pathdup(const char *fmt, ...)
 	__attribute__((format (printf, 1, 2)));
 extern char *mkpathdup(const char *fmt, ...)
 	__attribute__((format (printf, 1, 2)));
+extern void strbuf_mkpath(struct strbuf *sb, const char *fmt, ...)
+	__attribute__((format (printf, 2, 3)));
 extern char *git_pathdup_submodule(const char *path, const char *fmt, ...)
 	__attribute__((format (printf, 2, 3)));
 
diff --git a/path.c b/path.c
index 8b7e168..699af68 100644
--- a/path.c
+++ b/path.c
@@ -433,14 +433,19 @@ char *git_pathdup(const char *fmt, ...)
 	return strbuf_detach(&path, NULL);
 }
 
+static void do_mkpath(struct strbuf *buf, const char *fmt, va_list args)
+{
+	strbuf_vaddf(buf, fmt, args);
+	strbuf_cleanup_path(buf);
+}
+
 char *mkpathdup(const char *fmt, ...)
 {
 	struct strbuf sb = STRBUF_INIT;
 	va_list args;
 	va_start(args, fmt);
-	strbuf_vaddf(&sb, fmt, args);
+	do_mkpath(&sb, fmt, args);
 	va_end(args);
-	strbuf_cleanup_path(&sb);
 	return strbuf_detach(&sb, NULL);
 }
 
@@ -449,9 +454,17 @@ const char *mkpath(const char *fmt, ...)
 	va_list args;
 	struct strbuf *pathname = get_pathname();
 	va_start(args, fmt);
-	strbuf_vaddf(pathname, fmt, args);
+	do_mkpath(pathname, fmt, args);
+	va_end(args);
+	return pathname->buf;
+}
+
+void strbuf_mkpath(struct strbuf *buf, const char *fmt, ...)
+{
+	va_list args;
+	va_start(args, fmt);
+	do_mkpath(buf, fmt, args);
 	va_end(args);
-	return cleanup_path(pathname->buf);
 }
 
 static void do_submodule_path(struct strbuf *buf, const char *path,
-- 
2.7.4

[PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()

[Hui Yiqun]

This function is aimed to provide an uniform location to put
runtime files according to the xdg base dir spec[1] and stop using
$HOME. On the other hand, the safety is considered(with directory
permission).

This function will use `$XDG_RUNTIME_DIR/git` if XDG_RUNTIME_DIR exists,
otherwise `/tmp/git-$uid`.

The existence and the permission of the directory is ensured. However,
if the directory or its parents cannot be created or the directory exists
but have wrong permission, this function will give a warning and return NULL
for security.

[1] https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html

Signed-off-by: Hui Yiqun <huiyiqun@gmail.com>
---
 cache.h | 23 +++++++++++++++++++++++
 path.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)

diff --git a/cache.h b/cache.h
index ef843c1..f8b649b 100644
--- a/cache.h
+++ b/cache.h
@@ -1001,6 +1001,29 @@ extern int is_ntfs_dotgit(const char *name);
  */
 extern char *xdg_config_home(const char *filename);
 
+/**
+ * this function does the following:
+ *
+ * 1. if $XDG_RUNTIME_DIR is non-empty, `$XDG_RUNTIME_DIR/git` is used in next
+ * step, otherwise `/tmp/git-$uid` is taken.
+ * 2. ensure that above directory does exist. what's more, it must has correct
+ * permission and ownership.
+ * 3. a newly allocated string consisting of the path of above directory and
+ * $filename is returned.
+ *
+ * Under following situation, NULL will be returned:
+ *
+ * + the directory mentioned in step 1 exists but have wrong permission or
+ * ownership.
+ * + the directory or its parent cannot be created.
+ *
+ * Notice:
+ *
+ * + the caller is responsible for deallocating the returned string.
+ *
+ */
+extern char *xdg_runtime_dir(const char *filename);
+
 /* object replacement */
 #define LOOKUP_REPLACE_OBJECT 1
 #define LOOKUP_UNKNOWN_OBJECT 2
diff --git a/path.c b/path.c
index 699af68..2886e59 100644
--- a/path.c
+++ b/path.c
@@ -5,6 +5,7 @@
 #include "strbuf.h"
 #include "string-list.h"
 #include "dir.h"
+#include "git-compat-util.h"
 
 static int get_st_mode_bits(const char *path, int *mode)
 {
@@ -1206,6 +1207,61 @@ char *xdg_config_home(const char *filename)
 	return NULL;
 }
 
+char *xdg_runtime_dir(const char *filename)
+{
+	struct strbuf sb = STRBUF_INIT;
+	char *runtime_dir;
+	struct stat st;
+	uid_t uid = getuid();
+
+	assert(filename);
+	runtime_dir = getenv("XDG_RUNTIME_DIR");
+	if (runtime_dir && *runtime_dir)
+		strbuf_mkpath(&sb, "%s/git/", runtime_dir);
+	else
+		strbuf_mkpath(&sb, "/tmp/git-%d", uid);
+
+	if (!lstat(sb.buf, &st)) {
+		/*
+		 * As described in XDG base dir spec[1], the subdirectory
+		 * under $XDG_RUNTIME_DIR or its fallback MUST be owned by
+		 * the user, and its unix access mode MUST be 0700.
+		 *
+		 * Calling chmod or chown silently may cause security
+		 * problem if somebody chdir to it, sleep, and then, try
+		 * to open our protected runtime cache or socket.
+		 * So we just put warning and left it to user to solve.
+		 *
+		 * [1]https://specifications.freedesktop.org/basedir-spec/
+		 * basedir-spec-latest.html
+		 */
+		if ((st.st_mode & 0777) != S_IRWXU) {
+			warning("permission of runtime directory '%s' "
+					"MUST be 0700 instead of 0%o\n",
+					sb.buf, (st.st_mode & 0777));
+			return NULL;
+		} else if (st.st_uid != uid) {
+			warning("owner of runtime directory '%s' "
+					"MUST be %d instead of %d\n",
+					sb.buf, uid, st.st_uid);
+			return NULL;
+		}
+		/* TODO: check whether st.buf is an directory */
+	} else {
+		if (safe_create_leading_directories_const(sb.buf) < 0) {
+			warning("unable to create directories for '%s'\n",
+					sb.buf);
+			return NULL;
+		}
+		if (mkdir(sb.buf, 0700) < 0) {
+			warning("unable to mkdir '%s'\n", sb.buf);
+			return NULL;
+		}
+	}
+	strbuf_addf(&sb, "/%s", filename);
+	return strbuf_detach(&sb, NULL);
+}
+
 GIT_PATH_FUNC(git_path_cherry_pick_head, "CHERRY_PICK_HEAD")
 GIT_PATH_FUNC(git_path_revert_head, "REVERT_HEAD")
 GIT_PATH_FUNC(git_path_squash_msg, "SQUASH_MSG")
-- 
2.7.4

[PATCH v3/GSoC 3/5] git-credential-cache: put socket to xdg-compatible path

[Hui Yiqun]

move .git-credential-cache/socket to xdg_runtime_dir("credential-cache.sock")

Signed-off-by: Hui Yiqun <huiyiqun@gmail.com>
---
 credential-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/credential-cache.c b/credential-cache.c
index f4afdc6..40d838b 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -105,7 +105,7 @@ int main(int argc, const char **argv)
 	op = argv[0];
 
 	if (!socket_path)
-		socket_path = expand_user_path("~/.git-credential-cache/socket");
+		socket_path = xdg_runtime_dir("credential-cache.sock");
 	if (!socket_path)
 		die("unable to find a suitable socket path; use --socket");
 
-- 
2.7.4

[PATCH v3/GSoC 4/5] test-lib.sh: unset all environment variables defined in xdg base dir spec[1]

[Hui Yiqun]

Otherwise, on environments where these variables and set, an assignment
to one of these variables will cause the variable being implicitly exported.

For example:

    $ XDG_RUNTIME_DIR=/run/user/2000 bash
    $ XDG_RUNTIME_DIR=/tmp/whatever # it should not be exported!
    $ bash
    $ echo $XDG_RUNTIME_DIR
    /tmp/whatever # instead of empty

[1] https://specifications.freedesktop.org/basedir-spec
    /basedir-spec-latest.html

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Hui Yiqun <huiyiqun@gmail.com>
---
 t/test-lib.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0b47eb6..60a837a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -93,7 +93,17 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e '
 	my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
 	print join("\n", @vars);
 ')
+# Unset all environment variables defined in xdg base dir spec[1]
+# to make sure that the test are running with a known state.
+#
+# [1] https://specifications.freedesktop.org/basedir-spec
+#     /basedir-spec-latest.html
+unset XDG_DATA_HOME
 unset XDG_CONFIG_HOME
+unset XDG_DATA_DIRS
+unset XDG_CONFIG_DIRS
+unset XDG_CACHE_HOME
+unset XDG_RUNTIME_DIR
 unset GITPERLLIB
 GIT_AUTHOR_EMAIL=author@example.com
 GIT_AUTHOR_NAME='A U Thor'
-- 
2.7.4

[PATCH v3/GSoC 5/5] t0301: test credential-cache support of XDG_RUNTIME_DIR

[Hui Yiqun]

t0301 now tests git-credential-cache support for XDG user-specific
runtime path. Specifically:

* if $XDG_RUNTIME_DIR exists, use socket at
  `$XDG_RUNTIME_DIR/git/credential-cache.sock`.

* otherwise, `/tmp/git-$uid/credential-cache.sock` is taken.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Hui Yiqun <huiyiqun@gmail.com>
---
 t/t0301-credential-cache.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index 82c8411..264c394 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -12,7 +12,36 @@ test -z "$NO_UNIX_SOCKETS" || {
 # don't leave a stale daemon running
 trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT
 
+test_expect_success 'set $XDG_RUNTIME_DIR' '
+	XDG_RUNTIME_DIR=$HOME/xdg_runtime/ &&
+	export XDG_RUNTIME_DIR
+'
+
+helper_test cache
+
+test_expect_success 'when $XDG_RUNTIME_DIR is set, `$XDG_RUNTIME_DIR/git` are used' '
+	test_path_is_missing "/tmp/git-$(id -u)/credential-cache.sock" &&
+	test -S "$HOME/xdg_runtime/git/credential-cache.sock"
+'
+
+test_expect_success 'force git-credential-cache to exit so that socket disappear' '
+	git credential-cache exit &&
+	test_path_is_missing "$HOME/xdg_runtime/git/credential-cache.sock" &&
+	unset XDG_RUNTIME_DIR
+'
+
 helper_test cache
+
+test_expect_success 'when $XDG_RUNTIME_DIR is not set, `/tmp/git-$(id -u) is used' '
+	test_path_is_missing "$HOME/xdg_runtime/git/credential-cache.sock" &&
+	test -S "/tmp/git-$(id -u)/credential-cache.sock"
+'
+
+# TODO: if $XDG_RUNTIME_DIR/git/ exists, but has wrong permission and ownership,
+# `helper_test cache` must fail.
+
+# TODO: check whether `--socket` works
+
 helper_test_timeout cache --timeout=1
 
 # we can't rely on our "trap" above working after test_done,
-- 
2.7.4

Re: [PATCH v3/GSoC 1/5] path.c: implement strbuf_mkpath()

[惠轶群]

So, are there some additional points I could improve?

2016-03-23 18:13 GMT+08:00 Hui Yiqun <huiyiqun@gmail.com>:
> There were already `mkpath`, `mkpathdup` and `mksnpath` for build
> filename, but lacked a version of `strbuf_` just like `strbuf_git_path`.
>
> It is convenient to build a path and manipulate the result later with
> strbuf.
>
> Signed-off-by: Hui Yiqun <huiyiqun@gmail.com>
> ---
>  cache.h |  2 ++
>  path.c  | 21 +++++++++++++++++----
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index b829410..ef843c1 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -781,6 +781,8 @@ extern char *git_pathdup(const char *fmt, ...)
>         __attribute__((format (printf, 1, 2)));
>  extern char *mkpathdup(const char *fmt, ...)
>         __attribute__((format (printf, 1, 2)));
> +extern void strbuf_mkpath(struct strbuf *sb, const char *fmt, ...)
> +       __attribute__((format (printf, 2, 3)));
>  extern char *git_pathdup_submodule(const char *path, const char *fmt, ...)
>         __attribute__((format (printf, 2, 3)));
>
> diff --git a/path.c b/path.c
> index 8b7e168..699af68 100644
> --- a/path.c
> +++ b/path.c
> @@ -433,14 +433,19 @@ char *git_pathdup(const char *fmt, ...)
>         return strbuf_detach(&path, NULL);
>  }
>
> +static void do_mkpath(struct strbuf *buf, const char *fmt, va_list args)
> +{
> +       strbuf_vaddf(buf, fmt, args);
> +       strbuf_cleanup_path(buf);
> +}
> +
>  char *mkpathdup(const char *fmt, ...)
>  {
>         struct strbuf sb = STRBUF_INIT;
>         va_list args;
>         va_start(args, fmt);
> -       strbuf_vaddf(&sb, fmt, args);
> +       do_mkpath(&sb, fmt, args);
>         va_end(args);
> -       strbuf_cleanup_path(&sb);
>         return strbuf_detach(&sb, NULL);
>  }
>
> @@ -449,9 +454,17 @@ const char *mkpath(const char *fmt, ...)
>         va_list args;
>         struct strbuf *pathname = get_pathname();
>         va_start(args, fmt);
> -       strbuf_vaddf(pathname, fmt, args);
> +       do_mkpath(pathname, fmt, args);
> +       va_end(args);
> +       return pathname->buf;
> +}
> +
> +void strbuf_mkpath(struct strbuf *buf, const char *fmt, ...)
> +{
> +       va_list args;
> +       va_start(args, fmt);
> +       do_mkpath(buf, fmt, args);
>         va_end(args);
> -       return cleanup_path(pathname->buf);
>  }
>
>  static void do_submodule_path(struct strbuf *buf, const char *path,
> --
> 2.7.4
>

Re: [PATCH v3/GSoC 1/5] path.c: implement strbuf_mkpath()

[Jeff King]

On Wed, Mar 23, 2016 at 06:13:21PM +0800, Hui Yiqun wrote:

> There were already `mkpath`, `mkpathdup` and `mksnpath` for build
> filename, but lacked a version of `strbuf_` just like `strbuf_git_path`.
> 
> It is convenient to build a path and manipulate the result later with
> strbuf.

Makes sense, and the patch looks fine to me.

-Peff

Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()

[Jeff King]

On Wed, Mar 23, 2016 at 06:13:22PM +0800, Hui Yiqun wrote:

> +/**
> + * this function does the following:
> + *
> + * 1. if $XDG_RUNTIME_DIR is non-empty, `$XDG_RUNTIME_DIR/git` is used in next
> + * step, otherwise `/tmp/git-$uid` is taken.
> + * 2. ensure that above directory does exist. what's more, it must has correct
> + * permission and ownership.
> + * 3. a newly allocated string consisting of the path of above directory and
> + * $filename is returned.
> + *
> + * Under following situation, NULL will be returned:
> + *
> + * + the directory mentioned in step 1 exists but have wrong permission or
> + * ownership.
> + * + the directory or its parent cannot be created.
> + *
> + * Notice:
> + *
> + * + the caller is responsible for deallocating the returned string.
> + *
> + */
> +extern char *xdg_runtime_dir(const char *filename);

There's a lot of "what" here that the caller doesn't really care about,
and which may go stale with respect to the implementation over time. Can
we make something more succinct like:

  /*
   * Return a path suitable for writing run-time files related to git,
   * or NULL if no such path can be established. The resulting string
   * should be freed by the caller.
   */

?

> --- a/path.c
> +++ b/path.c
> @@ -5,6 +5,7 @@
>  #include "strbuf.h"
>  #include "string-list.h"
>  #include "dir.h"
> +#include "git-compat-util.h"

Why do we need this? It should generally be the first file included, as
it sets up defines used by other header files. It looks like we include
"cache.h" in this file, which is enough (it explicitly includes
git-compat-util.h first to cover this case).

> +char *xdg_runtime_dir(const char *filename)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	char *runtime_dir;
> +	struct stat st;
> +	uid_t uid = getuid();
> +
> +	assert(filename);
> +	runtime_dir = getenv("XDG_RUNTIME_DIR");
> +	if (runtime_dir && *runtime_dir)
> +		strbuf_mkpath(&sb, "%s/git/", runtime_dir);
> +	else
> +		strbuf_mkpath(&sb, "/tmp/git-%d", uid);
> +
> +	if (!lstat(sb.buf, &st)) {
> +		/*
> +		 * As described in XDG base dir spec[1], the subdirectory
> +		 * under $XDG_RUNTIME_DIR or its fallback MUST be owned by
> +		 * the user, and its unix access mode MUST be 0700.
> +		 *
> +		 * Calling chmod or chown silently may cause security
> +		 * problem if somebody chdir to it, sleep, and then, try
> +		 * to open our protected runtime cache or socket.
> +		 * So we just put warning and left it to user to solve.
> +		 *

There are some minor English problems here (and elsewhere). E.g., you
probably want "So we just issue a warning and leave it to the user to
solve.".

> +		if ((st.st_mode & 0777) != S_IRWXU) {
> +			warning("permission of runtime directory '%s' "
> +					"MUST be 0700 instead of 0%o\n",
> +					sb.buf, (st.st_mode & 0777));
> +			return NULL;
> +		} else if (st.st_uid != uid) {
> +			warning("owner of runtime directory '%s' "
> +					"MUST be %d instead of %d\n",
> +					sb.buf, uid, st.st_uid);
> +			return NULL;
> +		}

These cases still leak "sb", I think.

> +		/* TODO: check whether st.buf is an directory */

Should we complete this todo? It's should just be S_ISDIR(st.st_mode).

> +	} else {
> +		if (safe_create_leading_directories_const(sb.buf) < 0) {
> +			warning("unable to create directories for '%s'\n",
> +					sb.buf);
> +			return NULL;
> +		}
> +		if (mkdir(sb.buf, 0700) < 0) {
> +			warning("unable to mkdir '%s'\n", sb.buf);
> +			return NULL;
> +		}

These ones leak, too.

-Peff

Re: [PATCH v3/GSoC 3/5] git-credential-cache: put socket to xdg-compatible path

[Jeff King]

On Wed, Mar 23, 2016 at 06:13:23PM +0800, Hui Yiqun wrote:

> move .git-credential-cache/socket to xdg_runtime_dir("credential-cache.sock")

Motivation?

> diff --git a/credential-cache.c b/credential-cache.c
> index f4afdc6..40d838b 100644
> --- a/credential-cache.c
> +++ b/credential-cache.c
> @@ -105,7 +105,7 @@ int main(int argc, const char **argv)
>  	op = argv[0];
>  
>  	if (!socket_path)
> -		socket_path = expand_user_path("~/.git-credential-cache/socket");
> +		socket_path = xdg_runtime_dir("credential-cache.sock");
>  	if (!socket_path)
>  		die("unable to find a suitable socket path; use --socket");

We do our own mkdir and chmod in credential-cache; this should be
redundant with what xdg_runtime_dir() does, and can be removed, right?

-Peff

Re: [PATCH v3/GSoC 4/5] test-lib.sh: unset all environment variables defined in xdg base dir spec[1]

[Jeff King]

On Wed, Mar 23, 2016 at 06:13:24PM +0800, Hui Yiqun wrote:

> Otherwise, on environments where these variables and set, an assignment
> to one of these variables will cause the variable being implicitly exported.

Yes, that's one problem, though I think the more general problem is
simply that they pollute the test environment (so git might actually be
touching your _real_ credential-cache socket, and not a fake test one).

I'd probably write:

  We try to clean the test environment of variables that may affect the
  outcome, so that the tests always start from a known state. We already
  clean XDG_CONFIG_HOME, but now that we support XDG_RUNTIME_DIR, we
  must clean that, too. While we're at it, let's simply cover all
  variables mentioned in the xdg spec[1] to future-proof and to cover
  any places where they might unexpectedly have an impact.

Feel free to use or adapt that as you see fit.

> +# Unset all environment variables defined in xdg base dir spec[1]
> +# to make sure that the test are running with a known state.
> +#
> +# [1] https://specifications.freedesktop.org/basedir-spec
> +#     /basedir-spec-latest.html
> +unset XDG_DATA_HOME
>  unset XDG_CONFIG_HOME
> +unset XDG_DATA_DIRS
> +unset XDG_CONFIG_DIRS
> +unset XDG_CACHE_HOME
> +unset XDG_RUNTIME_DIR

Thanks for being thorough here. This is much nicer than just adding
XDG_RUNTIME_DIR.

-Peff

Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()

[惠轶群]

2016-03-25 17:59 GMT+08:00 Jeff King <peff@peff.net>:
> On Wed, Mar 23, 2016 at 06:13:22PM +0800, Hui Yiqun wrote:
>
>> +/**
>> + * this function does the following:
>> + *
>> + * 1. if $XDG_RUNTIME_DIR is non-empty, `$XDG_RUNTIME_DIR/git` is used in next
>> + * step, otherwise `/tmp/git-$uid` is taken.
>> + * 2. ensure that above directory does exist. what's more, it must has correct
>> + * permission and ownership.
>> + * 3. a newly allocated string consisting of the path of above directory and
>> + * $filename is returned.
>> + *
>> + * Under following situation, NULL will be returned:
>> + *
>> + * + the directory mentioned in step 1 exists but have wrong permission or
>> + * ownership.
>> + * + the directory or its parent cannot be created.
>> + *
>> + * Notice:
>> + *
>> + * + the caller is responsible for deallocating the returned string.
>> + *
>> + */
>> +extern char *xdg_runtime_dir(const char *filename);
>
> There's a lot of "what" here that the caller doesn't really care about,
> and which may go stale with respect to the implementation over time. Can
> we make something more succinct like:
>
>   /*
>    * Return a path suitable for writing run-time files related to git,
>    * or NULL if no such path can be established. The resulting string
>    * should be freed by the caller.
>    */
>
> ?

That's clearer, but if I were the caller, I would worry about the
security of the path.
How about adding:

The security of the path is ensured by file permission.

>
>> --- a/path.c
>> +++ b/path.c
>> @@ -5,6 +5,7 @@
>>  #include "strbuf.h"
>>  #include "string-list.h"
>>  #include "dir.h"
>> +#include "git-compat-util.h"
>
> Why do we need this? It should generally be the first file included, as
> it sets up defines used by other header files. It looks like we include
> "cache.h" in this file, which is enough (it explicitly includes
> git-compat-util.h first to cover this case).

I include this header for `getuid` and `stat`. Now that there is an indirect
including, I will delete this one.

>
>> +char *xdg_runtime_dir(const char *filename)
>> +{
>> +     struct strbuf sb = STRBUF_INIT;
>> +     char *runtime_dir;
>> +     struct stat st;
>> +     uid_t uid = getuid();
>> +
>> +     assert(filename);
>> +     runtime_dir = getenv("XDG_RUNTIME_DIR");
>> +     if (runtime_dir && *runtime_dir)
>> +             strbuf_mkpath(&sb, "%s/git/", runtime_dir);
>> +     else
>> +             strbuf_mkpath(&sb, "/tmp/git-%d", uid);
>> +
>> +     if (!lstat(sb.buf, &st)) {
>> +             /*
>> +              * As described in XDG base dir spec[1], the subdirectory
>> +              * under $XDG_RUNTIME_DIR or its fallback MUST be owned by
>> +              * the user, and its unix access mode MUST be 0700.
>> +              *
>> +              * Calling chmod or chown silently may cause security
>> +              * problem if somebody chdir to it, sleep, and then, try
>> +              * to open our protected runtime cache or socket.
>> +              * So we just put warning and left it to user to solve.
>> +              *
>
> There are some minor English problems here (and elsewhere). E.g., you
> probably want "So we just issue a warning and leave it to the user to
> solve.".

Sorry for my English.

>> +             if ((st.st_mode & 0777) != S_IRWXU) {
>> +                     warning("permission of runtime directory '%s' "
>> +                                     "MUST be 0700 instead of 0%o\n",
>> +                                     sb.buf, (st.st_mode & 0777));
>> +                     return NULL;
>> +             } else if (st.st_uid != uid) {
>> +                     warning("owner of runtime directory '%s' "
>> +                                     "MUST be %d instead of %d\n",
>> +                                     sb.buf, uid, st.st_uid);
>> +                     return NULL;
>> +             }
>
> These cases still leak "sb", I think.
>
>> +             /* TODO: check whether st.buf is an directory */
>
> Should we complete this todo? It's should just be S_ISDIR(st.st_mode).
>
>> +     } else {
>> +             if (safe_create_leading_directories_const(sb.buf) < 0) {
>> +                     warning("unable to create directories for '%s'\n",
>> +                                     sb.buf);
>> +                     return NULL;
>> +             }
>> +             if (mkdir(sb.buf, 0700) < 0) {
>> +                     warning("unable to mkdir '%s'\n", sb.buf);
>> +                     return NULL;
>> +             }
>
> These ones leak, too.

I will deal with it.

I find there are some similar leakage in this file. I'll fix them in
another patch.

Do you think we need some additional comments for the release of strbuf?
>
> -Peff

Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()

[惠轶群]

> I find there are some similar leakage in this file. I'll fix them in
> another patch.
>
> Do you think we need some additional comments for the release of strbuf?

For example, in strbuf.h.

Re: [PATCH v3/GSoC 3/5] git-credential-cache: put socket to xdg-compatible path

[惠轶群]

2016-03-25 18:00 GMT+08:00 Jeff King <peff@peff.net>:
> On Wed, Mar 23, 2016 at 06:13:23PM +0800, Hui Yiqun wrote:
>
>> move .git-credential-cache/socket to xdg_runtime_dir("credential-cache.sock")
>
> Motivation?

I will improve my commit message.

>
>> diff --git a/credential-cache.c b/credential-cache.c
>> index f4afdc6..40d838b 100644
>> --- a/credential-cache.c
>> +++ b/credential-cache.c
>> @@ -105,7 +105,7 @@ int main(int argc, const char **argv)
>>       op = argv[0];
>>
>>       if (!socket_path)
>> -             socket_path = expand_user_path("~/.git-credential-cache/socket");
>> +             socket_path = xdg_runtime_dir("credential-cache.sock");
>>       if (!socket_path)
>>               die("unable to find a suitable socket path; use --socket");
>
> We do our own mkdir and chmod in credential-cache; this should be
> redundant with what xdg_runtime_dir() does, and can be removed, right?

But user may specify another path via --socket <path>, this path may have
wrong permission. I'm considering how to handle this situation.

>
> -Peff

Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()

[Junio C Hamano]

惠轶群 <huiyiqun@gmail.com> writes:

>> There's a lot of "what" here that the caller doesn't really care about,
>> and which may go stale with respect to the implementation over time. Can
>> we make something more succinct like:
>>
>>   /*
>>    * Return a path suitable for writing run-time files related to git,
>>    * or NULL if no such path can be established. The resulting string
>>    * should be freed by the caller.
>>    */
>>
>> ?
>
> That's clearer, but if I were the caller, I would worry about the
> security of the path.
> How about adding:
>
> The security of the path is ensured by file permission.

Is "by file permission" descriptive enough?

To protect /a/b/c/socket, what filesystem entities have the right
permission bits set?  If the parent directory is writable by an
attacker, the permission bits on 'socket' itself may not matter as
the attacker can rename it away and create new one herself, for
example.

> I will deal with it.
>
> I find there are some similar leakage in this file. I'll fix them in
> another patch.
>
> Do you think we need some additional comments for the release of strbuf?

As Documentation/technical/api-strbuf.txt has this, I think we are
already OK.

`strbuf_release`::

	Release a string buffer and the memory it used. You should not use the
	string buffer after using this function, unless you initialize it again.

Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()

[Jeff King]

On Fri, Mar 25, 2016 at 09:55:59AM -0700, Junio C Hamano wrote:

> 惠轶群 <huiyiqun@gmail.com> writes:
> 
> >> There's a lot of "what" here that the caller doesn't really care about,
> >> and which may go stale with respect to the implementation over time. Can
> >> we make something more succinct like:
> >>
> >>   /*
> >>    * Return a path suitable for writing run-time files related to git,
> >>    * or NULL if no such path can be established. The resulting string
> >>    * should be freed by the caller.
> >>    */
> >>
> >> ?
> >
> > That's clearer, but if I were the caller, I would worry about the
> > security of the path.
> > How about adding:
> >
> > The security of the path is ensured by file permission.
> 
> Is "by file permission" descriptive enough?
> 
> To protect /a/b/c/socket, what filesystem entities have the right
> permission bits set?  If the parent directory is writable by an
> attacker, the permission bits on 'socket' itself may not matter as
> the attacker can rename it away and create new one herself, for
> example.

I think that is discussed elsewhere, and referring to the xdg document
is enough. My main point is that the docstring about a function should
tell a potential caller what they need to know to use it, but if it gets
overly long, that information is lost in the noise.

-Peff

Re: [PATCH v3/GSoC 3/5] git-credential-cache: put socket to xdg-compatible path

[Jeff King]

On Fri, Mar 25, 2016 at 10:28:55PM +0800, 惠轶群 wrote:

> >> diff --git a/credential-cache.c b/credential-cache.c
> >> index f4afdc6..40d838b 100644
> >> --- a/credential-cache.c
> >> +++ b/credential-cache.c
> >> @@ -105,7 +105,7 @@ int main(int argc, const char **argv)
> >>       op = argv[0];
> >>
> >>       if (!socket_path)
> >> -             socket_path = expand_user_path("~/.git-credential-cache/socket");
> >> +             socket_path = xdg_runtime_dir("credential-cache.sock");
> >>       if (!socket_path)
> >>               die("unable to find a suitable socket path; use --socket");
> >
> > We do our own mkdir and chmod in credential-cache; this should be
> > redundant with what xdg_runtime_dir() does, and can be removed, right?
> 
> But user may specify another path via --socket <path>, this path may have
> wrong permission. I'm considering how to handle this situation.

Good point, we do need to cover that case.

Perhaps the work done by xdg_runtime_dir() needs to be split into two
fucntions: one to just provide the path, and the second to securely
create a given path.

-Peff

Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()

[Jeff King]

On Fri, Mar 25, 2016 at 10:21:48PM +0800, 惠轶群 wrote:

> > There are some minor English problems here (and elsewhere). E.g., you
> > probably want "So we just issue a warning and leave it to the user to
> > solve.".
> 
> Sorry for my English.

Thanks. And sorry if that sounded too harsh. I know that working in a
non-native language is tough. Usually in a review I'll try to provide
specific English fixes, but in this case, I think a lot of these
messages are still in flux, so I I didn't want to waste either of our
time going over specifics if the content is just going to change later.

> > These ones leak, too.
> 
> I will deal with it.
> 
> I find there are some similar leakage in this file. I'll fix them in
> another patch.

Great, thanks.

-Peff

Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > That's clearer, but if I were the caller, I would worry about the
>> > security of the path.
>> > How about adding:
>> >
>> > The security of the path is ensured by file permission.
>> 
>> Is "by file permission" descriptive enough?
>> 
>> To protect /a/b/c/socket, what filesystem entities have the right
>> permission bits set?  If the parent directory is writable by an
>> attacker, the permission bits on 'socket' itself may not matter as
>> the attacker can rename it away and create new one herself, for
>> example.
>
> I think that is discussed elsewhere, and referring to the xdg document
> is enough. My main point is that the docstring about a function should
> tell a potential caller what they need to know to use it, but if it gets
> overly long, that information is lost in the noise.

I agree with your main point, and I was wondering if "by file
permission" is merely adding yet another noise if there is
discussion elsewhere already, and/or if it does not refer to an
external document that has a fuller discussion, because it lacks any
useful information by itself.

Re: [PATCH v3/GSoC 3/5] git-credential-cache: put socket to xdg-compatible path

[惠轶群]

2016-03-26 1:56 GMT+08:00 Jeff King <peff@peff.net>:
> On Fri, Mar 25, 2016 at 10:28:55PM +0800, 惠轶群 wrote:
>
>> >> diff --git a/credential-cache.c b/credential-cache.c
>> >> index f4afdc6..40d838b 100644
>> >> --- a/credential-cache.c
>> >> +++ b/credential-cache.c
>> >> @@ -105,7 +105,7 @@ int main(int argc, const char **argv)
>> >>       op = argv[0];
>> >>
>> >>       if (!socket_path)
>> >> -             socket_path = expand_user_path("~/.git-credential-cache/socket");
>> >> +             socket_path = xdg_runtime_dir("credential-cache.sock");
>> >>       if (!socket_path)
>> >>               die("unable to find a suitable socket path; use --socket");
>> >
>> > We do our own mkdir and chmod in credential-cache; this should be
>> > redundant with what xdg_runtime_dir() does, and can be removed, right?
>>
>> But user may specify another path via --socket <path>, this path may have
>> wrong permission. I'm considering how to handle this situation.
>
> Good point, we do need to cover that case.
>
> Perhaps the work done by xdg_runtime_dir() needs to be split into two
> fucntions: one to just provide the path, and the second to securely
> create a given path.

Good, I will implement it like that.

>
> -Peff

Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()

[惠轶群]

2016-03-26 0:55 GMT+08:00 Junio C Hamano <gitster@pobox.com>:
> 惠轶群 <huiyiqun@gmail.com> writes:
>
>>> There's a lot of "what" here that the caller doesn't really care about,
>>> and which may go stale with respect to the implementation over time. Can
>>> we make something more succinct like:
>>>
>>>   /*
>>>    * Return a path suitable for writing run-time files related to git,
>>>    * or NULL if no such path can be established. The resulting string
>>>    * should be freed by the caller.
>>>    */
>>>
>>> ?
>>
>> That's clearer, but if I were the caller, I would worry about the
>> security of the path.
>> How about adding:
>>
>> The security of the path is ensured by file permission.
>
> Is "by file permission" descriptive enough?
>
> To protect /a/b/c/socket, what filesystem entities have the right
> permission bits set?  If the parent directory is writable by an
> attacker, the permission bits on 'socket' itself may not matter as
> the attacker can rename it away and create new one herself, for
> example.
>
>> I will deal with it.
>>
>> I find there are some similar leakage in this file. I'll fix them in
>> another patch.
>>
>> Do you think we need some additional comments for the release of strbuf?
>
> As Documentation/technical/api-strbuf.txt has this, I think we are
> already OK.
>
> `strbuf_release`::
>
>         Release a string buffer and the memory it used. You should not use the
>         string buffer after using this function, unless you initialize it again.
>

Excuse me, but I could not find `Documentation/technical/api-strbuf.txt` in
master branch. Do you refer to the header of `strbuf.h`? In which, I learnt how
to initialize the strbuf and how to take use of it when I began to use
it. If there
is also a note about whether I should release it and how do it, such as:

For every strbuf that has been initialized and buffer of it has not
been detached
with strbuf_detach, you should release the resource by strbuf_release.

It will save me (maybe others) much time to explore the entire method list.

Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()

[惠轶群]

2016-03-26 1:59 GMT+08:00 Jeff King <peff@peff.net>:
> On Fri, Mar 25, 2016 at 10:21:48PM +0800, 惠轶群 wrote:
>
>> > There are some minor English problems here (and elsewhere). E.g., you
>> > probably want "So we just issue a warning and leave it to the user to
>> > solve.".
>>
>> Sorry for my English.
>
> Thanks. And sorry if that sounded too harsh. I know that working in a
> non-native language is tough. Usually in a review I'll try to provide
> specific English fixes, but in this case, I think a lot of these
> messages are still in flux, so I I didn't want to waste either of our
> time going over specifics if the content is just going to change later.
>
>> > These ones leak, too.
>>
>> I will deal with it.
>>
>> I find there are some similar leakage in this file. I'll fix them in
>> another patch.
>
> Great, thanks.

After read the source code of strbuf more carefully, I get the conclusion
that if a strbuf is initialized with STRBUF_INIT but is not used, there is
no need to release it. Is it true?

> -Peff

Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()

[Junio C Hamano]

惠轶群 <huiyiqun@gmail.com> writes:

> Excuse me, but I could not find
> `Documentation/technical/api-strbuf.txt` in master branch. Do you
> refer to the header of `strbuf.h`? In which, I learnt how to
> initialize the strbuf and how to take use of it when I began to
> use it.

Ah, yes, it was a relatively recent development that usage text from
the former was moved to the latter.

> If there is also a note about whether I should release it
> and how do it, such as:
>
> For every strbuf that has been initialized and buffer of it has
> not been detached with strbuf_detach, you should release the
> resource by strbuf_release.
>
> It will save me (maybe others) much time to explore the entire method list.

While it may not hurt, it sounds somewhat extreme and funny me, as
it sounds as if you are saying "for every pointer variable that
points at a piece of memory allocated with malloc(3) and friends,
you must free(3) them after you are done".

The way _release() refers to its internal resource usage could be
improved, I guess.  "Release ... the memory it used" may not click
as freeing the memory to some readers.

 strbuf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/strbuf.h b/strbuf.h
index f72fd14..a783f09 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -82,7 +82,7 @@ extern char strbuf_slopbuf[];
 extern void strbuf_init(struct strbuf *, size_t);
 
 /**
- * Release a string buffer and the memory it used. You should not use the
+ * Release a string buffer and free the memory it used. You should not use the
  * string buffer after using this function, unless you initialize it again.
  */
 extern void strbuf_release(struct strbuf *);

Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()

[Junio C Hamano]

惠轶群 <huiyiqun@gmail.com> writes:

> After read the source code of strbuf more carefully, I get the conclusion
> that if a strbuf is initialized with STRBUF_INIT but is not used, there is
> no need to release it. Is it true?

If it is initialized with STRBUF_INIT and never used, there is no
reason for the variable to exist ;-)

Leaving the variable in the code, and not calling release on it at
the end, would be OK (i.e. there is no leak) today, but may invite
future bugs (e.g. people may use the variable to tentatively build
their string before the function returns to leave the scope of the
variable without adding _release() themselves).

Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()

[惠轶群]

2016-03-28 22:50 GMT+08:00 Junio C Hamano <gitster@pobox.com>:
> 惠轶群 <huiyiqun@gmail.com> writes:
>
>> After read the source code of strbuf more carefully, I get the conclusion
>> that if a strbuf is initialized with STRBUF_INIT but is not used, there is
>> no need to release it. Is it true?
>
> If it is initialized with STRBUF_INIT and never used, there is no
> reason for the variable to exist ;-)

I mean, if some developer return before the strbuf is used, there seems
no need to release the strbuf according to the current implementation.

But I'm not sure whether this is suitable for the abstraction of strbuf.

For example:

    const char *enter_repo(const char *path, int strict)
    {
        static struct strbuf validated_path = STRBUF_INIT;
        static struct strbuf used_path = STRBUF_INIT;

        if (!path)
            return NULL; // no need to release, right?
        ...
    }


> Leaving the variable in the code, and not calling release on it at
> the end, would be OK (i.e. there is no leak) today, but may invite
> future bugs (e.g. people may use the variable to tentatively build
> their string before the function returns to leave the scope of the
> variable without adding _release() themselves).

[PATCH] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage

[Hui Yiqun]

According to strbuf.h, strbuf_detach is the sole supported method
to unwrap a memory buffer from its strbuf shell.

So we should not return the pointer of strbuf.buf directly.

What's more, some memory leakages are solved.
---
 path.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/path.c b/path.c
index 969b494..b07e5a7 100644
--- a/path.c
+++ b/path.c
@@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)
 {
 	static struct strbuf validated_path = STRBUF_INIT;
 	static struct strbuf used_path = STRBUF_INIT;
+	char * dbuf = NULL;
 
 	if (!path)
 		return NULL;
@@ -654,7 +655,7 @@ const char *enter_repo(const char *path, int strict)
 		if (used_path.buf[0] == '~') {
 			char *newpath = expand_user_path(used_path.buf);
 			if (!newpath)
-				return NULL;
+				goto return_null;
 			strbuf_attach(&used_path, newpath, strlen(newpath),
 				      strlen(newpath));
 		}
@@ -671,22 +672,22 @@ const char *enter_repo(const char *path, int strict)
 			strbuf_setlen(&used_path, baselen);
 		}
 		if (!suffix[i])
-			return NULL;
+			goto return_null;
 		gitfile = read_gitfile(used_path.buf);
 		if (gitfile) {
 			strbuf_reset(&used_path);
 			strbuf_addstr(&used_path, gitfile);
 		}
 		if (chdir(used_path.buf))
-			return NULL;
-		path = validated_path.buf;
+			goto return_null;
+		path = dbuf = strbuf_detach(&validated_path, NULL);
 	}
 	else {
 		const char *gitfile = read_gitfile(path);
 		if (gitfile)
 			path = gitfile;
 		if (chdir(path))
-			return NULL;
+			goto return_null;
 	}
 
 	if (is_git_directory(".")) {
@@ -695,6 +696,10 @@ const char *enter_repo(const char *path, int strict)
 		return path;
 	}
 
+return_null:
+    free(dbuf);
+	strbuf_release(&used_path);
+	strbuf_release(&validated_path);
 	return NULL;
 }
 
-- 
2.7.4

[PATCH v2] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage

[Hui Yiqun]

According to strbuf.h, strbuf_detach is the sole supported method
to unwrap a memory buffer from its strbuf shell.

So we should not return the pointer of strbuf.buf directly.

What's more, some memory leakages are solved.
---
 path.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/path.c b/path.c
index 969b494..b07e5a7 100644
--- a/path.c
+++ b/path.c
@@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)
 {
 	static struct strbuf validated_path = STRBUF_INIT;
 	static struct strbuf used_path = STRBUF_INIT;
+	char * dbuf = NULL;
 
 	if (!path)
 		return NULL;
@@ -654,7 +655,7 @@ const char *enter_repo(const char *path, int strict)
 		if (used_path.buf[0] == '~') {
 			char *newpath = expand_user_path(used_path.buf);
 			if (!newpath)
-				return NULL;
+				goto return_null;
 			strbuf_attach(&used_path, newpath, strlen(newpath),
 				      strlen(newpath));
 		}
@@ -671,22 +672,22 @@ const char *enter_repo(const char *path, int strict)
 			strbuf_setlen(&used_path, baselen);
 		}
 		if (!suffix[i])
-			return NULL;
+			goto return_null;
 		gitfile = read_gitfile(used_path.buf);
 		if (gitfile) {
 			strbuf_reset(&used_path);
 			strbuf_addstr(&used_path, gitfile);
 		}
 		if (chdir(used_path.buf))
-			return NULL;
-		path = validated_path.buf;
+			goto return_null;
+		path = dbuf = strbuf_detach(&validated_path, NULL);
 	}
 	else {
 		const char *gitfile = read_gitfile(path);
 		if (gitfile)
 			path = gitfile;
 		if (chdir(path))
-			return NULL;
+			goto return_null;
 	}
 
 	if (is_git_directory(".")) {
@@ -695,6 +696,10 @@ const char *enter_repo(const char *path, int strict)
 		return path;
 	}
 
+return_null:
+    free(dbuf);
+	strbuf_release(&used_path);
+	strbuf_release(&validated_path);
 	return NULL;
 }
 
-- 
2.7.4

[PATCH v3] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage

[Hui Yiqun]

According to strbuf.h, strbuf_detach is the sole supported method
to unwrap a memory buffer from its strbuf shell.

So we should not return the pointer of strbuf.buf directly.

What's more, some memory leakages are solved.
---
 path.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/path.c b/path.c
index 969b494..9801617 100644
--- a/path.c
+++ b/path.c
@@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)
 {
 	static struct strbuf validated_path = STRBUF_INIT;
 	static struct strbuf used_path = STRBUF_INIT;
+	char * dbuf = NULL;
 
 	if (!path)
 		return NULL;
@@ -654,7 +655,7 @@ const char *enter_repo(const char *path, int strict)
 		if (used_path.buf[0] == '~') {
 			char *newpath = expand_user_path(used_path.buf);
 			if (!newpath)
-				return NULL;
+				goto return_null;
 			strbuf_attach(&used_path, newpath, strlen(newpath),
 				      strlen(newpath));
 		}
@@ -671,22 +672,22 @@ const char *enter_repo(const char *path, int strict)
 			strbuf_setlen(&used_path, baselen);
 		}
 		if (!suffix[i])
-			return NULL;
+			goto return_null;
 		gitfile = read_gitfile(used_path.buf);
 		if (gitfile) {
 			strbuf_reset(&used_path);
 			strbuf_addstr(&used_path, gitfile);
 		}
 		if (chdir(used_path.buf))
-			return NULL;
-		path = validated_path.buf;
+			goto return_null;
+		path = dbuf = strbuf_detach(&validated_path, NULL);
 	}
 	else {
 		const char *gitfile = read_gitfile(path);
 		if (gitfile)
 			path = gitfile;
 		if (chdir(path))
-			return NULL;
+			goto return_null;
 	}
 
 	if (is_git_directory(".")) {
@@ -695,6 +696,10 @@ const char *enter_repo(const char *path, int strict)
 		return path;
 	}
 
+return_null:
+	free(dbuf);
+	strbuf_release(&used_path);
+	strbuf_release(&validated_path);
 	return NULL;
 }
 
-- 
2.7.4

Re: [PATCH v3] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage

[惠轶群]

Sorry, I sent the patch repeatedly to fix a wrongly indent with space.

2016-03-28 23:57 GMT+08:00 Hui Yiqun <huiyiqun@gmail.com>:
> According to strbuf.h, strbuf_detach is the sole supported method
> to unwrap a memory buffer from its strbuf shell.
>
> So we should not return the pointer of strbuf.buf directly.
>
> What's more, some memory leakages are solved.
> ---
>  path.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/path.c b/path.c
> index 969b494..9801617 100644
> --- a/path.c
> +++ b/path.c
> @@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)
>  {
>         static struct strbuf validated_path = STRBUF_INIT;
>         static struct strbuf used_path = STRBUF_INIT;
> +       char * dbuf = NULL;
>
>         if (!path)
>                 return NULL;
> @@ -654,7 +655,7 @@ const char *enter_repo(const char *path, int strict)
>                 if (used_path.buf[0] == '~') {
>                         char *newpath = expand_user_path(used_path.buf);
>                         if (!newpath)
> -                               return NULL;
> +                               goto return_null;
>                         strbuf_attach(&used_path, newpath, strlen(newpath),
>                                       strlen(newpath));
>                 }
> @@ -671,22 +672,22 @@ const char *enter_repo(const char *path, int strict)
>                         strbuf_setlen(&used_path, baselen);
>                 }
>                 if (!suffix[i])
> -                       return NULL;
> +                       goto return_null;
>                 gitfile = read_gitfile(used_path.buf);
>                 if (gitfile) {
>                         strbuf_reset(&used_path);
>                         strbuf_addstr(&used_path, gitfile);
>                 }
>                 if (chdir(used_path.buf))
> -                       return NULL;
> -               path = validated_path.buf;
> +                       goto return_null;
> +               path = dbuf = strbuf_detach(&validated_path, NULL);
>         }
>         else {
>                 const char *gitfile = read_gitfile(path);
>                 if (gitfile)
>                         path = gitfile;
>                 if (chdir(path))
> -                       return NULL;
> +                       goto return_null;
>         }
>
>         if (is_git_directory(".")) {
> @@ -695,6 +696,10 @@ const char *enter_repo(const char *path, int strict)
>                 return path;
>         }
>
> +return_null:
> +       free(dbuf);
> +       strbuf_release(&used_path);
> +       strbuf_release(&validated_path);
>         return NULL;
>  }
>
> --
> 2.7.4
>

Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()

[Junio C Hamano]

惠轶群 <huiyiqun@gmail.com> writes:

> For example:
>
>     const char *enter_repo(const char *path, int strict)
>     {
>         static struct strbuf validated_path = STRBUF_INIT;
>         static struct strbuf used_path = STRBUF_INIT;
>
>         if (!path)
>             return NULL; // no need to release, right?
>         ...
>     }

Correct.

Re: [PATCH v2] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage

[Jeff King]

On Mon, Mar 28, 2016 at 11:56:10PM +0800, Hui Yiqun wrote:

> According to strbuf.h, strbuf_detach is the sole supported method
> to unwrap a memory buffer from its strbuf shell.
> 
> So we should not return the pointer of strbuf.buf directly.
> 
> What's more, some memory leakages are solved.

There is something else going on here, which makes this case different
than some others. Note that the function returns a const string:

> diff --git a/path.c b/path.c
> index 969b494..b07e5a7 100644
> --- a/path.c
> +++ b/path.c
> @@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)

By convention, that means that the result is not owned by the caller,
and should not be freed. We implement that by:

>  {
>  	static struct strbuf validated_path = STRBUF_INIT;
>  	static struct strbuf used_path = STRBUF_INIT;

...using static variables which persist after the call returns. So this
function retains ownership of the memory, and it remains valid until the
next call to enter_repo().

There's no leak when we return NULL, because the function retains
control of the memory (though it will hold onto it until the end of the
program if nobody calls enter_repo() again). And thus we shouldn't use
strbuf_detach(), which loses that reference to the memory (and since the
callers don't take ownership, it actually _creates_ a leak).

We could release the memory when returning, but I don't think it's a big
deal to do so.

-Peff

Re: [PATCH v3] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage

[Junio C Hamano]

Hui Yiqun <huiyiqun@gmail.com> writes:

> According to strbuf.h, strbuf_detach is the sole supported method
> to unwrap a memory buffer from its strbuf shell.
> ...
> diff --git a/path.c b/path.c
> index 969b494..9801617 100644
> --- a/path.c
> +++ b/path.c
> @@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)
>  {
>  	static struct strbuf validated_path = STRBUF_INIT;
>  	static struct strbuf used_path = STRBUF_INIT;
> ...
> +return_null:
> +	free(dbuf);
> +	strbuf_release(&used_path);
> +	strbuf_release(&validated_path);
>  	return NULL;
>  }

I see these strbuf's are "static" storage class, so that they do not
have to get freed.

Re: [PATCH v3] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage

[惠轶群]

2016-03-29 1:58 GMT+08:00 Junio C Hamano <gitster@pobox.com>:
> Hui Yiqun <huiyiqun@gmail.com> writes:
>
>> According to strbuf.h, strbuf_detach is the sole supported method
>> to unwrap a memory buffer from its strbuf shell.
>> ...
>> diff --git a/path.c b/path.c
>> index 969b494..9801617 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)
>>  {
>>       static struct strbuf validated_path = STRBUF_INIT;
>>       static struct strbuf used_path = STRBUF_INIT;
>> ...
>> +return_null:
>> +     free(dbuf);
>> +     strbuf_release(&used_path);
>> +     strbuf_release(&validated_path);
>>       return NULL;
>>  }
>
> I see these strbuf's are "static" storage class, so that they do not
> have to get freed.

I see, thanks.

Re: [PATCH v2] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage

[惠轶群]

2016-03-29 1:55 GMT+08:00 Jeff King <peff@peff.net>:
> On Mon, Mar 28, 2016 at 11:56:10PM +0800, Hui Yiqun wrote:
>
>> According to strbuf.h, strbuf_detach is the sole supported method
>> to unwrap a memory buffer from its strbuf shell.
>>
>> So we should not return the pointer of strbuf.buf directly.
>>
>> What's more, some memory leakages are solved.
>
> There is something else going on here, which makes this case different
> than some others. Note that the function returns a const string:
>
>> diff --git a/path.c b/path.c
>> index 969b494..b07e5a7 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)
>
> By convention, that means that the result is not owned by the caller,
> and should not be freed. We implement that by:
>
>>  {
>>       static struct strbuf validated_path = STRBUF_INIT;
>>       static struct strbuf used_path = STRBUF_INIT;
>
> ...using static variables which persist after the call returns. So this
> function retains ownership of the memory, and it remains valid until the
> next call to enter_repo().

Sorry for my carelessness. I didn't notice it.

> There's no leak when we return NULL, because the function retains
> control of the memory (though it will hold onto it until the end of the
> program if nobody calls enter_repo() again). And thus we shouldn't use
> strbuf_detach(), which loses that reference to the memory (and since the
> callers don't take ownership, it actually _creates_ a leak).
>
> We could release the memory when returning, but I don't think it's a big
> deal to do so.
>
> -Peff