[PATCH 1/2] read_sha1_file(): report correct name of packfile with a corrupt object

[PATCH 1/2] read_sha1_file(): report correct name of packfile with a corrupt object

[Junio C Hamano]

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(-)

diff --git a/sha1_file.c b/sha1_file.c
index 0cd9435..5ed5497 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1003,7 +1003,7 @@ static void mark_bad_packed_object(struct packed_git *p,
 	p->num_bad_objects++;
 }
 
-static int has_packed_and_bad(const unsigned char *sha1)
+static const struct packed_git *has_packed_and_bad(const unsigned char *sha1)
 {
 	struct packed_git *p;
 	unsigned i;
@@ -1011,8 +1011,8 @@ static int has_packed_and_bad(const unsigned char *sha1)
 	for (p = packed_git; p; p = p->next)
 		for (i = 0; i < p->num_bad_objects; i++)
 			if (!hashcmp(sha1, p->bad_object_sha1 + 20 * i))
-				return 1;
-	return 0;
+				return p;
+	return NULL;
 }
 
 int check_sha1_signature(const unsigned char *sha1, void *map, unsigned long size, const char *type)
@@ -2079,6 +2079,11 @@ static void *read_object(const unsigned char *sha1, enum object_type *type,
 	return read_packed_sha1(sha1, type, size);
 }
 
