From: Eric Sunshine Date: Wed, 08 Jun 2016 17:44:22 GMT Subject: Re: [PATCH v2 63/94] builtin/apply: make apply_all_patches() return -1 on error Message-ID: In-Reply-To: On Wed, Jun 8, 2016 at 12:37 PM, Christian Couder wrote: > On Mon, May 16, 2016 at 5:44 AM, Eric Sunshine wrote: >> On Wed, May 11, 2016 at 9:17 AM, Christian Couder >> wrote: >>> 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. Squashing may indeed be preferable over leaving it in a "broken" state until the next patch, though I haven't thought too hard about it. Alternately, can the two patches somehow be swapped?