From: Junio C Hamano <hidden> Date: 2016-06-15 22:43:24
Linus Torvalds [off-list ref] writes:
I'm not saying that's great programming, but the "git_pread()" that git
will use in the absense of a real pread() is actually even *less* of a
POSIX pread, since it doesn't even try to save/restore the old position
(it knows that git doesn't care).
If you mean the offset associated with fd, we actually do.
The original HP-UX error is confusing, as we ask pread() to
transfer 428 bytes and it returns 0 (not returning -1 with
EINTR). Return value of zero is understandable, if the starting
position is at or after the EOF, but the offset is 123601 and
56k objects packed from git.git repository should be longer than
that, so that also sounds implausible.
If you mean the offset associated with fd, we actually do.
Ahh, for some reason I thought we didn't (probably because the user likely
doesn't care at all), but right you are..
The original HP-UX error is confusing, as we ask pread() to
transfer 428 bytes and it returns 0 (not returning -1 with
EINTR). Return value of zero is understandable, if the starting
position is at or after the EOF, but the offset is 123601 and
56k objects packed from git.git repository should be longer than
that, so that also sounds implausible.
Yeah. It is suspicious.
If somebody could run git under gdb on HP-UX (preferably compiled
statically), and just disassemble the pread() thing, it would be
interesting.
PA-RISC assembly is *almost* entirely unreadable, but it might show
whether hpux-11.11 actually has a pread() system call or whether it is
doing the "emulate with lseek" and maybe obviously buggily at that..
It really isn't that complex a system call. So I'm surprised at bugs
there, and that makes me worry that there is something in git that
triggers this.
Linus
Maybe this may help:
I compiled Git under Linux with NO_PREAD=1, and also _disabled_
file position save/restore in git_pread(). Now (I clone small repo to
save traffic):
moonlight:/tmp$ git-clone git://people.freedesktop.org/~keithp/parsecvs
Initialized empty Git repository in /tmp/parsecvs/.git/
remote: Generating pack...
remote: Done counting 476 objects.
remote: Deltifying 476 objects.
remote: 100% (476/476) done
Indexing 476 objects...
remote: Total 476 (delta 339), reused 0 (delta 0)
100% (476/476) done
Resolving 339 deltas...
100% (339/339) done
error: packfile /tmp/parsecvs/.git/objects/pack/pack-dda2f32249fab26059a035bd273dce9feaf6bade.pack does not match index
error: packfile /tmp/parsecvs/.git/objects/pack/pack-dda2f32249fab26059a035bd273dce9feaf6bade.pack cannot be accessed
error: packfile /tmp/parsecvs/.git/objects/pack/pack-dda2f32249fab26059a035bd273dce9feaf6bade.pack does not match index
error: packfile /tmp/parsecvs/.git/objects/pack/pack-dda2f32249fab26059a035bd273dce9feaf6bade.pack cannot be accessed
fatal: failed to unpack tree object HEAD
Errors are different, but seems Git is sensitive to pread() that messes
file position. The problem may be in index-pack.c:
static const char *open_pack_file(const char *pack_name)
{
if (from_stdin) {
...
pack_fd = output_fd;
} else {
...
pack_fd = input_fd;
}
...
}
There's no dup() call, so when we mess pack_fd (that is used in
pread() only), we also mess one more file descriptor that is used
sequentially (output_fd in my case), and so may corrupt the pack.
--
Tomash Brechko
On Fri, Jul 27, 2007 at 13:50:13 +0400, Tomash Brechko wrote:
There's no dup() call, so when we mess pack_fd (that is used in
pread() only), we also mess one more file descriptor that is used
sequentially (output_fd in my case), and so may corrupt the pack.
I was wrong on the dup() part, since dup()'ed descriptors share the
same file position. Anyway, if my guess is right, the fix would
probably be not to use broken pread() that messes file position,
rather than to be ready and workaround that.
--
Tomash Brechko
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:43:24
On Thu, 26 Jul 2007, Linus Torvalds wrote:
On Thu, 26 Jul 2007, Junio C Hamano wrote:
quoted
If you mean the offset associated with fd, we actually do.
Ahh, for some reason I thought we didn't (probably because the user likely
doesn't care at all), but right you are..
The (only) user in Git does care indeed. The sequence of events goes
like this:
- Receive pack from remote location, parsing objects along and writing
a copy to the local pack file, as well as computing the SHA1 of non
delta objects.
- When all objects are parsed, the received pack SHA1 is verified.
If SHA1 doesn't match due to corruption in received data then everything
stops here with a "pack is corrupted (SHA1 mismatch)" error.
- For each received deltas: resolve those deltas to compute their
object SHA1. This is where pread() is involved on the local pack
file. If pread() fails to return proper data then the data won't
inflate properly and everything stops here with a "serious inflate
inconsistency" error.
- [OPTIONAL] Complete a thin pack with missing base objects for deltas
that weren't resolved yet. This involves pread() again, but it
_also_ append data to the same local pack file in the process. In
this case it is important that pread() restores the file position
when it returns or the appended objects won't be written where they
should.
This is optional because a thin pack is not received all the time,
not on a clone for example.
- Write trailing SHA1 to the local pack before moving it to its final
location. This also relies on pread() restoring the file position on
the local pack file or the trailing pack SHA1 won't be written where
it should.
So if pread() doesn't properly restore the file position then local pack
corruption will occur and the pack will be unusable. If pread() doesn't
properly read the asked data then index-pack will die.
It really isn't that complex a system call. So I'm surprised at bugs
there, and that makes me worry that there is something in git that
triggers this.
Well, we have usage cases for a real pread() as well as our own
emulation which work. And the emulated pread() works in all cases so
far, so...
Nicolas