patch, 2 partsA loose object is not corrupt if it cannot be read due to EMFILE

Clarify the error reporting logic by moving the normal codepath (i.e. we read the object we wanted to read correctly) up and return early.

The logic to report the name of the packfile with a corrupt object, introduced by e8b15e6 (sha1_file: Show the the type and path to corrupt objects, 2010-06-10), was totally bogus. The function that knows which bad object came from what packfile is has_packed_and_bad(); make it report which packfile the problem was found.

"Corrupt" is already an adjective, e.g. an object is "corrupt"; we do not have to say "corrupted".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_file.c |   41 ++++++++++++++++++++++++-----------------
 1 files changed, 24 insertions(+), 17 deletions(-)

"git fsck" bails out with a claim that a loose object that cannot be read but exists on the filesystem to be corrupt, which is wrong when read_object() failed due to e.g. EMFILE.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_file.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

On Thu, Oct 28, 2010 at 10:53 PM, Junio C Hamano <gitster@pobox.com> wrote:

This patch seems to break clone/fetch on Windows:

$ GIT_TRACE=1 git clone http://github.com/kusma/rocket.git clone-test trace: built-in: git 'clone' 'http://github.com/kusma/rocket.git' 'clone-test' Cloning into clone-test... trace: run_command: 'git-remote-http' 'origin' 'http://github.com/kusma/rocket.git' trace: run_command: 'fetch-pack' '--stateless-rpc' '--lock-pack' '--thin' 'http://github.com/kusma/rocket.git/' 'refs/heads/master' 'refs/heads/work/icon' 'refs/heads/work/io-callback' 'refs/heads/work/sdl-example' 'refs/tags/v0.8' trace: built-in: git 'fetch-pack' '--stateless-rpc' '--lock-pack' '--thin' 'http://github.com/kusma/rocket.git/' 'refs/heads/master' 'refs/heads/work/icon' 'refs/heads/work/io-callback' 'refs/heads/work/sdl-example' 'refs/tags/v0.8' fatal: failed to read object 7ea7cabd967429886c26e78b3ec573d54a95fa9d: No error fatal: The remote end hung up unexpectedly

What happens, is that read_object returns NULL, but errno is 0. Further, it looks to me like read_object can only return NULL through the unpack_sha1_file (problem with the compressed data) or read_packed_sha1 (find_pack_entry() failure) code-paths.

errno is set to ENOENT by open_sha1_file (through map_sha1_file) before any possible error-points. I guess this makes the "errno = 0" redundant, but I think it improves readability of the code. I'm guessing that errno gets overwritten by some other call, losing the ENOENT. Perhaps some unintended side-effect of one of the compat/mingw.[ch]-wrappers?

I did find such a case, but fixing it doesn't do the trick for me:

---8<---

Am 11/18/2010 15:19, schrieb Erik Faye-Lund:

The problem is in opendir() called via prepare_packed_git_one() via prepare_packed_git(). It resets errno to 0 on success.

You can test this easily by inserting test_done after the 3rd test of t5530 and run it with --debug; in the trash-directory you can run

  ../../git-pack-objects --revs --all --stdout >/dev/null </dev/null

and observe the different failure modes on Windows and Linux.

This makes me question whether the approach of Junio's fix is sane. It depends on errno being set *way* before it is checked and after *a*lot* of potentially failing system and library calls have been called. Which function is it that is expected to fail with ENOENT? git_open_noatime()?

-- Hannes

On Thu, Nov 18, 2010 at 5:43 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:

