Re: Re: [PATCH] mingw: do not crash on open(NULL, ...)

[Erik Faye-Lund <kusmabite@gmail.com>]

On Mon, Sep 27, 2010 at 3:31 PM, Pat Thoyts <patthoyts@gmail.com> wrote:
> On 27 September 2010 14:19, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> On Fri, Sep 24, 2010 at 12:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>>
>>>> On Thu, Sep 23, 2010 at 10:35 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>>>>> Since open() already sets errno correctly for the NULL-case, let's just
>>>>> avoid the problematic strcmp.
>>>>>
>>>>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>>>>
>>>> I guess I should add a comment as to why this patch is needed:
>>>>
>>>> This seems to be the culprit for issue 523 in the msysGit issue
>>>> tracker: http://code.google.com/p/msysgit/issues/detail?id=523
>>>>
>>>> fetch_and_setup_pack_index() apparently pass a NULL-pointer to
>>>> parse_pack_index(), which in turn pass it to check_packed_git_idx(),
>>>> which again pass it to open(). This all looks intentional to my
>>>> (http.c-untrained) eye.
>>>
>>> Surely, open(NULL) should be rejected by a sane system, and your patch
>>> looks sane to me.
>>>
>>
>> Since this doesn't seem to be in git.git yet, perhaps you could squash
>> this on top? I didn't notice it in time, but fopen lacked the same
>> check (freopen already had the check). It's not as important, because
>> it doesn't seem like we have any code reaching this path so far, but
>> it would IMO be better to fix this now rather than having to chase
>> down the issue again later...
>>
>> diff --git a/compat/mingw.c b/compat/mingw.c
>> index 4595aaa..f069fea 100644
>> --- a/compat/mingw.c
>> +++ b/compat/mingw.c
>> @@ -160,7 +160,7 @@ ssize_t mingw_write(int fd, const void *buf, size_t count)
>>  #undef fopen
>>  FILE *mingw_fopen (const char *filename, const char *otype)
>>  {
>> -       if (!strcmp(filename, "/dev/null"))
>> +       if (filename && !strcmp(filename, "/dev/null"))
>>                filename = "nul";
>>        return fopen(filename, otype);
>>  }
>>
>
> I'll apply this to the devel branch and try to remember to squash it
> on the next rebase-merge.
> Cheers,
> Pat
>

Wouldn't it be better to just get this squashed in git.git, and drop
the patch in the next rebase-merge? Since this bug is in git.git as
well, it makes sense to get the patch merged there, no?