threads / discuss / 5409

Re: Problem with pack

Subject: Re: Problem with pack

## tl;dr

4 messages between Aug 27, 2006 and Aug 27, 2006.

replies: 3people: 4as markdown or json

Sergio Callegari· Aug 27, 2006, 17:45 UTC · lore
Show 7 quoted lines
>
> I do think that your synchronization using unison is _somehow_ part of the 
> reason why bad things happened, but I really can't see why it would cause 
> problems, and perhaps more importantly, git should have noticed them 
> earlier (and, in particular, failed the repack). So a git bug and/or 
> misfeature is involved somehow.
>   
Glad if my broken pack can help finding out!
Show 9 quoted lines
> One thing that may have happened is that the use of unison somehow 
> corrupted an older pack (or you had a disk corruption), and that it was 
> missed because the corruption was in a delta of the old pack that was 
> silently re-used for the new one.
>
> That would explain how the SHA1 of the pack-file matches - the repack 
> would have re-computed the SHA1 properly, but since the source delta 
> itself was corrupt, the resulting new pack is corrupt.
>

There is something that I still do not understand... (sorry if I ask stupid questions)... Since packs have an sha signature too, if there was a data corruption (disk or transfer), shouldn't that have been detected at the repack? I.e. doesn't repack -d verify the available data before cancelling anything?

Show 5 quoted lines
> If you had used git itself to synchronize the two repositories, that 
> corruption of one repo would have been noticed when it transfers the data 
> over to the other side, which is one reason why the native git syncing 
> tools are so superior to doing a filesystem-level synchronization.
>   
I think I learnt the lesson!
> With a filesystem-level sync (unison or anything else - rsync, cp -r, 
> etc), a problem introduced in one repository will be copied to another one 
> without any sanity checking.
>   
Idem!
Show 6 quoted lines
> but in the meantime, when you find a place to put the corrupt pack/index 
> file, please include me and Junio at a minimum into the group of people 
> who you tell where to find it (and/or passwords to access it). I'll 
> happily keep your data private (I've done it before for others).
>
>   
Sure... I have already sent an email to Junio to arrange this.

Thanks, Sergio

Linus Torvalds· Aug 27, 2006, 18:27 UTC · re: Sergio Callegari · lore
On Sun, 27 Aug 2006, Sergio Callegari wrote:
Show 6 quoted lines
>
> There is something that I still do not understand... (sorry if I ask stupid
> questions)...
> Since packs have an sha signature too, if there was a data corruption (disk or
> transfer), shouldn't that have been detected at the repack? I.e. doesn't
> repack -d verify the available data before cancelling anything?

The packs do have a SHA1 signature, but verifying it is too expensive for normal operations. It's only verified when you explicitly ask for it, ie by git-fsck-objects and git-verify-pack.

Now, for small projects we could easily verify the SHA1 csum when we load the pack, but imagine doing the same thing when the pack is half a gigabyte in size, and I think you see the problem.. Especially as most normal operations wouldn't even otherwise touch more than a small small fraction of the pack contents, so verifying the SHA1 would be relatively very expensive indeed.

Now, "git repack -a" is obviously special in that the "-a" will mean that we will generally touch all of the old pack _anyway_, and thus verifying the signature is no longer at all as unreasonable as it is under other circumstances. And very arguably, if you _also_ do "-d", then since that is a fairly dangerous operation with the potential for real data loss, you could well argue that we should do it.

However, since the data was _already_ corrupt in that situation, and since a "git-fsck-objects --full" _will_ pick up the corruption in that case both before and after, equally arguably it's also true that there's really not a huge advantage to checking it in "git repack -a -d".

In other words, in your case, the reason you ended up with the corruption spreading was _not_ that "git repack -a -d" might have silently not noticed it, but really the fact that unison meant that the corruption would spread from one archive to another in the first place.

NOTE! This is all assuming my theory that a packed entry was broken in the first place was correct. We obviously still don't _know_ what the problem was. So far it's just a theory.

Final note: a "git repack -a -d" normally actually _does_ do almost as much checking as a "git-fsck-objects". It's literally just the "copy the already packed object from an old pack to a new one" that it an optimization that short-circuits all the normal git sanity checks. All the other cases will effectively do a lot of integrity checking just by virtue of unpacking the data in the object that is packed, before re-packing it.

So it might well be the case that we should simply add an extra integrity check to the raw data copy in builtin-pack-objects.c: write_object().

Now, these days there is actually two cases of that: the pack-to-pack copy (which has existed for a long while) and the new "!legacy_loose_object()" case. They should perhaps both verify the integrity of what they copy.

Junio, comments?
		Linus
Nicolas Pitre· Aug 27, 2006, 19:26 UTC · re: Linus Torvalds · lore
On Sun, 27 Aug 2006, Linus Torvalds wrote:
Show 6 quoted lines
> Now, "git repack -a" is obviously special in that the "-a" will mean that 
> we will generally touch all of the old pack _anyway_, and thus verifying 
> the signature is no longer at all as unreasonable as it is under other 
> circumstances. And very arguably, if you _also_ do "-d", then since that 
> is a fairly dangerous operation with the potential for real data loss, you 
> could well argue that we should do it.
We definitely should do it with -d.
> However, since the data was _already_ corrupt in that situation, and since 
> a "git-fsck-objects --full" _will_ pick up the corruption in that case 
> both before and after, equally arguably it's also true that there's really 
> not a huge advantage to checking it in "git repack -a -d".

There really is an advantage. Given that the absence of -d leaves old objects/packs around, there is a greater chance for still finding an early copy of the bad object.

> In other words, in your case, the reason you ended up with the corruption 
> spreading was _not_ that "git repack -a -d" might have silently not 
> noticed it, but really the fact that unison meant that the corruption 
> would spread from one archive to another in the first place.

But -d would definitely delete old packs that could have had a non corrupted copy of the desired object.

Show 13 quoted lines
> Final note: a "git repack -a -d" normally actually _does_ do almost as 
> much checking as a "git-fsck-objects". It's literally just the "copy the 
> already packed object from an old pack to a new one" that it an 
> optimization that short-circuits all the normal git sanity checks. All the 
> other cases will effectively do a lot of integrity checking just by virtue 
> of unpacking the data in the object that is packed, before re-packing it.
> 
> So it might well be the case that we should simply add an extra integrity 
> check to the raw data copy in builtin-pack-objects.c: write_object().
> 
> Now, these days there is actually two cases of that: the pack-to-pack copy 
> (which has existed for a long while) and the new "!legacy_loose_object()" 
> case. They should perhaps both verify the integrity of what they copy.

I think that git-pack-object should grow another flag: --verify-src or something. That flag would force the verification of the pack checksum for any pack used for the repack operation. Then it should be used anytime -d is provided to git-repack. When -d is not provided then we can skip that --verify-src security measure since nothing gets deleted and therefore no risk of loosing a good object over a corrupted one would happen.

Nicolas
Junio C Hamano· Aug 27, 2006, 21:55 UTC · re: Linus Torvalds · lore
Linus Torvalds <torvalds@osdl.org> writes:
> NOTE! This is all assuming my theory that a packed entry was broken in the 
> first place was correct. We obviously still don't _know_ what the problem 
> was. So far it's just a theory.

But it is a good theory. More plausible than alpha particle hitting the output buffer of zlib at the right moment, although the effects are the same ;-).

