* [PATCH] mingw: do not crash on open(NULL, ...) @ 2010-09-23 17:35 Erik Faye-Lund 2010-09-23 17:59 ` Erik Faye-Lund 2010-09-23 20:08 ` [msysGit] " Johannes Sixt 0 siblings, 2 replies; 12+ messages in thread From: Erik Faye-Lund @ 2010-09-23 17:35 UTC (permalink / raw) To: msysgit; +Cc: git, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] mingw: do not crash on open(NULL, ...) 2010-09-23 17:35 [PATCH] mingw: do not crash on open(NULL, ...) Erik Faye-Lund @ 2010-09-23 17:59 ` Erik Faye-Lund 2010-09-23 20:27 ` Pat Thoyts ` (2 more replies) 2010-09-23 20:08 ` [msysGit] " Johannes Sixt 1 sibling, 3 replies; 12+ messages in thread From: Erik Faye-Lund @ 2010-09-23 17:59 UTC (permalink / raw) To: msysgit; +Cc: git, 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: [PATCH] mingw: do not crash on open(NULL, ...) 2010-09-23 17:59 ` Erik Faye-Lund @ 2010-09-23 20:27 ` Pat Thoyts 2010-09-23 21:06 ` [msysGit] " Erik Faye-Lund 2010-09-23 22:50 ` Junio C Hamano 2010-09-24 0:32 ` [msysGit] " Johannes Schindelin 2 siblings, 1 reply; 12+ messages in thread From: Pat Thoyts @ 2010-09-23 20:27 UTC (permalink / raw) To: kusmabite; +Cc: git, msysgit [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [msysGit] Re: [PATCH] mingw: do not crash on open(NULL, ...) 2010-09-23 20:27 ` Pat Thoyts @ 2010-09-23 21:06 ` Erik Faye-Lund 0 siblings, 0 replies; 12+ messages in thread From: Erik Faye-Lund @ 2010-09-23 21:06 UTC (permalink / raw) To: Pat Thoyts; +Cc: git, msysgit 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mingw: do not crash on open(NULL, ...) 2010-09-23 17:59 ` Erik Faye-Lund 2010-09-23 20:27 ` Pat Thoyts @ 2010-09-23 22:50 ` Junio C Hamano 2010-09-27 13:19 ` Erik Faye-Lund 2010-09-24 0:32 ` [msysGit] " Johannes Schindelin 2 siblings, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2010-09-23 22:50 UTC (permalink / raw) To: kusmabite; +Cc: msysgit, git, Shawn O. Pearce 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mingw: do not crash on open(NULL, ...) 2010-09-23 22:50 ` Junio C Hamano @ 2010-09-27 13:19 ` Erik Faye-Lund 2010-09-27 13:31 ` [msysGit] " Pat Thoyts 0 siblings, 1 reply; 12+ messages in thread From: Erik Faye-Lund @ 2010-09-27 13:19 UTC (permalink / raw) To: Junio C Hamano; +Cc: msysgit, git, Shawn O. Pearce 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); } ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [msysGit] Re: [PATCH] mingw: do not crash on open(NULL, ...) 2010-09-27 13:19 ` Erik Faye-Lund @ 2010-09-27 13:31 ` Pat Thoyts 2010-09-27 13:37 ` Erik Faye-Lund 2010-09-27 13:59 ` Johannes Schindelin 0 siblings, 2 replies; 12+ messages in thread From: Pat Thoyts @ 2010-09-27 13:31 UTC (permalink / raw) To: kusmabite; +Cc: Junio C Hamano, msysgit, git, Shawn O. Pearce 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: [PATCH] mingw: do not crash on open(NULL, ...) 2010-09-27 13:31 ` [msysGit] " Pat Thoyts @ 2010-09-27 13:37 ` Erik Faye-Lund 2010-09-27 13:59 ` Johannes Schindelin 1 sibling, 0 replies; 12+ messages in thread From: Erik Faye-Lund @ 2010-09-27 13:37 UTC (permalink / raw) To: Pat Thoyts; +Cc: Junio C Hamano, msysgit, git, Shawn O. Pearce 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: [PATCH] mingw: do not crash on open(NULL, ...) 2010-09-27 13:31 ` [msysGit] " Pat Thoyts 2010-09-27 13:37 ` Erik Faye-Lund @ 2010-09-27 13:59 ` Johannes Schindelin 1 sibling, 0 replies; 12+ messages in thread From: Johannes Schindelin @ 2010-09-27 13:59 UTC (permalink / raw) To: Pat Thoyts; +Cc: kusmabite, Junio C Hamano, msysgit, git, Shawn O. Pearce 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [msysGit] Re: [PATCH] mingw: do not crash on open(NULL, ...) 2010-09-23 17:59 ` Erik Faye-Lund 2010-09-23 20:27 ` Pat Thoyts 2010-09-23 22:50 ` Junio C Hamano @ 2010-09-24 0:32 ` Johannes Schindelin 2 siblings, 0 replies; 12+ messages in thread From: Johannes Schindelin @ 2010-09-24 0:32 UTC (permalink / raw) To: Erik Faye-Lund; +Cc: msysgit, git, Johannes Sixt 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [msysGit] [PATCH] mingw: do not crash on open(NULL, ...) 2010-09-23 17:35 [PATCH] mingw: do not crash on open(NULL, ...) Erik Faye-Lund 2010-09-23 17:59 ` Erik Faye-Lund @ 2010-09-23 20:08 ` Johannes Sixt 2010-09-24 14:22 ` Pat Thoyts 1 sibling, 1 reply; 12+ messages in thread From: Johannes Sixt @ 2010-09-23 20:08 UTC (permalink / raw) To: Erik Faye-Lund; +Cc: msysgit, git 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [msysGit] [PATCH] mingw: do not crash on open(NULL, ...) 2010-09-23 20:08 ` [msysGit] " Johannes Sixt @ 2010-09-24 14:22 ` Pat Thoyts 0 siblings, 0 replies; 12+ messages in thread From: Pat Thoyts @ 2010-09-24 14:22 UTC (permalink / raw) To: Johannes Sixt; +Cc: Erik Faye-Lund, msysgit, git 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2010-09-27 14:00 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-09-23 17:35 [PATCH] mingw: do not crash on open(NULL, ...) Erik Faye-Lund 2010-09-23 17:59 ` Erik Faye-Lund 2010-09-23 20:27 ` Pat Thoyts 2010-09-23 21:06 ` [msysGit] " Erik Faye-Lund 2010-09-23 22:50 ` Junio C Hamano 2010-09-27 13:19 ` Erik Faye-Lund 2010-09-27 13:31 ` [msysGit] " Pat Thoyts 2010-09-27 13:37 ` Erik Faye-Lund 2010-09-27 13:59 ` Johannes Schindelin 2010-09-24 0:32 ` [msysGit] " Johannes Schindelin 2010-09-23 20:08 ` [msysGit] " Johannes Sixt 2010-09-24 14:22 ` Pat Thoyts
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).