Re: [PATCH v8 22/44] fetch.c: clear errno before calling functions that might set it
On Fri, May 16, 2014 at 11:33:48AM -0700, Jonathan Nieder wrote:
> (cc-ing peff, who may remember this STORE_REF_ERROR_DF_CONFLICT case
> from long ago)
No, but I have very good tools for searching the list archive. ;)
> > 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.
I'd agree. It's OK for a function not to set errno if it is
_successful_. But if setting errno on error is part of the interface for
lock_any_ref_for_update, it should do so consistently.
We have not been very good about documenting which functions use errno,
or using it consistently (e.g., in my original patch, we are explicitly
converting errno into a numeric return value!), so I can definitely see
how it is confusing.
> 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?
> [...]
Yeah, I think this is a better direction.
> 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;
> + }
check_refname_format comes up in a few places. I wonder if it should set
EINVAL itself.
Other than that, I just skimmed through your list. All looked
reasonable to me, though I was not thorough enough to be sure we got
call cases (OTOH, we can add them later on top).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 22/44] fetch.c: clear errno before calling functions that might set it
errno is hairy :-(
You probably also want something like this :
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4603cb6..55e7dd8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -426,7 +426,7 @@ static int update_local_ref(struct ref *ref,
_("! %-*s %-*s -> %s (can't fetch in current branch
TRANSPORT_SUMMARY(_("[rejected]")),
REFCOL_WIDTH, remote, pretty_ref);
- return 1;
+ return STORE_REF_ERROR_OTHER;
}
if (!is_null_sha1(ref->old_sha1) &&
@@ -513,7 +513,7 @@ static int update_local_ref(struct ref *ref,
TRANSPORT_SUMMARY(_("[rejected]")),
REFCOL_WIDTH, remote, pretty_ref,
_("(non-fast-forward)"));
- return 1;
+ return STORE_REF_ERROR_OTHER;
}
}
To make it more clear that this function returns a specific error and
not just a generic not-zero.
On Fri, May 16, 2014 at 11:33 AM, Jonathan Nieder wrote:
> (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)
>
Re: [PATCH v8 22/44] fetch.c: clear errno before calling functions that might set it
(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_unsaf
