Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return

From: Philip Oakley <philipoakley@iee.org>
Date: Apr 1, 2016, 07:26 UTC
Message-ID: <55AD693FD6AC44119B8368ABC36FB1A9@PhilipOakley>
In-Reply-To: <CAGZ79kYMxJdh_8n8zaaaEQP_BqWfS12JEry1MHkYqwQPJTWTNw@mail.gmail.com>

From: "Stefan Beller" <sbeller@google.com>

Show 27 quoted lines

> On Thu, Mar 31, 2016 at 12:00 PM, Philip Oakley <philipoakley@iee.org> 
> wrote:
>> From: "Stefan Beller" <sbeller@google.com>
>>>
>>> In successful operation `write_pack_data` will close the `bundle_fd`,
>>> but when we exit early, we need to take care of the file descriptor
>>> as well as the lock file ourselves. The lock file may be deleted at the
>>> end of running the program, but we are in library code, so we should
>>> not rely on that.
>>>
>>> Helped-by: Jeff King <peff@peff.net>
>>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>
>>
>> Has this been tested on Windows? I had a similar problem very recently
>> https://groups.google.com/forum/#!msg/git-for-windows/6LPxf9xZKhI/-s7XD18yCwAJ
>> where a bad rev-list-arg would cause the `bundle create` to die, and, on
>> windows, leave the incomplete bundle file locked.
>>
>> dscho suggested one possible solution for that, but I haven't had any 
>> time
>> to try any patches.
>
> I think with Jeffs suggestion to only rollback the lock in case of
> (!bundle_to_stdout)
> we're not making it worse. I do not have a Windows machine at hand to test 
> it :(

Thant's fine. I just wanted to make folks aware of the problem so it doesn't get worse.

It looks like the cause is that the bundle.c:compute_and_write_prerequisites(), which includes parsing the arg-list (and die on bad arg), is called after the creation and locking of the bundle file descriptor and header, thus leaving it hanging on Windows.

--

Philip