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

["Shawn O. Pearce" <spearce@spearce.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?

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

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