{"thread":{"id":"29012","subject":"Infinite loop in cascade_filter_fn()","startedAt":"2011-11-23T17:40:47Z","lastAt":"2011-12-19T20:23:09Z","messageCount":16,"participants":["Henrik Grubbström","Carlos Martín Nieto","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"179897","messageId":"Pine.GSO.4.63.1111231801580.5099@shipon.roxen.com","threadId":"29012","inReplyTo":null,"subject":"Infinite loop in cascade_filter_fn()","fromName":"Henrik Grubbström","fromEmail":"grubba@grubba.org","sentAt":"2011-11-23T17:40:47Z","receivedAt":"2011-11-23T17:40:47Z","isPatch":false,"sender":{"key":"grubba@grubba.org","avatar":"https://avatars.githubusercontent.com/u/1169458?v=4"},"body":"Hi.\n\nMy git repository walker just got bitten by what seems to be a reasonably \nnew bug in convert.c:cascade_filter_fn() (git 1.7.8.rc3 (gentoo)).\n\nHow to reproduce:\n\n git clone git@github.com:pikelang/Pike.git\n\n git checkout -f 0e2080f838c6f0bc7d670ac7549676a353451dca^\n\n git checkout -f 0e2080f838c6f0bc7d670ac7549676a353451dca\n\nThe first two commands complete as expected, while the last hangs forever.\nPerforming the same with git 1.7.6.4 works as expected.\n\nThe problematic file seems to be /src/modules/_Crypto/rijndael_ecb_vt.txt \nwhich has the attributes: text ident eol=crlf\n\nThanks,\n\n--\nHenrik Grubbström\t\t\t\t\tgrubba@grubba.org\nRoxen Internet Software AB\t\t\t\tgrubba@roxen.com"},{"id":"179966","messageId":"20111125143131.GA10417@beez.lab.cmartin.tk","threadId":"29012","inReplyTo":"Pine.GSO.4.63.1111231801580.5099@shipon.roxen.com","subject":"Re: Infinite loop in cascade_filter_fn()","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-11-25T14:31:31Z","receivedAt":"2011-11-25T14:31:31Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Wed, Nov 23, 2011 at 06:40:47PM +0100, Henrik Grubbström wrote:\n> Hi.\n> \n> My git repository walker just got bitten by what seems to be a\n> reasonably new bug in convert.c:cascade_filter_fn() (git 1.7.8.rc3\n> (gentoo)).\n\nIt looks like it's a bug between cascade_filter_fn and the actual\nfilter function lf_to_crlf_filter_fn that gets triggered when the\noutput buffer is too small. In this particular case, *isize_p=378 and\n*osize_p=1 which causes cascade_filter_fn to feed the filter data\nwhich it can't process because it doesn't have anywhere to put it.\n\nI think that the function assumes that the output buffer is always\nlarge enough, but there are many indirections, so it might be an\noff-by-one.\n\n> \n> How to reproduce:\n> \n> git clone git@github.com:pikelang/Pike.git\n> \n> git checkout -f 0e2080f838c6f0bc7d670ac7549676a353451dca^\n> \n> git checkout -f 0e2080f838c6f0bc7d670ac7549676a353451dca\n> \n> The first two commands complete as expected, while the last hangs forever.\n> Performing the same with git 1.7.6.4 works as expected.\n> \n> The problematic file seems to be\n> /src/modules/_Crypto/rijndael_ecb_vt.txt which has the attributes:\n> text ident eol=crlf\n> \n> Thanks,\n> \n> --\n> Henrik Grubbström\t\t\t\t\tgrubba@grubba.org\n> Roxen Internet Software AB\t\t\t\tgrubba@roxen.com\n\n"},{"id":"179969","messageId":"20111125153829.GB10417@beez.lab.cmartin.tk","threadId":"29012","inReplyTo":"Pine.GSO.4.63.1111231801580.5099@shipon.roxen.com","subject":"Re: Infinite loop in cascade_filter_fn()","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-11-25T15:38:29Z","receivedAt":"2011-11-25T15:38:29Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Wed, Nov 23, 2011 at 06:40:47PM +0100, Henrik Grubbström wrote:\n> Hi.\n> \n> My git repository walker just got bitten by what seems to be a\n> reasonably new bug in convert.c:cascade_filter_fn() (git 1.7.8.rc3\n> (gentoo)).\n> \n> How to reproduce:\n> \n> git clone git@github.com:pikelang/Pike.git\n> \n> git checkout -f 0e2080f838c6f0bc7d670ac7549676a353451dca^\n> \n> git checkout -f 0e2080f838c6f0bc7d670ac7549676a353451dca\n> \n> The first two commands complete as expected, while the last hangs forever.\n> Performing the same with git 1.7.6.4 works as expected.\n> \n> The problematic file seems to be\n> /src/modules/_Crypto/rijndael_ecb_vt.txt which has the attributes:\n> text ident eol=crlf\n\nIt looks like you won the lottery. The problem was that the output\nbuffer only has one byte available when we see a LF. We check whether\nthere is enough space (two bytes) to store CRLF in the output buffer,\nsee that there isn't and return. cascade_filter_fn sees that the\nbuffer hasn't been written fully and calls lf_to_crlf_filter_fn with\nthe same output buffer, which we still can't fill, because it's too\nshort.\n\nThis patch fixes this, but I think it would still break if the LF is\nat the end of the file. Changing the `if (!input)` to put the LF in\nthe output buffer may or may not be the right soulution. I feel like\nthis should be handled by cascade_filter_fn rather than the actual\nfilter somehow, but Junio's comment (4ae66704 'stream filter: add \"no\nmore input\" to the filters') suggests otherwise.\n\nI'm working on a cleaner patch that takes care of a bit of state, but\nthis is the general idea.\n\n cmn\n--- 8< ---\nSubject: [PATCH] convert: don't loop indefintely if at LF-to-CRLF streaming\n\nIf we find a LF when the output buffer is only has one byte remaining,\ncascade_filter_fn won't notice that we need more input and won't drain\nthe output buffer.\n\nIn such a case, store whether we've outputted the CR so we can retake\nit from there.\n\nSigned-off-by: Carlos Martín Nieto \n---\n convert.c | 11 ++++++++---\n 1 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 86e9c29..4218f40 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -881,6 +881,7 @@ static int lf_to_crlf_filter_fn(struct stream_filter *filter,\n \t\t\t\tchar *output, size_t *osize_p)\n {\n \tsize_t count;\n+\tstatic int put_cr = 0;\n \n \tif (!input)\n \t\treturn 0; /* we do not keep any states */\n@@ -890,10 +891,14 @@ static int lf_to_crlf_filter_fn(struct stream_filter *filter,\n \t\tfor (i = o = 0; o < *osize_p && i < count; i++) {\n \t\t\tchar ch = input[i];\n \t\t\tif (ch == '\\n') {\n-\t\t\t\tif (o + 1 < *osize_p)\n+\t\t\t\tif (put_cr) {\n+\t\t\t\t\tput_cr = 0;\n+\t\t\t\t} else {\n \t\t\t\t\toutput[o++] = '\\r';\n-\t\t\t\telse\n-\t\t\t\t\tbreak;\n+\t\t\t\t\tput_cr = 1;\n+\t\t\t\t\ti--;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n \t\t\t}\n \t\t\toutput[o++] = ch;\n \t\t}\n-- \n1.7.8.rc3.31.g017d1\n\n"},{"id":"179970","messageId":"Pine.GSO.4.63.1111251629500.22588@shipon.roxen.com","threadId":"29012","inReplyTo":"Pine.GSO.4.63.1111231801580.5099@shipon.roxen.com","subject":"Re: Infinite loop in cascade_filter_fn()","fromName":"Henrik Grubbström","fromEmail":"grubba@roxen.com","sentAt":"2011-11-25T15:43:41Z","receivedAt":"2011-11-25T15:43:41Z","isPatch":false,"sender":{"key":"grubba@roxen.com","avatar":null},"body":"On Wed, 23 Nov 2011, Henrik Grubbström wrote:\n\n> Hi.\n>\n> My git repository walker just got bitten by what seems to be a reasonably new \n> bug in convert.c:cascade_filter_fn() (git 1.7.8.rc3 (gentoo)).\n\nAfter some tracing, the problem is triggered by the variable \"remaining\"\nbeing set to 1 in the beginning of the cascade_filter_fn() loop, which \ncauses filter \"two\" to be called with an output buffer size of 1.\nFilter \"two\" in this case is lf_to_crlf_filter_fn(), and the next input \ncharacter is a \"\\n\". lf_to_crlf_filter_fn() wants to convert this to \n\"\\r\\n\", but that doesn't fit into the buffer, so it breaks out and returns \nzero. Upon seing the zero cascade_filter_fn() thinks all is well, even \nthough nothing has happened, and loops.\n\nThe bug is probably that lf_to_crlf_filter_fn() should return non-zero in \nthis case (ie o and/or i being zero).\n\n> Thanks,\n\n--\nHenrik Grubbström\t\t\t\t\tgrubba@roxen.com\nRoxen Internet Software AB"},{"id":"179971","messageId":"20111125155301.GC10417@beez.lab.cmartin.tk","threadId":"29012","inReplyTo":"Pine.GSO.4.63.1111251629500.22588@shipon.roxen.com","subject":"Re: Infinite loop in cascade_filter_fn()","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-11-25T15:53:01Z","receivedAt":"2011-11-25T15:53:01Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Fri, Nov 25, 2011 at 04:43:41PM +0100, Henrik Grubbström wrote:\n> On Wed, 23 Nov 2011, Henrik Grubbström wrote:\n> \n> >Hi.\n> >\n> >My git repository walker just got bitten by what seems to be a\n> >reasonably new bug in convert.c:cascade_filter_fn() (git 1.7.8.rc3\n> >(gentoo)).\n> \n> After some tracing, the problem is triggered by the variable \"remaining\"\n> being set to 1 in the beginning of the cascade_filter_fn() loop,\n> which causes filter \"two\" to be called with an output buffer size of\n> 1.\n> Filter \"two\" in this case is lf_to_crlf_filter_fn(), and the next\n> input character is a \"\\n\". lf_to_crlf_filter_fn() wants to convert\n> this to \"\\r\\n\", but that doesn't fit into the buffer, so it breaks\n> out and returns zero. Upon seing the zero cascade_filter_fn() thinks\n> all is well, even though nothing has happened, and loops.\n> \n> The bug is probably that lf_to_crlf_filter_fn() should return\n> non-zero in this case (ie o and/or i being zero).\n\nnon-zero? That would cause the filter to abort, which definitely not\nwhat we want. Have you seen my other e-mails regarding this? I'm\ntrying to figure out which is the best way to go about this. The\nsolution is to keep track of the fact that we're missing a LF in the\noutput buffer.\n\n cmn\n"},{"id":"179972","messageId":"Pine.GSO.4.63.1111251656200.22588@shipon.roxen.com","threadId":"29012","inReplyTo":"20111125155301.GC10417@beez.lab.cmartin.tk","subject":"Re: Infinite loop in cascade_filter_fn()","fromName":"Henrik Grubbström","fromEmail":"grubba@roxen.com","sentAt":"2011-11-25T15:59:00Z","receivedAt":"2011-11-25T15:59:00Z","isPatch":false,"sender":{"key":"grubba@roxen.com","avatar":null},"body":"On Fri, 25 Nov 2011, Carlos Martín Nieto wrote:\n\n> On Fri, Nov 25, 2011 at 04:43:41PM +0100, Henrik Grubbström wrote:\n>>\n>> The bug is probably that lf_to_crlf_filter_fn() should return\n>> non-zero in this case (ie o and/or i being zero).\n>\n> non-zero? That would cause the filter to abort, which definitely not\n> what we want. Have you seen my other e-mails regarding this? I'm\n> trying to figure out which is the best way to go about this. The\n> solution is to keep track of the fact that we're missing a LF in the\n> output buffer.\n\nTrue, I misread the code.\n\nKeeping track of the filter state is the way to go.\n\n> cmn\n\n--\nHenrik Grubbström\t\t\t\t\tgrubba@roxen.com\nRoxen Internet Software AB"},{"id":"179973","messageId":"Pine.GSO.4.63.1111251705330.22588@shipon.roxen.com","threadId":"29012","inReplyTo":"20111125153829.GB10417@beez.lab.cmartin.tk","subject":"Re: Infinite loop in cascade_filter_fn()","fromName":"Henrik Grubbström","fromEmail":"grubba@roxen.com","sentAt":"2011-11-25T16:14:17Z","receivedAt":"2011-11-25T16:14:17Z","isPatch":false,"sender":{"key":"grubba@roxen.com","avatar":null},"body":"On Fri, 25 Nov 2011, Carlos Martín Nieto wrote:\n\n> This patch fixes this, but I think it would still break if the LF is\n> at the end of the file. Changing the `if (!input)` to put the LF in\n> the output buffer may or may not be the right soulution. I feel like\n> this should be handled by cascade_filter_fn rather than the actual\n> filter somehow, but Junio's comment (4ae66704 'stream filter: add \"no\n> more input\" to the filters') suggests otherwise.\n>\n> I'm working on a cleaner patch that takes care of a bit of state, but\n> this is the general idea.\n\nLooks good to me (and seems to work in my case).\nTypo in the commit subject though.\n\n> cmn\n> --- 8< ---\n> Subject: [PATCH] convert: don't loop indefintely if at LF-to-CRLF streaming\n ^^^^^^^^^^^\nThis should be either \"infinitely\", or \"indefinitely\", but since we know \nthat the loop won't terminate \"infinitely\" is to be preferred.\n\nThanks,\n\n--\nHenrik Grubbström\t\t\t\t\tgrubba@roxen.com\nRoxen Internet Software AB"},{"id":"179976","messageId":"20111125170219.GD10417@beez.lab.cmartin.tk","threadId":"29012","inReplyTo":"Pine.GSO.4.63.1111251705330.22588@shipon.roxen.com","subject":"Re: Infinite loop in cascade_filter_fn()","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-11-25T17:02:19Z","receivedAt":"2011-11-25T17:02:19Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Fri, Nov 25, 2011 at 05:14:17PM +0100, Henrik Grubbström wrote:\n> On Fri, 25 Nov 2011, Carlos Martín Nieto wrote:\n> \n> >This patch fixes this, but I think it would still break if the LF is\n> >at the end of the file. Changing the `if (!input)` to put the LF in\n> >the output buffer may or may not be the right soulution. I feel like\n> >this should be handled by cascade_filter_fn rather than the actual\n> >filter somehow, but Junio's comment (4ae66704 'stream filter: add \"no\n> >more input\" to the filters') suggests otherwise.\n> >\n> >I'm working on a cleaner patch that takes care of a bit of state, but\n> >this is the general idea.\n> \n> Looks good to me (and seems to work in my case).\n\nThat patch would give wrong output if the same happened at the end of\na file. The attached patch should also cover this case.\n\n> Typo in the commit subject though.\n> \n> > cmn\n> >--- 8< ---\n> >Subject: [PATCH] convert: don't loop indefintely if at LF-to-CRLF streaming\n> ^^^^^^^^^^^\n> This should be either \"infinitely\", or \"indefinitely\", but since we\n> know that the loop won't terminate \"infinitely\" is to be preferred.\n\nThanks for noticing. I went with a different title in the end. Junio,\ncould you consider this one for inclusion in the next RC?\n\n--- 8< ---\nSubject: [PATCH] convert: track state in LF-to-CRLF filter\n\nThere may not be enough space to store CRLF in the output. If we don't\nfill the buffer, then the filter will keep getting called with the same\nshort buffer and will loop forever.\n\nInstead, always store the CR and record there's a missing LF if\nnecessary it so we store it in the output buffer the next time the\nfunction gets called.\n\nReported-by: Henrik Grubbström \nSigned-off-by: Carlos Martín Nieto \n---\n convert.c | 23 ++++++++++++++++-------\n 1 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 86e9c29..c050b86 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -880,20 +880,29 @@ static int lf_to_crlf_filter_fn(struct stream_filter *filter,\n \t\t\t\tconst char *input, size_t *isize_p,\n \t\t\t\tchar *output, size_t *osize_p)\n {\n-\tsize_t count;\n+\tsize_t count, o = 0;\n+\tstatic int want_lf = 0;\n+\n+\t/* Output a pending LF if we need to */\n+\tif (want_lf) {\n+\t\toutput[o++] = '\\n';\n+\t\twant_lf = 0;\n+\t}\n \n \tif (!input)\n-\t\treturn 0; /* we do not keep any states */\n+\t\treturn 0; /* We've already dealt with the state */\n+\n \tcount = *isize_p;\n \tif (count) {\n-\t\tsize_t i, o;\n-\t\tfor (i = o = 0; o < *osize_p && i < count; i++) {\n+\t\tsize_t i;\n+\t\tfor (i = 0; o < *osize_p && i < count; i++) {\n \t\t\tchar ch = input[i];\n \t\t\tif (ch == '\\n') {\n-\t\t\t\tif (o + 1 < *osize_p)\n-\t\t\t\t\toutput[o++] = '\\r';\n-\t\t\t\telse\n+\t\t\t\toutput[o++] = '\\r';\n+\t\t\t\tif (o >= *osize_p) {\n+\t\t\t\t\twant_lf = 1;\n \t\t\t\t\tbreak;\n+\t\t\t\t}\n \t\t\t}\n \t\t\toutput[o++] = ch;\n \t\t}\n-- \n1.7.8.rc3.31.g017d1\n\n\n"},{"id":"179990","messageId":"7vy5v2wleb.fsf@alter.siamese.dyndns.org","threadId":"29012","inReplyTo":"20111125170219.GD10417@beez.lab.cmartin.tk","subject":"Re: Infinite loop in cascade_filter_fn()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-26T22:48:12Z","receivedAt":"2011-11-26T22:48:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> diff --git a/convert.c b/convert.c\n> index 86e9c29..c050b86 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -880,20 +880,29 @@ static int lf_to_crlf_filter_fn(struct stream_filter *filter,\n> \t\t\t\tconst char *input, size_t *isize_p,\n> \t\t\t\tchar *output, size_t *osize_p)\n> {\n> -\tsize_t count;\n> +\tsize_t count, o = 0;\n> +\tstatic int want_lf = 0;\n\nI do not think we want function scope static state anywhere in the cascade\nfilter chain, as it will forbid us from running more than one output chain\nat the same time in the future. I think the correct way to structure it\nwould be to create lf_to_crlf_filter as a proper subclass of stream_filter\n(see how cascade_filter_fn() casts its filter argument down to an instance\nof the cascade_filter class and uses it to keep track of its state) and\nkeep this variable as its own filter state [*1*].\n\n[Footnote]\n\n*1* We currently use a singleton instance of lf_to_crlf_filter object\nbecause the implementation assumed there is no need for per-instance\nstate.\n"},{"id":"180041","messageId":"20111128104812.GA2386@beez.lab.cmartin.tk","threadId":"29012","inReplyTo":"7vy5v2wleb.fsf@alter.siamese.dyndns.org","subject":"Re: Infinite loop in cascade_filter_fn()","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-11-28T10:48:12Z","receivedAt":"2011-11-28T10:48:12Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Sat, Nov 26, 2011 at 02:48:12PM -0800, Junio C Hamano wrote:\n> Carlos Martín Nieto writes:\n> \n> > diff --git a/convert.c b/convert.c\n> > index 86e9c29..c050b86 100644\n> > --- a/convert.c\n> > +++ b/convert.c\n> > @@ -880,20 +880,29 @@ static int lf_to_crlf_filter_fn(struct stream_filter *filter,\n> > \t\t\t\tconst char *input, size_t *isize_p,\n> > \t\t\t\tchar *output, size_t *osize_p)\n> > {\n> > -\tsize_t count;\n> > +\tsize_t count, o = 0;\n> > +\tstatic int want_lf = 0;\n> \n> I do not think we want function scope static state anywhere in the cascade\n> filter chain, as it will forbid us from running more than one output chain\n> at the same time in the future. I think the correct way to structure it\n> would be to create lf_to_crlf_filter as a proper subclass of stream_filter\n> (see how cascade_filter_fn() casts its filter argument down to an instance\n> of the cascade_filter class and uses it to keep track of its state) and\n> keep this variable as its own filter state [*1*].\n\nGood point, here's a patch that does that.\n\n cmn\n\n--- 8< ---\nSubject: [PATCHv2] convert: track state in LF-to-CRLF filter\n\nThere may not be enough space to store CRLF in the output. If we don't\nfill the buffer, then the filter will keep getting called with the same\nshort buffer and will loop forever.\n\nInstead, always store the CR and record whether there's a missing LF\nif so we store it in the output buffer the next time the function gets\ncalled.\n\nReported-by: Henrik Grubbström \nSigned-off-by: Carlos Martín Nieto \n---\n convert.c | 50 +++++++++++++++++++++++++++++++++++++-------------\n 1 files changed, 37 insertions(+), 13 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 86e9c29..1c91409 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -876,24 +876,39 @@ int is_null_stream_filter(struct stream_filter *filter)\n /*\n * LF-to-CRLF filter\n */\n+\n+struct lf_to_crlf_filter {\n+\tstruct stream_filter filter;\n+\tint want_lf;\n+};\n+\n static int lf_to_crlf_filter_fn(struct stream_filter *filter,\n \t\t\t\tconst char *input, size_t *isize_p,\n \t\t\t\tchar *output, size_t *osize_p)\n {\n-\tsize_t count;\n+\tsize_t count, o = 0;\n+\tstruct lf_to_crlf_filter *lfcrlf = (struct lf_to_crlf_filter *) filter;\n+\n+\t/* Output a pending LF if we need to */\n+\tif (lfcrlf->want_lf) {\n+\t\toutput[o++] = '\\n';\n+\t\tlfcrlf->want_lf = 0;\n+\t}\n \n \tif (!input)\n-\t\treturn 0; /* we do not keep any states */\n+\t\treturn 0; /* We've already dealt with the state */\n+\n \tcount = *isize_p;\n \tif (count) {\n-\t\tsize_t i, o;\n-\t\tfor (i = o = 0; o < *osize_p && i < count; i++) {\n+\t\tsize_t i;\n+\t\tfor (i = 0; o < *osize_p && i < count; i++) {\n \t\t\tchar ch = input[i];\n \t\t\tif (ch == '\\n') {\n-\t\t\t\tif (o + 1 < *osize_p)\n-\t\t\t\t\toutput[o++] = '\\r';\n-\t\t\t\telse\n-\t\t\t\t\tbreak;\n+\t\t\t\toutput[o++] = '\\r';\n+\t\t\t\tif (o >= *osize_p) {\n+\t\t\t\t\tlfcrlf->want_lf = 1;\n+\t\t\t\t\tcontinue; /* We need to increase i */\n+\t\t\t\t}\n \t\t\t}\n \t\t\toutput[o++] = ch;\n \t\t}\n@@ -904,15 +919,24 @@ static int lf_to_crlf_filter_fn(struct stream_filter *filter,\n \treturn 0;\n }\n \n+static void lf_to_crlf_free_fn(struct stream_filter *filter)\n+{\n+\tfree(filter);\n+}\n+\n static struct stream_filter_vtbl lf_to_crlf_vtbl = {\n \tlf_to_crlf_filter_fn,\n-\tnull_free_fn,\n+\tlf_to_crlf_free_fn,\n };\n \n-static struct stream_filter lf_to_crlf_filter_singleton = {\n-\t&lf_to_crlf_vtbl,\n-};\n+static struct stream_filter *lf_to_crlf_filter(void)\n+{\n+\tstruct lf_to_crlf_filter *lfcrlf = xmalloc(sizeof(*lfcrlf));\n \n+\tlfcrlf->filter.vtbl = &lf_to_crlf_vtbl;\n+\tlfcrlf->want_lf = 0;\n+\treturn (struct stream_filter *)lfcrlf;\n+}\n \n /*\n * Cascade filter\n@@ -1194,7 +1218,7 @@ struct stream_filter *get_stream_filter(const char *path, const unsigned char *s\n \n \telse if (output_eol(crlf_action) == EOL_CRLF &&\n \t\t !(crlf_action == CRLF_AUTO || crlf_action == CRLF_GUESS))\n-\t\tfilter = cascade_filter(filter, &lf_to_crlf_filter_singleton);\n+\t\tfilter = cascade_filter(filter, lf_to_crlf_filter());\n \n \treturn filter;\n }\n-- \n1.7.8.rc3.31.g017d1\n\n\n"},{"id":"180059","messageId":"7vobvwukcv.fsf@alter.siamese.dyndns.org","threadId":"29012","inReplyTo":"20111128104812.GA2386@beez.lab.cmartin.tk","subject":"Re: Infinite loop in cascade_filter_fn()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-28T19:18:08Z","receivedAt":"2011-11-28T19:18:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n>> Content-Type: multipart/signed; micalg=pgp-sha1; protocol=\"application/pgp-signature\"; boundary=\"M9NhX3UHpAaciwkO\"\n>> Content-Disposition: inline\n\nPlease do not do this. It makes it unnecessarily cumbersome to handle\npatches without adding much value to the patch.\n\n> --- 8< ---\n> Subject: [PATCHv2] convert: track state in LF-to-CRLF filter\n>\n> There may not be enough space to store CRLF in the output. If we don't\n> fill the buffer, then the filter will keep getting called with the same\n> short buffer and will loop forever.\n>\n> Instead, always store the CR and record whether there's a missing LF\n> if so we store it in the output buffer the next time the function gets\n> called.\n>\n> Reported-by: Henrik Grubbström \n> Signed-off-by: Carlos Martín Nieto \n> ---\n> convert.c | 50 +++++++++++++++++++++++++++++++++++++-------------\n> 1 files changed, 37 insertions(+), 13 deletions(-)\n>\n> diff --git a/convert.c b/convert.c\n> index 86e9c29..1c91409 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -876,24 +876,39 @@ int is_null_stream_filter(struct stream_filter *filter)\n> /*\n> * LF-to-CRLF filter\n> */\n> +\n> +struct lf_to_crlf_filter {\n> +\tstruct stream_filter filter;\n> +\tint want_lf;\n> +};\n> +\n> static int lf_to_crlf_filter_fn(struct stream_filter *filter,\n> \t\t\t\tconst char *input, size_t *isize_p,\n> \t\t\t\tchar *output, size_t *osize_p)\n> {\n> -\tsize_t count;\n> +\tsize_t count, o = 0;\n> +\tstruct lf_to_crlf_filter *lfcrlf = (struct lf_to_crlf_filter *) filter;\n> ...\n> -};\n> +static struct stream_filter *lf_to_crlf_filter(void)\n> +{\n> +\tstruct lf_to_crlf_filter *lfcrlf = xmalloc(sizeof(*lfcrlf));\n> \n> +\tlfcrlf->filter.vtbl = &lf_to_crlf_vtbl;\n> +\tlfcrlf->want_lf = 0;\n> +\treturn (struct stream_filter *)lfcrlf;\n> +}\n\nPatch looks sane; you may want to rename the variable to lf_crlf at least,\nthough. The name does not consist of three tokens (\"lf\", \"cr\" and \"lf\")\nbut of two (\"lf\" and \"crlf\"), and your naming loses it.\n"},{"id":"181339","messageId":"7viplggoq9.fsf@alter.siamese.dyndns.org","threadId":"29012","inReplyTo":"20111128104812.GA2386@beez.lab.cmartin.tk","subject":"Re: Infinite loop in cascade_filter_fn()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-16T22:01:50Z","receivedAt":"2011-12-16T22:01:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> Subject: [PATCHv2] convert: track state in LF-to-CRLF filter\n>\n> There may not be enough space to store CRLF in the output. If we don't\n> fill the buffer, then the filter will keep getting called with the same\n> short buffer and will loop forever.\n>\n> Instead, always store the CR and record whether there's a missing LF\n> if so we store it in the output buffer the next time the function gets\n> called.\n>\n> Reported-by: Henrik Grubbström \n> Signed-off-by: Carlos Martín Nieto \n> ---\n> convert.c | 50 +++++++++++++++++++++++++++++++++++++-------------\n> 1 files changed, 37 insertions(+), 13 deletions(-)\n>\n> diff --git a/convert.c b/convert.c\n> index 86e9c29..1c91409 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -876,24 +876,39 @@ int is_null_stream_filter(struct stream_filter *filter)\n> /*\n> * LF-to-CRLF filter\n> */\n> +\n> +struct lf_to_crlf_filter {\n> +\tstruct stream_filter filter;\n> +\tint want_lf;\n> +};\n> +\n> static int lf_to_crlf_filter_fn(struct stream_filter *filter,\n> \t\t\t\tconst char *input, size_t *isize_p,\n> \t\t\t\tchar *output, size_t *osize_p)\n> {\n> -\tsize_t count;\n> +\tsize_t count, o = 0;\n> +\tstruct lf_to_crlf_filter *lfcrlf = (struct lf_to_crlf_filter *) filter;\n> +\n> +\t/* Output a pending LF if we need to */\n> +\tif (lfcrlf->want_lf) {\n> +\t\toutput[o++] = '\\n';\n> +\t\tlfcrlf->want_lf = 0;\n> +\t}\n> \n> \tif (!input)\n> -\t\treturn 0; /* we do not keep any states */\n> +\t\treturn 0; /* We've already dealt with the state */\n> +\n\nShouldn't we be decrementing *osize_p by 'o' to signal that we used that\nmany bytes in the output buffer here before returning to the caller?\n\n> \tcount = *isize_p;\n> \tif (count) {\n> -\t\tsize_t i, o;\n> -\t\tfor (i = o = 0; o < *osize_p && i < count; i++) {\n> +\t\tsize_t i;\n> +\t\tfor (i = 0; o < *osize_p && i < count; i++) {\n> \t\t\tchar ch = input[i];\n> \t\t\tif (ch == '\\n') {\n> -\t\t\t\tif (o + 1 < *osize_p)\n> -\t\t\t\t\toutput[o++] = '\\r';\n> -\t\t\t\telse\n> -\t\t\t\t\tbreak;\n> +\t\t\t\toutput[o++] = '\\r';\n> +\t\t\t\tif (o >= *osize_p) {\n> +\t\t\t\t\tlfcrlf->want_lf = 1;\n> +\t\t\t\t\tcontinue; /* We need to increase i */\n> +\t\t\t\t}\n> \t\t\t}\n> \t\t\toutput[o++] = ch;\n> \t\t}\n"},{"id":"181346","messageId":"7vaa6sgmt3.fsf_-_@alter.siamese.dyndns.org","threadId":"29012","inReplyTo":"7viplggoq9.fsf@alter.siamese.dyndns.org","subject":"[PATCH] lf_to_crlf_filter(): tell the caller we added \"\\n\" when draining","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-16T22:43:20Z","receivedAt":"2011-12-16T22:43:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This can only happen when the input size is multiple of the\nbuffer size of the cascade filter (16k) and ends with an LF,\nbut in such a case, the code forgot to tell the caller that\nit added the \"\\n\" it could not add during the last round.\n\nSigned-off-by: Junio C Hamano \n---\n\n convert.c | 12 +++++++-----\n 1 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex c2c2c11..c028275 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -879,7 +879,7 @@ int is_null_stream_filter(struct stream_filter *filter)\n \n struct lf_to_crlf_filter {\n \tstruct stream_filter filter;\n-\tint want_lf;\n+\tunsigned want_lf:1;\n };\n \n static int lf_to_crlf_filter_fn(struct stream_filter *filter,\n@@ -895,8 +895,11 @@ static int lf_to_crlf_filter_fn(struct stream_filter *filter,\n \t\tlf_to_crlf->want_lf = 0;\n \t}\n \n-\tif (!input)\n-\t\treturn 0; /* We've already dealt with the state */\n+\t/* We are told to drain */\n+\tif (!input) {\n+\t\t*osize_p -= o;\n+\t\treturn 0;\n+\t}\n \n \tcount = *isize_p;\n \tif (count) {\n@@ -931,10 +934,9 @@ static struct stream_filter_vtbl lf_to_crlf_vtbl = {\n \n static struct stream_filter *lf_to_crlf_filter(void)\n {\n-\tstruct lf_to_crlf_filter *lf_to_crlf = xmalloc(sizeof(*lf_to_crlf));\n+\tstruct lf_to_crlf_filter *lf_to_crlf = xcalloc(1, sizeof(*lf_to_crlf));\n \n \tlf_to_crlf->filter.vtbl = &lf_to_crlf_vtbl;\n-\tlf_to_crlf->want_lf = 0;\n \treturn (struct stream_filter *)lf_to_crlf;\n }\n \n-- \n1.7.8.351.g9111b\n"},{"id":"181460","messageId":"Pine.GSO.4.63.1112191114010.4136@shipon.roxen.com","threadId":"29012","inReplyTo":"7vaa6sgmt3.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] lf_to_crlf_filter(): tell the caller we added \"\\n\" when draining","fromName":"Henrik Grubbström","fromEmail":"grubba@roxen.com","sentAt":"2011-12-19T10:19:19Z","receivedAt":"2011-12-19T10:19:19Z","isPatch":true,"sender":{"key":"grubba@roxen.com","avatar":null},"body":"On Fri, 16 Dec 2011, Junio C Hamano wrote:\n\n> This can only happen when the input size is multiple of the\n> buffer size of the cascade filter (16k) and ends with an LF,\n> but in such a case, the code forgot to tell the caller that\n> it added the \"\\n\" it could not add during the last round.\n\nWe probably ought to have a corresponding test in the testsuite.\nA blob consisting of a singe 'A' followed by 8192 linefeeds should\nbe sufficient to trigger the problems.\n\n--\nHenrik Grubbström\t\t\t\t\tgrubba@roxen.com\nRoxen Internet Software AB"},{"id":"181473","messageId":"20111219164214.GA2160@beez.lab.cmartin.tk","threadId":"29012","inReplyTo":"7viplggoq9.fsf@alter.siamese.dyndns.org","subject":"Re: Infinite loop in cascade_filter_fn()","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-12-19T16:42:14Z","receivedAt":"2011-12-19T16:42:14Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Fri, Dec 16, 2011 at 02:01:50PM -0800, Junio C Hamano wrote:\n> Carlos Martín Nieto writes:\n> \n> > Subject: [PATCHv2] convert: track state in LF-to-CRLF filter\n> >\n> > There may not be enough space to store CRLF in the output. If we don't\n> > fill the buffer, then the filter will keep getting called with the same\n> > short buffer and will loop forever.\n> >\n> > Instead, always store the CR and record whether there's a missing LF\n> > if so we store it in the output buffer the next time the function gets\n> > called.\n> >\n> > Reported-by: Henrik Grubbström \n> > Signed-off-by: Carlos Martín Nieto \n> > ---\n> > convert.c | 50 +++++++++++++++++++++++++++++++++++++-------------\n> > 1 files changed, 37 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/convert.c b/convert.c\n> > index 86e9c29..1c91409 100644\n> > --- a/convert.c\n> > +++ b/convert.c\n> > @@ -876,24 +876,39 @@ int is_null_stream_filter(struct stream_filter *filter)\n> > /*\n> > * LF-to-CRLF filter\n> > */\n> > +\n> > +struct lf_to_crlf_filter {\n> > +\tstruct stream_filter filter;\n> > +\tint want_lf;\n> > +};\n> > +\n> > static int lf_to_crlf_filter_fn(struct stream_filter *filter,\n> > \t\t\t\tconst char *input, size_t *isize_p,\n> > \t\t\t\tchar *output, size_t *osize_p)\n> > {\n> > -\tsize_t count;\n> > +\tsize_t count, o = 0;\n> > +\tstruct lf_to_crlf_filter *lfcrlf = (struct lf_to_crlf_filter *) filter;\n> > +\n> > +\t/* Output a pending LF if we need to */\n> > +\tif (lfcrlf->want_lf) {\n> > +\t\toutput[o++] = '\\n';\n> > +\t\tlfcrlf->want_lf = 0;\n> > +\t}\n> > \n> > \tif (!input)\n> > -\t\treturn 0; /* we do not keep any states */\n> > +\t\treturn 0; /* We've already dealt with the state */\n> > +\n> \n> Shouldn't we be decrementing *osize_p by 'o' to signal that we used that\n> many bytes in the output buffer here before returning to the caller?\n\nYes we should, thanks for spotting it.\n\n> \n> > \tcount = *isize_p;\n> > \tif (count) {\n> > -\t\tsize_t i, o;\n> > -\t\tfor (i = o = 0; o < *osize_p && i < count; i++) {\n> > +\t\tsize_t i;\n> > +\t\tfor (i = 0; o < *osize_p && i < count; i++) {\n> > \t\t\tchar ch = input[i];\n> > \t\t\tif (ch == '\\n') {\n> > -\t\t\t\tif (o + 1 < *osize_p)\n> > -\t\t\t\t\toutput[o++] = '\\r';\n> > -\t\t\t\telse\n> > -\t\t\t\t\tbreak;\n> > +\t\t\t\toutput[o++] = '\\r';\n> > +\t\t\t\tif (o >= *osize_p) {\n> > +\t\t\t\t\tlfcrlf->want_lf = 1;\n> > +\t\t\t\t\tcontinue; /* We need to increase i */\n> > +\t\t\t\t}\n> > \t\t\t}\n> > \t\t\toutput[o++] = ch;\n> > \t\t}\n> \n"},{"id":"181486","messageId":"7v7h1smhua.fsf@alter.siamese.dyndns.org","threadId":"29012","inReplyTo":"Pine.GSO.4.63.1112191114010.4136@shipon.roxen.com","subject":"Re: [PATCH] lf_to_crlf_filter(): tell the caller we added \"\\n\" when draining","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-19T20:23:09Z","receivedAt":"2011-12-19T20:23:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Henrik Grubbström writes:\n\n> We probably ought to have a corresponding test in the testsuite.\n> A blob consisting of a singe 'A' followed by 8192 linefeeds should\n> be sufficient to trigger the problems.\n\nSounds good. Please make it so.\n"}]}