Re: [PATCH v2 7/9] rebase -i: skip unnecessary picks using the rebase--helper

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Peff,

On Wed, 26 Apr 2017, Jeff King wrote:

> On Tue, Apr 25, 2017 at 03:52:10PM +0200, Johannes Schindelin wrote:
> 
> > diff --git a/sequencer.c b/sequencer.c
> > index 3a935fa4cbc..bbbc98c9116 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2616,3 +2616,93 @@ int check_todo_list(void)
> >  
> >  	return res;
> >  }
> > +
> > +/* skip picking commits whose parents are unchanged */
> > +int skip_unnecessary_picks(void)
> 
> Coverity warns of some descriptor leaks in this function (and in
> rearrange_squash). I think you get those emails, so I won't repeat the
> details here.

I do. The leaks in rearrange_squash() seem to be false positives (I will
have to have another look later, I spent way too many hours pouring over
those Coverity reports this week).

The leaks in skip_unnecessary_picks() are real, though, and I have a patch
to fix them this way:

-- snip --
Subject: [PATCH] sequencer: plug resource leak when skipping unnecessary picks

The resource leak only happens in case of an error writing or truncating
the file, therefore it seems less critical, but we should still fix it
nonetheless.

Discovered by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index e25a4e1180c..9c765e8850a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2678,11 +2678,13 @@ int skip_unnecessary_picks(void)
 		if (write_in_full(fd, todo_list.buf.buf + offset,
 				todo_list.buf.len - offset) < 0) {
 			todo_list_release(&todo_list);
+			close(fd);
 			return error_errno(_("could not write to '%s'"),
 				rebase_path_todo());
 		}
 		if (ftruncate(fd, todo_list.buf.len - offset) < 0) {
 			todo_list_release(&todo_list);
+			close(fd);
 			return error_errno(_("could not truncate '%s'"),
 				rebase_path_todo());
 		}
-- snap --

> But I while looking at them I did notice something it didn't mention:
> 
> > +	if (i > 0) {
> > +		int offset = i < todo_list.nr ?
> > +			todo_list.items[i].offset_in_buf : todo_list.buf.len;
> > +		const char *done_path = rebase_path_done();
> > +
> > +		fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666);
> > +		if (write_in_full(fd, todo_list.buf.buf, offset) < 0) {
> > +			todo_list_release(&todo_list);
> > +			return error_errno(_("could not write to '%s'"),
> > +				done_path);
> > +		}
> > +		close(fd);
> 
> This should probably check the result of open().

Indeed.

> I know write_in_full() will fail if fd is -1, but we'd rather show the
> user the errno from open(), not EBADF.

I guess Coverity follows that code path and determines that it is handled.

If only it would also follow the code paths of the FLEX_ARRAYs and figure
out that we play games with struct definitions whose last entries are
technically incorrect.

> Technically the free() calls from todo_list_release() can also munge
> errno before you print it. You might want to just call error_errno()
> first, then do the cleanup (including the missing close()).

Ah, you're right. I'll have to rework that patch I mentioned above.

> > +		fd = open(rebase_path_todo(), O_WRONLY, 0666);
> > +		if (write_in_full(fd, todo_list.buf.buf + offset,
> > +				todo_list.buf.len - offset) < 0) {
> > +			todo_list_release(&todo_list);
> > +			return error_errno(_("could not write to '%s'"),
> > +				rebase_path_todo());
> > +		}
> 
> Ditto here.

Right.

Ciao,
Dscho