{"thread":{"id":"32976","subject":"[PATCH] Fix in Git.pm cat_blob crashes on large files (resubmit with reviewed-by)","startedAt":"2013-02-22T17:30:57Z","lastAt":"2013-02-22T19:17:17Z","messageCount":5,"participants":["Joshua Clayton","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"210041","messageId":"CAMB+bfLvpKNLaEUyUUYsO5n2y+9tyd_QcnPVzX0s2Z2t3Fr9=g@mail.gmail.com","threadId":"32976","inReplyTo":null,"subject":"[PATCH] Fix in Git.pm cat_blob crashes on large files (resubmit with reviewed-by)","fromName":"Joshua Clayton","fromEmail":"stillcompiling@gmail.com","sentAt":"2013-02-22T17:30:57Z","receivedAt":"2013-02-22T17:30:57Z","isPatch":true,"sender":{"key":"stillcompiling@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1380815?v=4"},"body":"Read and write each 1024 byte buffer, rather than trying to buffer\nthe entire content of the file.\nPrevious code would crash on all files > 2 Gib, when the offset variable\nbecame negative (perhaps below the level of perl), resulting in a crash.\nOn a 32 bit system, or a system with low memory it might crash before\nreaching 2 GiB due to memory exhaustion.\n\nSigned-off-by: Joshua Clayton \nReviewed-by: Jeff King \n---\n perl/Git.pm | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 931047c..cc91288 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -949,13 +949,16 @@ sub cat_blob {\n last unless $bytesLeft;\n\n my $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;\n- my $read = read($in, $blob, $bytesToRead, $bytesRead);\n+ my $read = read($in, $blob, $bytesToRead);\n unless (defined($read)) {\n $self->_close_cat_blob();\n throw Error::Simple(\"in pipe went bad\");\n }\n-\n $bytesRead += $read;\n+ unless (print $fh $blob) {\n+ $self->_close_cat_blob();\n+ throw Error::Simple(\"couldn't write to passed\nin filehandle\");\n+ }\n }\n\n # Skip past the trailing newline.\n@@ -970,11 +973,6 @@ sub cat_blob {\n throw Error::Simple(\"didn't find newline after blob\");\n }\n\n- unless (print $fh $blob) {\n- $self->_close_cat_blob();\n- throw Error::Simple(\"couldn't write to passed in filehandle\");\n- }\n-\n return $size;\n }\n\n--\n1.7.10.4\n"},{"id":"210044","messageId":"20130222183419.GB18934@sigill.intra.peff.net","threadId":"32976","inReplyTo":"CAMB+bfLvpKNLaEUyUUYsO5n2y+9tyd_QcnPVzX0s2Z2t3Fr9=g@mail.gmail.com","subject":"Re: [PATCH] Fix in Git.pm cat_blob crashes on large files (resubmit with reviewed-by)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-22T18:34:19Z","receivedAt":"2013-02-22T18:34:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 22, 2013 at 09:30:57AM -0800, Joshua Clayton wrote:\n\n> Read and write each 1024 byte buffer, rather than trying to buffer\n> the entire content of the file.\n\nOK. Did you ever repeat your timing with a larger symmetric buffer? That\nshould probably be a separate patch on top, but it might be worth doing\nwhile we are thinking about it.\n\n> Previous code would crash on all files > 2 Gib, when the offset variable\n> became negative (perhaps below the level of perl), resulting in a crash.\n\nI'm still slightly dubious of this, just because it doesn't match my\nknowledge of perl (which is admittedly imperfect). I'm curious how you\ndiagnosed it?\n\n> On a 32 bit system, or a system with low memory it might crash before\n> reaching 2 GiB due to memory exhaustion.\n> \n> Signed-off-by: Joshua Clayton \n> Reviewed-by: Jeff King \n\nThe commit message is a good place to mention any side effects, and why\nthey are not a problem. Something like:\n\n The previous code buffered the whole blob before writing, so any error\n reading from cat-file would result in zero bytes being written to the\n output stream. After this change, the output may be left in a\n partially written state (or even fully written, if we fail when\n parsing the final newline from cat-file). However, it's not reasonable\n for callers to expect anything about the state of the output when we\n return an error (after all, even with full buffering, we might fail\n during the writing process). So any caller which cares about this is\n broken already, and we do not have to worry about them.\n\n> ---\n> perl/Git.pm | 12 +++++-------\n> 1 file changed, 5 insertions(+), 7 deletions(-)\n\nThe patch itself looks fine to me.\n\n-Peff\n"},{"id":"210046","messageId":"20130222183703.GC18934@sigill.intra.peff.net","threadId":"32976","inReplyTo":"CAMB+bfLvpKNLaEUyUUYsO5n2y+9tyd_QcnPVzX0s2Z2t3Fr9=g@mail.gmail.com","subject":"Re: [PATCH] Fix in Git.pm cat_blob crashes on large files (resubmit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-22T18:37:03Z","receivedAt":"2013-02-22T18:37:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 22, 2013 at 09:30:57AM -0800, Joshua Clayton wrote:\n\n> Subject: Re: [PATCH] Fix in Git.pm cat_blob crashes on large files\n> (resubmit with reviewed-by)\n\nOne thing I forgot to note in my other response: the subject is part of\nthe commit message, so information like \"resubmit with...\" should go\nwith other cover letter material after the \"---\".\n\nIt's also customary to say something like \"PATCHv2\" in the brackets\n(which does get stripped off), and mention what is changed since v1\nafter the \"---\". That can help reviewers remember what happened, and it\ncan help the maintainer understand where in the process of review the\npatch is.\n\n-Peff\n"},{"id":"210047","messageId":"7v7gm0dw0m.fsf@alter.siamese.dyndns.org","threadId":"32976","inReplyTo":"CAMB+bfLvpKNLaEUyUUYsO5n2y+9tyd_QcnPVzX0s2Z2t3Fr9=g@mail.gmail.com","subject":"Re: [PATCH] Fix in Git.pm cat_blob crashes on large files (resubmit with reviewed-by)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-22T18:49:13Z","receivedAt":"2013-02-22T18:49:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joshua Clayton writes:\n\n> Read and write each 1024 byte buffer, rather than trying to buffer\n> the entire content of the file.\n> Previous code would crash on all files > 2 Gib, when the offset variable\n> became negative (perhaps below the level of perl), resulting in a crash.\n> On a 32 bit system, or a system with low memory it might crash before\n> reaching 2 GiB due to memory exhaustion.\n>\n> Signed-off-by: Joshua Clayton \n> Reviewed-by: Jeff King \n> ---\n\nThanks.\n\n>> Subject: Re: [PATCH] Fix in Git.pm cat_blob crashes on large files (resubmit with reviewed-by)\n\nPlease drop the () part. A rule of thumb is to make \"git show\"\noutput understandable by people who read it 6 months from now. They\ndo not care if the commit is a re-submission.\n\nIt seems that this issue was with us since the very beginning of\nthis sub since it was introduced at 7182530d8cad (Git.pm: Add\nhash_and_insert_object and cat_blob, 2008-05-23).\n\n> perl/Git.pm | 12 +++++-------\n> 1 file changed, 5 insertions(+), 7 deletions(-)\n>\n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index 931047c..cc91288 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -949,13 +949,16 @@ sub cat_blob {\n> last unless $bytesLeft;\n>\n> my $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;\n> - my $read = read($in, $blob, $bytesToRead, $bytesRead);\n> + my $read = read($in, $blob, $bytesToRead);\n> unless (defined($read)) {\n> $self->_close_cat_blob();\n> throw Error::Simple(\"in pipe went bad\");\n> }\n> -\n> $bytesRead += $read;\n> + unless (print $fh $blob) {\n> + $self->_close_cat_blob();\n> + throw Error::Simple(\"couldn't write to passed\n> in filehandle\");\n> + }\n\nCorrupt patch, line-wrapped by your MUA.\n\nI wonder if we still need $bytesRead variable. You have $size that\nis the size of the whole blob, so\n\n\tmy $bytesLeft = $size;\n\twhile (1) {\n\t\tmy $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;\n\t\tmy $bytesRead = read($in, $blob, $bytesToRead);\n\t\t... check errors and use the $blob ...\n $bytesLeft -= $bytesRead;\n\t}\n\nmay be simpler and easier to read, no?\n"},{"id":"210049","messageId":"CAMB+bf+LkrnfW=gQsdDhGC9LzfP_vvVG5QdsytD--Hr5_uZeEA@mail.gmail.com","threadId":"32976","inReplyTo":"20130222183419.GB18934@sigill.intra.peff.net","subject":"Re: [PATCH] Fix in Git.pm cat_blob crashes on large files (resubmit with reviewed-by)","fromName":"Joshua Clayton","fromEmail":"stillcompiling@gmail.com","sentAt":"2013-02-22T19:17:17Z","receivedAt":"2013-02-22T19:17:17Z","isPatch":true,"sender":{"key":"stillcompiling@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1380815?v=4"},"body":"Thanks for all the input and patience.\n\nOn Fri, Feb 22, 2013 at 10:34 AM, Jeff King wrote:\n> On Fri, Feb 22, 2013 at 09:30:57AM -0800, Joshua Clayton wrote:\n>\n>> Read and write each 1024 byte buffer, rather than trying to buffer\n>> the entire content of the file.\n>\n> OK. Did you ever repeat your timing with a larger symmetric buffer? That\n> should probably be a separate patch on top, but it might be worth doing\n> while we are thinking about it.\n>\n>> Previous code would crash on all files > 2 Gib, when the offset variable\n>> became negative (perhaps below the level of perl), resulting in a crash.\n>\n> I'm still slightly dubious of this, just because it doesn't match my\n> knowledge of perl (which is admittedly imperfect). I'm curious how you\n> diagnosed it?\n\nI first had the memory exhaustion problem running my git repo on a 32 vm.\nAfter bumping the memory from 512 to 4 GiB, and that failing to fix it\nI moved to my workstation with 16 GiB\n...reproduced\nAfter the initial crash, I added\n\nprint $size, \" \", $bytesToRead, \" \", $bytesRead, \"\\n\";\n\nright before the read command, and it does indeed crash right after\nthe $bytesRead variable crosses LONG_MAX\n...\n2567089913 1024 2147482624\n2567089913 1024 2147483648\n2567089913 1024 2147484672\nOffset outside string at /usr/share/perl5/Git.pm line 901, line 2604.\n\nNote that $bytesRead is still positive.\nI know very little perl, but that symptom seems pretty clear\n\n>\n>> On a 32 bit system, or a system with low memory it might crash before\n>> reaching 2 GiB due to memory exhaustion.\n>>\n>> Signed-off-by: Joshua Clayton \n>> Reviewed-by: Jeff King \n>\n> The commit message is a good place to mention any side effects, and why\n> they are not a problem. Something like:\n>\n> The previous code buffered the whole blob before writing, so any error\n> reading from cat-file would result in zero bytes being written to the\n> output stream. After this change, the output may be left in a\n> partially written state (or even fully written, if we fail when\n> parsing the final newline from cat-file). However, it's not reasonable\n> for callers to expect anything about the state of the output when we\n> return an error (after all, even with full buffering, we might fail\n> during the writing process). So any caller which cares about this is\n> broken already, and we do not have to worry about them.\n>\n>> ---\n>> perl/Git.pm | 12 +++++-------\n>> 1 file changed, 5 insertions(+), 7 deletions(-)\n>\n> The patch itself looks fine to me.\n>\n> -Peff\n"}]}