{"thread":{"id":"25212","subject":"[PATCH] mingw: do not crash on open(NULL, ...)","startedAt":"2010-09-23T17:35:25Z","lastAt":"2010-09-27T13:59:56Z","messageCount":12,"participants":["Erik Faye-Lund","Johannes Sixt","Pat Thoyts","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"151377","messageId":"1285263325-2016-1-git-send-email-kusmabite@gmail.com","threadId":"25212","inReplyTo":null,"subject":"[PATCH] mingw: do not crash on open(NULL, ...)","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2010-09-23T17:35:25Z","receivedAt":"2010-09-23T17:35:25Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Since open() already sets errno correctly for the NULL-case, let's just\navoid the problematic strcmp.\n\nSigned-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\n compat/mingw.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex b0ad919..b408c3c 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -297,7 +297,7 @@ int mingw_open (const char *filename, int oflags, ...)\n \tmode = va_arg(args, int);\n \tva_end(args);\n \n-\tif (!strcmp(filename, \"/dev/null\"))\n+\tif (filename && !strcmp(filename, \"/dev/null\"))\n \t\tfilename = \"nul\";\n \n \tfd = open(filename, oflags, mode);\n-- \n1.7.2.3.msysgit.0.207.gda0b3\n"},{"id":"151382","messageId":"AANLkTinJ4kKRsKO6HyqQH4Oy12E1mdqCXxPb2z+59818@mail.gmail.com","threadId":"25212","inReplyTo":"1285263325-2016-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH] mingw: do not crash on open(NULL, ...)","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2010-09-23T17:59:46Z","receivedAt":"2010-09-23T17:59:46Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Sep 23, 2010 at 10:35 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> Since open() already sets errno correctly for the NULL-case, let's just\n> avoid the problematic strcmp.\n>\n> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n\nI guess I should add a comment as to why this patch is needed:\n\nThis seems to be the culprit for issue 523 in the msysGit issue\ntracker: http://code.google.com/p/msysgit/issues/detail?id=523\n\nfetch_and_setup_pack_index() apparently pass a NULL-pointer to\nparse_pack_index(), which in turn pass it to check_packed_git_idx(),\nwhich again pass it to open(). This all looks intentional to my\n(http.c-untrained) eye.\n\nThe code in mingw_open was introduced in commit 3e4a1ba (by Johannes\nSixt), and the lack of a NULL-check looks like a simple oversight.\n"},{"id":"151390","messageId":"201009232208.56916.j6t@kdbg.org","threadId":"25212","inReplyTo":"1285263325-2016-1-git-send-email-kusmabite@gmail.com","subject":"Re: [msysGit] [PATCH] mingw: do not crash on open(NULL, ...)","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-09-23T20:08:56Z","receivedAt":"2010-09-23T20:08:56Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 23. September 2010, Erik Faye-Lund wrote:\n> @@ -297,7 +297,7 @@ int mingw_open (const char *filename, int oflags, ...)\n>  \tmode = va_arg(args, int);\n>  \tva_end(args);\n>\n> -\tif (!strcmp(filename, \"/dev/null\"))\n> +\tif (filename && !strcmp(filename, \"/dev/null\"))\n>  \t\tfilename = \"nul\";\n>\n>  \tfd = open(filename, oflags, mode);\n\nGood catch, thank you!\n\nAcked-by: Johannes Sixt <j6t@kdbg.org>\n\n-- Hannes\n"},{"id":"151392","messageId":"AANLkTimviqUqcy40aR=DC--tqKHWXfX9gLMoX7tyjafe@mail.gmail.com","threadId":"25212","inReplyTo":"AANLkTinJ4kKRsKO6HyqQH4Oy12E1mdqCXxPb2z+59818@mail.gmail.com","subject":"Re: Re: [PATCH] mingw: do not crash on open(NULL, ...)","fromName":"Pat Thoyts","fromEmail":"patthoyts@gmail.com","sentAt":"2010-09-23T20:27:35Z","receivedAt":"2010-09-23T20:27:35Z","isPatch":true,"sender":{"key":"patthoyts@gmail.com","avatar":"https://gravatar.com/avatar/bee887a777c790bd241f398217723fbe4b854428671db83db32216a28654cb25?d=mp&s=160"},"body":"(Apparently gmail on a phone insists on top posting)\nIt looks like this problem was missed by the test suite. Any chance of a\ntest as well? Got to catch those regressions.\n\nOn 23 Sep 2010 19:00, \"Erik Faye-Lund\" <kusmabite@gmail.com> wrote:\n> On Thu, Sep 23, 2010 at 10:35 AM, Erik Faye-Lund <kusmabite@gmail.com>\nwrote:\n>> Since open() already sets errno correctly for the NULL-case, let's just\n>> avoid the problematic strcmp.\n>>\n>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n>\n> I guess I should add a comment as to why this patch is needed:\n>\n> This seems to be the culprit for issue 523 in the msysGit issue\n> tracker: http://code.google.com/p/msysgit/issues/detail?id=523\n>\n> fetch_and_setup_pack_index() apparently pass a NULL-pointer to\n> parse_pack_index(), which in turn pass it to check_packed_git_idx(),\n> which again pass it to open(). This all looks intentional to my\n> (http.c-untrained) eye.\n>\n> The code in mingw_open was introduced in commit 3e4a1ba (by Johannes\n> Sixt), and the lack of a NULL-check looks like a simple oversight.\n"},{"id":"151401","messageId":"AANLkTinW=1M81X4-7iqVXX0gKoAW8Rvvt+A7zkoQBnN8@mail.gmail.com","threadId":"25212","inReplyTo":"AANLkTimviqUqcy40aR=DC--tqKHWXfX9gLMoX7tyjafe@mail.gmail.com","subject":"Re: [msysGit] Re: [PATCH] mingw: do not crash on open(NULL, ...)","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2010-09-23T21:06:29Z","receivedAt":"2010-09-23T21:06:29Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Sep 23, 2010 at 1:27 PM, Pat Thoyts <patthoyts@gmail.com> wrote:\n> (Apparently gmail on a phone insists on top posting)\n\nFixed ;)\n\n> On 23 Sep 2010 19:00, \"Erik Faye-Lund\" <kusmabite@gmail.com> wrote:\n>> On Thu, Sep 23, 2010 at 10:35 AM, Erik Faye-Lund <kusmabite@gmail.com>\n>> wrote:\n>>> Since open() already sets errno correctly for the NULL-case, let's just\n>>> avoid the problematic strcmp.\n>>>\n>>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n>>\n>> I guess I should add a comment as to why this patch is needed:\n>>\n>> This seems to be the culprit for issue 523 in the msysGit issue\n>> tracker: http://code.google.com/p/msysgit/issues/detail?id=523\n>>\n>> fetch_and_setup_pack_index() apparently pass a NULL-pointer to\n>> parse_pack_index(), which in turn pass it to check_packed_git_idx(),\n>> which again pass it to open(). This all looks intentional to my\n>> (http.c-untrained) eye.\n>>\n>> The code in mingw_open was introduced in commit 3e4a1ba (by Johannes\n>> Sixt), and the lack of a NULL-check looks like a simple oversight.\n>\n> It looks like this problem was missed by the test suite. Any chance of a\n> test as well? Got to catch those regressions.\n>\n\nI don't think it's practical to test CRT functions directly, but\nperhaps a test for parse_pack_index() or some level above that might\nmake sense. I tried looking at the this has been done for sha1_file\npreviously, but it seems there's not really any tests at this level.\nAnd all tests for http.c seems to depend on Apache being installed\n(something we do not have in msysGit), so adding a test at this level\nwouldn't have helped us any...\n\nIn other words, I don't think there is any natural point to add a test\nfor this. Feel free to make suggestions, though.\n"},{"id":"151413","messageId":"7vy6asoz0i.fsf@alter.siamese.dyndns.org","threadId":"25212","inReplyTo":"AANLkTinJ4kKRsKO6HyqQH4Oy12E1mdqCXxPb2z+59818@mail.gmail.com","subject":"Re: [PATCH] mingw: do not crash on open(NULL, ...)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-23T22:50:37Z","receivedAt":"2010-09-23T22:50:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund <kusmabite@gmail.com> writes:\n\n> On Thu, Sep 23, 2010 at 10:35 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n>> Since open() already sets errno correctly for the NULL-case, let's just\n>> avoid the problematic strcmp.\n>>\n>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n>\n> I guess I should add a comment as to why this patch is needed:\n>\n> This seems to be the culprit for issue 523 in the msysGit issue\n> tracker: http://code.google.com/p/msysgit/issues/detail?id=523\n>\n> fetch_and_setup_pack_index() apparently pass a NULL-pointer to\n> parse_pack_index(), which in turn pass it to check_packed_git_idx(),\n> which again pass it to open(). This all looks intentional to my\n> (http.c-untrained) eye.\n\nSurely, open(NULL) should be rejected by a sane system, and your patch\nlooks sane to me.\n\nBut depending on and exploiting the fact sounds like a horrible hack in\nthe caller of parse_pack_index(..., NULL) to me.\n\nShawn may have intentionally done that in 750ef42 (http-fetch: Use\ntemporary files for pack-*.idx until verified, 2010-04-19), but at least\n7b64469 (Allow parse_pack_index on temporary files, 2010-04-19) should\nhave documented that idx_path is allowed to be NULL under what\ncircumstance (and for what purpose it is useful to do so) when it\nintroduced the second parameter to the API.\n\nWhat were we smoking?\n"},{"id":"151417","messageId":"alpine.DEB.1.00.1009240230420.4461@bonsai2","threadId":"25212","inReplyTo":"AANLkTinJ4kKRsKO6HyqQH4Oy12E1mdqCXxPb2z+59818@mail.gmail.com","subject":"Re: [msysGit] Re: [PATCH] mingw: do not crash on open(NULL, ...)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-09-24T00:32:11Z","receivedAt":"2010-09-24T00:32:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 23 Sep 2010, Erik Faye-Lund wrote:\n\n> The code in mingw_open was introduced in commit 3e4a1ba (by Johannes \n> Sixt), and the lack of a NULL-check looks like a simple oversight.\n\nErik, you have all the information at your finger tips. So why not Cc: \nJohannes Sixt, who you blame so openly, so that he has a chance to react \n(and less of a chance to miss your criticism)?\n\nCiao,\nDscho\n"},{"id":"151483","messageId":"AANLkTikEvoXuN+vrxry5mz7CM2AuzgBUvUoQxW+zs5oY@mail.gmail.com","threadId":"25212","inReplyTo":"201009232208.56916.j6t@kdbg.org","subject":"Re: [msysGit] [PATCH] mingw: do not crash on open(NULL, ...)","fromName":"Pat Thoyts","fromEmail":"patthoyts@gmail.com","sentAt":"2010-09-24T14:22:58Z","receivedAt":"2010-09-24T14:22:58Z","isPatch":true,"sender":{"key":"patthoyts@gmail.com","avatar":"https://gravatar.com/avatar/bee887a777c790bd241f398217723fbe4b854428671db83db32216a28654cb25?d=mp&s=160"},"body":"On 23 September 2010 21:08, Johannes Sixt <j6t@kdbg.org> wrote:\n> On Donnerstag, 23. September 2010, Erik Faye-Lund wrote:\n>> @@ -297,7 +297,7 @@ int mingw_open (const char *filename, int oflags, ...)\n>>       mode = va_arg(args, int);\n>>       va_end(args);\n>>\n>> -     if (!strcmp(filename, \"/dev/null\"))\n>> +     if (filename && !strcmp(filename, \"/dev/null\"))\n>>               filename = \"nul\";\n>>\n>>       fd = open(filename, oflags, mode);\n>\n> Good catch, thank you!\n>\n> Acked-by: Johannes Sixt <j6t@kdbg.org>\n>\n> -- Hannes\n\nApplied to devel and pushed.\n"},{"id":"151818","messageId":"AANLkTi=p13eTY-dqGZJYaogRyj0Z5uO3YM8n1RW4iBUi@mail.gmail.com","threadId":"25212","inReplyTo":"7vy6asoz0i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] mingw: do not crash on open(NULL, ...)","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2010-09-27T13:19:51Z","receivedAt":"2010-09-27T13:19:51Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Sep 24, 2010 at 12:50 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Erik Faye-Lund <kusmabite@gmail.com> writes:\n>\n>> On Thu, Sep 23, 2010 at 10:35 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n>>> Since open() already sets errno correctly for the NULL-case, let's just\n>>> avoid the problematic strcmp.\n>>>\n>>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n>>\n>> I guess I should add a comment as to why this patch is needed:\n>>\n>> This seems to be the culprit for issue 523 in the msysGit issue\n>> tracker: http://code.google.com/p/msysgit/issues/detail?id=523\n>>\n>> fetch_and_setup_pack_index() apparently pass a NULL-pointer to\n>> parse_pack_index(), which in turn pass it to check_packed_git_idx(),\n>> which again pass it to open(). This all looks intentional to my\n>> (http.c-untrained) eye.\n>\n> Surely, open(NULL) should be rejected by a sane system, and your patch\n> looks sane to me.\n>\n\nSince this doesn't seem to be in git.git yet, perhaps you could squash\nthis on top? I didn't notice it in time, but fopen lacked the same\ncheck (freopen already had the check). It's not as important, because\nit doesn't seem like we have any code reaching this path so far, but\nit would IMO be better to fix this now rather than having to chase\ndown the issue again later...\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 4595aaa..f069fea 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -160,7 +160,7 @@ ssize_t mingw_write(int fd, const void *buf, size_t count)\n #undef fopen\n FILE *mingw_fopen (const char *filename, const char *otype)\n {\n-\tif (!strcmp(filename, \"/dev/null\"))\n+\tif (filename && !strcmp(filename, \"/dev/null\"))\n \t\tfilename = \"nul\";\n \treturn fopen(filename, otype);\n }\n"},{"id":"151819","messageId":"AANLkTikv8M8xuESQzO7qfPB72d51hTcosUgKreLu7Y=C@mail.gmail.com","threadId":"25212","inReplyTo":"AANLkTi=p13eTY-dqGZJYaogRyj0Z5uO3YM8n1RW4iBUi@mail.gmail.com","subject":"Re: [msysGit] Re: [PATCH] mingw: do not crash on open(NULL, ...)","fromName":"Pat Thoyts","fromEmail":"patthoyts@gmail.com","sentAt":"2010-09-27T13:31:27Z","receivedAt":"2010-09-27T13:31:27Z","isPatch":true,"sender":{"key":"patthoyts@gmail.com","avatar":"https://gravatar.com/avatar/bee887a777c790bd241f398217723fbe4b854428671db83db32216a28654cb25?d=mp&s=160"},"body":"On 27 September 2010 14:19, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> On Fri, Sep 24, 2010 at 12:50 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Erik Faye-Lund <kusmabite@gmail.com> writes:\n>>\n>>> On Thu, Sep 23, 2010 at 10:35 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n>>>> Since open() already sets errno correctly for the NULL-case, let's just\n>>>> avoid the problematic strcmp.\n>>>>\n>>>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n>>>\n>>> I guess I should add a comment as to why this patch is needed:\n>>>\n>>> This seems to be the culprit for issue 523 in the msysGit issue\n>>> tracker: http://code.google.com/p/msysgit/issues/detail?id=523\n>>>\n>>> fetch_and_setup_pack_index() apparently pass a NULL-pointer to\n>>> parse_pack_index(), which in turn pass it to check_packed_git_idx(),\n>>> which again pass it to open(). This all looks intentional to my\n>>> (http.c-untrained) eye.\n>>\n>> Surely, open(NULL) should be rejected by a sane system, and your patch\n>> looks sane to me.\n>>\n>\n> Since this doesn't seem to be in git.git yet, perhaps you could squash\n> this on top? I didn't notice it in time, but fopen lacked the same\n> check (freopen already had the check). It's not as important, because\n> it doesn't seem like we have any code reaching this path so far, but\n> it would IMO be better to fix this now rather than having to chase\n> down the issue again later...\n>\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 4595aaa..f069fea 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -160,7 +160,7 @@ ssize_t mingw_write(int fd, const void *buf, size_t count)\n>  #undef fopen\n>  FILE *mingw_fopen (const char *filename, const char *otype)\n>  {\n> -       if (!strcmp(filename, \"/dev/null\"))\n> +       if (filename && !strcmp(filename, \"/dev/null\"))\n>                filename = \"nul\";\n>        return fopen(filename, otype);\n>  }\n>\n\nI'll apply this to the devel branch and try to remember to squash it\non the next rebase-merge.\nCheers,\nPat\n"},{"id":"151820","messageId":"AANLkTik5mr7MTPzrWPG00T40a08oiV7X8ZXBHkXva+DO@mail.gmail.com","threadId":"25212","inReplyTo":"AANLkTikv8M8xuESQzO7qfPB72d51hTcosUgKreLu7Y=C@mail.gmail.com","subject":"Re: Re: [PATCH] mingw: do not crash on open(NULL, ...)","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2010-09-27T13:37:16Z","receivedAt":"2010-09-27T13:37:16Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Sep 27, 2010 at 3:31 PM, Pat Thoyts <patthoyts@gmail.com> wrote:\n> On 27 September 2010 14:19, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n>> On Fri, Sep 24, 2010 at 12:50 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Erik Faye-Lund <kusmabite@gmail.com> writes:\n>>>\n>>>> On Thu, Sep 23, 2010 at 10:35 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n>>>>> Since open() already sets errno correctly for the NULL-case, let's just\n>>>>> avoid the problematic strcmp.\n>>>>>\n>>>>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n>>>>\n>>>> I guess I should add a comment as to why this patch is needed:\n>>>>\n>>>> This seems to be the culprit for issue 523 in the msysGit issue\n>>>> tracker: http://code.google.com/p/msysgit/issues/detail?id=523\n>>>>\n>>>> fetch_and_setup_pack_index() apparently pass a NULL-pointer to\n>>>> parse_pack_index(), which in turn pass it to check_packed_git_idx(),\n>>>> which again pass it to open(). This all looks intentional to my\n>>>> (http.c-untrained) eye.\n>>>\n>>> Surely, open(NULL) should be rejected by a sane system, and your patch\n>>> looks sane to me.\n>>>\n>>\n>> Since this doesn't seem to be in git.git yet, perhaps you could squash\n>> this on top? I didn't notice it in time, but fopen lacked the same\n>> check (freopen already had the check). It's not as important, because\n>> it doesn't seem like we have any code reaching this path so far, but\n>> it would IMO be better to fix this now rather than having to chase\n>> down the issue again later...\n>>\n>> diff --git a/compat/mingw.c b/compat/mingw.c\n>> index 4595aaa..f069fea 100644\n>> --- a/compat/mingw.c\n>> +++ b/compat/mingw.c\n>> @@ -160,7 +160,7 @@ ssize_t mingw_write(int fd, const void *buf, size_t count)\n>>  #undef fopen\n>>  FILE *mingw_fopen (const char *filename, const char *otype)\n>>  {\n>> -       if (!strcmp(filename, \"/dev/null\"))\n>> +       if (filename && !strcmp(filename, \"/dev/null\"))\n>>                filename = \"nul\";\n>>        return fopen(filename, otype);\n>>  }\n>>\n>\n> I'll apply this to the devel branch and try to remember to squash it\n> on the next rebase-merge.\n> Cheers,\n> Pat\n>\n\nWouldn't it be better to just get this squashed in git.git, and drop\nthe patch in the next rebase-merge? Since this bug is in git.git as\nwell, it makes sense to get the patch merged there, no?\n"},{"id":"151821","messageId":"alpine.DEB.1.00.1009271559110.2187@intel-tinevez-2-302","threadId":"25212","inReplyTo":"AANLkTikv8M8xuESQzO7qfPB72d51hTcosUgKreLu7Y=C@mail.gmail.com","subject":"Re: Re: [PATCH] mingw: do not crash on open(NULL, ...)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-09-27T13:59:56Z","receivedAt":"2010-09-27T13:59:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 27 Sep 2010, Pat Thoyts wrote:\n\n> On 27 September 2010 14:19, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> > On Fri, Sep 24, 2010 at 12:50 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> >> Erik Faye-Lund <kusmabite@gmail.com> writes:\n> >>\n> >>> On Thu, Sep 23, 2010 at 10:35 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> >>>> Since open() already sets errno correctly for the NULL-case, let's just\n> >>>> avoid the problematic strcmp.\n> >>>>\n> >>>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n> >>>\n> >>> I guess I should add a comment as to why this patch is needed:\n> >>>\n> >>> This seems to be the culprit for issue 523 in the msysGit issue\n> >>> tracker: http://code.google.com/p/msysgit/issues/detail?id=523\n> >>>\n> >>> fetch_and_setup_pack_index() apparently pass a NULL-pointer to\n> >>> parse_pack_index(), which in turn pass it to check_packed_git_idx(),\n> >>> which again pass it to open(). This all looks intentional to my\n> >>> (http.c-untrained) eye.\n> >>\n> >> Surely, open(NULL) should be rejected by a sane system, and your patch\n> >> looks sane to me.\n> >>\n> >\n> > Since this doesn't seem to be in git.git yet, perhaps you could squash\n> > this on top? I didn't notice it in time, but fopen lacked the same\n> > check (freopen already had the check). It's not as important, because\n> > it doesn't seem like we have any code reaching this path so far, but\n> > it would IMO be better to fix this now rather than having to chase\n> > down the issue again later...\n> >\n> > diff --git a/compat/mingw.c b/compat/mingw.c\n> > index 4595aaa..f069fea 100644\n> > --- a/compat/mingw.c\n> > +++ b/compat/mingw.c\n> > @@ -160,7 +160,7 @@ ssize_t mingw_write(int fd, const void *buf, size_t count)\n> >  #undef fopen\n> >  FILE *mingw_fopen (const char *filename, const char *otype)\n> >  {\n> > -       if (!strcmp(filename, \"/dev/null\"))\n> > +       if (filename && !strcmp(filename, \"/dev/null\"))\n> >                filename = \"nul\";\n> >        return fopen(filename, otype);\n> >  }\n> >\n> \n> I'll apply this to the devel branch and try to remember to squash it\n> on the next rebase-merge.\n\nUsually I mark commits like this with (\"amend deadcafe\") in the commit \nsubject so I do not forget... ;-)\n\nCiao,\nDscho\n"}]}