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

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

[Erik Faye-Lund]

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>
---
 compat/mingw.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index b0ad919..b408c3c 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -297,7 +297,7 @@ int mingw_open (const char *filename, int oflags, ...)
 	mode = va_arg(args, int);
 	va_end(args);
 
-	if (!strcmp(filename, "/dev/null"))
+	if (filename && !strcmp(filename, "/dev/null"))
 		filename = "nul";
 
 	fd = open(filename, oflags, mode);
-- 
1.7.2.3.msysgit.0.207.gda0b3

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

[Erik Faye-Lund]

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.

The code in mingw_open was introduced in commit 3e4a1ba (by Johannes
Sixt), and the lack of a NULL-check looks like a simple oversight.

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

[Johannes Sixt]

On Donnerstag, 23. September 2010, Erik Faye-Lund wrote:
> @@ -297,7 +297,7 @@ int mingw_open (const char *filename, int oflags, ...)
>  	mode = va_arg(args, int);
>  	va_end(args);
>
> -	if (!strcmp(filename, "/dev/null"))
> +	if (filename && !strcmp(filename, "/dev/null"))
>  		filename = "nul";
>
>  	fd = open(filename, oflags, mode);

Good catch, thank you!

Acked-by: Johannes Sixt <j6t@kdbg.org>

-- Hannes

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

[Pat Thoyts]

[-- Attachment #1: Type: text/plain, Size: 1059 bytes --]

(Apparently gmail on a phone insists on top posting)
It looks like this problem was missed by the test suite. Any chance of a
test as well? Got to catch those regressions.

On 23 Sep 2010 19:00, "Erik Faye-Lund" <kusmabite@gmail.com> wrote:
> 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.
>
> The code in mingw_open was introduced in commit 3e4a1ba (by Johannes
> Sixt), and the lack of a NULL-check looks like a simple oversight.

[-- Attachment #2: Type: text/html, Size: 1457 bytes --]

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

[Erik Faye-Lund]

On Thu, Sep 23, 2010 at 1:27 PM, Pat Thoyts <patthoyts@gmail.com> wrote:
> (Apparently gmail on a phone insists on top posting)

Fixed ;)

> On 23 Sep 2010 19:00, "Erik Faye-Lund" <kusmabite@gmail.com> wrote:
>> 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.
>>
>> The code in mingw_open was introduced in commit 3e4a1ba (by Johannes
>> Sixt), and the lack of a NULL-check looks like a simple oversight.
>
> It looks like this problem was missed by the test suite. Any chance of a
> test as well? Got to catch those regressions.
>

I don't think it's practical to test CRT functions directly, but
perhaps a test for parse_pack_index() or some level above that might
make sense. I tried looking at the this has been done for sha1_file
previously, but it seems there's not really any tests at this level.
And all tests for http.c seems to depend on Apache being installed
(something we do not have in msysGit), so adding a test at this level
wouldn't have helped us any...

In other words, I don't think there is any natural point to add a test
for this. Feel free to make suggestions, though.

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

[Junio C Hamano]

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.

But depending on and exploiting the fact sounds like a horrible hack in
the caller of parse_pack_index(..., NULL) to me.

Shawn may have intentionally done that in 750ef42 (http-fetch: Use
temporary files for pack-*.idx until verified, 2010-04-19), but at least
7b64469 (Allow parse_pack_index on temporary files, 2010-04-19) should
have documented that idx_path is allowed to be NULL under what
circumstance (and for what purpose it is useful to do so) when it
introduced the second parameter to the API.

What were we smoking?

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

[Johannes Schindelin]

Hi,

On Thu, 23 Sep 2010, Erik Faye-Lund wrote:

> The code in mingw_open was introduced in commit 3e4a1ba (by Johannes 
> Sixt), and the lack of a NULL-check looks like a simple oversight.

Erik, you have all the information at your finger tips. So why not Cc: 
Johannes Sixt, who you blame so openly, so that he has a chance to react 
(and less of a chance to miss your criticism)?

Ciao,
Dscho

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

[Pat Thoyts]

On 23 September 2010 21:08, Johannes Sixt <j6t@kdbg.org> wrote:
> On Donnerstag, 23. September 2010, Erik Faye-Lund wrote:
>> @@ -297,7 +297,7 @@ int mingw_open (const char *filename, int oflags, ...)
>>       mode = va_arg(args, int);
>>       va_end(args);
>>
>> -     if (!strcmp(filename, "/dev/null"))
>> +     if (filename && !strcmp(filename, "/dev/null"))
>>               filename = "nul";
>>
>>       fd = open(filename, oflags, mode);
>
> Good catch, thank you!
>
> Acked-by: Johannes Sixt <j6t@kdbg.org>
>
> -- Hannes

Applied to devel and pushed.

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

[Erik Faye-Lund]

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);
 }

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

[Pat Thoyts]

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

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

[Erik Faye-Lund]

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?

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

[Johannes Schindelin]

Hi,

On Mon, 27 Sep 2010, Pat Thoyts 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.

Usually I mark commits like this with ("amend deadcafe") in the commit 
subject so I do not forget... ;-)

Ciao,
Dscho