patchFix in Git.pm cat_blob crashes on large files

Greetings. This is my first patch here. Hopefully I get the stylistic & political details right... :) Patch applies against maint and master

(If I understand the mechanics, in theory a negative offset should work,
 if the values lined up just right, but would be very wrong, overwriting the
lower contents of the file)

        Developer's Certificate of Origin 1.1

        By making a contribution to this project, I certify that:

        (a) The contribution was created in whole or in part by me and I
            have the right to submit it under the open source license
            indicated in the file; or

        (b) The contribution is based upon previous work that, to the best
            of my knowledge, is covered under an appropriate open source
            license and I have the right under that license to submit that
            work with modifications, whether created in whole or in part
            by me, under the same open source license (unless I am
            permitted to submit under a different license), as indicated
            in the file; or

        (c) The contribution was provided directly to me by some other
            person who certified (a), (b) or (c) and I have not modified
            it.

(d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved.

Affects git svn clone/fetch Original code loaded entire file contents into a variable before writing to disk. If the offset within the variable passed 2 GiB, it becrame negative, resulting in a crash. On a 32 bit system, or a system with low memory it may crash before reaching 2 GiB due to memory exhaustion. Fix writes in smaller 64K increments. Tested to work on git svn fetch

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 perl/Git.pm |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

On Thu, Feb 21, 2013 at 02:13:32PM -0800, Joshua Clayton wrote:

I have some comments. :)

The body of your email should contain the commit message (i.e., whatever people reading "git log" a year from now would see). Cover letter bits like this should go after the "---". That way "git am" knows which part is which.

You don't need to include the DCO. Your "Signed-off-by" is an indication that you agree to it.

Interesting. I didn't think perl had signed wrap-around issues like this, as its numeric variables are not strictly integers. But I don't have a 32-bit machine to test on (and numbers larger than 2G obviously work on 64-bit machines). At any rate, though:

Yeah, it is stupid to read the whole thing into memory if we are just going to dump it to another filehandle.

Hmph. The existing code already reads in 1024-byte chunks. For no reason, as far as I can tell, since we are just loading the blob buffer incrementally into memory, only to then flush it all out at once.

Why do you read at the $blobSize offset? If we are just reading in chunks, we be able to just keep writing to the start of our small buffer, as we flush each chunk out before trying to read more.

IOW, shouldn't the final code look like this:

  my $bytesLeft = $size;
  while ($bytesLeft > 0) {
          my $buf;
          my $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;
          my $read = read($in, $buf, $bytesToRead);
          unless (defined($read)) {
                  $self->_close_cat_blob();
                  throw Error::Simple("unable to read cat-blob pipe");
          }
          unless (print $fh $buf) {
                  $self->_close_cat_blob();
                  throw Error::Simple("unable to write blob output");
          }

          $bytesLeft -= $read;
  }

By having the read and flush size be the same, it's much simpler.

Your change (and my proposed code) do mean that an error during the read operation will result in a truncated output file, rather than an empty one. I think that is OK, though. That can happen anyway in the original due to a failure in the "print" step. Any caller who wants to be careful that they leave only a full file in place must either:

  1. Check the return value of cat_blob and verify that the result has
     $size bytes, and otherwise delete it.

  2. Write to a temporary file, then once success has been returned from
     cat_blob, rename the result into place.

Neither of which is affected by this change.

-Peff

On Thu, Feb 21, 2013 at 2:43 PM, Jeff King <peff@peff.net> wrote:

My original bugfix did just read 1024, and write 1024. That works fine and, yes, is simpler. I changed it to be more similar to the original code in case there were performance reasons for doing it that way. That was the only reason I could think of for the design, and adding the $flushSize variable means that some motivated person could easily optimize it.

So far I have been too lazy to profile the two versions.... I guess I'll try a trivial git svn init; git svn fetch and check back in. Its running now.

In git svn fetch (which is how I discovered it) the file being passed to cat_blob is a temporary file, which is checksummed before putting it into place.

On Thu, Feb 21, 2013 at 03:18:40PM -0800, Joshua Clayton wrote:

I doubt it will make much of a difference; we are already writing to a perl filehandle, so it will be buffered there (which I assume is 4K, but I haven't checked). And your version retains the 1024-byte read. I do think 1024 is quite low for this sort of descriptor-to-descriptor copying. I'd be tempted to just bump that 1024 to 64K.

Great. There may be other callers outside of our tree, of course, but I think it's pretty clear that the responsibility is on the caller to make sure the function succeeded. We are changing what gets put on the output stream for various error conditions, but ultimately that is an implementation detail that the caller should not be depending on.

-Peff

running git svn fetch on a remote repository (yes I know there are a lot of possible outside variables, including network latency) Code with 1024 reads and 64k writes:

real 75m19.906s user 16m43.919s sys 29m16.326s

Code with 1024 reads and 1024 writes:

real 71m21.006s user 12m36.275s sys 24m26.112s

...so the simpler code wins the trivial test. I would say go with it. Should I resubmit?

On Thu, Feb 21, 2013 at 3:24 PM, Jeff King <peff@peff.net> wrote:

On Fri, Feb 22, 2013 at 07:11:54AM -0800, Joshua Clayton wrote:

Interesting; I'd have expected no change or a slight win for your version, which makes me wonder if the outside variables are dominating. I wonder what 64K/64K would look like.

Yes, please.

-Peff