Re: [PATCH v8 22/44] fetch.c: clear errno before calling functions that might set it
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- May 16, 2014, 18:33 UTC
- Message-ID
- <20140516183348.GC12314@google.com>
- In-Reply-To
- <1400174999-26786-23-git-send-email-sahlberg@google.com>
(cc-ing peff, who may remember this STORE_REF_ERROR_DF_CONFLICT case from long ago) Ronnie Sahlberg wrote:
> In s_update_ref there are two calls that when they fail we return an error > based on the errno value. In particular we want to return a specific error > if ENOTDIR happened. Both these functions do have failure modes where they > may return an error without updating errno
That's a bug. Any function for which errno is supposed to be meaningful when it returns an error should always set errno. Otherwise errno may be set to ENOTDIR within the function by an unrelated system call.
Functions that fail and lead callers to check errno, based on a quick search with 'git grep -e 'errno *[!=]=':
fopen lstat builtin/apply.c::try_create_file (= mkdir, symlink, or open) rmdir open mkdir unlink lock_any_ref_for_update write_ref_sha1 strtol kill odb_pack_keep opendir fgets read poll pread strtoimax strtoumax git_parse_int git_parse_int64 git_parse_ulong write_in_full credential-cache.c::send_request fstat run_command_v_opt git.c::run_argv readlink resolve_ref_unsafe hold_lock_file_for_update unlink_or_warn rename execvp waitpid execv_git_cmd execv_shell_cmd sane_execvp stat read_object git_mkstemp_mode create_tmpfile start_command xwrite iconv fast_export_ls fast_export_ls_rev delete_ref
lock_any_ref_for_update has failure paths * check_ref_format [!] * lock_ref_sha1_basic
lock_ref_sha1_basic has failure paths * remove_empty_directories * resolve_ref_unsafe * safe_create_leading_directories * verify_lock
remove_empty_directories has one failure path * remove_dir_recursively but could be more explicit about the need to preserve errno.
errno from remove_dir_recursively is meaningful.
resolve_ref_unsafe has failure paths * check_refname_format [!] * too much recursion [!] * lstat, readlink, open * handle_missing_loose_ref * read_in_full, but errno gets clobbered * invalid ref [!] * invalid symref [!]
verify_lock has failure paths * read_ref_full, but errno gets clobbered * old_sha1 didn't match [!] write_ref_sha1 has failure paths * !lock [!] * missing object [!] * non-commit object [!] * write_in_full, close_ref, but errno gets clobbered * log_ref_write * commit_ref
log_ref_write has failure paths * log_ref_setup * write_in_full, close. errno gets clobbered
commit_ref just calls commit_lock_file.
log_ref_setup has failure paths * safe_create_leading_directories, but errno gets clobbered * remove_empty_directories, but errno gets clobbered * open, but errno gets clobbered
safe_create_leading_directories doesn't set errno for SCLD_EXISTS but otherwise its errno is useful.
That should cover the cases affecting the code path in fetch.c you mentioned (I haven't checked the others).
How about something like this?
It's also tempting to teach vreportf and vwritef to save errno, which would handle some of these cases automatically.
diff --git i/refs.c w/refs.c index 82a8d4e..f98acf0 100644 --- i/refs.c +++ w/refs.c @@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea if (flag) *flag = 0; - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) + if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { + errno = EINVAL; return NULL; + } for (;;) { char path[PATH_MAX]; @@ -1342,8 +1344,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea char *buf; int fd; - if (--depth < 0) + if (--depth < 0) { + errno = ELOOP; return NULL; + } git_snpath(path, sizeof(path), "%s", refname); @@ -1405,9 +1409,13 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea return NULL; } len = read_in_full(fd, buffer, sizeof(buffer)-1); - close(fd); - if (len < 0) + if (len < 0) { + int save_errno = errno; + close(fd); + errno = save_errno; return NULL; + } + close(fd); while (len && isspace(buffer[len-1])) len--; buffer[len] = '\0'; @@ -1424,6 +1432,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea (buffer[40] != '\0' && !isspace(buffer[40]))) { if (flag) *flag |= REF_ISBROKEN; + errno = EINVAL; return NULL; } return refname; @@ -1436,6 +1445,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) { if (flag) *flag |= REF_ISBROKEN; + errno = EINVAL; return NULL; } refname = strcpy(refname_buffer, buf); @@ -1908,14 +1918,17 @@ static struct ref_lock *verify_lock(struct ref_lock *lock, const unsigned char *old_sha1, int mustexist) { if (read_ref_full(lock->ref_name, lock->old_sha1, mustexist, NULL)) { + int save_errno = errno; error("Can't verify ref %s", lock->ref_name); unlock_ref(lock); + errno = save_errno; return NULL; } if (hashcmp(lock->old_sha1, old_sha1)) { error("Ref %s is at %s but expected %s", lock->ref_name, sha1_to_hex(lock->old_sha1), sha1_to_hex(old_sha1)); unlock_ref(lock); + errno = EBUSY; return NULL; } return lock; @@ -1928,15 +1941,17 @@ static int remove_empty_directories(const char *file) * only empty directories), remove them. */ struct strbuf path; - int result; + int result, save_errno; strbuf_init(&path, 20); strbuf_addstr(&path, file); result = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY); + save_errno = errno; strbuf_release(&path); + errno = save_errno; return result; } @@ -2137,8 +2152,10 @@ struct ref_lock *lock_any_ref_for_update(const char *refname, const unsigned char *old_sha1, int flags, int *type_p) { - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) + if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { + errno = EINVAL; return NULL; + } return lock_ref_sha1_basic(refname, old_sha1, flags, type_p); } @@ -2734,9 +2751,12 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize) starts_with(refname, "refs/remotes/") || starts_with(refname, "refs/notes/") || !strcmp(refname, "HEAD"))) { - if (safe_create_leading_directories(logfile) < 0) - return error("unable to create directory for %s", - logfile); + if (safe_create_leading_directories(logfile) < 0) { + int save_errno = errno; + error("unable to create directory for %s", logfile); + errno = save_errno; + return -1; + } oflags |= O_CREAT; } @@ -2747,15 +2767,21 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize) if ((oflags & O_CREAT) && errno == EISDIR) { if (remove_empty_directories(logfile)) { - return error("There are still logs under '%s'", - logfile); + int save_errno = errno; + error("There are still logs under '%s'", logfile); + errno = save_errno; + return -1; } logfd = open(logfile, oflags, 0666); } - if (logfd < 0) - return error("Unable to append to %s: %s", + if (logfd < 0) { + int save_errno = errno; + error("Unable to append to %s: %s", logfile, strerror(errno)); + errno = save_errno; + return -1; + } } adjust_shared_perm(logfile); @@ -2795,8 +2821,19 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1, len += copy_msg(logrec + len - 1, msg) - 1; written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1; free(logrec); - if (close(logfd) != 0 || written != len) - return error("Unable to append to %s", log_file); + if (written != len) { + int save_errno = errno; + close(logfd); + error("Unable to append to %s", log_file); + errno = save_errno; + return -1; + } + if (close(logfd)) { + int save_errno = errno; + error("Unable to append to %s", log_file); + errno = save_errno; + return -1; + } return 0; } @@ -2811,8 +2848,10 @@ int write_ref_sha1(struct ref_lock *lock, static char term = '\n'; struct object *o; - if (!lock) + if (!lock) { + errno = EINVAL; return -1; + } if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) { unlock_ref(lock); return 0; @@ -2822,26 +2861,32 @@ int write_ref_sha1(struct ref_lock *lock, error("Trying to write ref %s with nonexistent object %s", lock->ref_name, sha1_to_hex(sha1)); unlock_ref(lock); + errno = EINVAL; return -1; } if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) { error("Trying to write non-commit object %s to branch %s", sha1_to_hex(sha1), lock->ref_name); unlock_ref(lock); + errno = EINVAL; return -1; } if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 || - write_in_full(lock->lock_fd, &term, 1) != 1 - || close_ref(lock) < 0) { + write_in_full(lock->lock_fd, &term, 1) != 1 || + close_ref(lock) < 0) { + int save_errno = errno; error("Couldn't write %s", lock->lk->filename); unlock_ref(lock); + errno = save_errno; return -1; } clear_loose_ref_cache(&ref_cache); if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 || (strcmp(lock->ref_name, lock->orig_ref_name) && log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) { + int save_errno = errno; unlock_ref(lock); + errno = save_errno; return -1; } if (strcmp(lock->orig_ref_name, "HEAD") != 0) { @@ -2866,8 +2911,10 @@ int write_ref_sha1(struct ref_lock *lock, log_ref_write("HEAD", lock->old_sha1, sha1, logmsg); } if (commit_ref(lock)) { + int save_errno = errno; error("Couldn't set %s", lock->ref_name); unlock_ref(lock); + errno = save_errno; return -1; } unlock_ref(lock); diff --git i/sha1_file.c w/sha1_file.c index 3e9f55f..a7bbba7 100644 --- i/sha1_file.c +++ w/sha1_file.c @@ -136,8 +136,10 @@ enum scld_error safe_create_leading_directories(char *path) *slash = '\0'; if (!stat(path, &st)) { /* path exists */ - if (!S_ISDIR(st.st_mode)) + if (!S_ISDIR(st.st_mode)) { + errno = EEXIST; ret = SCLD_EXISTS; + } } else if (mkdir(path, 0777)) { if (errno == EEXIST && !stat(path, &st) && S_ISDIR(st.st_mode))