Hi,
On Mon, Jun 1, 2009 at 4:21 AM, Junio C Hamano <gitster@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html