From: Tay Ray Chuan Date: Mon, 01 Jun 2009 13:52:33 GMT Subject: Re: [PATCH v2] http*: cleanup slot->local after fclose Message-ID: In-Reply-To: <7vws7xqazi.fsf@alter.siamese.dyndns.org> Hi, On Mon, Jun 1, 2009 at 4:21 AM, Junio C Hamano wrote: > 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