From: 惠轶群 <huiyiqun@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Git List <git@vger.kernel.org>, Your friend <pickfire@riseup.net>
Subject: Re: [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir()
Date: Fri, 25 Mar 2016 22:21:48 +0800 [thread overview]
Message-ID: <CAKqreux8FHdJoKDishjQkbi9g1oUc265EUK4nOJ_sgeFivGSNA@mail.gmail.com> (raw)
In-Reply-To: <20160325095923.GB8880@sigill.intra.peff.net>
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
next prev parent reply other threads:[~2016-03-25 14:21 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-23 10:13 [PATCH v3/GSoC 1/5] path.c: implement strbuf_mkpath() Hui Yiqun
2016-03-23 10:13 ` [PATCH v3/GSoC 2/5] path.c: implement xdg_runtime_dir() Hui Yiqun
2016-03-25 9:59 ` Jeff King
2016-03-25 14:21 ` 惠轶群 [this message]
2016-03-25 14:23 ` 惠轶群
2016-03-25 16:55 ` Junio C Hamano
2016-03-25 17:55 ` Jeff King
2016-03-25 18:00 ` Junio C Hamano
2016-03-28 13:37 ` 惠轶群
2016-03-28 14:35 ` Junio C Hamano
2016-03-25 17:59 ` Jeff King
2016-03-28 14:12 ` 惠轶群
2016-03-28 14:50 ` Junio C Hamano
2016-03-28 15:00 ` 惠轶群
2016-03-28 17:03 ` Junio C Hamano
2016-03-28 15:51 ` [PATCH] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage Hui Yiqun
2016-03-28 15:56 ` [PATCH v2] " Hui Yiqun
2016-03-28 17:55 ` Jeff King
2016-03-29 2:40 ` 惠轶群
2016-03-28 15:57 ` [PATCH v3] " Hui Yiqun
2016-03-28 15:59 ` 惠轶群
2016-03-28 17:58 ` Junio C Hamano
2016-03-29 2:38 ` 惠轶群
2016-03-23 10:13 ` [PATCH v3/GSoC 3/5] git-credential-cache: put socket to xdg-compatible path Hui Yiqun
2016-03-25 10:00 ` Jeff King
2016-03-25 14:28 ` 惠轶群
2016-03-25 17:56 ` Jeff King
2016-03-25 18:00 ` 惠轶群
2016-03-23 10:13 ` [PATCH v3/GSoC 4/5] test-lib.sh: unset all environment variables defined in xdg base dir spec[1] Hui Yiqun
2016-03-25 10:05 ` Jeff King
2016-03-23 10:13 ` [PATCH v3/GSoC 5/5] t0301: test credential-cache support of XDG_RUNTIME_DIR Hui Yiqun
2016-03-25 7:13 ` [PATCH v3/GSoC 1/5] path.c: implement strbuf_mkpath() 惠轶群
2016-03-25 9:51 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKqreux8FHdJoKDishjQkbi9g1oUc265EUK4nOJ_sgeFivGSNA@mail.gmail.com \
--to=huiyiqun@gmail.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
--cc=pickfire@riseup.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).