{"thread":{"id":"22739","subject":"Re: [PATCH] sha1_file: don't malloc the whole compressed result when writing out objects","startedAt":"2010-02-21T04:27:31Z","lastAt":"2010-02-22T19:55:43Z","messageCount":13,"participants":["Nicolas Pitre","Junio C Hamano","Dmitry Potapov"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"135222","messageId":"alpine.LFD.2.00.1002202323500.1946@xanadu.home","threadId":"22739","inReplyTo":null,"subject":"[PATCH] sha1_file: don't malloc the whole compressed result when writing out objects","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-21T04:27:31Z","receivedAt":"2010-02-21T04:27:31Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"There is no real advantage to malloc the whole output buffer and\ndeflate the data in a single pass when writing loose objects. That is\nlike only 1% faster while using more memory, especially with large\nfiles where memory usage is far more. It is best to deflate and write\nthe data out in small chunks reusing the same memory instead.\n\nFor example, using 'git add' on a few large files averaging 40 MB ...\n\nBefore:\n21.45user 1.10system 0:22.57elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+828040outputs (0major+142640minor)pagefaults 0swaps\n\nAfter:\n21.50user 1.25system 0:22.76elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+828040outputs (0major+104408minor)pagefaults 0swaps\n\nWhile the runtime stayed relatively the same, the number of minor page\nfaults went down significantly.\n\nSigned-off-by: Nicolas Pitre <nico@fluxnic.net>\n---\n\nI think this is worth doing independently of the paranoid mode being \ndiscussed.\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 657825e..9196b57 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2281,8 +2281,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \t\t\t      void *buf, unsigned long len, time_t mtime)\n {\n \tint fd, ret;\n-\tsize_t size;\n-\tunsigned char *compressed;\n+\tunsigned char compressed[4096];\n \tz_stream stream;\n \tchar *filename;\n \tstatic char tmpfile[PATH_MAX];\n@@ -2301,12 +2300,8 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \t/* Set it up */\n \tmemset(&stream, 0, sizeof(stream));\n \tdeflateInit(&stream, zlib_compression_level);\n-\tsize = 8 + deflateBound(&stream, len+hdrlen);\n-\tcompressed = xmalloc(size);\n-\n-\t/* Compress it */\n \tstream.next_out = compressed;\n-\tstream.avail_out = size;\n+\tstream.avail_out = sizeof(compressed);\n \n \t/* First header.. */\n \tstream.next_in = (unsigned char *)hdr;\n@@ -2317,20 +2312,21 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \t/* Then the data itself.. */\n \tstream.next_in = buf;\n \tstream.avail_in = len;\n-\tret = deflate(&stream, Z_FINISH);\n+\tdo {\n+\t\tret = deflate(&stream, Z_FINISH);\n+\t\tif (write_buffer(fd, compressed, stream.next_out - compressed) < 0)\n+\t\t\tdie(\"unable to write sha1 file\");\n+\t\tstream.next_out = compressed;\n+\t\tstream.avail_out = sizeof(compressed);\n+\t} while (ret == Z_OK);\n+\n \tif (ret != Z_STREAM_END)\n \t\tdie(\"unable to deflate new object %s (%d)\", sha1_to_hex(sha1), ret);\n-\n \tret = deflateEnd(&stream);\n \tif (ret != Z_OK)\n \t\tdie(\"deflateEnd on object %s failed (%d)\", sha1_to_hex(sha1), ret);\n \n-\tsize = stream.total_out;\n-\n-\tif (write_buffer(fd, compressed, size) < 0)\n-\t\tdie(\"unable to write sha1 file\");\n \tclose_sha1_file(fd);\n-\tfree(compressed);\n \n \tif (mtime) {\n \t\tstruct utimbuf utb;\n"},{"id":"299153","messageId":"7vd3zys79d.fsf@alter.siamese.dyndns.org","threadId":"22739","inReplyTo":"alpine.LFD.2.00.1002202323500.1946@xanadu.home","subject":"Re: [PATCH] sha1_file: don't malloc the whole compressed result when writing out objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-21T19:45:02Z","receivedAt":"2010-02-21T19:45:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@fluxnic.net> writes:\n\n> I think this is worth doing independently of the paranoid mode being \n> discussed.\n\nWhile I agree it might be worth doing, I can see that you really hate\n\"paranoia\".  Now your loop is letting deflate() decide how much it happens\nto like to consume in a given round, it is much trickier to plug the\nparanoia in without majorly rewriting the loop this patch introduces.\n\n"},{"id":"299154","messageId":"alpine.LFD.2.00.1002211522120.1946@xanadu.home","threadId":"22739","inReplyTo":"7vd3zys79d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] sha1_file: don't malloc the whole compressed result when writing out objects","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-21T21:26:51Z","receivedAt":"2010-02-21T21:26:51Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 21 Feb 2010, Junio C Hamano wrote:\n\n> Nicolas Pitre <nico@fluxnic.net> writes:\n> \n> > I think this is worth doing independently of the paranoid mode being \n> > discussed.\n> \n> While I agree it might be worth doing, I can see that you really hate\n> \"paranoia\".  Now your loop is letting deflate() decide how much it happens\n> to like to consume in a given round, it is much trickier to plug the\n> paranoia in without majorly rewriting the loop this patch introduces.\n\nI disagree.\n\nHere's my take on the paranoia issue.  Now the question is whether or \nnot this should really be optional.  I would think no.\n\nFWIW, we already have that double SHA1 protection when dealing with pack \nfiles with fixup_pack_header_footer() (see commit abeb40e5aa).\n\n---------- >8\nFrom: Nicolas Pitre <nico@fluxnic.net>\nDate: Sun, 21 Feb 2010 15:48:06 -0500\nSubject: [PATCH] sha1_file: be paranoid when creating loose objects\n\nWe don't want the data being deflated and stored into loose objects\nto be different from what we expect.  While the deflated data is\nprotected by a CRC which is good enough for safe data retrieval\noperations, we still want to be doubly sure that the source data used\nat object creation time is still what we expected once that data has\nbeen deflated and its CRC32 computed.\n\nThe most plausible data corruption may occur if the source file is\nmodified while Git is deflating and writing it out in a loose object.\nOr Git itself could have a bug causing memory corruption.  Or even bad\nRAM could cause trouble.  So it is best to make sure everything is\ncoherent and checksum protected from beginning to end.\n\nTo do so we compute the SHA1 of the data being deflated _after_ the\ndeflate operation has consumed that data, and make sure it matches\nwith the expected SHA1.  This way we can rely on the CRC32 checked by\nthe inflate operation to provide a good indication that the data is still\ncoherent with its SHA1 hash.\n\nThere is some overhead of course. Using 'git add' on a set of large files:\n\nBefore:\n\n\treal    0m25.210s\n\tuser    0m23.783s\n\tsys     0m1.408s\n\nAfter:\n\n\treal    0m26.537s\n\tuser    0m25.175s\n\tsys     0m1.358s\n\nThe overhead is around 5% for full data coherency guarantee.\n\nSigned-off-by: Nicolas Pitre <nico@fluxnic.net>\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 9196b57..c0214d7 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2283,6 +2283,8 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \tint fd, ret;\n \tunsigned char compressed[4096];\n \tz_stream stream;\n+\tgit_SHA_CTX c;\n+\tunsigned char parano_sha1[20];\n \tchar *filename;\n \tstatic char tmpfile[PATH_MAX];\n \n@@ -2302,18 +2304,22 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \tdeflateInit(&stream, zlib_compression_level);\n \tstream.next_out = compressed;\n \tstream.avail_out = sizeof(compressed);\n+\tgit_SHA1_Init(&c);\n \n \t/* First header.. */\n \tstream.next_in = (unsigned char *)hdr;\n \tstream.avail_in = hdrlen;\n \twhile (deflate(&stream, 0) == Z_OK)\n \t\t/* nothing */;\n+\tgit_SHA1_Update(&c, hdr, hdrlen);\n \n \t/* Then the data itself.. */\n \tstream.next_in = buf;\n \tstream.avail_in = len;\n \tdo {\n+\t\tunsigned char *in0 = stream.next_in;\n \t\tret = deflate(&stream, Z_FINISH);\n+\t\tgit_SHA1_Update(&c, in0, stream.next_in - in0);\n \t\tif (write_buffer(fd, compressed, stream.next_out - compressed) < 0)\n \t\t\tdie(\"unable to write sha1 file\");\n \t\tstream.next_out = compressed;\n@@ -2325,6 +2331,9 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \tret = deflateEnd(&stream);\n \tif (ret != Z_OK)\n \t\tdie(\"deflateEnd on object %s failed (%d)\", sha1_to_hex(sha1), ret);\n+\tgit_SHA1_Final(parano_sha1, &c);\n+\tif (hashcmp(sha1, parano_sha1) != 0)\n+\t\tdie(\"confused by unstable object source data for %s\", sha1_to_hex(sha1));\n \n \tclose_sha1_file(fd);\n \n"},{"id":"299155","messageId":"7v7hq6mdpi.fsf@alter.siamese.dyndns.org","threadId":"22739","inReplyTo":"alpine.LFD.2.00.1002211522120.1946@xanadu.home","subject":"Re: [PATCH] sha1_file: don't malloc the whole compressed result when writing out objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-21T22:22:17Z","receivedAt":"2010-02-21T22:22:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@fluxnic.net> writes:\n\n> I disagree.\n>\n> Here's my take on the paranoia issue.\n\nAhh, yes, of course.\n\nYou are always a better programmer than I am and I keep getting reminded.\n\nThanks, and I agree it is a sane thing to do this unconditionally.\n"},{"id":"299156","messageId":"7v3a0umdb8.fsf@alter.siamese.dyndns.org","threadId":"22739","inReplyTo":"alpine.LFD.2.00.1002211522120.1946@xanadu.home","subject":"Re: [PATCH] sha1_file: don't malloc the whole compressed result when writing out objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-21T22:30:51Z","receivedAt":"2010-02-21T22:30:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@fluxnic.net> writes:\n\n>  \t/* Then the data itself.. */\n>  \tstream.next_in = buf;\n>  \tstream.avail_in = len;\n>  \tdo {\n> +\t\tunsigned char *in0 = stream.next_in;\n>  \t\tret = deflate(&stream, Z_FINISH);\n> +\t\tgit_SHA1_Update(&c, in0, stream.next_in - in0);\n\nActually, I have to take my earlier comment back.  This is not \"paranoia\".\n\nI do not see anything that protects the memory area between in0 and\nstream.next_in from getting modified while deflate() nor SHA1_Update() run\nfrom the outside.  Unless you copy the data away to somewhere stable at\nthe beginning of each iteration of this loop and run deflate() and\nSHA1_Update(), you cannot have \"paranoia\".\n\nMy comment about \"trickier\" is about determining the size of that buffer\nused as \"somewhere stable\".\n"},{"id":"135162","messageId":"alpine.LFD.2.00.1002211950250.1946@xanadu.home","threadId":"22739","inReplyTo":"7v3a0umdb8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] sha1_file: don't malloc the whole compressed result when writing out objects","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-22T01:35:48Z","receivedAt":"2010-02-22T01:35:48Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 21 Feb 2010, Junio C Hamano wrote:\n\n> Nicolas Pitre <nico@fluxnic.net> writes:\n> \n> >  \t/* Then the data itself.. */\n> >  \tstream.next_in = buf;\n> >  \tstream.avail_in = len;\n> >  \tdo {\n> > +\t\tunsigned char *in0 = stream.next_in;\n> >  \t\tret = deflate(&stream, Z_FINISH);\n> > +\t\tgit_SHA1_Update(&c, in0, stream.next_in - in0);\n> \n> Actually, I have to take my earlier comment back.  This is not \"paranoia\".\n> \n> I do not see anything that protects the memory area between in0 and\n> stream.next_in from getting modified while deflate() nor SHA1_Update() run\n> from the outside.\n\nSo what?\n\n> Unless you copy the data away to somewhere stable at\n> the beginning of each iteration of this loop and run deflate() and\n> SHA1_Update(), you cannot have \"paranoia\".\n\nNo.\n\nThe whole point is to detect data incoherencyes.\n\nSo current sequence of events is as follows:\n\nT0\twrite_sha1_file_prepare() is called\nT1\tstart initial SHA1 computation on data buffer\nT2\tin the middle of initial SHA1 computation\nT3\tend of initial SHA1 computation -> object name is determined\nT4\twrite_loose_object() is called\n...\tenter the write loop\nT5+n\tdeflate() called on buffer n\nT6+n\tgit_SHA1_Update(() called on the same buffer n\nT7+n\tdeflated data written out\n...\nTend\tabort if result of T6+n doesn't match object name from T3\n\nSo... what can happen:\n\n1) Data is externally modified before T5+n: deflated data and its CRC32 \n   will be coherent with the SHA1 computed in T6+n, but incoherent with \n   the SHA1 used for the object name. Wrong data is written to the \n   object even if it will inflate OK. We really want to prevent that \n   from happening. The test in Tend will fail.\n\n2) Data is externally modified between T5+n and T6+n: the deflated data \n   and CRC32 will be coherent with the object name but incoherent with \n   the parano_sha1.  Although written data will be OK, this is way too \n   close from being wrong, and the test in Tend will fail.  If there is \n   more than one round into the loop and the external modifications are \n   large enough then this becomes the same as case 1 above.\n\n3) Data is externally modified in T2: again the test in Tend will fail.\n\nSo in all possible cases I can think of, the write will abort.  No copy \nbuffer needed, no filesystem mtime required, etc.  If the whole data is \nnot stable between T1 and Tend then the object is not added to the \nrepository.  Of course it is possible that the data be modified at the \nbeginning of the file while the loop in T[5-7] is passed that point.  \nBut still, there is no data inconsistency at that point.\n\n> My comment about \"trickier\" is about determining the size of that buffer\n> used as \"somewhere stable\".\n\nWe don't care about such buffer.\n\n\nNicolas\n"},{"id":"135241","messageId":"7v635p4z26.fsf@alter.siamese.dyndns.org","threadId":"22739","inReplyTo":"alpine.LFD.2.00.1002211950250.1946@xanadu.home","subject":"Re: [PATCH] sha1_file: don't malloc the whole compressed result when writing out objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-22T05:30:41Z","receivedAt":"2010-02-22T05:30:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@fluxnic.net> writes:\n\n> The whole point is to detect data incoherencyes.\n\nYes.  We want to make sure that the SHA-1 we compute is over what we fed\ndeflate().\n\n> So current sequence of events is as follows:\n>\n> T0\twrite_sha1_file_prepare() is called\n> T1\tstart initial SHA1 computation on data buffer\n> T2\tin the middle of initial SHA1 computation\n> T3\tend of initial SHA1 computation -> object name is determined\n> T4\twrite_loose_object() is called\n> ...\tenter the write loop\n> T5+n\tdeflate() called on buffer n\n> T6+n\tgit_SHA1_Update(() called on the same buffer n\n> T7+n\tdeflated data written out\n> ...\n> Tend\tabort if result of T6+n doesn't match object name from T3\n>\n> So... what can happen:\n>\n> 1) Data is externally modified before T5+n: deflated data and its CRC32 \n>    will be coherent with the SHA1 computed in T6+n, but incoherent with \n>    the SHA1 used for the object name. Wrong data is written to the \n>    object even if it will inflate OK. We really want to prevent that \n>    from happening. The test in Tend will fail.\n>\n> 2) Data is externally modified between T5+n and T6+n: the deflated data \n>    and CRC32 will be coherent with the object name but incoherent with \n>    the parano_sha1.  Although written data will be OK, this is way too \n>    close from being wrong, and the test in Tend will fail.  If there is \n>    more than one round into the loop and the external modifications are \n>    large enough then this becomes the same as case 1 above.\n>\n> 3) Data is externally modified in T2: again the test in Tend will fail.\n>\n> So in all possible cases I can think of, the write will abort.\n\nThere is one pathological case.\n\nImmediately before T5+n (or between T5+n and T6+n), the external process\nchanges the data deflate() is working on, but before T6+n, the external\nprocess changes the data back.  Two SHA-1's computed may match, but it is\nnot a hash over what was deflated(); you won't be able to abort.\n"},{"id":"135244","messageId":"alpine.LFD.2.00.1002220034540.1946@xanadu.home","threadId":"22739","inReplyTo":"7v635p4z26.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] sha1_file: don't malloc the whole compressed result when writing out objects","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-22T05:50:18Z","receivedAt":"2010-02-22T05:50:18Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 21 Feb 2010, Junio C Hamano wrote:\n\n> Nicolas Pitre <nico@fluxnic.net> writes:\n> \n> > The whole point is to detect data incoherencyes.\n> \n> Yes.  We want to make sure that the SHA-1 we compute is over what we fed\n> deflate().\n> \n> > So current sequence of events is as follows:\n> >\n> > T0\twrite_sha1_file_prepare() is called\n> > T1\tstart initial SHA1 computation on data buffer\n> > T2\tin the middle of initial SHA1 computation\n> > T3\tend of initial SHA1 computation -> object name is determined\n> > T4\twrite_loose_object() is called\n> > ...\tenter the write loop\n> > T5+n\tdeflate() called on buffer n\n> > T6+n\tgit_SHA1_Update(() called on the same buffer n\n> > T7+n\tdeflated data written out\n> > ...\n> > Tend\tabort if result of T6+n doesn't match object name from T3\n> >\n> > So... what can happen:\n> >\n> > 1) Data is externally modified before T5+n: deflated data and its CRC32 \n> >    will be coherent with the SHA1 computed in T6+n, but incoherent with \n> >    the SHA1 used for the object name. Wrong data is written to the \n> >    object even if it will inflate OK. We really want to prevent that \n> >    from happening. The test in Tend will fail.\n> >\n> > 2) Data is externally modified between T5+n and T6+n: the deflated data \n> >    and CRC32 will be coherent with the object name but incoherent with \n> >    the parano_sha1.  Although written data will be OK, this is way too \n> >    close from being wrong, and the test in Tend will fail.  If there is \n> >    more than one round into the loop and the external modifications are \n> >    large enough then this becomes the same as case 1 above.\n> >\n> > 3) Data is externally modified in T2: again the test in Tend will fail.\n> >\n> > So in all possible cases I can think of, the write will abort.\n> \n> There is one pathological case.\n> \n> Immediately before T5+n (or between T5+n and T6+n), the external process\n> changes the data deflate() is working on, but before T6+n, the external\n> process changes the data back.  Two SHA-1's computed may match, but it is\n> not a hash over what was deflated(); you won't be able to abort.\n\nAnd what real life case would trigger this?  Given the size of the \nwindow for this to happen, what are your chances?\n\nOf course the odds for me to be struck by lightning also exist.  And if \nI work really really hard at it then I might be able to trigger that \npathological case above even before the next thunderstorm.  But in \npractice I'm hardly concerned by either of those possibilities.\n\n\nNicolas\n"},{"id":"135245","messageId":"7v8walyesu.fsf@alter.siamese.dyndns.org","threadId":"22739","inReplyTo":"alpine.LFD.2.00.1002220034540.1946@xanadu.home","subject":"Re: [PATCH] sha1_file: don't malloc the whole compressed result when writing out objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-22T06:17:53Z","receivedAt":"2010-02-22T06:17:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@fluxnic.net> writes:\n\n> And what real life case would trigger this?  Given the size of the \n> window for this to happen, what are your chances?\n\n> Of course the odds for me to be struck by lightning also exist.  And if \n> I work really really hard at it then I might be able to trigger that \n> pathological case above even before the next thunderstorm.  But in \n> practice I'm hardly concerned by either of those possibilities.\n\nThe real life case for any of this triggers for me is zero, as I won't be\nmistreating git as a continuous & asynchronous back-up tool.\n\nBut then that would make the whole discussion moot.  There are people who\nfile \"bug reports\" with an artificial reproduction recipe built around a\nloop that runs dd continuously overwriting a file while \"git add\" is asked\nto add it.\n"},{"id":"135266","messageId":"20100222062713.GD10191@dpotapov.dyndns.org","threadId":"22739","inReplyTo":"alpine.LFD.2.00.1002220034540.1946@xanadu.home","subject":"Re: [PATCH] sha1_file: don't malloc the whole compressed result when writing out objects","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-22T06:27:13Z","receivedAt":"2010-02-22T06:27:13Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Mon, Feb 22, 2010 at 12:50:18AM -0500, Nicolas Pitre wrote:\n> \n> And what real life case would trigger this?  Given the size of the \n> window for this to happen, what are your chances?\n\nIf some process changes just one byte (or one word) back and forth\nthen the possibility of this is 25%. Whether such a process can\nexist, I don't know... I could not imagine that anyone would want\nto change the file when we add it to the repository. In any case,\nit is wrong to call it a _paranoic_ mode if there is even a small\n(but practically feasable) chance of this to happen.\n\n\nDmitry\n"},{"id":"135265","messageId":"7v4ol9vl0l.fsf@alter.siamese.dyndns.org","threadId":"22739","inReplyTo":"7v8walyesu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] sha1_file: don't malloc the whole compressed result when writing out objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-22T06:31:54Z","receivedAt":"2010-02-22T06:31:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Nicolas Pitre <nico@fluxnic.net> writes:\n>\n>> And what real life case would trigger this?  Given the size of the \n>> window for this to happen, what are your chances?\n>\n>> Of course the odds for me to be struck by lightning also exist.  And if \n>> I work really really hard at it then I might be able to trigger that \n>> pathological case above even before the next thunderstorm.  But in \n>> practice I'm hardly concerned by either of those possibilities.\n>\n> The real life case for any of this triggers for me is zero, as I won't be\n> mistreating git as a continuous & asynchronous back-up tool.\n>\n> But then that would make the whole discussion moot.  There are people who\n> file \"bug reports\" with an artificial reproduction recipe built around a\n> loop that runs dd continuously overwriting a file while \"git add\" is asked\n> to add it.\n\nHaving said all that, I like your approach better.  It is not worth paying\nthe price of unnecessary memcpy(3) that would _only_ help catching the\ninsanely artificial test case, but your patch strikes a good balance of\nsmall overhead to catch the easier-to-trigger (either by stupidity, malice\nor mistake) cases.\n\nSo I am tempted to discard the \"paranoia\" patch, and replace with your two\npatches, with the following caveats in the log message.\n\n--- /var/tmp/2\t2010-02-21 22:23:30.000000000 -0800\n+++ /var/tmp/1\t2010-02-21 22:23:22.000000000 -0800\n@@ -21,7 +21,9 @@\n     deflate operation has consumed that data, and make sure it matches\n     with the expected SHA1.  This way we can rely on the CRC32 checked by\n     the inflate operation to provide a good indication that the data is still\n-    coherent with its SHA1 hash.\n+    coherent with its SHA1 hash.  One pathological case we ignore is when\n+    the data is modified before (or during) deflate call, but changed back\n+    before it is hashed.\n     \n     There is some overhead of course. Using 'git add' on a set of large files:\n     \n"},{"id":"135327","messageId":"alpine.LFD.2.00.1002221233000.1946@xanadu.home","threadId":"22739","inReplyTo":"7v4ol9vl0l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] sha1_file: don't malloc the whole compressed result when writing out objects","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-22T17:36:39Z","receivedAt":"2010-02-22T17:36:39Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 21 Feb 2010, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Nicolas Pitre <nico@fluxnic.net> writes:\n> >\n> >> And what real life case would trigger this?  Given the size of the \n> >> window for this to happen, what are your chances?\n> >\n> >> Of course the odds for me to be struck by lightning also exist.  And if \n> >> I work really really hard at it then I might be able to trigger that \n> >> pathological case above even before the next thunderstorm.  But in \n> >> practice I'm hardly concerned by either of those possibilities.\n> >\n> > The real life case for any of this triggers for me is zero, as I won't be\n> > mistreating git as a continuous & asynchronous back-up tool.\n> >\n> > But then that would make the whole discussion moot.  There are people who\n> > file \"bug reports\" with an artificial reproduction recipe built around a\n> > loop that runs dd continuously overwriting a file while \"git add\" is asked\n> > to add it.\n> \n> Having said all that, I like your approach better.  It is not worth paying\n> the price of unnecessary memcpy(3) that would _only_ help catching the\n> insanely artificial test case, but your patch strikes a good balance of\n> small overhead to catch the easier-to-trigger (either by stupidity, malice\n> or mistake) cases.\n\nI think it also catches the bad RAM case which is probably more common \ntoo.\n\n> So I am tempted to discard the \"paranoia\" patch, and replace with your two\n> patches, with the following caveats in the log message.\n> \n> --- /var/tmp/2\t2010-02-21 22:23:30.000000000 -0800\n> +++ /var/tmp/1\t2010-02-21 22:23:22.000000000 -0800\n> @@ -21,7 +21,9 @@\n>      deflate operation has consumed that data, and make sure it matches\n>      with the expected SHA1.  This way we can rely on the CRC32 checked by\n>      the inflate operation to provide a good indication that the data is still\n> -    coherent with its SHA1 hash.\n> +    coherent with its SHA1 hash.  One pathological case we ignore is when\n> +    the data is modified before (or during) deflate call, but changed back\n> +    before it is hashed.\n\nACK.\n\n\nNicolas\n"},{"id":"299157","messageId":"7vmxz1dozk.fsf@alter.siamese.dyndns.org","threadId":"22739","inReplyTo":"alpine.LFD.2.00.1002221233000.1946@xanadu.home","subject":"Re: [PATCH] sha1_file: don't malloc the whole compressed result when writing out objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-22T19:55:43Z","receivedAt":"2010-02-22T19:55:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@fluxnic.net> writes:\n\n>> Having said all that, I like your approach better.  It is not worth paying\n>> the price of unnecessary memcpy(3) that would _only_ help catching the\n>> insanely artificial test case, but your patch strikes a good balance of\n>> small overhead to catch the easier-to-trigger (either by stupidity, malice\n>> or mistake) cases.\n>\n> I think it also catches the bad RAM case which is probably more common \n> too.\n\nThat is true; a broken RAM that returns unstable values will yield\ndifferent values between the time the first hash runs and the time the\ndeflate loop runs will trigger the safety.\n"}]}