From: J. Bruce Fields <hidden> Date: 2016-06-15 22:45:13
Today I got this:
fatal: failed to read delta base object at 3025976 from
/home/bfields/local/linux-2.6/.git/objects/pack/pack-f7261d96cf1161b1b0a1593f673a67d0f2469e9b.pack
This has happened once before recently, I believe with a pack that had
just been created on a recent fetch. (If I remember correctly, this was
soon after a failed suspend/resume cycle that might have interrupted an
in-progress fetch; could that possible explain the error?) In that case
I reset origin/master, deleted a tag or two, and fetched, and the
problem seemed to be fixed.
--b.
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:13
On Mon, 25 Aug 2008, J. Bruce Fields wrote:
Today I got this:
fatal: failed to read delta base object at 3025976 from
/home/bfields/local/linux-2.6/.git/objects/pack/pack-f7261d96cf1161b1b0a1593f673a67d0f2469e9b.pack
This has happened once before recently, I believe with a pack that had
just been created on a recent fetch. (If I remember correctly, this was
soon after a failed suspend/resume cycle that might have interrupted an
in-progress fetch; could that possible explain the error?) In that case
I reset origin/master, deleted a tag or two, and fetched, and the
problem seemed to be fixed.
The above error is indicative of a corrupted pack on disk. To confirm
it you could use 'git verify-pack' with the given pack file.
With a sufficiently recent git, you only need to copy over another pack
containing the corrupted object, or the object itself in loose form,
into your object store to "fix" it.
As to the source of disk corruptions... that's up to you to find the
cause amongst many (including a failed suspend).
Nicolas
Today I got this:
fatal: failed to read delta base object at 3025976 from
/home/bfields/local/linux-2.6/.git/objects/pack/pack-f7261d96cf1161b1b0a1593f673a67d0f2469e9b.pack
This is almost certainly due to some corruption. Basically, the call to
"cache_or_unpack_entry()" failed, which in turn is because
'unpack_entry()' will have failed.
And since you didn't see any other error, that failure is almost certainly
due to unpack_compressed_entry() having failed. We don't print out _why_
(which is a bit sad), but the only thing that unpack_compressed_entry()
does is to just "inflate()" the data at that offset.
So it probably got a zlib data error, or an adler32 crc failure.
This has happened once before recently, I believe with a pack that had
just been created on a recent fetch. (If I remember correctly, this was
soon after a failed suspend/resume cycle that might have interrupted an
in-progress fetch; could that possible explain the error?) In that case
I reset origin/master, deleted a tag or two, and fetched, and the
problem seemed to be fixed.
An interrupted fetch shouldn't have caused this, it really should only
happen if you have some actual filesystem data error. Something didn't get
written back correctly, or the page cache isn't coherent (or it got
corrupted by something else like a wild kernel pointer, of course).
Linus
From: J. Bruce Fields <hidden> Date: 2016-06-15 22:45:13
On Mon, Aug 25, 2008 at 02:58:05PM -0400, Nicolas Pitre wrote:
On Mon, 25 Aug 2008, J. Bruce Fields wrote:
quoted
Today I got this:
fatal: failed to read delta base object at 3025976 from
/home/bfields/local/linux-2.6/.git/objects/pack/pack-f7261d96cf1161b1b0a1593f673a67d0f2469e9b.pack
This has happened once before recently, I believe with a pack that had
just been created on a recent fetch. (If I remember correctly, this was
soon after a failed suspend/resume cycle that might have interrupted an
in-progress fetch; could that possible explain the error?) In that case
I reset origin/master, deleted a tag or two, and fetched, and the
problem seemed to be fixed.
The above error is indicative of a corrupted pack on disk. To confirm
it you could use 'git verify-pack' with the given pack file.
That gives:
error: Packfile .git/objects/pack/pack-f7261d96cf1161b1b0a1593f673a67d0f2469e9b.pack SHA1 mismatch with itself
No surprise, I assume.
With a sufficiently recent git, you only need to copy over another pack
containing the corrupted object, or the object itself in loose form,
into your object store to "fix" it.
Yeah, I just did a git repack -a -d in a known good repository and
copied the resulting pack over. Seems OK. Thanks.
--b.
As to the source of disk corruptions... that's up to you to find the
cause amongst many (including a failed suspend).
From: J. Bruce Fields <hidden> Date: 2016-06-15 22:45:13
Thanks to you and Nicolas for the responses.
On Mon, Aug 25, 2008 at 12:01:42PM -0700, Linus Torvalds wrote:
On Mon, 25 Aug 2008, J. Bruce Fields wrote:
quoted
Today I got this:
fatal: failed to read delta base object at 3025976 from
/home/bfields/local/linux-2.6/.git/objects/pack/pack-f7261d96cf1161b1b0a1593f673a67d0f2469e9b.pack
This is almost certainly due to some corruption. Basically, the call to
"cache_or_unpack_entry()" failed, which in turn is because
'unpack_entry()' will have failed.
And since you didn't see any other error, that failure is almost certainly
due to unpack_compressed_entry() having failed. We don't print out _why_
(which is a bit sad), but the only thing that unpack_compressed_entry()
does is to just "inflate()" the data at that offset.
So it probably got a zlib data error, or an adler32 crc failure.
quoted
This has happened once before recently, I believe with a pack that had
just been created on a recent fetch. (If I remember correctly, this was
soon after a failed suspend/resume cycle that might have interrupted an
in-progress fetch; could that possible explain the error?) In that case
I reset origin/master, deleted a tag or two, and fetched, and the
problem seemed to be fixed.
An interrupted fetch shouldn't have caused this, it really should only
happen if you have some actual filesystem data error. Something didn't get
written back correctly, or the page cache isn't coherent (or it got
corrupted by something else like a wild kernel pointer, of course).
OK. I seem to recall these pack files are created with something like
open
write
sync
close
rename
? This is just ext3 with data=writeback on a local laptop disk,
ubuntu's 2.6.24-21-generic. Would it be any use trying to look more
closely at the pack in connection for any hints?
(But with my git repo back I'm happy enough to just forget this for now
if there's not anything obvious to try.)
--b.
OK. I seem to recall these pack files are created with something like
open
write
sync
close
rename
?
Yes. We're trying to be _extremely_ safe and only do things that should
work for everything.
This is just ext3 with data=writeback on a local laptop disk,
ubuntu's 2.6.24-21-generic. Would it be any use trying to look more
closely at the pack in connection for any hints?
You still have the packfile that caused problems available somewhere? If
so, absolutely yes. If you have the corrupt pack, please make it
available.
(But with my git repo back I'm happy enough to just forget this for now
if there's not anything obvious to try.)
With the actual corrupt pack, we can make a fairly intelligent guess about
exactly what the corruption was. Was it a flipped bit, or what? So if you
have it, please do send it over.
Linus
From: J. Bruce Fields <hidden> Date: 2016-06-15 22:45:13
On Mon, Aug 25, 2008 at 02:37:39PM -0700, Linus Torvalds wrote:
On Mon, 25 Aug 2008, J. Bruce Fields wrote:
quoted
OK. I seem to recall these pack files are created with something like
open
write
sync
close
rename
?
Yes. We're trying to be _extremely_ safe and only do things that should
work for everything.
quoted
This is just ext3 with data=writeback on a local laptop disk,
ubuntu's 2.6.24-21-generic. Would it be any use trying to look more
closely at the pack in connection for any hints?
You still have the packfile that caused problems available somewhere? If
so, absolutely yes. If you have the corrupt pack, please make it
available.
quoted
(But with my git repo back I'm happy enough to just forget this for now
if there's not anything obvious to try.)
With the actual corrupt pack, we can make a fairly intelligent guess about
exactly what the corruption was. Was it a flipped bit, or what? So if you
have it, please do send it over.
It makes things slightly easier.
But yes, the file is corrupt. The most obvious corruption is simply the
comparison that git-fsck does with the SHA1 that the pack-file itself
contains: all pack-files have at the end the SHA1 of the contents of the
pack-file, and git fsck says:
error: .git/objects/pack/pack-f7261d96cf1161b1b0a1593f673a67d0f2469e9b.pack SHA1 checksum mismatch
so it's definitely not a valid pack-file, and perhaps more importantly,
since git calculates the SHA1 as it writes the file out, the data really
doesn't match what git wrote, and it got corrupted at some point in
between the generation of the data and the reading back.
(It _could_ still be some git corruption where git itself corrupted it
after it had done the SHA1 writing, but I doubt it).
As to the exact error: zlib reports "invalid distance too far back" as the
error message when unpacking, which doesn't really tell me much. It's just
a common data error. But since I actually _have_ that corrupt object in my
pack too, I can actually look at what it should be.
It's object 784716eb7fc5735a3e6b6209223508984013819f (it's a blob encoding
for arch/arm/mach-pxa/sleep.S from earlier this year), and in my pack-file
it's at byte offset 48094431-48098049.
So I can actually extract just those bytes from my pack, and compare them
against the bytes in your corrupt pack. It's interesting, because the
differences aren't all that big, but they aren't a single word either.
Here's a "diff -u" of "od -t x1" output:
--- good-object.hex 2008-08-25 16:33:41.000000000 -0700
+++ bad-object.hex 2008-08-25 16:33:50.000000000 -0700
@@ -113,16 +113,16 @@
0003400 e1 b1 dc ef 45 fa b1 ec 0f a0 2c 72 1b 25 55 f7
0003420 5a d6 f7 5c 72 dd 8b b2 44 dd 25 e6 c6 33 09 98
0003440 03 9b d1 49 8e 30 e6 7a df 76 76 bb cc c9 fb 44
-0003460 a0 14 8d 7d cd 12 75 81 2b c4 d9 71 27 62 ae b5
-0003500 87 a9 4f 76 c5 90 82 04 2b 15 53 47 ad 97 d1 02
-0003520 98 14 df 45 7c 5f 93 b1 08 fd b6 bf e1 62 1b 00
-0003540 b5 f6 ee 6c 70 30 78 73 74 38 3c 7f f3 e3 e5 90
-0003560 39 7e 2e df e5 e1 c5 c1 29 62 02 fd 61 36 61 7d
-0003600 c0 83 a0 4f 17 04 02 c6 ec 59 2e c1 7d 20 41 b1
-0003620 fd 51 56 d0 b1 74 b9 76 81 4c 47 63 f0 10 76 15
-0003640 db a7 0b b8 d1 6a f8 59 73 8c c4 e5 c9 ab 8b cb
-0003660 37 32 5d 4a 91 c3 c1 c9 77 33 b4 2c b7 61 a0 37
-0003700 47 f7 9b 3d b6 a5 99 5a cf 49 ef e0 3f 54 08 f4
+0003460 a0 14 8d 7d 22 00 00 00 6b 57 fe ff 55 57 fe ff
+0003500 aa 57 fe ff b0 00 0e 00 d9 66 22 00 00 00 00 e0
+0003520 19 9f fe ff ff ff d1 43 fe ff d1 43 fe ff 00 c0
+0003540 bf fe f0 33 69 bf b2 33 69 bf 00 00 00 10 01 fe
+0003560 bf fe 66 01 00 00 01 1f 37 17 2e 59 fe ff 00 00
+0003600 00 00 c2 41 fe ff 00 00 01 1f 37 17 44 42 fe ff
+0003620 00 01 fe ff 01 1f 37 07 d4 42 fe ff 02 01 a8 44
+0003640 00 f8 bf fe 6b 57 fe ff 55 57 fe ff 97 57 fe ff
+0003660 00 f8 bf fe 04 00 fe ff ab 45 fe ff 02 00 fe ff
+0003700 ae 41 49 50 aa 40 fe ff cf 49 ef e0 3f 54 08 f4
0003720 1e 2b 6f 59 e5 47 06 f4 80 a7 76 3a 38 a7 88 0a
0003740 30 72 a2 0a ed e1 b0 d5 6d 6f 9e ca 96 c7 28 d0
0003760 2c 19 b6 db 51 97 a9 f6 c3 31 8a 50 22 87 7a 97
and the difference literally seems to be just a block of less than 160
bytes. But it's certainly not a single-bit error, and it's also not a disk
block or anything like that.
Looking at the first line that differs, they are:
-0003460 a0 14 8d 7d cd 12 75 81 2b c4 d9 71 27 62 ae b5
+0003460 a0 14 8d 7d 22 00 00 00 6b 57 fe ff 55 57 fe ff
so the differences start at offset 03464. The last line is:
-0003700 47 f7 9b 3d b6 a5 99 5a cf 49 ef e0 3f 54 08 f4
+0003700 ae 41 49 50 aa 40 fe ff cf 49 ef e0 3f 54 08 f4
so they re-join at 03708 (octal offsets because of 'od' behavior). But in
between there seems to be nothing in common.
So the corrupt data looks like
22 00 00 00 6b 57 fe ff 55 57 fe ff
0003500 aa 57 fe ff b0 00 0e 00 d9 66 22 00 00 00 00 e0
0003520 19 9f fe ff ff ff d1 43 fe ff d1 43 fe ff 00 c0
0003540 bf fe f0 33 69 bf b2 33 69 bf 00 00 00 10 01 fe
0003560 bf fe 66 01 00 00 01 1f 37 17 2e 59 fe ff 00 00
0003600 00 00 c2 41 fe ff 00 00 01 1f 37 17 44 42 fe ff
0003620 00 01 fe ff 01 1f 37 07 d4 42 fe ff 02 01 a8 44
0003640 00 f8 bf fe 6b 57 fe ff 55 57 fe ff 97 57 fe ff
0003660 00 f8 bf fe 04 00 fe ff ab 45 fe ff 02 00 fe ff
0003700 ae 41 49 50 aa 40 fe ff
and I don't see what the patern is, except that there's a lot of "fe ff"
in there. 148 bytes, no obvious source. It definitely does _not_ look like
compressed input (it's too regular to be something zlib spits out), nor is
it text. Nor is it valid x86 assembly, so it's not some code-segment data
either.
And as far as I can tell, that's the _only_ corruption in the whole file,
but I didn't really double-check.
Does anybody see a pattern?
Linus
From: Jason McMullan <hidden> Date: 2016-06-15 22:45:14
Linus Torvalds wrote:
So the corrupt data looks like
[snip]
And as far as I can tell, that's the _only_ corruption in the whole file,
but I didn't really double-check.
Does anybody see a pattern?
Was this pack created on a journaled file system? Reiserfs? Ext3?
If there's journal corruption in a commonly used filesystem,
That Would Be Bad.
I would suspect the Reiserfs 'file tail' behavour and journalling
have something to do with it, only because I still use and like
ReiserV3+LVM (dynamic grow and offline shrink baby!).
Only something like silently knifing files in the back would cause me
to leave my beloved RedrumFS.
Which I probably should. Eventually. Once ZFS changes it's license. HA!
- Jason McMullan
From: J. Bruce Fields <hidden> Date: 2016-06-15 22:45:14
On Mon, Aug 25, 2008 at 04:59:26PM -0700, Linus Torvalds wrote:
and the difference literally seems to be just a block of less than 160
bytes. But it's certainly not a single-bit error, and it's also not a disk
block or anything like that.
Looking at the first line that differs, they are:
-0003460 a0 14 8d 7d cd 12 75 81 2b c4 d9 71 27 62 ae b5
+0003460 a0 14 8d 7d 22 00 00 00 6b 57 fe ff 55 57 fe ff
so the differences start at offset 03464. The last line is:
-0003700 47 f7 9b 3d b6 a5 99 5a cf 49 ef e0 3f 54 08 f4
+0003700 ae 41 49 50 aa 40 fe ff cf 49 ef e0 3f 54 08 f4
so they re-join at 03708 (octal offsets because of 'od' behavior). But in
between there seems to be nothing in common.
So the corrupt data looks like
22 00 00 00 6b 57 fe ff 55 57 fe ff
0003500 aa 57 fe ff b0 00 0e 00 d9 66 22 00 00 00 00 e0
0003520 19 9f fe ff ff ff d1 43 fe ff d1 43 fe ff 00 c0
0003540 bf fe f0 33 69 bf b2 33 69 bf 00 00 00 10 01 fe
0003560 bf fe 66 01 00 00 01 1f 37 17 2e 59 fe ff 00 00
0003600 00 00 c2 41 fe ff 00 00 01 1f 37 17 44 42 fe ff
0003620 00 01 fe ff 01 1f 37 07 d4 42 fe ff 02 01 a8 44
0003640 00 f8 bf fe 6b 57 fe ff 55 57 fe ff 97 57 fe ff
0003660 00 f8 bf fe 04 00 fe ff ab 45 fe ff 02 00 fe ff
0003700 ae 41 49 50 aa 40 fe ff
and I don't see what the patern is, except that there's a lot of "fe ff"
in there.
and all aligned on 2-byte boundaries? I don't see anything suggestive
there either.
148 bytes, no obvious source. It definitely does _not_ look like
compressed input (it's too regular to be something zlib spits out), nor is
it text. Nor is it valid x86 assembly, so it's not some code-segment data
either.
And as far as I can tell, that's the _only_ corruption in the whole file,
but I didn't really double-check.
Does anybody see a pattern?
From: Jason McMullan <hidden> Date: 2016-06-15 22:45:14
Jason McMullan wrote:
Was this pack created on a journaled file system? Reiserfs? Ext3?
If there's journal corruption in a commonly used filesystem,
That Would Be Bad.
In private mail, it was indicated that this was an ext3 with the
data=writeback option. From mount(1), man page, ext3 section:
writeback
Data ordering is not preserved - data may be written into
the main file system after its metadata has been commit‐
ted to the journal. This is rumoured to be the highest-
throughput option. It guarantees internal file system
integrity, however it can allow old data to appear in
files after a crash and journal recovery.
All bets are off when data=writeback.
- Jason McMullan
Not the way git writes pack-files. It does a fsync() before moving them
into place (at least newer git versions do), so the data is stable.
I do worry about wild pointers. I can't recognize the data, and it
definitely doesn't look like any git internal data structures, but 16-bit
data _is_ what zlib internally uses for things like the decoding tables.
So if there is some use-after-free issue, I could imagine things like this
happening inside of git. People do occasionally run valgrind on git,
though, and it's been clean in the past, but I don't know if that has ever
been done on the threaded packing, for example.
For example, the corrupting data had patterns like this:
00 f8 bf fe 6b 57 fe ff 55 57 fe ff 97 57 fe ff
where the pattern _could_ be something like
{ 00 f8 febf },
{ 6b 57 fffe },
{ 55 57 fffe },
{ 97 57 fffe },
assuming that the "fe ff" pattern really is meaningful and is a 16-bit
little-endian word.
And the thign is, zlib "code" tables look exactly like that:
typedef struct {
unsigned char op; /* operation, extra bits, table bits */
unsigned char bits; /* bits in this part of the code */
unsigned short val; /* offset in table or code value */
} code;
/* op values as set by inflate_table():
00000000 - literal
0000tttt - table link, tttt != 0 is the number of table index bits
0001eeee - length or distance, eeee is the number of extra bits
01100000 - end of block
01000000 - invalid code
*/
but those particular op/val things don't make sense in that context
either. But I don't know zlib that well, maybe the deflate routines use
some other model.
Linus
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:14
On Wed, 27 Aug 2008, Linus Torvalds wrote:
On Tue, 26 Aug 2008, Jason McMullan wrote:
quoted
All bets are off when data=writeback.
Not the way git writes pack-files. It does a fsync() before moving them
into place (at least newer git versions do), so the data is stable.
And isn't the bad data block size and alignment a bit odd for a
filesystem crash corruption?
I do worry about wild pointers. I can't recognize the data, and it
definitely doesn't look like any git internal data structures, but 16-bit
data _is_ what zlib internally uses for things like the decoding tables.
So if there is some use-after-free issue, I could imagine things like this
happening inside of git. People do occasionally run valgrind on git,
though, and it's been clean in the past, but I don't know if that has ever
been done on the threaded packing, for example.
However, in the pack-objects case, it is almost impossible to have such
a corruption since the data is SHA1 summed immediately before being
written out. In the index-pack case, it is even less likely since the
data is SHA1 summed immediately after being written out. So there is no
window for random pointer access corrupting the data without also
influencing the pack checksum outcome. Therefore, if the pack content
doesn't match its checksum, then the corruption must have occurred
outside of git, otherwise the checksum would have included corrupted
data already and the pack checksum would match.
Nicolas
And isn't the bad data block size and alignment a bit odd for a
filesystem crash corruption?
Yes. If it was a filesystem issue, I'd expect it to be at least disk block
aligned (512 bytes, most of the time) and more likely filesystem block
aligned (ie mostly 4kB).
However, if we were to re-write the file afterwards, it could still get
non-block-aligned corruption - simply because there was a
non-block-aligned rewrite that got lost. But we don't actually ever do
that, except for the header and the SHA1 at the end in some unusual cases.
However, in the pack-objects case, it is almost impossible to have such
a corruption since the data is SHA1 summed immediately before being
written out.
Yes. Anything that uses the "sha1write()" model (which includes the
regular pack-file _and_ the index) should generally be pretty safe.
However, we do have this odd case of fixing up the pack after-the-fact
when we receive it from somebody else (because we get a thin pack and
don't know how many objects the final result will have). And that case
seems to be not as safe, because it
- re-reads the file to recompute the SHA1
This is understandable, and it's fairly ok, but it does mean that there
is a bigger chance of the SHA1 matching if something has corrupted the
file in the meantime!
(That was not the case of this corruption, obviously, since the SHA1
didn't match)
- but it also forgets to fsync the result, because it only did that in
one path rather in all cases of fixup.
Again, this wasn't actually the cause of this corruption, because the
corruption wasn't near the header or tail, so if it had been due to a
missed write due to missing an fsync, the pattern would have been
different.
Anyway, we should fix the latter problem regardless, even if it's (a) damn
unlikely and (b) definietly not the case in this thing.
The fix is trivial - just move the "fsync_or_die()" into the fixup routine
rather than doing it in one of the callers.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Linus
---
builtin-pack-objects.c | 1 -
pack-write.c | 1 +
2 files changed, 1 insertions(+), 1 deletions(-)
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:14
On Wed, 27 Aug 2008, Linus Torvalds wrote:
On Wed, 27 Aug 2008, Nicolas Pitre wrote:
quoted
However, in the pack-objects case, it is almost impossible to have such
a corruption since the data is SHA1 summed immediately before being
written out.
Yes. Anything that uses the "sha1write()" model (which includes the
regular pack-file _and_ the index) should generally be pretty safe.
What that means is that if git was the cause of the corruption itself
then the pack would still match its checksum (verify-pâck would still
fail nevertheless).
However, we do have this odd case of fixing up the pack after-the-fact
when we receive it from somebody else (because we get a thin pack and
don't know how many objects the final result will have). And that case
seems to be not as safe, because it
- re-reads the file to recompute the SHA1
This is understandable, and it's fairly ok, but it does mean that there
is a bigger chance of the SHA1 matching if something has corrupted the
file in the meantime!
I think that can be fixed. When reading the file back, it is possible
to compute 2 sha1s: one to compare with the recieved one using original
pack header, and the second which would be the final one. FRom a
certain offset, new objects were added, so that first sha1 is validated
against the received one and reset, and at the end, it should correspond
to the sha1 of added objects that we should compute when writing them.
Nicolas
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:15
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.
Signed-off-by: Nicolas Pitre <redacted>
---
builtin-pack-objects.c | 4 ++--
csum-file.c | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
@@ -42,11 +42,11 @@ int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags)sha1flush(f,offset);f->offset=0;}+SHA1_Final(f->buffer,&f->ctx);+if(result)+hashcpy(result,f->buffer);if(flags&(CSUM_CLOSE|CSUM_FSYNC)){/* write checksum and close fd */-SHA1_Final(f->buffer,&f->ctx);-if(result)-hashcpy(result,f->buffer);sha1flush(f,20);if(flags&CSUM_FSYNC)fsync_or_die(f->fd,f->name);
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:15
On Wed, 27 Aug 2008, Nicolas Pitre wrote:
On Wed, 27 Aug 2008, Linus Torvalds wrote:
quoted
However, we do have this odd case of fixing up the pack after-the-fact
when we receive it from somebody else (because we get a thin pack and
don't know how many objects the final result will have). And that case
seems to be not as safe, because it
- re-reads the file to recompute the SHA1
This is understandable, and it's fairly ok, but it does mean that there
is a bigger chance of the SHA1 matching if something has corrupted the
file in the meantime!
I think that can be fixed. When reading the file back, it is possible
to compute 2 sha1s: one to compare with the recieved one using original
pack header, and the second which would be the final one. FRom a
certain offset, new objects were added, so that first sha1 is validated
against the received one and reset, and at the end, it should correspond
to the sha1 of added objects that we should compute when writing them.
So here is 3 patches for this in C git (Shawn already did it for jgit).
There are 3 spots where this needs to be plugged: in pack-objects,
index-pack and fast-import. I did the first two.
I don't think this is that pertinent in fast-import because:
- we want fast-import to be fast
- after fast-import is done, a full repack is typically done which
would revalidate everything (maybe fast-import should use index v2
with its per object CRC32 though)
But maybe I'm wrong about that. In any case, I am leaving the
fast-import to someone more knowledgeable about its code than I do.
Nicolas Pitre (3):
improve reliability of fixup_pack_header_footer()
pack-objects: use fixup_pack_header_footer()'s validation mode
index-pack: use fixup_pack_header_footer()'s validation mode
Nicolas
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:15
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.
Signed-off-by: Nicolas Pitre <redacted>
---
builtin-pack-objects.c | 3 +-
fast-import.c | 3 +-
index-pack.c | 3 +-
pack-write.c | 78 ++++++++++++++++++++++++++++++++++++++++-------
pack.h | 2 +-
5 files changed, 73 insertions(+), 16 deletions(-)
@@ -144,41 +144,95 @@ char *write_idx_file(char *index_name, struct pack_idx_entry **objects,returnindex_name;}+/*+*Updatepackheaderwithobject_countandcomputenewSHA1forpackdata+*associatedtopack_fd,andwritethatSHA1attheend.ThatnewSHA1+*isalsoreturnedinnew_pack_sha1.+*+*Ifpartial_pack_sha1isnonnull,thentheSHA1oftheexistingpack+*(withouttheheaderupdate)iscomputedandvalidatedagainsttheone+*providedinpartial_pack_sha1.Thevalidationisperformedat+*partial_pack_offsetbytesinthepackfile,orattheendofthepack+*fileifpartial_pack_offsetiszero.Also,whenpartial_pack_offsetis+*nonzero,theSHA1oftheremainingdata(i.e.frompartial_pack_offset+*totheend)isreturnedinpartial_pack_sha1.+*+*Notethatnew_pack_sha1isupdatedlast,sobothnew_pack_sha1and+*partial_pack_sha1canrefertothesamebufferifthecallerisnot+*interestedintheresultingSHA1ofpackdataabovepartial_pack_offset.+*/voidfixup_pack_header_footer(intpack_fd,-unsignedchar*pack_file_sha1,+unsignedchar*new_pack_sha1,constchar*pack_name,-uint32_tobject_count)+uint32_tobject_count,+unsignedchar*partial_pack_sha1,+off_tpartial_pack_offset){staticconstintbuf_sz=128*1024;-SHA_CTXc;+SHA_CTXold_sha1_ctx,new_sha1_ctx;structpack_headerhdr;char*buf;+SHA1_Init(&old_sha1_ctx);+SHA1_Init(&new_sha1_ctx);++if(partial_pack_sha1&&!partial_pack_offset){+partial_pack_offset=lseek(pack_fd,0,SEEK_CUR);+if(partial_pack_offset==(off_t)-1)+die("Can't get size of %s: %s",pack_name,strerror(errno));+}+if(lseek(pack_fd,0,SEEK_SET)!=0)-die("Failed seeking to start: %s",strerror(errno));+die("Failed seeking to start of %s: %s",pack_name,strerror(errno));if(read_in_full(pack_fd,&hdr,sizeof(hdr))!=sizeof(hdr))die("Unable to reread header of %s: %s",pack_name,strerror(errno));if(lseek(pack_fd,0,SEEK_SET)!=0)-die("Failed seeking to start: %s",strerror(errno));+die("Failed seeking to start of %s: %s",pack_name,strerror(errno));+SHA1_Update(&old_sha1_ctx,&hdr,sizeof(hdr));hdr.hdr_entries=htonl(object_count);+SHA1_Update(&new_sha1_ctx,&hdr,sizeof(hdr));write_or_die(pack_fd,&hdr,sizeof(hdr));--SHA1_Init(&c);-SHA1_Update(&c,&hdr,sizeof(hdr));+partial_pack_offset-=sizeof(hdr);buf=xmalloc(buf_sz);for(;;){-ssize_tn=xread(pack_fd,buf,buf_sz);+ssize_tm,n;+m=(partial_pack_sha1&&partial_pack_offset<buf_sz)?+partial_pack_offset:buf_sz;+n=xread(pack_fd,buf,m);if(!n)break;if(n<0)die("Failed to checksum %s: %s",pack_name,strerror(errno));-SHA1_Update(&c,buf,n);+SHA1_Update(&new_sha1_ctx,buf,n);++if(!partial_pack_sha1)+continue;++SHA1_Update(&old_sha1_ctx,buf,n);+partial_pack_offset-=n;+if(partial_pack_offset==0){+unsignedcharsha1[20];+SHA1_Final(sha1,&old_sha1_ctx);+if(hashcmp(sha1,partial_pack_sha1)!=0)+die("Unexpected checksum for %s "+"(disk corruption?)",pack_name);+/*+*Nowlet'scomputetheSHA1oftheremainderofthe+*pack,whichalsomeansmakingpartial_pack_offset+*bigenoughnottomatteranymore.+*/+SHA1_Init(&old_sha1_ctx);+partial_pack_offset=~partial_pack_offset;+partial_pack_offset-=MSB(partial_pack_offset,1);+}}free(buf);-SHA1_Final(pack_file_sha1,&c);-write_or_die(pack_fd,pack_file_sha1,20);+if(partial_pack_sha1)+SHA1_Final(partial_pack_sha1,&old_sha1_ctx);+SHA1_Final(new_pack_sha1,&new_sha1_ctx);+write_or_die(pack_fd,new_pack_sha1,20);fsync_or_die(pack_fd,pack_name);}
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:15
When completing a thin pack, 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 for both:
the original pack and the appended objects.
To do so, a couple write_or_die() calls were converted to sha1write()
which has the advantage of doing some buffering as well as handling
SHA1 and CRC32 checksum already.
Signed-off-by: Nicolas Pitre <redacted>
---
index-pack.c | 43 +++++++++++++++++++++++++------------------
1 files changed, 25 insertions(+), 18 deletions(-)
@@ -875,7 +874,7 @@ int main(int argc, char **argv)constchar*keep_name=NULL,*keep_msg=NULL;char*index_name_buf=NULL,*keep_name_buf=NULL;structpack_idx_entry**idx_objects;-unsignedcharsha1[20];+unsignedcharpack_sha1[20];intnongit=0;setup_git_directory_gently(&nongit);
@@ -962,13 +961,15 @@ int main(int argc, char **argv)parse_pack_header();objects=xmalloc((nr_objects+1)*sizeof(structobject_entry));deltas=xmalloc(nr_objects*sizeof(structdelta_entry));-parse_pack_objects(sha1);+parse_pack_objects(pack_sha1);if(nr_deltas==nr_resolved_deltas){stop_progress(&progress);/* Flush remaining pack final 20-byte SHA1. */flush();}else{if(fix_thin_pack){+structsha1file*f;+unsignedcharread_sha1[20],tail_sha1[20];charmsg[48];intnr_unresolved=nr_deltas-nr_resolved_deltas;intnr_objects_initial=nr_objects;
@@ -977,13 +978,19 @@ int main(int argc, char **argv)objects=xrealloc(objects,(nr_objects+nr_unresolved+1)*sizeof(*objects));-fix_unresolved_deltas(nr_unresolved);+f=sha1fd(output_fd,curr_pack);+fix_unresolved_deltas(f,nr_unresolved);sprintf(msg,"completed with %d local objects",nr_objects-nr_objects_initial);stop_progress_msg(&progress,msg);-fixup_pack_header_footer(output_fd,sha1,+sha1close(f,tail_sha1,0);+hashcpy(read_sha1,pack_sha1);+fixup_pack_header_footer(output_fd,pack_sha1,curr_pack,nr_objects,-NULL,0);+read_sha1,consumed_bytes-20);+if(hashcmp(read_sha1,tail_sha1)!=0)+die("Unexpected tail checksum for %s "+"(disk corruption?)",curr_pack);}if(nr_deltas!=nr_resolved_deltas)die("pack has %d unresolved deltas",
@@ -996,13 +1003,13 @@ int main(int argc, char **argv)idx_objects=xmalloc((nr_objects)*sizeof(structpack_idx_entry*));for(i=0;i<nr_objects;i++)idx_objects[i]=&objects[i].idx;-curr_index=write_idx_file(index_name,idx_objects,nr_objects,sha1);+curr_index=write_idx_file(index_name,idx_objects,nr_objects,pack_sha1);free(idx_objects);final(pack_name,curr_pack,index_name,curr_index,keep_name,keep_msg,-sha1);+pack_sha1);free(objects);free(index_name_buf);free(keep_name_buf);
From: Shawn O. Pearce <hidden> Date: 2016-06-15 22:45:15
Nicolas Pitre [off-list ref] 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.
From: Shawn O. Pearce <hidden> Date: 2016-06-15 22:45:15
Nicolas Pitre [off-list ref] wrote:
So here is 3 patches for this in C git (Shawn already did it for jgit).
There are 3 spots where this needs to be plugged: in pack-objects,
index-pack and fast-import. I did the first two.
I don't think this is that pertinent in fast-import because:
- we want fast-import to be fast
- after fast-import is done, a full repack is typically done which
would revalidate everything (maybe fast-import should use index v2
with its per object CRC32 though)
But maybe I'm wrong about that. In any case, I am leaving the
fast-import to someone more knowledgeable about its code than I do.
fast-import is the backend to a number of incremental import tools.
They stream data in from foreign VCS systems (notably Perforce)
and write into a live repository. That repository may not get
repacked for days/weeks. We should treat it just like index-pack.
So I think we should plug it. The cost to keep a running SHA-1
for what we have written is pretty low. I'd rather trade off a
minute or so on an hour long import for data safety than have a
chance for data corruption going unnoticed by the end user.
And now that index v2 is available, yea, fast-import should write
its index in that format so that a later repack can safely delta
reuse if it (by rare chance) decides the current packed form is the
best representation. This is especially true with "git gc --auto"
triggering a repack every so often due to the incremental import
packs I just metioned above.
I've just been too busy with other things to add index v2 support.
--
Shawn.
From: Shawn O. Pearce <hidden> Date: 2016-06-15 22:45:15
Nicolas Pitre [off-list ref] wrote:
On Thu, 28 Aug 2008, Shawn O. Pearce wrote:
quoted
I found your implementation in fixup_pack_header to be very
contorted. Did you read the JGit patch I posted?
Well, actually, I don't find the JGit implementation easier to follow at
all. Of course I wrote the C version while you wrote the Java version.
Maybe without our respective bias then things are just fine in both
cases.
I probably should have gone and edited my eariler comments after I
reached the end of the patch and the light dawned about why you are
summing the tail. Most of what I found to be complex in your code
was just to handle that boundary condition at partial_pack_offset
and restart the checksum for the tail. But I honestly cannot see
an easier way to write that, so what you have is just fine.
quoted
quoted
void fixup_pack_header_footer(int pack_fd,
...
quoted
+ 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.
This is not for thin packs but for split packs in repack-objects. And
yeah, the caller has the offset information already in that case too, so
this could be removed. It just felt more generic that way originally.
Oh, yea, that makes sense. It still seems like playing with fire.
I'd rather the caller pass in the proper offset than rely on it
being the current position of the fd. Especially if the caller
does actually have it available.
If you change anything, I'd like to see this lseek(SEEK_CUR) go away.
And another thing I had in store (but for which you _again_ beat me to :-) )
is to realign data reads onto filesystem blocks.
That _really_ made the JGit code ugly. But I think its worth it.
I also want to try and buffer the whole object appending we do
during fixThinPack(), as right now we write the object header in
one write and then compressed data bursts in the others. Moving it
to at least write a full 4k at a time should remove about 2 write
calls per object.
quoted
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.
Again maybe that's just because you're just too biased by your own
implementation. I don't consider this code particularly obscur.
My own code in JGit got "obscure" when I added this check too.
I take back the remark above.
--
Shawn.
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:15
On Thu, 28 Aug 2008, Shawn O. Pearce wrote:
Nicolas Pitre [off-list ref] wrote:
quoted
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. ;-)
Well, actually, I don't find the JGit implementation easier to follow at
all. Of course I wrote the C version while you wrote the Java version.
Maybe without our respective bias then things are just fine in both
cases.
quoted
void fixup_pack_header_footer(int pack_fd,
...
quoted
+ 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?
This is not for thin packs but for split packs in repack-objects. And
yeah, the caller has the offset information already in that case too, so
this could be removed. It just felt more generic that way originally.
quoted
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.
I doubt it. That would only move the offset test from before the read
to after it, and actually complicate things for restarting the
checksumming after the special offset was crossed. And we're talking
about a comparison with a conditional move instructions only.
And another thing I had in store (but for which you _again_ beat me to :-) )
is to realign data reads onto filesystem blocks.
quoted
+ 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.
Again maybe that's just because you're just too biased by your own
implementation. I don't consider this code particularly obscur.
Nicolas
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:15
This replaces my earlier 3-patch series called "don't let disk
corruptions escape pack SHA1 checksum" with this set. Same purpose
but better!
Once again I'm leaving fast-import out. I'm sure Shawn will figure
it out much faster than I could.
Nicolas
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:15
This function returns 0 when the current object couldn't be written
due to the pack size limit, otherwise the current offset in the pack.
There is a problem with this approach however, since current object
could be a delta and its delta base might just have been written in
the same write_one() call, but those successfully written objects are
not accounted in the offset variable tracked by the caller. Currently
this is not an issue but a subsequent patch will need this.
Signed-off-by: Nicolas Pitre <redacted>
---
builtin-pack-objects.c | 29 ++++++++++++-----------------
1 files changed, 12 insertions(+), 17 deletions(-)
@@ -410,25 +410,22 @@ static unsigned long write_object(struct sha1file *f,returnhdrlen+datalen;}-staticoff_twrite_one(structsha1file*f,+staticintwrite_one(structsha1file*f,structobject_entry*e,-off_toffset)+off_t*offset){unsignedlongsize;/* offset is non zero if object is written already. */if(e->idx.offset||e->preferred_base)-returnoffset;+return1;/* if we are deltified, write out base object first. */-if(e->delta){-offset=write_one(f,e->delta,offset);-if(!offset)-return0;-}+if(e->delta&&!write_one(f,e->delta,offset))+return0;-e->idx.offset=offset;-size=write_object(f,e,offset);+e->idx.offset=*offset;+size=write_object(f,e,*offset);if(!size){e->idx.offset=0;return0;
@@ -436,9 +433,10 @@ static off_t write_one(struct sha1file *f,written_list[nr_written++]=&e->idx;/* make sure off_t is sufficiently large not to wrap */-if(offset>offset+size)+if(*offset>*offset+size)die("pack too large for current definition of off_t");-returnoffset+size;+*offset+=size;+return1;}/* forward declaration for write_pack_file */
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:15
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.
Signed-off-by: Nicolas Pitre <redacted>
---
builtin-pack-objects.c | 3 +-
fast-import.c | 3 +-
index-pack.c | 3 +-
pack-write.c | 71 +++++++++++++++++++++++++++++++++++++++--------
pack.h | 2 +-
5 files changed, 66 insertions(+), 16 deletions(-)
@@ -144,41 +144,88 @@ char *write_idx_file(char *index_name, struct pack_idx_entry **objects,returnindex_name;}+/*+*Updatepackheaderwithobject_countandcomputenewSHA1forpackdata+*associatedtopack_fd,andwritethatSHA1attheend.ThatnewSHA1+*isalsoreturnedinnew_pack_sha1.+*+*Ifpartial_pack_sha1isnonnull,thentheSHA1oftheexistingpack+*(withouttheheaderupdate)iscomputedandvalidatedagainstthe+*oneprovidedinpartial_pack_sha1.Thevalidationisperformedat+*partial_pack_offsetbytesinthepackfile.TheSHA1oftheremaining+*data(i.e.frompartial_pack_offsettotheend)isthencomputedand+*returnedinpartial_pack_sha1.+*+*Notethatnew_pack_sha1isupdatedlast,sobothnew_pack_sha1and+*partial_pack_sha1canrefertothesamebufferifthecallerisnot+*interestedintheresultingSHA1ofpackdataabovepartial_pack_offset.+*/voidfixup_pack_header_footer(intpack_fd,-unsignedchar*pack_file_sha1,+unsignedchar*new_pack_sha1,constchar*pack_name,-uint32_tobject_count)+uint32_tobject_count,+unsignedchar*partial_pack_sha1,+off_tpartial_pack_offset){staticconstintbuf_sz=128*1024;-SHA_CTXc;+SHA_CTXold_sha1_ctx,new_sha1_ctx;structpack_headerhdr;char*buf;+SHA1_Init(&old_sha1_ctx);+SHA1_Init(&new_sha1_ctx);+if(lseek(pack_fd,0,SEEK_SET)!=0)-die("Failed seeking to start: %s",strerror(errno));+die("Failed seeking to start of %s: %s",pack_name,strerror(errno));if(read_in_full(pack_fd,&hdr,sizeof(hdr))!=sizeof(hdr))die("Unable to reread header of %s: %s",pack_name,strerror(errno));if(lseek(pack_fd,0,SEEK_SET)!=0)-die("Failed seeking to start: %s",strerror(errno));+die("Failed seeking to start of %s: %s",pack_name,strerror(errno));+SHA1_Update(&old_sha1_ctx,&hdr,sizeof(hdr));hdr.hdr_entries=htonl(object_count);+SHA1_Update(&new_sha1_ctx,&hdr,sizeof(hdr));write_or_die(pack_fd,&hdr,sizeof(hdr));--SHA1_Init(&c);-SHA1_Update(&c,&hdr,sizeof(hdr));+partial_pack_offset-=sizeof(hdr);buf=xmalloc(buf_sz);for(;;){-ssize_tn=xread(pack_fd,buf,buf_sz);+ssize_tm,n;+m=(partial_pack_sha1&&partial_pack_offset<buf_sz)?+partial_pack_offset:buf_sz;+n=xread(pack_fd,buf,m);if(!n)break;if(n<0)die("Failed to checksum %s: %s",pack_name,strerror(errno));-SHA1_Update(&c,buf,n);+SHA1_Update(&new_sha1_ctx,buf,n);++if(!partial_pack_sha1)+continue;++SHA1_Update(&old_sha1_ctx,buf,n);+partial_pack_offset-=n;+if(partial_pack_offset==0){+unsignedcharsha1[20];+SHA1_Final(sha1,&old_sha1_ctx);+if(hashcmp(sha1,partial_pack_sha1)!=0)+die("Unexpected checksum for %s "+"(disk corruption?)",pack_name);+/*+*Nowlet'scomputetheSHA1oftheremainderofthe+*pack,whichalsomeansmakingpartial_pack_offset+*bigenoughnottomatteranymore.+*/+SHA1_Init(&old_sha1_ctx);+partial_pack_offset=~partial_pack_offset;+partial_pack_offset-=MSB(partial_pack_offset,1);+}}free(buf);-SHA1_Final(pack_file_sha1,&c);-write_or_die(pack_fd,pack_file_sha1,20);+if(partial_pack_sha1)+SHA1_Final(partial_pack_sha1,&old_sha1_ctx);+SHA1_Final(new_pack_sha1,&new_sha1_ctx);+write_or_die(pack_fd,new_pack_sha1,20);fsync_or_die(pack_fd,pack_name);}
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:15
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.
Signed-off-by: Nicolas Pitre <redacted>
---
builtin-pack-objects.c | 4 ++--
csum-file.c | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
@@ -42,11 +42,11 @@ int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags)sha1flush(f,offset);f->offset=0;}+SHA1_Final(f->buffer,&f->ctx);+if(result)+hashcpy(result,f->buffer);if(flags&(CSUM_CLOSE|CSUM_FSYNC)){/* write checksum and close fd */-SHA1_Final(f->buffer,&f->ctx);-if(result)-hashcpy(result,f->buffer);sha1flush(f,20);if(flags&CSUM_FSYNC)fsync_or_die(f->fd,f->name);
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:15
When completing a thin pack, 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 for both:
the original pack and the appended objects.
To do so, a couple write_or_die() calls were converted to sha1write()
which has the advantage of doing some buffering as well as handling
SHA1 and CRC32 checksum already.
Signed-off-by: Nicolas Pitre <redacted>
---
index-pack.c | 43 +++++++++++++++++++++++++------------------
1 files changed, 25 insertions(+), 18 deletions(-)
@@ -875,7 +874,7 @@ int main(int argc, char **argv)constchar*keep_name=NULL,*keep_msg=NULL;char*index_name_buf=NULL,*keep_name_buf=NULL;structpack_idx_entry**idx_objects;-unsignedcharsha1[20];+unsignedcharpack_sha1[20];intnongit=0;setup_git_directory_gently(&nongit);
@@ -962,13 +961,15 @@ int main(int argc, char **argv)parse_pack_header();objects=xmalloc((nr_objects+1)*sizeof(structobject_entry));deltas=xmalloc(nr_objects*sizeof(structdelta_entry));-parse_pack_objects(sha1);+parse_pack_objects(pack_sha1);if(nr_deltas==nr_resolved_deltas){stop_progress(&progress);/* Flush remaining pack final 20-byte SHA1. */flush();}else{if(fix_thin_pack){+structsha1file*f;+unsignedcharread_sha1[20],tail_sha1[20];charmsg[48];intnr_unresolved=nr_deltas-nr_resolved_deltas;intnr_objects_initial=nr_objects;
@@ -977,13 +978,19 @@ int main(int argc, char **argv)objects=xrealloc(objects,(nr_objects+nr_unresolved+1)*sizeof(*objects));-fix_unresolved_deltas(nr_unresolved);+f=sha1fd(output_fd,curr_pack);+fix_unresolved_deltas(f,nr_unresolved);sprintf(msg,"completed with %d local objects",nr_objects-nr_objects_initial);stop_progress_msg(&progress,msg);-fixup_pack_header_footer(output_fd,sha1,+sha1close(f,tail_sha1,0);+hashcpy(read_sha1,pack_sha1);+fixup_pack_header_footer(output_fd,pack_sha1,curr_pack,nr_objects,-NULL,0);+read_sha1,consumed_bytes-20);+if(hashcmp(read_sha1,tail_sha1)!=0)+die("Unexpected tail checksum for %s "+"(disk corruption?)",curr_pack);}if(nr_deltas!=nr_resolved_deltas)die("pack has %d unresolved deltas",
@@ -996,13 +1003,13 @@ int main(int argc, char **argv)idx_objects=xmalloc((nr_objects)*sizeof(structpack_idx_entry*));for(i=0;i<nr_objects;i++)idx_objects[i]=&objects[i].idx;-curr_index=write_idx_file(index_name,idx_objects,nr_objects,sha1);+curr_index=write_idx_file(index_name,idx_objects,nr_objects,pack_sha1);free(idx_objects);final(pack_name,curr_pack,index_name,curr_index,keep_name,keep_msg,-sha1);+pack_sha1);free(objects);free(index_name_buf);free(keep_name_buf);
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:15
It should be more efficient to use nicely aligned buffer sizes, either
for filesystem operations or SHA1 checksums. Also, using a relatively
small nominal size might allow for the data to remain in L1 cache
between both SHA1_Update() calls.
Signed-off-by: Nicolas Pitre <redacted>
---
pack-write.c | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:15
On Fri, 29 Aug 2008, Shawn O. Pearce wrote:
Oh, yea, that makes sense. It still seems like playing with fire.
I'd rather the caller pass in the proper offset than rely on it
being the current position of the fd. Especially if the caller
does actually have it available.
If you change anything, I'd like to see this lseek(SEEK_CUR) go away.
Done. I struggled a bit since it simply didn't work initially -- see
the first patch in the updated series.
quoted
And another thing I had in store (but for which you _again_ beat me to :-) )
is to realign data reads onto filesystem blocks.
That _really_ made the JGit code ugly. But I think its worth it.
See my version. I think it is reasonably clear.
I also want to try and buffer the whole object appending we do
during fixThinPack(), as right now we write the object header in
one write and then compressed data bursts in the others. Moving it
to at least write a full 4k at a time should remove about 2 write
calls per object.
Yep. In the C git case, moving to sha1write() added that buffering
for free.
Nicolas