fetch fails with a short read of received pack

fetch fails with a short read of received pack

[Alex Riesen]

Somehow I got my gnulib mirror to a strange state where I can't fetch
anything from it:

    $ cd gnulib
    $ git fetch -f ../gnulib.git 'refs/heads/*:refs/remotes/origin/*'
    remote: Counting objects: 2202, done.
    remote: Compressing objects: 100% (633/633), done.
    remote: Total 1769 (delta 1448), reused 1455 (delta 1134)
    Receiving objects: 100% (1769/1769), 404.11 KiB, done.
    fatal: premature end of pack file, 1745 bytes missing
    fatal: index-pack failed

This is with current Shawn's master (Junio's master erroneously gives
an error). Bisect points at ac0463ed44e859c84e354cd0ae47d9b5b124e112

    pack-objects: use fixup_pack_header_footer()'s validation mode

    When limiting the pack size, a new header has to be written to the
    pack and a new SHA1 computed.  Make sure that the SHA1 of what is being
    read back matches the SHA1 of what was written.

This and the diff make no sense to me (as to what could be broken).
I made both repos available at:

    The source, it unpacks in gitlib.git (as in example above)
	http://vin-soft.org/~ftp-tmp/gnulib-broken-src.tar.bz2

    The target, where the fetch in the example is run
	http://vin-soft.org/~ftp-tmp/gnulib-broken-tgt.tar.bz2

Could someone who actually understands the code please have a look?

Re: fetch fails with a short read of received pack

[Samuel Tardieu]

>>>>> "Alex" == Alex Riesen <raa.lkml@gmail.com> writes:

Alex> Somehow I got my gnulib mirror to a strange state where I can't
Alex> fetch anything from it:

You're the third person to report this problem in three days (I was
the second one). I thought my repository or the upstream one (GCC) was
corrupted in some ways, but it looks like a general problem.

Alex> fatal: premature end of pack file, 1745 bytes missing
Alex> fatal: index-pack failed

Alex> This is with current Shawn's master (Junio's master erroneously
Alex> gives an error).

The error message has been fixed three days ago, it hasn't reached
Junio's repository yet.

Alex> Bisect points at ac0463ed44e859c84e354cd0ae47d9b5b124e112

Incidentally, my first "git fetch" problem was around September 8 if I
remember correctly, so this looks plausible to have had the bug around
the date of the commit you mention (Aug 29).

 Sam
-- 
Samuel Tardieu -- sam@rfc1149.net -- http://www.rfc1149.net/

Re: fetch fails with a short read of received pack

[Nicolas Pitre]

On Thu, 9 Oct 2008, Samuel Tardieu wrote:

> >>>>> "Alex" == Alex Riesen <raa.lkml@gmail.com> writes:
> 
> Alex> Somehow I got my gnulib mirror to a strange state where I can't
> Alex> fetch anything from it:
> 
> You're the third person to report this problem in three days (I was
> the second one). I thought my repository or the upstream one (GCC) was
> corrupted in some ways, but it looks like a general problem.

Alex has a good test case and his bissection points at me.
So I'm on it.


Nicolas

[PATCH] fix pread()'s short read in index-pack

[Nicolas Pitre]

Since v1.6.0.2~13^2~ the completion of a thin pack uses sha1write() for 
its ability to compute a SHA1 on the written data.  This also provides 
data buffering which, along with commit 92392b4a45, will confuse pread() 
whenever an appended object is 1) freed due to memory pressure because 
of the depth-first delta processing, and 2) needed again because it has 
many delta children, and 3) its data is still buffered by sha1write().

Let's fix the issue by simply forcing cached data out when such an 
object is written so it can be pread()'d at leisure.

Signed-off-by: Nicolas Pitre <nico@cam.org>

---

On Thu, 9 Oct 2008, Alex Riesen wrote:

> Somehow I got my gnulib mirror to a strange state where I can't fetch
> anything from it:
> 
>     $ cd gnulib
>     $ git fetch -f ../gnulib.git 'refs/heads/*:refs/remotes/origin/*'
>     remote: Counting objects: 2202, done.
>     remote: Compressing objects: 100% (633/633), done.
>     remote: Total 1769 (delta 1448), reused 1455 (delta 1134)
>     Receiving objects: 100% (1769/1769), 404.11 KiB, done.
>     fatal: premature end of pack file, 1745 bytes missing
>     fatal: index-pack failed
> 
> This is with current Shawn's master (Junio's master erroneously gives
> an error). Bisect points at ac0463ed44e859c84e354cd0ae47d9b5b124e112

Thanks for providing a good test data set.  As you can see above, this 
is something that is tricky to reproduce otherwise.  This patch is meant 
for the maint branch but should be included in master as well.

diff --git a/csum-file.c b/csum-file.c
index bb70c75..3b3e090 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -11,7 +11,7 @@
 #include "progress.h"
 #include "csum-file.h"
 
-static void sha1flush(struct sha1file *f, void *buf, unsigned int count)
+static void flush(struct sha1file *f, void *buf, unsigned int count)
 {
 	for (;;) {
 		int ret = xwrite(f->fd, buf, count);
@@ -30,22 +30,28 @@ static void sha1flush(struct sha1file *f, void *buf, unsigned int count)
 	}
 }
 
