Re: [PATCH 1/3] improve reliability of fixup_pack_header_footer()

From: Shawn O. Pearce <spearce@spearce.org>
Date: Aug 29, 2008, 04:44 UTC
Message-ID: <20080829044459.GA28492@spearce.org>
In-Reply-To: <1219975624-7653-1-git-send-email-nico@cam.org>

Nicolas Pitre <nico@cam.org> wrote:

> Currently, this function has the potential to read corrupted pack data
> from disk and give it a valid SHA1 checksum.  Let's add the ability to
> validate SHA1 checksum of existing data along the way, including before
> and after any arbitrary point in the pack.

I found your implementation in fixup_pack_header to be very
contorted.  Did you read the JGit patch I posted?  I think its
implementation is more elegant and easier to follow.  Oh, and its
BSD licensed... so easy for a GPLv2 project to borrow.  ;-)

>  void fixup_pack_header_footer(int pack_fd,

...

> +	if (partial_pack_sha1 && !partial_pack_offset) {
> +		partial_pack_offset = lseek(pack_fd, 0, SEEK_CUR);
> +		if (partial_pack_offset == (off_t)-1)

Eh?

I find this to be nonsense. If the caller gave us a SHA-1 but wants us to do fixup then they have increased the size of the pack. Which means they must pass us the original length. Computing it here is relying on the caller leaving the file pointer positioned at the old end. Who the heck does that after fixing a thin pack?

Show 7 quoted lines

>  	buf = xmalloc(buf_sz);
>  	for (;;) {
> -		ssize_t n = xread(pack_fd, buf, buf_sz);
> +		ssize_t m, n;
> +		m = (partial_pack_sha1 && partial_pack_offset < buf_sz) ?
> +			partial_pack_offset : buf_sz;
> +		n = xread(pack_fd, buf, m);

Why not read the full buf_sz per round and then use only part of the buffer in SHA1_Update(&old_sha1_ctx)? It took me a while to figure out what the heck you were trying to measure here. It also would be a few less CPU instructions if you always just read buf_sz and leave the min test to after the "if (!partial_pack_sha1)" below.

Show 19 quoted lines

> +		if (!partial_pack_sha1)
> +			continue;
> +
> +		SHA1_Update(&old_sha1_ctx, buf, n);
> +		partial_pack_offset -= n;
> +		if (partial_pack_offset == 0) {
> +			unsigned char sha1[20];
> +			SHA1_Final(sha1, &old_sha1_ctx);
> +			if (hashcmp(sha1, partial_pack_sha1) != 0)
> +				die("Unexpected checksum for %s "
> +				    "(disk corruption?)", pack_name);
> +			/*
> +			 * Now let's compute the SHA1 of the remainder of the
> +			 * pack, which also means making partial_pack_offset
> +			 * big enough not to matter anymore.
> +			 */
> +			SHA1_Init(&old_sha1_ctx);
> +			partial_pack_offset = ~partial_pack_offset;
> +			partial_pack_offset -= MSB(partial_pack_offset, 1);

That's freaking brilliant.  And something I missed in JGit.

The way its implemented is downright difficult to follow though. I'll admit it took me a good 10 minutes to understand what you were doing here, and why.

-- 
Shawn.