From: Erik Faye-Lund <kusmabite@gmail.com>
To: Pat Thoyts <patthoyts@gmail.com>
Cc: git@vger.kernel.org, msysgit@googlegroups.com
Subject: Re: [msysGit] Re: [PATCH] mingw: do not crash on open(NULL, ...)
Date: Thu, 23 Sep 2010 14:06:29 -0700 [thread overview]
Message-ID: <AANLkTinW=1M81X4-7iqVXX0gKoAW8Rvvt+A7zkoQBnN8@mail.gmail.com> (raw)
In-Reply-To: <AANLkTimviqUqcy40aR=DC--tqKHWXfX9gLMoX7tyjafe@mail.gmail.com>
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.
next prev parent reply other threads:[~2010-09-23 21:06 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
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 ` Erik Faye-Lund [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='AANLkTinW=1M81X4-7iqVXX0gKoAW8Rvvt+A7zkoQBnN8@mail.gmail.com' \
--to=kusmabite@gmail.com \
--cc=git@vger.kernel.org \
--cc=msysgit@googlegroups.com \
--cc=patthoyts@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).