-int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags)
+void sha1flush(struct sha1file *f)
 {
-	int fd;
 	unsigned offset = f->offset;
 
 	if (offset) {
 		SHA1_Update(&f->ctx, f->buffer, offset);
-		sha1flush(f, f->buffer, offset);
+		flush(f, f->buffer, offset);
 		f->offset = 0;
 	}
+}
+
+int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags)
+{
+	int fd;
+
+	sha1flush(f);
 	SHA1_Final(f->buffer, &f->ctx);
 	if (result)
 		hashcpy(result, f->buffer);
 	if (flags & (CSUM_CLOSE | CSUM_FSYNC)) {
 		/* write checksum and close fd */
-		sha1flush(f, f->buffer, 20);
+		flush(f, f->buffer, 20);
 		if (flags & CSUM_FSYNC)
 			fsync_or_die(f->fd, f->name);
 		if (close(f->fd))
@@ -83,7 +89,7 @@ int sha1write(struct sha1file *f, void *buf, unsigned int count)
 		left -= nr;
 		if (!left) {
 			SHA1_Update(&f->ctx, data, offset);
-			sha1flush(f, data, offset);
+			flush(f, data, offset);
 			offset = 0;
 		}
 		f->offset = offset;
diff --git a/csum-file.h b/csum-file.h
index 72c9487..01f13b5 100644
--- a/csum-file.h
+++ b/csum-file.h
@@ -24,6 +24,7 @@ extern struct sha1file *sha1fd(int fd, const char *name);
 extern struct sha1file *sha1fd_throughput(int fd, const char *name, struct progress *tp);
 extern int sha1close(struct sha1file *, unsigned char *, unsigned int);
 extern int sha1write(struct sha1file *, void *, unsigned int);
+extern void sha1flush(struct sha1file *f);
 extern void crc32_begin(struct sha1file *);
 extern uint32_t crc32_end(struct sha1file *);
 
diff --git a/index-pack.c b/index-pack.c
index 530d820..ca72329 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -704,6 +704,7 @@ static struct object_entry *append_obj_to_pack(struct sha1file *f,
 	obj[1].idx.offset = obj[0].idx.offset + n;
 	obj[1].idx.offset += write_compressed(f, buf, size);
 	obj[0].idx.crc32 = crc32_end(f);
+	sha1flush(f);
 	hashcpy(obj->idx.sha1, sha1);
 	return obj;
 }

Re: [PATCH] fix pread()'s short read in index-pack

[Alex Riesen]

2008/10/10 Nicolas Pitre <nico@cam.org>:
>
> Since v1.6.0.2~13^2~ the completion of a thin pack uses sha1write() for
> its ability to compute a SHA1 on the written data.  This also provides
> data buffering which, along with commit 92392b4a45, will confuse pread()
> whenever an appended object is 1) freed due to memory pressure because
> of the depth-first delta processing, and 2) needed again because it has
> many delta children, and 3) its data is still buffered by sha1write().

So I seem to have had luck not doing gc in either of source or target repos
(I usually repack, than fetch. Especially this one repo, it's very active).

BTW, I run into complications trying to test the fix on Shawn's
master: conflicts,
and my trivial resolution wasn't good enough.

Aside from that, I confirm the case fixed.

Re: [PATCH] fix pread()'s short read in index-pack

[Shawn O. Pearce]

Alex Riesen <raa.lkml@gmail.com> wrote:
> 2008/10/10 Nicolas Pitre <nico@cam.org>:
> >
> > Since v1.6.0.2~13^2~ the completion of a thin pack uses sha1write() for
> > its ability to compute a SHA1 on the written data.  This also provides
> > data buffering which, along with commit 92392b4a45, will confuse pread()
> > whenever an appended object is 1) freed due to memory pressure because
> > of the depth-first delta processing, and 2) needed again because it has
> > many delta children, and 3) its data is still buffered by sha1write().
> 
> BTW, I run into complications trying to test the fix on Shawn's
> master: conflicts,
> and my trivial resolution wasn't good enough.

Huh.

Color me confused.  What the heck was the base version?

Near as I can tell Nico wrote this fix against a8032d12 (sha1write:
don't copy full sized buffers).  This is in Junio's master.

But the changes conflict with what is in Junio's maint.  So the
patch doesn't apply cleanly due to the sha1flush function's
signature changing.

And the changes conflict with 9126f009 (fix openssl headers
conflicting with custom SHA1 implementations), which is in my
master, and was written by Nico.

;-)

Anyway, its merged now.

-- 
Shawn.

Re: [PATCH] fix pread()'s short read in index-pack

[Nicolas Pitre]

On Fri, 10 Oct 2008, Shawn O. Pearce wrote:

> Alex Riesen <raa.lkml@gmail.com> wrote:
> > 2008/10/10 Nicolas Pitre <nico@cam.org>:
> > >
> > > Since v1.6.0.2~13^2~ the completion of a thin pack uses sha1write() for
> > > its ability to compute a SHA1 on the written data.  This also provides
> > > data buffering which, along with commit 92392b4a45, will confuse pread()
> > > whenever an appended object is 1) freed due to memory pressure because
> > > of the depth-first delta processing, and 2) needed again because it has
> > > many delta children, and 3) its data is still buffered by sha1write().
> > 
> > BTW, I run into complications trying to test the fix on Shawn's
> > master: conflicts,
> > and my trivial resolution wasn't good enough.
> 
> Huh.
> 
> Color me confused.  What the heck was the base version?
> 
> Near as I can tell Nico wrote this fix against a8032d12 (sha1write:
> don't copy full sized buffers).  This is in Junio's master.
> 
> But the changes conflict with what is in Junio's maint.  So the
> patch doesn't apply cleanly due to the sha1flush function's
> signature changing.
> 
> And the changes conflict with 9126f009 (fix openssl headers
> conflicting with custom SHA1 implementations), which is in my
> master, and was written by Nico.
> 
> ;-)

Whatever.

Your truly is still recovering from a knee surgery and therefore is 
literally on drugs.

> Anyway, its merged now.

Thanks.  The important thing is that it must be included in what would 
be v1.6.0.3.


Nicolas