Re: [PATCH v2 7/9] rebase -i: skip unnecessary picks using the rebase--helper
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Apr 26, 2017, 11:31 UTC
- Message-ID
- <alpine.DEB.2.20.1704261321470.3480@virtualbox>
- In-Reply-To
- <20170426105503.wewlbefrsdmjqnob@sigill.intra.peff.net>
Hi Peff,
On Wed, 26 Apr 2017, Jeff King wrote:
Show 17 quoted lines
> 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