Re: [PATCH] Fix in Git.pm cat_blob crashes on large files (resubmit with reviewed-by)

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2013, 18:49 UTC
Message-ID: <7v7gm0dw0m.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAMB+bfLvpKNLaEUyUUYsO5n2y+9tyd_QcnPVzX0s2Z2t3Fr9=g@mail.gmail.com>

Joshua Clayton <stillcompiling@gmail.com> writes:

Show 10 quoted lines

> Read and write each 1024 byte buffer, rather than trying to buffer
> the entire content of the file.
> Previous code would crash on all files > 2 Gib, when the offset variable
> became negative (perhaps below the level of perl), resulting in a crash.
> On a 32 bit system, or a system with low memory it might crash before
> reaching 2 GiB due to memory exhaustion.
>
> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> Reviewed-by: Jeff King <peff@peff.net>
> ---

Thanks.

>> Subject: Re: [PATCH] Fix in Git.pm cat_blob crashes on large files (resubmit with reviewed-by)

Please drop the () part. A rule of thumb is to make "git show" output understandable by people who read it 6 months from now. They do not care if the commit is a re-submission.

It seems that this issue was with us since the very beginning of this sub since it was introduced at 7182530d8cad (Git.pm: Add hash_and_insert_object and cat_blob, 2008-05-23).

Show 24 quoted lines

>  perl/Git.pm |   12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 931047c..cc91288 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -949,13 +949,16 @@ sub cat_blob {
>                 last unless $bytesLeft;
>
>                 my $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;
> -               my $read = read($in, $blob, $bytesToRead, $bytesRead);
> +               my $read = read($in, $blob, $bytesToRead);
>                 unless (defined($read)) {
>                         $self->_close_cat_blob();
>                         throw Error::Simple("in pipe went bad");
>                 }
> -
>                 $bytesRead += $read;
> +               unless (print $fh $blob) {
> +                       $self->_close_cat_blob();
> +                       throw Error::Simple("couldn't write to passed
> in filehandle");
> +               }

Corrupt patch, line-wrapped by your MUA.

I wonder if we still need $bytesRead variable. You have $size that is the size of the whole blob, so

	my $bytesLeft = $size;
	while (1) {
		my $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;
		my $bytesRead = read($in, $blob, $bytesToRead);
		... check errors and use the $blob ...
                $bytesLeft -= $bytesRead;
	}

may be simpler and easier to read, no?