Re: Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file

On Wed, 7 Jan 2009, R. Tyler Ballance wrote:

Btw, this is an interesting error message, mostly because of what is _not_ there.

In particular, it doesn't report any reason _why_ it failed to read the object, which as far as I can tell can happen for only one reason: unpack_compressed_entry() returns NULL, and that path is the only thing that can do so without a message.

And it only does it if zlib fails.

Now, zlib can fail because the unpacking fails, but it can fail for other cases too.

And the thing is, we don't check/report those kinds of failure cases very well. Which really bit us here, because if we had checked the return value of inflateInit(), we'd almost certainly would have gotten a nice big "you ran out of memory" thing, and we wouldn't have been chasing this down as a corruption issue.

We probably should wrap all the "inflateInit()" calls, and do something like

	static void xinflateInit(z_streamp strm)
	{
		const char *err;

		switch (inflateInit(strm)) {
		case Z_OK:
			return;
		case Z_MEM_ERROR:
			err = "out of memory";
			break;
		case Z_VERSION_ERROR:
			err = "wrong version";
			break;
		default:
			err = "error";
		}
		die("inflateInit: %s (%s)", err,
			strm->msg ? strm->msg : "no message");
	}

or similar. That way we'd get good error reports when we run out of memory, rather than consider it to be a corruption issue.

We could also try to make a few of these wrappers actually release some of the memory (the way xmmap() does), but there are likely diminishing returns. And the much more important issue is the proper error reporting: if we had reported "out of memory", we'd not have spent so much time chasing disk corruption etc.

			Linus