On Mon, May 16, 2016 at 5:44 AM, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> On Wed, May 11, 2016 at 9:17 AM, Christian Couder
> <christian.cou...@gmail.com> wrote:
>> To finish libifying the apply functionality, apply_all_patches() should not
>> die() or exit() in case of error, but return -1.
>>
>> While doing that we must take care that file descriptors are properly closed
>> and, if needed, reset a sensible value.
>>
>> Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
>> ---
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> @@ -4613,9 +4613,10 @@ static int apply_all_patches(struct apply_state
>> *state,
>> }
>>
>> if (state->update_index) {
>> - if (write_locked_index(&the_index, state->lock_file,
>> COMMIT_LOCK))
>> - die(_("Unable to write new index file"));
>> + res = write_locked_index(&the_index, state->lock_file,
>> COMMIT_LOCK);
>> state->newfd = -1;
>
> Does write_locked_index() unconditionally close the file descriptor
> even when an error occurs? If not, then isn't this potentially leaking
> 'newfd'?
>
> (My very cursory read of write_locked_index() seems to reveal that the
> file descriptor may indeed remain open upon index write failure.)
You are right, it is leaking newfd if write_locked_index() fails.
The solution to that is to call `rollback_lock_file(state->lock_file)`
and the following patch was supposed to do that:
[PATCH v2 82/94] apply: roll back index lock file in case of error
but it would do that only if `state->newfd >= 0` so we should set
state->newfd to -1 only if write_locked_index() succeeds.
I will fix this.
I am also going to add a comment to this patch saying that this patch
needs a following patch to call rollback_lock_file(state->lock_file)
in case of errors.
Or if you prefer, I can squash the patch that call
rollback_lock_file(state->lock_file) in case of errors into this
patch.
Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
