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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 23, 2010, 22:50 UTC
Message-ID: <7vy6asoz0i.fsf@alter.siamese.dyndns.org>
In-Reply-To: <AANLkTinJ4kKRsKO6HyqQH4Oy12E1mdqCXxPb2z+59818@mail.gmail.com>

Erik Faye-Lund <kusmabite@gmail.com> writes:

Show 15 quoted lines

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