Re: index-pack died on pread

From: Nicolas Pitre <nico@cam.org>
Date: Jul 27, 2007, 13:38 UTC
Message-ID: <alpine.LFD.0.999.0707270913270.3484@xanadu.home>
In-Reply-To: <alpine.LFD.0.999.0707262231280.3442@woody.linux-foundation.org>

On Thu, 26 Jul 2007, Linus Torvalds wrote:

Show 8 quoted lines

> 
> 
> On Thu, 26 Jul 2007, Junio C Hamano wrote:
> > 
> > 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