Re: [PATCH v2] http*: cleanup slot->local after fclose

From: Tay Ray Chuan <rctay89@gmail.com>
Date: Jun 1, 2009, 13:52 UTC
Message-ID: <be6fef0d0906010652v4fc814f9j631795cbad61be9@mail.gmail.com>
In-Reply-To: <7vws7xqazi.fsf@alter.siamese.dyndns.org>

Hi,

On Mon, Jun 1, 2009 at 4:21 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

> That "*could*" leaves me puzzled even more.  Could you clarify?
>
> Do you mean "earlier, nobody tried to fclose(slot->local) twice, but with
> the four patches there are codepaths that do so, which triggered the bug
> reported by Clemens"?  But then the issue couldn't have happened without
> those patches.
>
> If existing code always knew when slot->local is and is not valid without
> having to rely on its NULLness, and that was the reason why nobody tried
> to fclose(slot->local) twice, then I'd agree that it is a bug waiting to
> happen, and stuffing NULL to slot->local after the code fclose() it would
> be a good clean-up.

the problem isn't triggered by fclose(slot->local) being done twice, but a ftell() being done on slot->local, which points to a FILE* handle that has already been fclose()'d (that is what the valgrind log provided by Clemens reported). The offending ftell() is found in http.c::run_active_slot(), and the code there does indeed depend on the NULLness of slot->local (it checks whether slot->local is NULL before doing the ftell()).

run_active_slot() is used rather heavily, and users of slot->local always neglect to set it to NULL doing a fclose() on the FILE* handle it points to. This statement is true in both 'master', and 'pu' ('rc/http-push'). Therefore, I said "*could*": although the problem Clemens reported occurs only in 'pu' (or more accurately, the first four patches of 'rc/http-push'), it's not totally impossible for the problem to occur without those four patches.

-- 
Cheers,
Ray Chuan