> So it might well be the case that we should simply add an extra integrity 
> check to the raw data copy in builtin-pack-objects.c: write_object().

I would agree that it is a sensible thing to do to insert check at places shown in the attached. The revalidate_pack_piece() would:

 - decode object header to make sure it decodes to sensible
   enum object_type value from the start of the buffer given as
   its first argument;
 - if it is of type OBJ_DELTA, skip 20-byte base object name;
 - the second argument is the length of the piece -- make sure
   the above steps did not require more than the length;
 - make sure the remainder is sane by running inflate() into
   void.  When fed the remainder in full, inflate() should
   return Z_OK.

For the first step we need to refactor unpack_object_header() in sha1_file.c a tiny bit to reuse it.

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 46f524d..0521cad 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -276,6 +282,7 @@ static unsigned long write_object(struct
 		map = map_sha1_file(entry->sha1, &mapsize);
 		if (map && !legacy_loose_object(map)) {
 			/* We can copy straight into the pack file */
+			revalidate_pack_piece(map, mapsize);
 			sha1write(f, map, mapsize);
 			munmap(map, mapsize);
 			written++;
@@ -319,6 +326,7 @@ static unsigned long write_object(struct
 
 		datalen = find_packed_object_size(p, entry->in_pack_offset);
 		buf = (char *) p->pack_base + entry->in_pack_offset;
+		revalidate_pack_piece(buf, datalen);
 		sha1write(f, buf, datalen);
 		unuse_packed_git(p);
 		hdrlen = 0; /* not really */

← back to recent threads