# [PATCH 1/6] Add a testcase for ACL with restrictive umask. 11 messages from 2010-02-19 to 2010-02-22. Participants: Matthieu Moy, Junio C Hamano. Thread: https://gitlist.dev/t/22734 ## Matthieu Moy, 2010-02-19 23:19 Subject: Re: [PATCH 4/4] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later. Message-ID: URL: https://gitlist.dev/e/vpq7hq8stjt.fsf%40bauges.imag.fr In-Reply-To: <1266597207-32036-5-git-send-email-Matthieu.Moy@imag.fr> ``` Junio C Hamano writes: > By not using mkstemp() from the platform but having our own would allow us > to not even worry about this issue (this can be seen by the removal of a > call to umask() in [PATCH 4/4]). Exactly. BTW, we probably want to do the same for object files. The problem is less important since object files are created 0444, and therefore do not break ACLs, but that would at least increase consistancy a bit. Matthieu Moy writes: > - > + /* > + * we let the umask do it's job, don't try to be more > + * restrictive. > + */ > + int mode = 0666; Actually, it may be safer to put 0444 here. With my patch, git still creates read-only pack files, but to be honnest, I don't understand which piece of code cuts the 'w' bit! New patch serie comming with additional patches for the first point, and a corrected [PATCH 4] for the second. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Matthieu Moy, 2010-02-19 23:21 Subject: [PATCH 1/6] Add a testcase for ACL with restrictive umask. Message-ID: <1266621718-4879-1-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266621718-4879-1-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: ``` Right now, Git creates unreadable pack files on non-shared repositories when the user has a umask of 077, even when the default ACLs for the directory would give read/write access to a specific user. Signed-off-by: Matthieu Moy --- t/t1304-default-acl.sh | 49 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 49 insertions(+), 0 deletions(-) create mode 100755 t/t1304-default-acl.sh diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh new file mode 100755 index 0000000..4ee44a1 --- /dev/null +++ b/t/t1304-default-acl.sh @@ -0,0 +1,49 @@ +#!/bin/sh +# +# Copyright (c) 2010 Matthieu Moy +# + +test_description='Test repository with default ACL' + +. ./test-lib.sh + +# We need an arbitrary other user give permission to using ACLs. root +# is a good candidate: exists on all unices, and it has permission +# anyway, so we don't create a security hole running the testsuite. + +if ! setfacl -Rm u:root:rwx .; then + say "Skipping ACL tests: unable to use setfacl" + test_done +fi + +setfacl -Rm d:u:"$LOGNAME":rwx . +setfacl -Rm d:u:root:rwx . + +touch file.txt +git add file.txt +git commit -m "init" + +modebits () { + ls -l "$1" | sed -e 's|^\(..........\).*|\1|' +} + +test_expect_failure 'git gc does not break ACLs with restrictive umask' ' + umask 077 && + git gc && + actual=$(modebits .git/objects/pack/*.pack) && + case "$actual" in + -r--r-----*) + : happy + ;; + *) + false + ;; + esac && + getfacl .git/objects/pack/*.pack > actual && + grep -q "user:root:rwx" actual && + grep -q "user:${LOGNAME}:rwx" actual && + grep -q "mask::r--" actual && + grep -q "group::---" actual +' + +test_done -- 1.7.0.rc2.92.gb6a04 ``` ## Matthieu Moy, 2010-02-19 23:21 Subject: [PATCH 2/6] Move gitmkstemps to path.c Message-ID: <1266621718-4879-2-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266621718-4879-2-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: ``` This function used to be only a compatibility function, but we're going to extend it and actually use it, so make it part of Git. Signed-off-by: Matthieu Moy --- Makefile | 1 - compat/mkstemps.c | 70 ----------------------------------------------------- path.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 71 deletions(-) delete mode 100644 compat/mkstemps.c diff --git a/Makefile b/Makefile index 7bf2fca..4387d42 100644 --- a/Makefile +++ b/Makefile @@ -1200,7 +1200,6 @@ ifdef NO_MKDTEMP endif ifdef NO_MKSTEMPS COMPAT_CFLAGS += -DNO_MKSTEMPS - COMPAT_OBJS += compat/mkstemps.o endif ifdef NO_UNSETENV COMPAT_CFLAGS += -DNO_UNSETENV diff --git a/compat/mkstemps.c b/compat/mkstemps.c deleted file mode 100644 index 14179c8..0000000 --- a/compat/mkstemps.c +++ /dev/null @@ -1,70 +0,0 @@ -#include "../git-compat-util.h" - -/* Adapted from libiberty's mkstemp.c. */ - -#undef TMP_MAX -#define TMP_MAX 16384 - -int gitmkstemps(char *pattern, int suffix_len) -{ - static const char letters[] = - "abcdefghijklmnopqrstuvwxyz" - "ABCDEFGHIJKLMNOPQRSTUVWXYZ" - "0123456789"; - static const int num_letters = 62; - uint64_t value; - struct timeval tv; - char *template; - size_t len; - int fd, count; - - len = strlen(pattern); - - if (len < 6 + suffix_len) { - errno = EINVAL; - return -1; - } - - if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) { - errno = EINVAL; - return -1; - } - - /* - * Replace pattern's XXXXXX characters with randomness. - * Try TMP_MAX different filenames. - */ - gettimeofday(&tv, NULL); - value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid(); - template = &pattern[len - 6 - suffix_len]; - for (count = 0; count < TMP_MAX; ++count) { - uint64_t v = value; - /* Fill in the random bits. */ - template[0] = letters[v % num_letters]; v /= num_letters; - template[1] = letters[v % num_letters]; v /= num_letters; - template[2] = letters[v % num_letters]; v /= num_letters; - template[3] = letters[v % num_letters]; v /= num_letters; - template[4] = letters[v % num_letters]; v /= num_letters; - template[5] = letters[v % num_letters]; v /= num_letters; - - fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600); - if (fd > 0) - return fd; - /* - * Fatal error (EPERM, ENOSPC etc). - * It doesn't make sense to loop. - */ - if (errno != EEXIST) - break; - /* - * This is a random value. It is only necessary that - * the next TMP_MAX values generated by adding 7777 to - * VALUE are different with (module 2^32). - */ - value += 7777; - } - /* We return the null string if we can't find a unique file name. */ - pattern[0] = '\0'; - errno = EINVAL; - return -1; -} diff --git a/path.c b/path.c index d1fccbd..60f5b78 100644 --- a/path.c +++ b/path.c @@ -157,6 +157,75 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len) return mkstemps(path, suffix_len); } +/* Adapted from libiberty's mkstemp.c. */ + +#undef TMP_MAX +#define TMP_MAX 16384 + +int gitmkstemps(char *pattern, int suffix_len) +{ + static const char letters[] = + "abcdefghijklmnopqrstuvwxyz" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ" + "0123456789"; + static const int num_letters = 62; + uint64_t value; + struct timeval tv; + char *template; + size_t len; + int fd, count; + + len = strlen(pattern); + + if (len < 6 + suffix_len) { + errno = EINVAL; + return -1; + } + + if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) { + errno = EINVAL; + return -1; + } + + /* + * Replace pattern's XXXXXX characters with randomness. + * Try TMP_MAX different filenames. + */ + gettimeofday(&tv, NULL); + value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid(); + template = &pattern[len - 6 - suffix_len]; + for (count = 0; count < TMP_MAX; ++count) { + uint64_t v = value; + /* Fill in the random bits. */ + template[0] = letters[v % num_letters]; v /= num_letters; + template[1] = letters[v % num_letters]; v /= num_letters; + template[2] = letters[v % num_letters]; v /= num_letters; + template[3] = letters[v % num_letters]; v /= num_letters; + template[4] = letters[v % num_letters]; v /= num_letters; + template[5] = letters[v % num_letters]; v /= num_letters; + + fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600); + if (fd > 0) + return fd; + /* + * Fatal error (EPERM, ENOSPC etc). + * It doesn't make sense to loop. + */ + if (errno != EEXIST) + break; + /* + * This is a random value. It is only necessary that + * the next TMP_MAX values generated by adding 7777 to + * VALUE are different with (module 2^32). + */ + value += 7777; + } + /* We return the null string if we can't find a unique file name. */ + pattern[0] = '\0'; + errno = EINVAL; + return -1; +} + int validate_headref(const char *path) { struct stat st; -- 1.7.0.rc2.92.gb6a04 ``` ## Matthieu Moy, 2010-02-19 23:21 Subject: [PATCH 3/6] git_mkstemp_mode, xmkstemp_mode: variants of gitmkstemps with mode argument. Message-ID: <1266621718-4879-3-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266621718-4879-3-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: ``` gitmkstemps emulates the behavior of mkstemps, which is usually used to create files in a shared directory like /tmp/, hence, it creates files with permission 0600. But we use it also to create pack files, which do not need this protection. Signed-off-by: Matthieu Moy --- cache.h | 4 ++++ path.c | 16 ++++++++++++++-- wrapper.c | 10 ++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/cache.h b/cache.h index d478eff..0319637 100644 --- a/cache.h +++ b/cache.h @@ -641,6 +641,10 @@ int git_mkstemp(char *path, size_t n, const char *template); int git_mkstemps(char *path, size_t n, const char *template, int suffix_len); +/* set default permissions by passing mode arguments to open(2) */ +int git_mkstemps_mode(char *pattern, int suffix_len, int mode); +int git_mkstemp_mode(char *pattern, int mode); + /* * NOTE NOTE NOTE!! * diff --git a/path.c b/path.c index 60f5b78..005b836 100644 --- a/path.c +++ b/path.c @@ -162,7 +162,7 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len) #undef TMP_MAX #define TMP_MAX 16384 -int gitmkstemps(char *pattern, int suffix_len) +int git_mkstemps_mode(char *pattern, int suffix_len, int mode) { static const char letters[] = "abcdefghijklmnopqrstuvwxyz" @@ -204,7 +204,7 @@ int gitmkstemps(char *pattern, int suffix_len) template[4] = letters[v % num_letters]; v /= num_letters; template[5] = letters[v % num_letters]; v /= num_letters; - fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600); + fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode); if (fd > 0) return fd; /* @@ -226,6 +226,17 @@ int gitmkstemps(char *pattern, int suffix_len) return -1; } +int git_mkstemp_mode(char *pattern, int mode) +{ + /* mkstemp is just mkstemps with no suffix */ + return git_mkstemps_mode(pattern, 0, mode); +} + +int gitmkstemps(char *pattern, int suffix_len) +{ + return git_mkstemps_mode(pattern, suffix_len, 0600); +} + int validate_headref(const char *path) { struct stat st; @@ -718,3 +729,4 @@ int daemon_avoid_alias(const char *p) } } } + diff --git a/wrapper.c b/wrapper.c index 0e3e20a..673762f 100644 --- a/wrapper.c +++ b/wrapper.c @@ -204,6 +204,16 @@ int xmkstemp(char *template) return fd; } +int xmkstemp_mode(char *template, int mode) +{ + int fd; + + fd = git_mkstemp_mode(template, mode); + if (fd < 0) + die_errno("Unable to create temporary file"); + return fd; +} + /* * zlib wrappers to make sure we don't silently miss errors * at init time. -- 1.7.0.rc2.92.gb6a04 ``` ## Matthieu Moy, 2010-02-19 23:21 Subject: [PATCH 4/6 v2] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later. Message-ID: <1266621718-4879-4-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266621718-4879-4-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: ``` We used to create 0600 files, and then use chmod to set the group and other permission bits to the umask. This usually has the same effect as a normal file creation with a umask. But in the presence of ACLs, the group permission plays the role of the ACL mask: the "g" bits of newly created files are chosen according to default ACL mask of the directory, not according to the umask, and doing a chmod() on these "g" bits affect the ACL's mask instead of actual group permission. In other words, creating files with 0600 and then doing a chmod to the umask creates files which are unreadable by users allowed in the default ACL. To create the files without breaking ACLs, we let the umask do it's job at the file's creation time, and get rid of the later chmod. Signed-off-by: Matthieu Moy --- Change since v1: + /* + * we let the umask do its job, don't try to be more + * restrictive except to remove write permission. + */ + int mode = 0444; => the 0666 became a 0444 to create the file read-only (+typo in comment). builtin-pack-objects.c | 18 ++---------------- t/t1304-default-acl.sh | 2 +- wrapper.c | 10 +++++++--- 3 files changed, 10 insertions(+), 20 deletions(-) diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index e1d3adf..539e75d 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -464,9 +464,6 @@ static int write_one(struct sha1file *f, return 1; } -/* forward declaration for write_pack_file */ -static int adjust_perm(const char *path, mode_t mode); - static void write_pack_file(void) { uint32_t i = 0, j; @@ -523,21 +520,17 @@ static void write_pack_file(void) } if (!pack_to_stdout) { - mode_t mode = umask(0); struct stat st; const char *idx_tmp_name; char tmpname[PATH_MAX]; - umask(mode); - mode = 0444 & ~mode; - idx_tmp_name = write_idx_file(NULL, written_list, nr_written, sha1); snprintf(tmpname, sizeof(tmpname), "%s-%s.pack", base_name, sha1_to_hex(sha1)); free_pack_by_name(tmpname); - if (adjust_perm(pack_tmp_name, mode)) + if (adjust_shared_perm(pack_tmp_name)) die_errno("unable to make temporary pack file readable"); if (rename(pack_tmp_name, tmpname)) die_errno("unable to rename temporary pack file"); @@ -565,7 +558,7 @@ static void write_pack_file(void) snprintf(tmpname, sizeof(tmpname), "%s-%s.idx", base_name, sha1_to_hex(sha1)); - if (adjust_perm(idx_tmp_name, mode)) + if (adjust_shared_perm(idx_tmp_name)) die_errno("unable to make temporary index file readable"); if (rename(idx_tmp_name, tmpname)) die_errno("unable to rename temporary index file"); @@ -2125,13 +2118,6 @@ static void get_object_list(int ac, const char **av) loosen_unused_packed_objects(&revs); } -static int adjust_perm(const char *path, mode_t mode) -{ - if (chmod(path, mode)) - return -1; - return adjust_shared_perm(path); -} - int cmd_pack_objects(int argc, const char **argv, const char *prefix) { int use_internal_rev_list = 0; diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh index 4ee44a1..b8e9c34 100755 --- a/t/t1304-default-acl.sh +++ b/t/t1304-default-acl.sh @@ -27,7 +27,7 @@ modebits () { ls -l "$1" | sed -e 's|^\(..........\).*|\1|' } -test_expect_failure 'git gc does not break ACLs with restrictive umask' ' +test_expect_success 'git gc does not break ACLs with restrictive umask' ' umask 077 && git gc && actual=$(modebits .git/objects/pack/*.pack) && diff --git a/wrapper.c b/wrapper.c index 673762f..9c71b21 100644 --- a/wrapper.c +++ b/wrapper.c @@ -277,10 +277,14 @@ int git_inflate(z_streamp strm, int flush) int odb_mkstemp(char *template, size_t limit, const char *pattern) { int fd; - + /* + * we let the umask do its job, don't try to be more + * restrictive except to remove write permission. + */ + int mode = 0444; snprintf(template, limit, "%s/%s", get_object_directory(), pattern); - fd = mkstemp(template); + fd = git_mkstemp_mode(template, mode); if (0 <= fd) return fd; @@ -289,7 +293,7 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern) snprintf(template, limit, "%s/%s", get_object_directory(), pattern); safe_create_leading_directories(template); - return xmkstemp(template); + return xmkstemp_mode(template, mode); } int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1) -- 1.7.0.rc2.92.gb6a04 ``` ## Matthieu Moy, 2010-02-19 23:21 Subject: [PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL for any error. Message-ID: <1266621718-4879-5-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266621718-4879-5-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: ``` Signed-off-by: Matthieu Moy --- path.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/path.c b/path.c index 005b836..2886eb6 100644 --- a/path.c +++ b/path.c @@ -222,7 +222,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) } /* We return the null string if we can't find a unique file name. */ pattern[0] = '\0'; - errno = EINVAL; + /* Make sure errno signals an error on failure */ + if (errno <= 0) + errno = EINVAL; return -1; } -- 1.7.0.rc2.92.gb6a04 ``` ## Matthieu Moy, 2010-02-19 23:21 Subject: [PATCH 6/6] Use git_mkstemp_mode instead of plain mkstemp to create object files Message-ID: <1266621718-4879-6-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266621718-4879-6-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: ``` We used to unnecessarily give the read permission to group and others, regardless of the umask, which isn't serious because the objects are still protected by their containing directory, but isn't necessari either. Signed-off-by: Matthieu Moy --- sha1_file.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 657825e..3316f28 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -2206,7 +2206,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename) } out: - if (set_shared_perm(filename, (S_IFREG|0444))) + if (adjust_shared_perm(filename)) return error("unable to set permission to '%s'", filename); return 0; } @@ -2262,7 +2262,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename) } memcpy(buffer, filename, dirlen); strcpy(buffer + dirlen, "tmp_obj_XXXXXX"); - fd = mkstemp(buffer); + fd = git_mkstemp_mode(buffer, 0444); if (fd < 0 && dirlen && errno == ENOENT) { /* Make sure the directory exists */ memcpy(buffer, filename, dirlen); @@ -2272,7 +2272,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename) /* Try again */ strcpy(buffer + dirlen - 1, "/tmp_obj_XXXXXX"); - fd = mkstemp(buffer); + fd = git_mkstemp_mode(buffer, 0444); } return fd; } -- 1.7.0.rc2.92.gb6a04 ``` ## Junio C Hamano, 2010-02-20 18:13 Subject: Re: [PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL for any error. Message-ID: <7v4olbpyh3.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4olbpyh3.fsf%40alter.siamese.dyndns.org In-Reply-To: <1266621718-4879-5-git-send-email-Matthieu.Moy@imag.fr> ``` Matthieu Moy writes: > Signed-off-by: Matthieu Moy > --- > path.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/path.c b/path.c > index 005b836..2886eb6 100644 > --- a/path.c > +++ b/path.c > @@ -222,7 +222,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) > } > /* We return the null string if we can't find a unique file name. */ > pattern[0] = '\0'; > - errno = EINVAL; > + /* Make sure errno signals an error on failure */ > + if (errno <= 0) > + errno = EINVAL; > return -1; > } Please explain this change a bit better. A successful call into system library does not have to clear errno (POSIX even says "no function in ... POSIX ... shall set errno to 0"), and you do not clear errno to zero anywhere in this function either, so it looks as if you are reading an undefined errno and basing your action on that result. Because TMP_MAX is non-zero, you are always reading from errno left by open() in the loop, so the above paragraph is a misunderstanding. But that needs to be in the log message, no? I think you are trying to avoid stomping on the errno when we broke out of the loop early, due to getting an error. But errno is always valid at this point in this codepath, and errno.h macros shall expand to integer constant expressions with type int, distinct positive values. So I think you can safely remove the assignment without "if (errno <= 0)". Returning EINVAL from a variant of mkstemp when the error is anything but "The last six characters were not XXXXXX" is wrong. ``` ## Junio C Hamano, 2010-02-20 19:22 Subject: Re: [PATCH 3/6] git_mkstemp_mode, xmkstemp_mode: variants of gitmkstemps with mode argument. Message-ID: <7veikfk8zl.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7veikfk8zl.fsf%40alter.siamese.dyndns.org In-Reply-To: <1266621718-4879-3-git-send-email-Matthieu.Moy@imag.fr> ``` Matthieu Moy writes: > gitmkstemps emulates the behavior of mkstemps, which is usually used > to create files in a shared directory like /tmp/, hence, it creates > files with permission 0600. But we use it also to create pack files, > which do not need this protection. ... and the conclusion is??? Add git_mkstemps_mode() that allows us to specify the desired mode, and make git_mkstemps() a wrapper that always uses 0600 to call it. Later we will use git_mkstemps_mode() when creating pack files. Without it, it sounds as if "so we change it not to do 0600 because I only care about packfile creation, and do not care about future bugs when people assume that git_mkstemps() behaves just like mkstemps()", but that is not what you did. ``` ## Matthieu Moy, 2010-02-22 07:36 Subject: Re: [PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL for any error. Message-ID: URL: https://gitlist.dev/e/vpq7hq5n2mk.fsf%40bauges.imag.fr In-Reply-To: <7v4olbpyh3.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > Matthieu Moy writes: > >> Signed-off-by: Matthieu Moy >> --- >> path.c | 4 +++- >> 1 files changed, 3 insertions(+), 1 deletions(-) >> >> diff --git a/path.c b/path.c >> index 005b836..2886eb6 100644 >> --- a/path.c >> +++ b/path.c >> @@ -222,7 +222,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) >> } >> /* We return the null string if we can't find a unique file name. */ >> pattern[0] = '\0'; >> - errno = EINVAL; >> + /* Make sure errno signals an error on failure */ >> + if (errno <= 0) >> + errno = EINVAL; >> return -1; >> } > > Please explain this change a bit better. I think we agree about the - errno = EINVAL; part. Setting errno to EINVAL unconditionally means discarding the errno left from open(), so we can't know the reason for failure anymore with this line. > Because TMP_MAX is non-zero, you are always reading from errno left by > open() in the loop, so the above paragraph is a misunderstanding. But > that needs to be in the log message, no? Will add a sentence, yes. > I think you are trying to avoid stomping on the errno when we broke out of > the loop early, due to getting an error. But errno is always valid at > this point in this codepath, and errno.h macros shall expand to integer > constant expressions with type int, distinct positive values. So I think > you can safely remove the assignment without "if (errno <= 0)". Returning > EINVAL from a variant of mkstemp when the error is anything but "The last > six characters were not XXXXXX" is wrong. Just removing the line would work with the current code, but this "if (errno <= 0) errno = EINVAL;" allows enforcing the invariant that errno > 0 when reaching "return -1" in a simple and reliable way (i.e. changing the for loop later cannot break this invariant by mistake). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Junio C Hamano, 2010-02-22 19:56 Subject: Re: [PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL for any error. Message-ID: <7vzl31cad6.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzl31cad6.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Matthieu Moy writes: > Just removing the line would work with the current code, but this "if > (errno <= 0) errno = EINVAL;" allows enforcing the invariant that > errno > 0 when reaching "return -1" in a simple and reliable way (i.e. > changing the for loop later cannot break this invariant by mistake). I do not think "errno > 0 when -1 is returned" is a meaningful invariant. You may return EINVAL which is positive and that may keep that invariant, but you should not be returning that errno unless the error is "template not ending with 6-X" to begin with, so there isn't a real gain, no? ```