threads / patch / 41793

v3, 5 partstest-lib.sh: unset all environment variables defined in xdg base dir spec[1]

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

## tl;dr

33 messages between Mar 23, 2016 and Mar 29, 2016. Diffs are folded; open one to read it.

replies: 32people: 3as markdown or json

Hui Yiqun· Mar 23, 2016, 10:13 UTC · lore

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

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(-)
Show changes to 2 files +19 −4

cache.h, path.c

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
Hui Yiqun· Mar 23, 2016, 10:13 UTC · re: Hui Yiqun · lore

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

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(+)
Show changes to 2 files +79 −0

cache.h, path.c

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
Jeff King· Mar 25, 2016, 09:59 UTC · re: Hui Yiqun · lore

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

On Wed, Mar 23, 2016 at 06:13:22PM +0800, Hui Yiqun wrote:
Show 22 quoted lines
> +/**
> + * 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.
   */
?
Show 7 quoted lines
> --- 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).

Show 25 quoted lines
> +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.".

Show 11 quoted lines
> +		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).
Show 10 quoted lines
> +	} 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
惠轶群· Mar 25, 2016, 14:21 UTC · re: Jeff King · lore

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>:
Show 36 quoted lines
> 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.
Show 13 quoted lines
>
>> --- 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.

Show 30 quoted lines
>
>> +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.
Show 30 quoted lines
>> +             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
惠轶群· Mar 25, 2016, 14:23 UTC · re: 惠轶群 · lore

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.
Junio C Hamano· Mar 25, 2016, 16:55 UTC · re: 惠轶群 · lore

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

惠轶群 <huiyiqun@gmail.com> writes:
Show 17 quoted lines
>> 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.

Show 6 quoted lines
> 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.
Jeff King· Mar 25, 2016, 17:55 UTC · re: Junio C Hamano · lore

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

On Fri, Mar 25, 2016 at 09:55:59AM -0700, Junio C Hamano wrote:
Show 27 quoted lines
> 惠轶群 <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
Junio C Hamano· Mar 25, 2016, 18:00 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 18 quoted lines
>> > 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.

惠轶群· Mar 28, 2016, 13:37 UTC · re: Junio C Hamano · lore

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>:
Show 43 quoted lines
> 惠轶群 <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.
Junio C Hamano· Mar 28, 2016, 14:35 UTC · re: 惠轶群 · lore

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

惠轶群 <huiyiqun@gmail.com> writes:
Show 5 quoted lines
> 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.

Show 8 quoted lines
> 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(-)
Show changes to strbuf.h +1 −1
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 *);
Jeff King· Mar 25, 2016, 17:59 UTC · re: 惠轶群 · lore

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

On Fri, Mar 25, 2016 at 10:21:48PM +0800, 惠轶群 wrote:
Show 5 quoted lines
> > 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.

Show 6 quoted lines
> > 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
惠轶群· Mar 28, 2016, 14:12 UTC · re: Jeff King · lore

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>:
Show 22 quoted lines
> 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
Junio C Hamano· Mar 28, 2016, 14:50 UTC · re: 惠轶群 · lore

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

惠轶群 <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).

惠轶群· Mar 28, 2016, 15:00 UTC · re: Junio C Hamano · lore

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>:
Show 8 quoted lines
> 惠轶群 <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?
        ...
    }
Show 5 quoted lines
> 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).
Junio C Hamano· Mar 28, 2016, 17:03 UTC · re: 惠轶群 · lore

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

惠轶群 <huiyiqun@gmail.com> writes:
Show 11 quoted lines
> 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.
Hui Yiqun· Mar 28, 2016, 15:51 UTC · re: Jeff King · lore

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

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(-)
Show changes to path.c +10 −5
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
Hui Yiqun· Mar 28, 2016, 15:56 UTC · re: Jeff King · lore

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

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(-)
Show changes to path.c +10 −5
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
Jeff King· Mar 28, 2016, 17:55 UTC · re: Hui Yiqun · lore

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

On Mon, Mar 28, 2016 at 11:56:10PM +0800, Hui Yiqun wrote:
Show 6 quoted lines
> 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:

Show 5 quoted lines
> 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
惠轶群· Mar 29, 2016, 02:40 UTC · re: Jeff King · lore

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>:
Show 28 quoted lines
> 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.
Show 10 quoted lines
> 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
Hui Yiqun· Mar 28, 2016, 15:57 UTC · re: Jeff King · lore

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

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(-)
Show changes to path.c +10 −5
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
惠轶群· Mar 28, 2016, 15:59 UTC · re: Hui Yiqun · lore

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>:
Show 72 quoted lines
> 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
>
Junio C Hamano· Mar 28, 2016, 17:58 UTC · re: Hui Yiqun · lore

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

Hui Yiqun <huiyiqun@gmail.com> writes:
Show 18 quoted lines
> 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.

惠轶群· Mar 29, 2016, 02:38 UTC · re: Junio C Hamano · lore

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>:
Show 23 quoted lines
> 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.
Hui Yiqun· Mar 23, 2016, 10:13 UTC · re: Hui Yiqun · lore

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

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(-)
Show changes to credential-cache.c +1 −1
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
Jeff King· Mar 25, 2016, 10:00 UTC · re: Hui Yiqun · lore

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

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?
Show 12 quoted lines
> 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
惠轶群· Mar 25, 2016, 14:28 UTC · re: Jeff King · lore

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>:
Show 5 quoted lines
> 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.
Show 16 quoted lines
>
>> 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
Jeff King· Mar 25, 2016, 17:56 UTC · re: 惠轶群 · lore

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

On Fri, Mar 25, 2016 at 10:28:55PM +0800, 惠轶群 wrote:
Show 18 quoted lines
> >> 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
惠轶群· Mar 25, 2016, 18:00 UTC · re: Jeff King · lore

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>:
Show 26 quoted lines
> 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
Hui Yiqun· Mar 23, 2016, 10:13 UTC · re: Hui Yiqun · lore

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(+)
Show changes to t/test-lib.sh +10 −0
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
Jeff King· Mar 25, 2016, 10:05 UTC · re: Hui Yiqun · lore

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

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.
Show 11 quoted lines
> +# 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
Hui Yiqun· Mar 23, 2016, 10:13 UTC · re: Hui Yiqun · lore

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

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(+)
Show changes to t/t0301-credential-cache.sh +29 −0
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
惠轶群· Mar 25, 2016, 07:13 UTC · re: Hui Yiqun · lore

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>:
Show 74 quoted lines
> 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
>
Jeff King· Mar 25, 2016, 09:51 UTC · re: Hui Yiqun · lore

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

On Wed, Mar 23, 2016 at 06:13:21PM +0800, Hui Yiqun wrote:
Show 5 quoted lines
> 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

← back to recent threads