Hmm, this is not one of our wrappings, but a bug in the mingw-runtine, right? Perhaps we should report it? I see that "errno = 0;" is the first line of code of _topendir() in the mingw-runtime (see http://cygwin.com/cgi-bin/cvsweb.cgi/src/winsup/mingw/mingwex/dirent.c?rev=1.9&cvsroot=src), and from the looks of it it should simply be removed. The same goes for closedir() and readdir().

That being said, we should probably work around this problem with opendir() ourselves. This patch fixes the problem for me:

---8<---

Erik Faye-Lund wrote:

Other platforms and are allowed to (and do) clobber errno in many non-error situations, so the main effect would be to work around/hide some git bugs.

For mingw-runtime, the balance might be different. Making buggy, non-portable application code easier to port does not sound like a bad thing.

Just my two cents, Jonathan

On Thu, Nov 18, 2010 at 6:29 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

I don't think that's the case; K&R (section B1.7) says "In addition, the integer expression errno (declared in <errno.h>) may contain an error number that contains information about the most recent error.". A non-error condition would not be information about the most recent error. I interpret the use of the word "may" to mean "some functions does not update errno, so errno could also have information about older errors" not as "some functions can clear errno", but other might disagree with me.

POSIX (which defines opendir) says "No function in this volume of IEEE Std 1003.1-2001 shall set errno to 0", so I think it's clear cut in this case. (see http://www.opengroup.org/onlinepubs/009695399/functions/errno.html for the full text)

We also seem to depend quite heavily on errno being preserved on non-errors some places around in the code.

On Thu, Nov 18, 2010 at 07:18:37PM +0100, Erik Faye-Lund wrote:

Errno can be clobbered if some subset of the operation in question failed but the operation itself succeeded. For example suppose opendir did a memory allocation that failed, but was able to recover from it and return successfully. Errno might contain ENOMEM at the exit.

tl;dr - errno can be clobbered when functions that succeed call functions that fail while progressing.

--CJD

Erik Faye-Lund wrote:

Hmm, sounds worrying.

- builtin/fetch.c::s_update_ref appears to have some of this kind of
  bogosity.
- safe_create_leading_directories_const and git_config_set_multivar let
  free() touch errno.  Probably not a problem.
- lock_file ought to preserve or set errno on failure.
- daemon::run_service sometimes sets errno on failure and sometimes not.
  This is confusing even if callers don't seem to care.
- fast-import::dump_marks_helper ought to do its own error reporting.
- resolve_ref seems to rely on get_packed_refs not to clobber errno.
- sha1_file::open_packed_git_1 seems to rely on unuse_one_window not to
  clobber errno.

Aside from that:

The following does not take into account correctness of error messages. (That is probably a good reason to fix compat/mingw's opendir. Sorry, I forgot to take that into account before.)

Builtins
--------
apply.c:
	checks errno following lstat() failure.
	checks errno following try_create_file() return from
	 mkdir(), symlink(), or open() failure.
clone.c:	checks errno following mkdir() and unlink() failure.
commit.c:	saves errno following strbuf_read_file() failure.
fetch.c:
	checks errno following write_in_full(), close(), fopen() failure.
	see above for the s_update_ref() checks.
for-each-ref.c:	checks errno following strtol() failure.
grep.c:	checks errno following open() and lstat() failure.
index-pack.c:
	checks errno following open() failure.
	checks errno following odb_pack_keep() [= open()] failure.
init-db.c:	checks errno following mkdir(), lstat() failure.
log.c:	checks errno following mkdir() failure.
mailsplit.c:	checks errno following opendir() failure.
pack-objects.c:	checks errno following fgets() failure.
rm.c:	checks errno following lstat() failure.
update-index.c:
	checks errno following lstat() failure.
	relies on strtoul() not to clobber errno on success.
	checks errno following hold_locked_index() return from
	 lock_file() failure (see above).
upload-archive.c:
	checks errno following read(), pipe(), fork(), and poll() failure.

Compatibility routines ---------------------- cygwin.c, inet_ntop.c, mingw.c, and mmap.c use errno in noncosmetic ways. I haven't checked them.

git wrapper ----------- git.c: checks errno following run_command_v_opt() failure.

libgit
------
config.c:	checks errno following open() failure, with intervening free().
connect.c:	checks errno following connect(), socket() failure.
copy.c:	saves errno following xread(), xwrite() failure.
daemon.c:	checks errno following poll(), accept() failure.
diff-lib.c:	checks errno following lstat() failure.
diff.c:	checks errno following lstat() failure.
dir.c:	checks errno following unlink() failure.
entry.c:	checks errno following mkdir() failure.
fast-import.c:
	saves errno following fdopen(), fprintf(), fclose() failure.
	saves errno following commit_lock_file [=close() or rename()] failure.
	relies on strtoul() not to clobber errno on success.
git-compat-util.h:
	relies on strtoul(), strtol() not to clobber errno on success.
http.c:	checks errno following open() failure.
lockfile.c:	checks errno following open() failure.
merge-recursive.c:
	checks errno following write_in_full(), unlink() failure.
path.c:	checks errno following snprintf() failure.
read-cache.c:	checks errno following lstat(), open() failure.
refs.c:	see above for resolve_ref() worries.
rerere.c:	checks errno following fwrite(), unlink() failure.
run-command.c:
	checks errno following waitpid, pipe, fork, execv, failure.
	wait_or_whine() does not set errno when child died by signal.
setup.c:	checks errno following lstat() failure.
sha1_file.c:
	checks errno following open(), opendir() failure.
	see above re unuse_one_window() failure.
sha1_name.c:	checks errno following lstat() failure.
strbuf.c:	checks errno following readlink() failure.
symlinks.c:	checks errno following stat(), lstat() failure.
transport-helper.c:	checks errno following start_command() failure.
unpack-trees.c:	checks errno following lstat() failure.
upload-pack.c:	checks errno following poll() failure.
utf8.c:	checks errno following iconv() failure.
wrapper.c:
	checks errno following read(), write(), unlink(), rmdir() failure.
write_or_die.c:	checks errno following fflush(), write() failures.

Test helpers ------------ test-run-command.c: checks errno following start_command() failure.

Am 10/28/2010 22:53, schrieb Junio C Hamano:

Except sometimes. ;)

-- Hannes