{"thread":{"id":"32965","subject":"[PATCH] Fix in Git.pm cat_blob crashes on large files","startedAt":"2013-02-21T22:13:32Z","lastAt":"2013-02-22T15:38:01Z","messageCount":6,"participants":["Joshua Clayton","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"209998","messageId":"CAMB+bfKYLjmDavcLaO7scBPfTLmzqAmH+k9uBj0WJ+dzj9vuyA@mail.gmail.com","threadId":"32965","inReplyTo":null,"subject":"[PATCH] Fix in Git.pm cat_blob crashes on large files","fromName":"Joshua Clayton","fromEmail":"stillcompiling@gmail.com","sentAt":"2013-02-21T22:13:32Z","receivedAt":"2013-02-21T22:13:32Z","isPatch":true,"sender":{"key":"stillcompiling@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1380815?v=4"},"body":"Greetings.\nThis is my first patch here. Hopefully I get the stylistic & political\ndetails right... :)\nPatch applies against maint and master\n\n(If I understand the mechanics, in theory a negative offset should work,\n if the values lined up just right, but would be very wrong, overwriting the\nlower contents of the file)\n\n        Developer's Certificate of Origin 1.1\n\n        By making a contribution to this project, I certify that:\n\n        (a) The contribution was created in whole or in part by me and I\n            have the right to submit it under the open source license\n            indicated in the file; or\n\n        (b) The contribution is based upon previous work that, to the best\n            of my knowledge, is covered under an appropriate open source\n            license and I have the right under that license to submit that\n            work with modifications, whether created in whole or in part\n            by me, under the same open source license (unless I am\n            permitted to submit under a different license), as indicated\n            in the file; or\n\n        (c) The contribution was provided directly to me by some other\n            person who certified (a), (b) or (c) and I have not modified\n            it.\n\n(d) I understand and agree that this project and the contribution\nare public and that a record of the contribution (including all\npersonal information I submit with it, including my sign-off) is\nmaintained indefinitely and may be redistributed consistent with\nthis project or the open source license(s) involved.\n\nAffects git svn clone/fetch\nOriginal code loaded entire file contents into a variable\nbefore writing to disk. If the offset within the variable passed\n2 GiB, it becrame negative, resulting in a crash.\nOn a 32 bit system, or a system with low memory it may crash before\nreaching 2 GiB due to memory exhaustion.\nFix writes in smaller 64K increments. Tested to work on git svn fetch\n\nSigned-off-by: Joshua Clayton <stillcompiling@gmail.com>\n---\n perl/Git.pm |   19 ++++++++++++-------\n 1 file changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 931047c..e55840f 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -942,6 +942,8 @@ sub cat_blob {\n \tmy $size = $1;\n\n \tmy $blob;\n+\tmy $blobSize = 0;\n+\tmy $flushSize = 1024*64;\n \tmy $bytesRead = 0;\n\n \twhile (1) {\n@@ -949,13 +951,21 @@ sub cat_blob {\n \t\tlast unless $bytesLeft;\n\n \t\tmy $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;\n-\t\tmy $read = read($in, $blob, $bytesToRead, $bytesRead);\n+\t\tmy $read = read($in, $blob, $bytesToRead, $blobSize);\n \t\tunless (defined($read)) {\n \t\t\t$self->_close_cat_blob();\n \t\t\tthrow Error::Simple(\"in pipe went bad\");\n \t\t}\n-\n \t\t$bytesRead += $read;\n+\t\t$blobSize += $read;\n+\t\tif (($blobSize >= $flushSize) || ($bytesLeft <= 1024)) {\n+\t\t\tunless (print $fh $blob) {\n+\t\t\t\t$self->_close_cat_blob();\n+\t\t\t\tthrow Error::Simple(\"couldn't write to passed in filehandle\");\n+\t\t\t}\n+\t\t\t$blob = \"\";\n+\t\t\t$blobSize = 0;\n+\t\t}\n \t}\n\n \t# Skip past the trailing newline.\n@@ -970,11 +980,6 @@ sub cat_blob {\n \t\tthrow Error::Simple(\"didn't find newline after blob\");\n \t}\n\n-\tunless (print $fh $blob) {\n-\t\t$self->_close_cat_blob();\n-\t\tthrow Error::Simple(\"couldn't write to passed in filehandle\");\n-\t}\n-\n \treturn $size;\n }\n\n-- \n1.7.10.4\n"},{"id":"209999","messageId":"20130221224319.GA19021@sigill.intra.peff.net","threadId":"32965","inReplyTo":"CAMB+bfKYLjmDavcLaO7scBPfTLmzqAmH+k9uBj0WJ+dzj9vuyA@mail.gmail.com","subject":"Re: [PATCH] Fix in Git.pm cat_blob crashes on large files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-21T22:43:20Z","receivedAt":"2013-02-21T22:43:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 21, 2013 at 02:13:32PM -0800, Joshua Clayton wrote:\n\n> Greetings.\n> This is my first patch here. Hopefully I get the stylistic & political\n> details right... :)\n> Patch applies against maint and master\n\nI have some comments. :)\n\nThe body of your email should contain the commit message (i.e., whatever\npeople reading \"git log\" a year from now would see). Cover letter bits\nlike this should go after the \"---\". That way \"git am\" knows which part\nis which.\n\n>         Developer's Certificate of Origin 1.1\n\nYou don't need to include the DCO. Your \"Signed-off-by\" is an indication\nthat you agree to it.\n\n> Affects git svn clone/fetch\n> Original code loaded entire file contents into a variable\n> before writing to disk. If the offset within the variable passed\n> 2 GiB, it becrame negative, resulting in a crash.\n\nInteresting. I didn't think perl had signed wrap-around issues like\nthis, as its numeric variables are not strictly integers. But I don't\nhave a 32-bit machine to test on (and numbers larger than 2G obviously\nwork on 64-bit machines). At any rate, though:\n\n> On a 32 bit system, or a system with low memory it may crash before\n> reaching 2 GiB due to memory exhaustion.\n\nYeah, it is stupid to read the whole thing into memory if we are just\ngoing to dump it to another filehandle.\n\n> @@ -949,13 +951,21 @@ sub cat_blob {\n>  \t\tlast unless $bytesLeft;\n> \n>  \t\tmy $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;\n> -\t\tmy $read = read($in, $blob, $bytesToRead, $bytesRead);\n> +\t\tmy $read = read($in, $blob, $bytesToRead, $blobSize);\n>  \t\tunless (defined($read)) {\n>  \t\t\t$self->_close_cat_blob();\n>  \t\t\tthrow Error::Simple(\"in pipe went bad\");\n>  \t\t}\n\nHmph. The existing code already reads in 1024-byte chunks. For no\nreason, as far as I can tell, since we are just loading the blob buffer\nincrementally into memory, only to then flush it all out at once.\n\nWhy do you read at the $blobSize offset? If we are just reading in\nchunks, we be able to just keep writing to the start of our small\nbuffer, as we flush each chunk out before trying to read more.\n\nIOW, shouldn't the final code look like this:\n\n  my $bytesLeft = $size;\n  while ($bytesLeft > 0) {\n          my $buf;\n          my $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;\n          my $read = read($in, $buf, $bytesToRead);\n          unless (defined($read)) {\n                  $self->_close_cat_blob();\n                  throw Error::Simple(\"unable to read cat-blob pipe\");\n          }\n          unless (print $fh $buf) {\n                  $self->_close_cat_blob();\n                  throw Error::Simple(\"unable to write blob output\");\n          }\n\n          $bytesLeft -= $read;\n  }\n\nBy having the read and flush size be the same, it's much simpler.\n\nYour change (and my proposed code) do mean that an error during the read\noperation will result in a truncated output file, rather than an empty\none. I think that is OK, though. That can happen anyway in the original\ndue to a failure in the \"print\" step. Any caller who wants to be careful\nthat they leave only a full file in place must either:\n\n  1. Check the return value of cat_blob and verify that the result has\n     $size bytes, and otherwise delete it.\n\n  2. Write to a temporary file, then once success has been returned from\n     cat_blob, rename the result into place.\n\nNeither of which is affected by this change.\n\n-Peff\n"},{"id":"210002","messageId":"CAMB+bf+whVFD03neCh-gBORXOBoNjgaCbfP_mh8HgDy6UqGFZA@mail.gmail.com","threadId":"32965","inReplyTo":"20130221224319.GA19021@sigill.intra.peff.net","subject":"Re: [PATCH] Fix in Git.pm cat_blob crashes on large files","fromName":"Joshua Clayton","fromEmail":"stillcompiling@gmail.com","sentAt":"2013-02-21T23:18:40Z","receivedAt":"2013-02-21T23:18:40Z","isPatch":true,"sender":{"key":"stillcompiling@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1380815?v=4"},"body":"On Thu, Feb 21, 2013 at 2:43 PM, Jeff King <peff@peff.net> wrote:\n> On Thu, Feb 21, 2013 at 02:13:32PM -0800, Joshua Clayton wrote:\n>\n>> Greetings.\n>> This is my first patch here. Hopefully I get the stylistic & political\n>> details right... :)\n>> Patch applies against maint and master\n>\n> I have some comments. :)\n>\n> The body of your email should contain the commit message (i.e., whatever\n> people reading \"git log\" a year from now would see). Cover letter bits\n> like this should go after the \"---\". That way \"git am\" knows which part\n> is which.\n>\n>>         Developer's Certificate of Origin 1.1\n>\n> You don't need to include the DCO. Your \"Signed-off-by\" is an indication\n> that you agree to it.\n>\n>> Affects git svn clone/fetch\n>> Original code loaded entire file contents into a variable\n>> before writing to disk. If the offset within the variable passed\n>> 2 GiB, it becrame negative, resulting in a crash.\n>\n> Interesting. I didn't think perl had signed wrap-around issues like\n> this, as its numeric variables are not strictly integers. But I don't\n> have a 32-bit machine to test on (and numbers larger than 2G obviously\n> work on 64-bit machines). At any rate, though:\n>\n>> On a 32 bit system, or a system with low memory it may crash before\n>> reaching 2 GiB due to memory exhaustion.\n>\n> Yeah, it is stupid to read the whole thing into memory if we are just\n> going to dump it to another filehandle.\n>\n>> @@ -949,13 +951,21 @@ 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, $blobSize);\n>>               unless (defined($read)) {\n>>                       $self->_close_cat_blob();\n>>                       throw Error::Simple(\"in pipe went bad\");\n>>               }\n>\n> Hmph. The existing code already reads in 1024-byte chunks. For no\n> reason, as far as I can tell, since we are just loading the blob buffer\n> incrementally into memory, only to then flush it all out at once.\n>\n> Why do you read at the $blobSize offset? If we are just reading in\n> chunks, we be able to just keep writing to the start of our small\n> buffer, as we flush each chunk out before trying to read more.\n>\n> IOW, shouldn't the final code look like this:\n>\n>   my $bytesLeft = $size;\n>   while ($bytesLeft > 0) {\n>           my $buf;\n>           my $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;\n>           my $read = read($in, $buf, $bytesToRead);\n>           unless (defined($read)) {\n>                   $self->_close_cat_blob();\n>                   throw Error::Simple(\"unable to read cat-blob pipe\");\n>           }\n>           unless (print $fh $buf) {\n>                   $self->_close_cat_blob();\n>                   throw Error::Simple(\"unable to write blob output\");\n>           }\n>\n>           $bytesLeft -= $read;\n>   }\n>\n> By having the read and flush size be the same, it's much simpler.\n\nMy original bugfix did just read 1024, and write 1024. That works fine\nand, yes, is simpler.\nI changed it to be more similar to the original code in case there\nwere performance reasons for doing it that way.\nThat was the only reason I could think of for the design, and adding\nthe $flushSize variable means that\nsome motivated person could easily optimize it.\n\nSo far I have been too lazy to profile the two versions....\nI guess I'll try a trivial git svn init; git svn fetch and check back in.\nIts running now.\n\n>\n> Your change (and my proposed code) do mean that an error during the read\n> operation will result in a truncated output file, rather than an empty\n> one. I think that is OK, though. That can happen anyway in the original\n> due to a failure in the \"print\" step. Any caller who wants to be careful\n> that they leave only a full file in place must either:\n>\n>   1. Check the return value of cat_blob and verify that the result has\n>      $size bytes, and otherwise delete it.\n>\n>   2. Write to a temporary file, then once success has been returned from\n>      cat_blob, rename the result into place.\n>\n> Neither of which is affected by this change.\n>\n> -Peff\n\nIn git svn fetch (which is how I discovered it) the file being passed\nto cat_blob is a temporary file, which is checksummed before putting\nit into place.\n"},{"id":"210004","messageId":"20130221232448.GA23736@sigill.intra.peff.net","threadId":"32965","inReplyTo":"CAMB+bf+whVFD03neCh-gBORXOBoNjgaCbfP_mh8HgDy6UqGFZA@mail.gmail.com","subject":"Re: [PATCH] Fix in Git.pm cat_blob crashes on large files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-21T23:24:48Z","receivedAt":"2013-02-21T23:24:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 21, 2013 at 03:18:40PM -0800, Joshua Clayton wrote:\n\n> > By having the read and flush size be the same, it's much simpler.\n> \n> My original bugfix did just read 1024, and write 1024. That works fine\n> and, yes, is simpler.\n> I changed it to be more similar to the original code in case there\n> were performance reasons for doing it that way.\n> That was the only reason I could think of for the design, and adding\n> the $flushSize variable means that\n> some motivated person could easily optimize it.\n> \n> So far I have been too lazy to profile the two versions....\n> I guess I'll try a trivial git svn init; git svn fetch and check back in.\n> Its running now.\n\nI doubt it will make much of a difference; we are already writing to a\nperl filehandle, so it will be buffered there (which I assume is 4K, but\nI haven't checked). And your version retains the 1024-byte read. I do\nthink 1024 is quite low for this sort of descriptor-to-descriptor\ncopying. I'd be tempted to just bump that 1024 to 64K.\n\n> In git svn fetch (which is how I discovered it) the file being passed\n> to cat_blob is a temporary file, which is checksummed before putting\n> it into place.\n\nGreat. There may be other callers outside of our tree, of course, but I\nthink it's pretty clear that the responsibility is on the caller to make\nsure the function succeeded. We are changing what gets put on the output\nstream for various error conditions, but ultimately that is an\nimplementation detail that the caller should not be depending on.\n\n-Peff\n"},{"id":"210028","messageId":"CAMB+bf+RyerXP3VP26KJM7wtVKOup15AeRpEewwKJx3=bL9+hQ@mail.gmail.com","threadId":"32965","inReplyTo":"20130221232448.GA23736@sigill.intra.peff.net","subject":"Re: [PATCH] Fix in Git.pm cat_blob crashes on large files","fromName":"Joshua Clayton","fromEmail":"stillcompiling@gmail.com","sentAt":"2013-02-22T15:11:54Z","receivedAt":"2013-02-22T15:11:54Z","isPatch":true,"sender":{"key":"stillcompiling@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1380815?v=4"},"body":"running git svn fetch on a remote repository (yes I know there are a\nlot of possible outside variables, including network latency)\nCode with 1024 reads and 64k writes:\n\nreal    75m19.906s\nuser    16m43.919s\nsys     29m16.326s\n\nCode with 1024 reads and 1024 writes:\n\nreal    71m21.006s\nuser    12m36.275s\nsys     24m26.112s\n\n...so the simpler code wins the trivial test.\nI would say go with it.\nShould I resubmit?\n\nOn Thu, Feb 21, 2013 at 3:24 PM, Jeff King <peff@peff.net> wrote:\n> On Thu, Feb 21, 2013 at 03:18:40PM -0800, Joshua Clayton wrote:\n>\n>> > By having the read and flush size be the same, it's much simpler.\n>>\n>> My original bugfix did just read 1024, and write 1024. That works fine\n>> and, yes, is simpler.\n>> I changed it to be more similar to the original code in case there\n>> were performance reasons for doing it that way.\n>> That was the only reason I could think of for the design, and adding\n>> the $flushSize variable means that\n>> some motivated person could easily optimize it.\n>>\n>> So far I have been too lazy to profile the two versions....\n>> I guess I'll try a trivial git svn init; git svn fetch and check back in.\n>> Its running now.\n>\n> I doubt it will make much of a difference; we are already writing to a\n> perl filehandle, so it will be buffered there (which I assume is 4K, but\n> I haven't checked). And your version retains the 1024-byte read. I do\n> think 1024 is quite low for this sort of descriptor-to-descriptor\n> copying. I'd be tempted to just bump that 1024 to 64K.\n>\n>> In git svn fetch (which is how I discovered it) the file being passed\n>> to cat_blob is a temporary file, which is checksummed before putting\n>> it into place.\n>\n> Great. There may be other callers outside of our tree, of course, but I\n> think it's pretty clear that the responsibility is on the caller to make\n> sure the function succeeded. We are changing what gets put on the output\n> stream for various error conditions, but ultimately that is an\n> implementation detail that the caller should not be depending on.\n>\n> -Peff\n"},{"id":"210031","messageId":"20130222153801.GA15318@sigill.intra.peff.net","threadId":"32965","inReplyTo":"CAMB+bf+RyerXP3VP26KJM7wtVKOup15AeRpEewwKJx3=bL9+hQ@mail.gmail.com","subject":"Re: [PATCH] Fix in Git.pm cat_blob crashes on large files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-22T15:38:01Z","receivedAt":"2013-02-22T15:38:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 22, 2013 at 07:11:54AM -0800, Joshua Clayton wrote:\n\n> running git svn fetch on a remote repository (yes I know there are a\n> lot of possible outside variables, including network latency)\n> Code with 1024 reads and 64k writes:\n> \n> real    75m19.906s\n> user    16m43.919s\n> sys     29m16.326s\n> \n> Code with 1024 reads and 1024 writes:\n> \n> real    71m21.006s\n> user    12m36.275s\n> sys     24m26.112s\n> \n> ...so the simpler code wins the trivial test.\n\nInteresting; I'd have expected no change or a slight win for your\nversion, which makes me wonder if the outside variables are dominating.\nI wonder what 64K/64K would look like.\n\n> I would say go with it.\n> Should I resubmit?\n\nYes, please.\n\n-Peff\n"}]}