{"thread":{"id":"22734","subject":"[PATCH 1/6] Add a testcase for ACL with restrictive umask.","startedAt":"2010-02-19T23:19:02Z","lastAt":"2010-02-22T19:56:53Z","messageCount":11,"participants":["Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"135123","messageId":"vpq7hq8stjt.fsf@bauges.imag.fr","threadId":"22734","inReplyTo":"1266597207-32036-5-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH 4/4] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-02-19T23:19:02Z","receivedAt":"2010-02-19T23:19:02Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> By not using mkstemp() from the platform but having our own would allow us\n> to not even worry about this issue (this can be seen by the removal of a\n> call to umask() in [PATCH 4/4]).\n\nExactly. BTW, we probably want to do the same for object files. The\nproblem is less important since object files are created 0444, and\ntherefore do not break ACLs, but that would at least increase\nconsistancy a bit.\n\nMatthieu Moy <Matthieu.Moy@imag.fr> writes:\n\n> -\n> +\t/*\n> +\t * we let the umask do it's job, don't try to be more\n> +\t * restrictive.\n> +\t */\n> +\tint mode = 0666;\n\nActually, it may be safer to put 0444 here. With my patch, git still\ncreates read-only pack files, but to be honnest, I don't understand\nwhich piece of code cuts the 'w' bit!\n\nNew patch serie comming with additional patches for the first point,\nand a corrected [PATCH 4] for the second.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"135122","messageId":"1266621718-4879-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"22734","inReplyTo":"vpq7hq8stjt.fsf@bauges.imag.fr","subject":"[PATCH 1/6] Add a testcase for ACL with restrictive umask.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2010-02-19T23:21:53Z","receivedAt":"2010-02-19T23:21:53Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Right now, Git creates unreadable pack files on non-shared\nrepositories when the user has a umask of 077, even when the default\nACLs for the directory would give read/write access to a specific\nuser.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\n t/t1304-default-acl.sh |   49 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 49 insertions(+), 0 deletions(-)\n create mode 100755 t/t1304-default-acl.sh\n\ndiff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh\nnew file mode 100755\nindex 0000000..4ee44a1\n--- /dev/null\n+++ b/t/t1304-default-acl.sh\n@@ -0,0 +1,49 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2010 Matthieu Moy\n+#\n+\n+test_description='Test repository with default ACL'\n+\n+. ./test-lib.sh\n+\n+# We need an arbitrary other user give permission to using ACLs. root\n+# is a good candidate: exists on all unices, and it has permission\n+# anyway, so we don't create a security hole running the testsuite.\n+\n+if ! setfacl -Rm u:root:rwx .; then\n+    say \"Skipping ACL tests: unable to use setfacl\"\n+    test_done\n+fi\n+\n+setfacl -Rm d:u:\"$LOGNAME\":rwx .\n+setfacl -Rm d:u:root:rwx .\n+\n+touch file.txt\n+git add file.txt\n+git commit -m \"init\"\n+\n+modebits () {\n+\tls -l \"$1\" | sed -e 's|^\\(..........\\).*|\\1|'\n+}\n+\n+test_expect_failure 'git gc does not break ACLs with restrictive umask' '\n+\tumask 077 &&\n+\tgit gc &&\n+\tactual=$(modebits .git/objects/pack/*.pack) &&\n+\tcase \"$actual\" in\n+\t-r--r-----*)\n+\t\t: happy\n+\t\t;;\n+\t*)\n+\t\tfalse\n+\t\t;;\n+\tesac &&\n+\tgetfacl .git/objects/pack/*.pack > actual &&\n+\tgrep -q \"user:root:rwx\" actual &&\n+\tgrep -q \"user:${LOGNAME}:rwx\" actual &&\n+\tgrep -q \"mask::r--\" actual &&\n+\tgrep -q \"group::---\" actual\n+'\n+\n+test_done\n-- \n1.7.0.rc2.92.gb6a04\n"},{"id":"135125","messageId":"1266621718-4879-2-git-send-email-Matthieu.Moy@imag.fr","threadId":"22734","inReplyTo":"vpq7hq8stjt.fsf@bauges.imag.fr","subject":"[PATCH 2/6] Move gitmkstemps to path.c","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2010-02-19T23:21:54Z","receivedAt":"2010-02-19T23:21:54Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"This function used to be only a compatibility function, but we're\ngoing to extend it and actually use it, so make it part of Git.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\n Makefile          |    1 -\n compat/mkstemps.c |   70 -----------------------------------------------------\n path.c            |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 69 insertions(+), 71 deletions(-)\n delete mode 100644 compat/mkstemps.c\n\ndiff --git a/Makefile b/Makefile\nindex 7bf2fca..4387d42 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1200,7 +1200,6 @@ ifdef NO_MKDTEMP\n endif\n ifdef NO_MKSTEMPS\n \tCOMPAT_CFLAGS += -DNO_MKSTEMPS\n-\tCOMPAT_OBJS += compat/mkstemps.o\n endif\n ifdef NO_UNSETENV\n \tCOMPAT_CFLAGS += -DNO_UNSETENV\ndiff --git a/compat/mkstemps.c b/compat/mkstemps.c\ndeleted file mode 100644\nindex 14179c8..0000000\n--- a/compat/mkstemps.c\n+++ /dev/null\n@@ -1,70 +0,0 @@\n-#include \"../git-compat-util.h\"\n-\n-/* Adapted from libiberty's mkstemp.c. */\n-\n-#undef TMP_MAX\n-#define TMP_MAX 16384\n-\n-int gitmkstemps(char *pattern, int suffix_len)\n-{\n-\tstatic const char letters[] =\n-\t\t\"abcdefghijklmnopqrstuvwxyz\"\n-\t\t\"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n-\t\t\"0123456789\";\n-\tstatic const int num_letters = 62;\n-\tuint64_t value;\n-\tstruct timeval tv;\n-\tchar *template;\n-\tsize_t len;\n-\tint fd, count;\n-\n-\tlen = strlen(pattern);\n-\n-\tif (len < 6 + suffix_len) {\n-\t\terrno = EINVAL;\n-\t\treturn -1;\n-\t}\n-\n-\tif (strncmp(&pattern[len - 6 - suffix_len], \"XXXXXX\", 6)) {\n-\t\terrno = EINVAL;\n-\t\treturn -1;\n-\t}\n-\n-\t/*\n-\t * Replace pattern's XXXXXX characters with randomness.\n-\t * Try TMP_MAX different filenames.\n-\t */\n-\tgettimeofday(&tv, NULL);\n-\tvalue = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();\n-\ttemplate = &pattern[len - 6 - suffix_len];\n-\tfor (count = 0; count < TMP_MAX; ++count) {\n-\t\tuint64_t v = value;\n-\t\t/* Fill in the random bits. */\n-\t\ttemplate[0] = letters[v % num_letters]; v /= num_letters;\n-\t\ttemplate[1] = letters[v % num_letters]; v /= num_letters;\n-\t\ttemplate[2] = letters[v % num_letters]; v /= num_letters;\n-\t\ttemplate[3] = letters[v % num_letters]; v /= num_letters;\n-\t\ttemplate[4] = letters[v % num_letters]; v /= num_letters;\n-\t\ttemplate[5] = letters[v % num_letters]; v /= num_letters;\n-\n-\t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);\n-\t\tif (fd > 0)\n-\t\t\treturn fd;\n-\t\t/*\n-\t\t * Fatal error (EPERM, ENOSPC etc).\n-\t\t * It doesn't make sense to loop.\n-\t\t */\n-\t\tif (errno != EEXIST)\n-\t\t\tbreak;\n-\t\t/*\n-\t\t * This is a random value.  It is only necessary that\n-\t\t * the next TMP_MAX values generated by adding 7777 to\n-\t\t * VALUE are different with (module 2^32).\n-\t\t */\n-\t\tvalue += 7777;\n-\t}\n-\t/* We return the null string if we can't find a unique file name.  */\n-\tpattern[0] = '\\0';\n-\terrno = EINVAL;\n-\treturn -1;\n-}\ndiff --git a/path.c b/path.c\nindex d1fccbd..60f5b78 100644\n--- a/path.c\n+++ b/path.c\n@@ -157,6 +157,75 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)\n \treturn mkstemps(path, suffix_len);\n }\n \n+/* Adapted from libiberty's mkstemp.c. */\n+\n+#undef TMP_MAX\n+#define TMP_MAX 16384\n+\n+int gitmkstemps(char *pattern, int suffix_len)\n+{\n+\tstatic const char letters[] =\n+\t\t\"abcdefghijklmnopqrstuvwxyz\"\n+\t\t\"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n+\t\t\"0123456789\";\n+\tstatic const int num_letters = 62;\n+\tuint64_t value;\n+\tstruct timeval tv;\n+\tchar *template;\n+\tsize_t len;\n+\tint fd, count;\n+\n+\tlen = strlen(pattern);\n+\n+\tif (len < 6 + suffix_len) {\n+\t\terrno = EINVAL;\n+\t\treturn -1;\n+\t}\n+\n+\tif (strncmp(&pattern[len - 6 - suffix_len], \"XXXXXX\", 6)) {\n+\t\terrno = EINVAL;\n+\t\treturn -1;\n+\t}\n+\n+\t/*\n+\t * Replace pattern's XXXXXX characters with randomness.\n+\t * Try TMP_MAX different filenames.\n+\t */\n+\tgettimeofday(&tv, NULL);\n+\tvalue = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();\n+\ttemplate = &pattern[len - 6 - suffix_len];\n+\tfor (count = 0; count < TMP_MAX; ++count) {\n+\t\tuint64_t v = value;\n+\t\t/* Fill in the random bits. */\n+\t\ttemplate[0] = letters[v % num_letters]; v /= num_letters;\n+\t\ttemplate[1] = letters[v % num_letters]; v /= num_letters;\n+\t\ttemplate[2] = letters[v % num_letters]; v /= num_letters;\n+\t\ttemplate[3] = letters[v % num_letters]; v /= num_letters;\n+\t\ttemplate[4] = letters[v % num_letters]; v /= num_letters;\n+\t\ttemplate[5] = letters[v % num_letters]; v /= num_letters;\n+\n+\t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);\n+\t\tif (fd > 0)\n+\t\t\treturn fd;\n+\t\t/*\n+\t\t * Fatal error (EPERM, ENOSPC etc).\n+\t\t * It doesn't make sense to loop.\n+\t\t */\n+\t\tif (errno != EEXIST)\n+\t\t\tbreak;\n+\t\t/*\n+\t\t * This is a random value.  It is only necessary that\n+\t\t * the next TMP_MAX values generated by adding 7777 to\n+\t\t * VALUE are different with (module 2^32).\n+\t\t */\n+\t\tvalue += 7777;\n+\t}\n+\t/* We return the null string if we can't find a unique file name.  */\n+\tpattern[0] = '\\0';\n+\terrno = EINVAL;\n+\treturn -1;\n+}\n+\n int validate_headref(const char *path)\n {\n \tstruct stat st;\n-- \n1.7.0.rc2.92.gb6a04\n"},{"id":"135126","messageId":"1266621718-4879-3-git-send-email-Matthieu.Moy@imag.fr","threadId":"22734","inReplyTo":"vpq7hq8stjt.fsf@bauges.imag.fr","subject":"[PATCH 3/6] git_mkstemp_mode, xmkstemp_mode: variants of gitmkstemps with mode argument.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2010-02-19T23:21:55Z","receivedAt":"2010-02-19T23:21:55Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"gitmkstemps emulates the behavior of mkstemps, which is usually used\nto create files in a shared directory like /tmp/, hence, it creates\nfiles with permission 0600. But we use it also to create pack files,\nwhich do not need this protection.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\n cache.h   |    4 ++++\n path.c    |   16 ++++++++++++++--\n wrapper.c |   10 ++++++++++\n 3 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex d478eff..0319637 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -641,6 +641,10 @@ int git_mkstemp(char *path, size_t n, const char *template);\n \n int git_mkstemps(char *path, size_t n, const char *template, int suffix_len);\n \n+/* set default permissions by passing mode arguments to open(2) */\n+int git_mkstemps_mode(char *pattern, int suffix_len, int mode);\n+int git_mkstemp_mode(char *pattern, int mode);\n+\n /*\n  * NOTE NOTE NOTE!!\n  *\ndiff --git a/path.c b/path.c\nindex 60f5b78..005b836 100644\n--- a/path.c\n+++ b/path.c\n@@ -162,7 +162,7 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)\n #undef TMP_MAX\n #define TMP_MAX 16384\n \n-int gitmkstemps(char *pattern, int suffix_len)\n+int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n {\n \tstatic const char letters[] =\n \t\t\"abcdefghijklmnopqrstuvwxyz\"\n@@ -204,7 +204,7 @@ int gitmkstemps(char *pattern, int suffix_len)\n \t\ttemplate[4] = letters[v % num_letters]; v /= num_letters;\n \t\ttemplate[5] = letters[v % num_letters]; v /= num_letters;\n \n-\t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);\n+\t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n \t\tif (fd > 0)\n \t\t\treturn fd;\n \t\t/*\n@@ -226,6 +226,17 @@ int gitmkstemps(char *pattern, int suffix_len)\n \treturn -1;\n }\n \n+int git_mkstemp_mode(char *pattern, int mode)\n+{\n+\t/* mkstemp is just mkstemps with no suffix */\n+\treturn git_mkstemps_mode(pattern, 0, mode);\n+}\n+\n+int gitmkstemps(char *pattern, int suffix_len)\n+{\n+\treturn git_mkstemps_mode(pattern, suffix_len, 0600);\n+}\n+\n int validate_headref(const char *path)\n {\n \tstruct stat st;\n@@ -718,3 +729,4 @@ int daemon_avoid_alias(const char *p)\n \t\t}\n \t}\n }\n+\ndiff --git a/wrapper.c b/wrapper.c\nindex 0e3e20a..673762f 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -204,6 +204,16 @@ int xmkstemp(char *template)\n \treturn fd;\n }\n \n+int xmkstemp_mode(char *template, int mode)\n+{\n+\tint fd;\n+\n+\tfd = git_mkstemp_mode(template, mode);\n+\tif (fd < 0)\n+\t\tdie_errno(\"Unable to create temporary file\");\n+\treturn fd;\n+}\n+\n /*\n  * zlib wrappers to make sure we don't silently miss errors\n  * at init time.\n-- \n1.7.0.rc2.92.gb6a04\n"},{"id":"135124","messageId":"1266621718-4879-4-git-send-email-Matthieu.Moy@imag.fr","threadId":"22734","inReplyTo":"vpq7hq8stjt.fsf@bauges.imag.fr","subject":"[PATCH 4/6 v2] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2010-02-19T23:21:56Z","receivedAt":"2010-02-19T23:21:56Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"We used to create 0600 files, and then use chmod to set the group and\nother permission bits to the umask. This usually has the same effect\nas a normal file creation with a umask.\n\nBut in the presence of ACLs, the group permission plays the role of\nthe ACL mask: the \"g\" bits of newly created files are chosen according\nto default ACL mask of the directory, not according to the umask, and\ndoing a chmod() on these \"g\" bits affect the ACL's mask instead of\nactual group permission.\n\nIn other words, creating files with 0600 and then doing a chmod to the\numask creates files which are unreadable by users allowed in the\ndefault ACL. To create the files without breaking ACLs, we let the\numask do it's job at the file's creation time, and get rid of the\nlater chmod.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\nChange since v1:\n\n+\t/*\n+\t * we let the umask do its job, don't try to be more\n+\t * restrictive except to remove write permission.\n+\t */\n+\tint mode = 0444;\n\n=> the 0666 became a 0444 to create the file read-only (+typo in comment).\n\n builtin-pack-objects.c |   18 ++----------------\n t/t1304-default-acl.sh |    2 +-\n wrapper.c              |   10 +++++++---\n 3 files changed, 10 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex e1d3adf..539e75d 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -464,9 +464,6 @@ static int write_one(struct sha1file *f,\n \treturn 1;\n }\n \n-/* forward declaration for write_pack_file */\n-static int adjust_perm(const char *path, mode_t mode);\n-\n static void write_pack_file(void)\n {\n \tuint32_t i = 0, j;\n@@ -523,21 +520,17 @@ static void write_pack_file(void)\n \t\t}\n \n \t\tif (!pack_to_stdout) {\n-\t\t\tmode_t mode = umask(0);\n \t\t\tstruct stat st;\n \t\t\tconst char *idx_tmp_name;\n \t\t\tchar tmpname[PATH_MAX];\n \n-\t\t\tumask(mode);\n-\t\t\tmode = 0444 & ~mode;\n-\n \t\t\tidx_tmp_name = write_idx_file(NULL, written_list,\n \t\t\t\t\t\t      nr_written, sha1);\n \n \t\t\tsnprintf(tmpname, sizeof(tmpname), \"%s-%s.pack\",\n \t\t\t\t base_name, sha1_to_hex(sha1));\n \t\t\tfree_pack_by_name(tmpname);\n-\t\t\tif (adjust_perm(pack_tmp_name, mode))\n+\t\t\tif (adjust_shared_perm(pack_tmp_name))\n \t\t\t\tdie_errno(\"unable to make temporary pack file readable\");\n \t\t\tif (rename(pack_tmp_name, tmpname))\n \t\t\t\tdie_errno(\"unable to rename temporary pack file\");\n@@ -565,7 +558,7 @@ static void write_pack_file(void)\n \n \t\t\tsnprintf(tmpname, sizeof(tmpname), \"%s-%s.idx\",\n \t\t\t\t base_name, sha1_to_hex(sha1));\n-\t\t\tif (adjust_perm(idx_tmp_name, mode))\n+\t\t\tif (adjust_shared_perm(idx_tmp_name))\n \t\t\t\tdie_errno(\"unable to make temporary index file readable\");\n \t\t\tif (rename(idx_tmp_name, tmpname))\n \t\t\t\tdie_errno(\"unable to rename temporary index file\");\n@@ -2125,13 +2118,6 @@ static void get_object_list(int ac, const char **av)\n \t\tloosen_unused_packed_objects(&revs);\n }\n \n-static int adjust_perm(const char *path, mode_t mode)\n-{\n-\tif (chmod(path, mode))\n-\t\treturn -1;\n-\treturn adjust_shared_perm(path);\n-}\n-\n int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n {\n \tint use_internal_rev_list = 0;\ndiff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh\nindex 4ee44a1..b8e9c34 100755\n--- a/t/t1304-default-acl.sh\n+++ b/t/t1304-default-acl.sh\n@@ -27,7 +27,7 @@ modebits () {\n \tls -l \"$1\" | sed -e 's|^\\(..........\\).*|\\1|'\n }\n \n-test_expect_failure 'git gc does not break ACLs with restrictive umask' '\n+test_expect_success 'git gc does not break ACLs with restrictive umask' '\n \tumask 077 &&\n \tgit gc &&\n \tactual=$(modebits .git/objects/pack/*.pack) &&\ndiff --git a/wrapper.c b/wrapper.c\nindex 673762f..9c71b21 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -277,10 +277,14 @@ int git_inflate(z_streamp strm, int flush)\n int odb_mkstemp(char *template, size_t limit, const char *pattern)\n {\n \tint fd;\n-\n+\t/*\n+\t * we let the umask do its job, don't try to be more\n+\t * restrictive except to remove write permission.\n+\t */\n+\tint mode = 0444;\n \tsnprintf(template, limit, \"%s/%s\",\n \t\t get_object_directory(), pattern);\n-\tfd = mkstemp(template);\n+\tfd = git_mkstemp_mode(template, mode);\n \tif (0 <= fd)\n \t\treturn fd;\n \n@@ -289,7 +293,7 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern)\n \tsnprintf(template, limit, \"%s/%s\",\n \t\t get_object_directory(), pattern);\n \tsafe_create_leading_directories(template);\n-\treturn xmkstemp(template);\n+\treturn xmkstemp_mode(template, mode);\n }\n \n int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1)\n-- \n1.7.0.rc2.92.gb6a04\n"},{"id":"135127","messageId":"1266621718-4879-5-git-send-email-Matthieu.Moy@imag.fr","threadId":"22734","inReplyTo":"vpq7hq8stjt.fsf@bauges.imag.fr","subject":"[PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL for any error.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2010-02-19T23:21:57Z","receivedAt":"2010-02-19T23:21:57Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\n path.c |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 005b836..2886eb6 100644\n--- a/path.c\n+++ b/path.c\n@@ -222,7 +222,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \t}\n \t/* We return the null string if we can't find a unique file name.  */\n \tpattern[0] = '\\0';\n-\terrno = EINVAL;\n+\t/* Make sure errno signals an error on failure */\n+\tif (errno <= 0)\n+\t\terrno = EINVAL;\n \treturn -1;\n }\n \n-- \n1.7.0.rc2.92.gb6a04\n"},{"id":"135128","messageId":"1266621718-4879-6-git-send-email-Matthieu.Moy@imag.fr","threadId":"22734","inReplyTo":"vpq7hq8stjt.fsf@bauges.imag.fr","subject":"[PATCH 6/6] Use git_mkstemp_mode instead of plain mkstemp to create object files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2010-02-19T23:21:58Z","receivedAt":"2010-02-19T23:21:58Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"We used to unnecessarily give the read permission to group and others,\nregardless of the umask, which isn't serious because the objects are\nstill protected by their containing directory, but isn't necessari\neither.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\n sha1_file.c |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 657825e..3316f28 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2206,7 +2206,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)\n \t}\n \n out:\n-\tif (set_shared_perm(filename, (S_IFREG|0444)))\n+\tif (adjust_shared_perm(filename))\n \t\treturn error(\"unable to set permission to '%s'\", filename);\n \treturn 0;\n }\n@@ -2262,7 +2262,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)\n \t}\n \tmemcpy(buffer, filename, dirlen);\n \tstrcpy(buffer + dirlen, \"tmp_obj_XXXXXX\");\n-\tfd = mkstemp(buffer);\n+\tfd = git_mkstemp_mode(buffer, 0444);\n \tif (fd < 0 && dirlen && errno == ENOENT) {\n \t\t/* Make sure the directory exists */\n \t\tmemcpy(buffer, filename, dirlen);\n@@ -2272,7 +2272,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)\n \n \t\t/* Try again */\n \t\tstrcpy(buffer + dirlen - 1, \"/tmp_obj_XXXXXX\");\n-\t\tfd = mkstemp(buffer);\n+\t\tfd = git_mkstemp_mode(buffer, 0444);\n \t}\n \treturn fd;\n }\n-- \n1.7.0.rc2.92.gb6a04\n"},{"id":"135174","messageId":"7v4olbpyh3.fsf@alter.siamese.dyndns.org","threadId":"22734","inReplyTo":"1266621718-4879-5-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL for any error.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-20T18:13:12Z","receivedAt":"2010-02-20T18:13:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@imag.fr> writes:\n\n> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n> ---\n>  path.c |    4 +++-\n>  1 files changed, 3 insertions(+), 1 deletions(-)\n>\n> diff --git a/path.c b/path.c\n> index 005b836..2886eb6 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -222,7 +222,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n>  \t}\n>  \t/* We return the null string if we can't find a unique file name.  */\n>  \tpattern[0] = '\\0';\n> -\terrno = EINVAL;\n> +\t/* Make sure errno signals an error on failure */\n> +\tif (errno <= 0)\n> +\t\terrno = EINVAL;\n>  \treturn -1;\n>  }\n\nPlease explain this change a bit better.\n\nA successful call into system library does not have to clear errno (POSIX\neven says \"no function in ... POSIX ... shall set errno to 0\"), and you do\nnot clear errno to zero anywhere in this function either, so it looks as\nif you are reading an undefined errno and basing your action on that\nresult.\n\nBecause TMP_MAX is non-zero, you are always reading from errno left by\nopen() in the loop, so the above paragraph is a misunderstanding.  But\nthat needs to be in the log message, no?\n\nI think you are trying to avoid stomping on the errno when we broke out of\nthe loop early, due to getting an error.  But errno is always valid at\nthis point in this codepath, and errno.h macros shall expand to integer\nconstant expressions with type int, distinct positive values.  So I think\nyou can safely remove the assignment without \"if (errno <= 0)\".  Returning\nEINVAL from a variant of mkstemp when the error is anything but \"The last\nsix characters were not XXXXXX\" is wrong.\n"},{"id":"135177","messageId":"7veikfk8zl.fsf@alter.siamese.dyndns.org","threadId":"22734","inReplyTo":"1266621718-4879-3-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH 3/6] git_mkstemp_mode, xmkstemp_mode: variants of gitmkstemps with mode argument.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-20T19:22:38Z","receivedAt":"2010-02-20T19:22:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@imag.fr> writes:\n\n> gitmkstemps emulates the behavior of mkstemps, which is usually used\n> to create files in a shared directory like /tmp/, hence, it creates\n> files with permission 0600. But we use it also to create pack files,\n> which do not need this protection.\n\n... and the conclusion is???\n\n\tAdd git_mkstemps_mode() that allows us to specify the desired\n        mode, and make git_mkstemps() a wrapper that always uses 0600\n        to call it.  Later we will use git_mkstemps_mode() when creating\n        pack files.\n\nWithout it, it sounds as if \"so we change it not to do 0600 because I only\ncare about packfile creation, and do not care about future bugs when\npeople assume that git_mkstemps() behaves just like mkstemps()\", but that\nis not what you did.\n"},{"id":"135270","messageId":"vpq7hq5n2mk.fsf@bauges.imag.fr","threadId":"22734","inReplyTo":"7v4olbpyh3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL for any error.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-02-22T07:36:19Z","receivedAt":"2010-02-22T07:36:19Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Matthieu Moy <Matthieu.Moy@imag.fr> writes:\n>\n>> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n>> ---\n>>  path.c |    4 +++-\n>>  1 files changed, 3 insertions(+), 1 deletions(-)\n>>\n>> diff --git a/path.c b/path.c\n>> index 005b836..2886eb6 100644\n>> --- a/path.c\n>> +++ b/path.c\n>> @@ -222,7 +222,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n>>  \t}\n>>  \t/* We return the null string if we can't find a unique file name.  */\n>>  \tpattern[0] = '\\0';\n>> -\terrno = EINVAL;\n>> +\t/* Make sure errno signals an error on failure */\n>> +\tif (errno <= 0)\n>> +\t\terrno = EINVAL;\n>>  \treturn -1;\n>>  }\n>\n> Please explain this change a bit better.\n\nI think we agree about the\n\n-\terrno = EINVAL;\n\npart. Setting errno to EINVAL unconditionally means discarding the\nerrno left from open(), so we can't know the reason for failure\nanymore with this line.\n\n> Because TMP_MAX is non-zero, you are always reading from errno left by\n> open() in the loop, so the above paragraph is a misunderstanding.  But\n> that needs to be in the log message, no?\n\nWill add a sentence, yes.\n\n> I think you are trying to avoid stomping on the errno when we broke out of\n> the loop early, due to getting an error.  But errno is always valid at\n> this point in this codepath, and errno.h macros shall expand to integer\n> constant expressions with type int, distinct positive values.  So I think\n> you can safely remove the assignment without \"if (errno <= 0)\".  Returning\n> EINVAL from a variant of mkstemp when the error is anything but \"The last\n> six characters were not XXXXXX\" is wrong.\n\nJust removing the line would work with the current code, but this \"if\n(errno <= 0) errno = EINVAL;\" allows enforcing the invariant that\nerrno > 0 when reaching \"return -1\" in a simple and reliable way (i.e.\nchanging the for loop later cannot break this invariant by mistake).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"299164","messageId":"7vzl31cad6.fsf@alter.siamese.dyndns.org","threadId":"22734","inReplyTo":"vpq7hq5n2mk.fsf@bauges.imag.fr","subject":"Re: [PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL for any error.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-22T19:56:53Z","receivedAt":"2010-02-22T19:56:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> Just removing the line would work with the current code, but this \"if\n> (errno <= 0) errno = EINVAL;\" allows enforcing the invariant that\n> errno > 0 when reaching \"return -1\" in a simple and reliable way (i.e.\n> changing the for loop later cannot break this invariant by mistake).\n\nI do not think \"errno > 0 when -1 is returned\" is a meaningful invariant.\nYou may return EINVAL which is positive and that may keep that invariant,\nbut you should not be returning that errno unless the error is \"template\nnot ending with 6-X\" to begin with, so there isn't a real gain, no?\n\n\n\n"}]}