Re: [PATCH 1/1] upload-pack: fix race condition in error messages

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 29, 2019, 14:27 UTC
Message-ID: <131dfa37-0f65-f0c3-6f30-5b6eca12d9c0@gmail.com>
In-Reply-To: <20190829141307.GE1797@sigill.intra.peff.net>

On 8/29/2019 10:13 AM, Jeff King wrote:

Show 14 quoted lines

> On Thu, Aug 29, 2019 at 08:58:55AM -0400, Derrick Stolee wrote:
> 
>> However, I do have a theory: the process exits before flushing the
>> packet line. Adding this line before exit(1) should fix it:
>>
>> 	packet_writer_flush(writer);
>>
>> I can send this in a v2, but it would be nice if you could test this
>> in your environment that already demonstrated the failure.
> 
> I don't think we should need such a call. For one thing, if it were
> necessary, that would mean we're not writing out the packet at all. But
> your whole problem is that we're writing the message twice, one of which
> comes from the packet.

The problem the flush() was trying to solve was the new "Broken pipe" error, which I had assumed was due to a communication race. (Looking at the message more closely now, I see that Szeder was able to repro this broken pipe both with and without my change. I am still unable to repro the broken pipe.)

Show 6 quoted lines

> Second is that this is not "flush the output stream", but "write a flush
> packet". The packet_writer_error() function immediately calls write()
> without buffering. And no matter where we are in the conversation, a
> flush packet would not be necessary, because the error packet we send
> would be interpreted immediately by the client as aborting the
> connection.

This clearly shows that my proposed solution is absolutely wrong.

Thanks, -Stolee