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

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Sep 23, 2010, 21:06 UTC
Message-ID: <AANLkTinW=1M81X4-7iqVXX0gKoAW8Rvvt+A7zkoQBnN8@mail.gmail.com>
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 ;)

Show 24 quoted lines

> 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.