+/*
+ * This function dies on corrupted objects; the callers who want to
+ * deal with them should arrange to call read_object() and give error
+ * messages themselves.
+ */
 void *read_sha1_file_repl(const unsigned char *sha1,
 			  enum object_type *type,
 			  unsigned long *size,
@@ -2087,28 +2092,30 @@ void *read_sha1_file_repl(const unsigned char *sha1,
 	const unsigned char *repl = lookup_replace_object(sha1);
 	void *data = read_object(repl, type, size);
 	char *path;
+	const struct packed_git *p;
+
+	if (data) {
+		if (replacement)
+			*replacement = repl;
+		return data;
+	}
 
 	/* die if we replaced an object with one that does not exist */
-	if (!data && repl != sha1)
+	if (repl != sha1)
 		die("replacement %s not found for %s",
 		    sha1_to_hex(repl), sha1_to_hex(sha1));
 
-	/* legacy behavior is to die on corrupted objects */
-	if (!data) {
-		if (has_loose_object(repl)) {
-			path = sha1_file_name(sha1);
-			die("loose object %s (stored in %s) is corrupted", sha1_to_hex(repl), path);
-		}
-		if (has_packed_and_bad(repl)) {
-			path = sha1_pack_name(sha1);
-			die("packed object %s (stored in %s) is corrupted", sha1_to_hex(repl), path);
-		}
+	if (has_loose_object(repl)) {
+		path = sha1_file_name(sha1);
+		die("loose object %s (stored in %s) is corrupt",
+		    sha1_to_hex(repl), path);
 	}
 
-	if (replacement)
-		*replacement = repl;
+	if ((p = has_packed_and_bad(repl)) != NULL)
+		die("packed object %s (stored in %s) is corrupt",
+		    sha1_to_hex(repl), p->pack_name);
 
-	return data;
+	return NULL;
 }
 
 void *read_object_with_reference(const unsigned char *sha1,
-- 
1.7.3.2.191.g2d0e57

[PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE

[Junio C Hamano]

"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(-)

diff --git a/sha1_file.c b/sha1_file.c
index 5ed5497..f709ed6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2090,16 +2090,21 @@ void *read_sha1_file_repl(const unsigned char *sha1,
 			  const unsigned char **replacement)
 {
 	const unsigned char *repl = lookup_replace_object(sha1);
-	void *data = read_object(repl, type, size);
+	void *data;
 	char *path;
 	const struct packed_git *p;
 
+	errno = 0;
+	data = read_object(repl, type, size);
 	if (data) {
 		if (replacement)
 			*replacement = repl;
 		return data;
 	}
 
+	if (errno != ENOENT)
+		die_errno("failed to read object %s", sha1_to_hex(sha1));
+
 	/* die if we replaced an object with one that does not exist */
 	if (repl != sha1)
 		die("replacement %s not found for %s",
-- 
1.7.3.2.191.g2d0e57

Re: [PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE

[Erik Faye-Lund]

On Thu, Oct 28, 2010 at 10:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "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(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 5ed5497..f709ed6 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2090,16 +2090,21 @@ void *read_sha1_file_repl(const unsigned char *sha1,
>                          const unsigned char **replacement)
>  {
>        const unsigned char *repl = lookup_replace_object(sha1);
> -       void *data = read_object(repl, type, size);
> +       void *data;
>        char *path;
>        const struct packed_git *p;
>
> +       errno = 0;
> +       data = read_object(repl, type, size);
>        if (data) {
>                if (replacement)
>                        *replacement = repl;
>                return data;
>        }
>
> +       if (errno != ENOENT)
> +               die_errno("failed to read object %s", sha1_to_hex(sha1));
> +
>        /* die if we replaced an object with one that does not exist */
>        if (repl != sha1)
>                die("replacement %s not found for %s",
> --
> 1.7.3.2.191.g2d0e57
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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<---
diff --git a/compat/mingw.c b/compat/mingw.c
index 29f4036..fdbf093 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -198,9 +198,10 @@ static inline time_t filetime_to_time_t(const FILETIME *ft)
  */
 static int do_lstat(int follow, const char *file_name, struct stat *buf)
 {
+	int err;
 	WIN32_FILE_ATTRIBUTE_DATA fdata;

-	if (!(errno = get_file_attr(file_name, &fdata))) {
+	if (!(err = get_file_attr(file_name, &fdata))) {
 		buf->st_ino = 0;
 		buf->st_gid = 0;
 		buf->st_uid = 0;
@@ -233,6 +234,7 @@ static int do_lstat(int follow, const char
*file_name, struct stat *buf)
 		}
 		return 0;
 	}
+	errno = err;
 	return -1;
 }

---8<---

The following patch makes clone complete again, but I doubt it's the
right solution. However, I'm out of ideas for now. Anyone? :)

diff --git a/sha1_file.c b/sha1_file.c
index 25f6965..9d4cb9c 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2102,7 +2102,7 @@ void *read_sha1_file_repl(const unsigned char *sha1,
 		return data;
 	}

-	if (errno != ENOENT)
+	if (errno && errno != ENOENT)
 		die_errno("failed to read object %s", sha1_to_hex(sha1));

 	/* die if we replaced an object with one that does not exist */

Re: [PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE

[Johannes Sixt]

Am 11/18/2010 15:19, schrieb Erik Faye-Lund:
> 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?

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

Re: [PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE

[Erik Faye-Lund]

On Thu, Nov 18, 2010 at 5:43 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 11/18/2010 15:19, schrieb Erik Faye-Lund:
>> 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?
>
> The problem is in opendir() called via prepare_packed_git_one() via
> prepare_packed_git(). It resets errno to 0 on success.
>

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<---
diff --git a/compat/mingw.h b/compat/mingw.h
index 35d9813..b5f16fa 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -133,6 +133,29 @@ static inline int mingw_unlink(const char *pathname)
 }
 #define unlink mingw_unlink

+static inline DIR *mingw_opendir(const char *path)
+{
+	DIR *ret;
+	int saved_errno = errno;
+
+	if (!(ret = opendir(path)))
+		return NULL;
+
+	errno = saved_errno;
+	return ret;
+}
+#define opendir mingw_opendir
+
+static inline int mingw_closedir(DIR *dir)
+{
+	int saved_errno = errno;
+	if (closedir(dir) == -1)
+		return -1;
+	errno = saved_errno;
+	return 0;
+}
+#define closedir mingw_closedir
+
 #define WNOHANG 1
 pid_t waitpid(pid_t pid, int *status, unsigned options);

---8<---

I didn't patch readdir(), since we already have our own
implementation, but perhaps the code should work together with
NO_MINGW_REPLACE_READDIR also... Can somebody remind me why we have an
opt-out for the readdir()-replacement? Debugging?

> 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.
>

Thanks.

> 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()?
>

I was wondering about the same thing, I don't think this approach is
very easy to follow. But either way I think we should make sure
opendir/closedir/readdir doesn't mess with errno.

Re: [PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE

[Jonathan Nieder]

Erik Faye-Lund wrote:

> I was wondering about the same thing, I don't think this approach is
> very easy to follow. But either way I think we should make sure
> opendir/closedir/readdir doesn't mess with errno.

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

Re: [PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE

[Erik Faye-Lund]

On Thu, Nov 18, 2010 at 6:29 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Erik Faye-Lund wrote:
>
>> I was wondering about the same thing, I don't think this approach is
>> very easy to follow. But either way I think we should make sure
>> opendir/closedir/readdir doesn't mess with errno.
>
> 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.

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.

Re: [PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE

[Casey Dahlin]

On Thu, Nov 18, 2010 at 07:18:37PM +0100, Erik Faye-Lund wrote:
> On Thu, Nov 18, 2010 at 6:29 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> > Erik Faye-Lund wrote:
> >
> >> I was wondering about the same thing, I don't think this approach is
> >> very easy to follow. But either way I think we should make sure
> >> opendir/closedir/readdir doesn't mess with errno.
> >
> > 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.
> 
> 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.

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

Re: [PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE

[Jonathan Nieder]

Erik Faye-Lund wrote:

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

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.

Re: [PATCH 1/2] read_sha1_file(): report correct name of packfile with a corrupt object

[Johannes Sixt]

Am 10/28/2010 22:53, schrieb Junio C Hamano:
> "Corrupt" is already an adjective, ... we do not
> have to say "corrupted".

> + * This function dies on corrupted objects; the callers who want to

Except sometimes. ;)

-- Hannes