Re: [PATCH v3 12/25] split_commit_in_progress(): fix memory leak
- From
René Scharfe <l.s.r@web.de>
- Date
- May 6, 2017, 17:13 UTC
- Message-ID
- <49fad7f1-0bad-dbd2-89ff-5476f9b5bf91@web.de>
- In-Reply-To
- <alpine.DEB.2.21.1.1705041231270.4905@virtualbox>
Am 04.05.2017 um 12:59 schrieb Johannes Schindelin:
Show 82 quoted lines
> Hi René,
>
> On Wed, 3 May 2017, René Scharfe wrote:
>
>> Am 02.05.2017 um 18:02 schrieb Johannes Schindelin:
>>> Reported via Coverity.
>>>
>>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>>> ---
>>> wt-status.c | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/wt-status.c b/wt-status.c
>>> index 0a6e16dbe0f..1f3f6bcb980 100644
>>> --- a/wt-status.c
>>> +++ b/wt-status.c
>>> @@ -1088,8 +1088,13 @@ static int split_commit_in_progress(struct wt_status
>>> *s)
>>> char *rebase_orig_head =
>>> read_line_from_git_path("rebase-merge/orig-head");
>>>
>>> if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
>>> - !s->branch || strcmp(s->branch, "HEAD"))
>>> + !s->branch || strcmp(s->branch, "HEAD")) {
>>> + free(head);
>>> + free(orig_head);
>>> + free(rebase_amend);
>>> + free(rebase_orig_head);
>>> return split_in_progress;
>>> + }
>>>
>>> if (!strcmp(rebase_amend, rebase_orig_head)) {
>>> if (strcmp(head, rebase_amend))
>>>
>>
>> The return line could be replaced by "; else" to achieve the same
>> result, without duplicating the free calls.
>
> True. It is much harder to explain it that way, though: the context looks
> like this:
>
> static int split_commit_in_progress(struct wt_status *s)
> {
> int split_in_progress = 0;
> char *head = read_line_from_git_path("HEAD");
> char *orig_head = read_line_from_git_path("ORIG_HEAD");
> char *rebase_amend = read_line_from_git_path("rebase-merge/amend");
> char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
>
> if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
> - !s->branch || strcmp(s->branch, "HEAD"))
> + !s->branch || strcmp(s->branch, "HEAD")) {
> + free(head);
> + free(orig_head);
> + free(rebase_amend);
> + free(rebase_orig_head);
> return split_in_progress;
> + }
>
> if (!strcmp(rebase_amend, rebase_orig_head)) {
> if (strcmp(head, rebase_amend))
> split_in_progress = 1;
> } else if (strcmp(orig_head, rebase_orig_head)) {
> split_in_progress = 1;
> }
>
> if (!s->amend && !s->nowarn && !s->workdir_dirty)
> split_in_progress = 0;
>
> free(head);
> free(orig_head);
> free(rebase_amend);
> free(rebase_orig_head);
> return split_in_progress;
> }
>
> So as you see, if we make the change you suggested, the next if() is hit
> which possibly sets split_in_progress = 0.
>
> The thing is: this variable has been initialized to 0 in the beginning! So
> this conditional assignment would be a noop, unless any of the code paths
> before this conditional modified split_in_progress.Right. It could have been used as a quick two-line fix.
Show 6 quoted lines
> After you fully wrapped your head around this code, I am sure you agree > that the code is unnecessarily confusing to begin with: why do we go out > of our way to allocate and read all those strings, compare them to figure > out whether there is a split in progress, just to look at bits in the > wt_status struct (that we had available from the beginning) to potentially > undo all of that.
The only function with a higher cyclomatic complexity in this file is wt_longstatus_print(), which spans more than 100 lines. Amazing.
> What's worse, I cannot even find a logical explanation why the code is so > confusing, as it has been added as it is today in commit 2d1ccebae41 > (status: better advices when splitting a commit (during rebase -i), > 2012-06-10).
Repeatedly I get the impression that defects or shortcomings tend to cluster. It pays to take a good look at the code surrounding a find of an automatic checker for other possible improvements.
> So I think this function really wants to be fixed more fully (although I > feel bad for inserting such a non-trivial fix into a patch series > otherwise populated by trivial memory/resource leak plugs):
It could be done in two steps. Doing it in one is probably OK as well.
Show 59 quoted lines
> -- snipsnap --
> Subject: [PATCH] squash! split_commit_in_progress(): fix memory leak
>
> split_commit_in_progress(): simplify & fix memory leak
>
> This function did a whole lot of unnecessary work, such as reading in
> four files just to figure out that, oh, hey, we do not need to look at
> them after all because the HEAD is not detached.
>
> Simplify the entire function to return early when possible, to read in
> the files only when necessary, and to release the allocated memory
> always (there was a leak, reported via Coverity, where we failed to
> release the allocated strings if the HEAD is not detached).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> wt-status.c | 39 +++++++++++++++++----------------------
> 1 file changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index 1f3f6bcb980..117ac8cfb01 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1082,34 +1082,29 @@ static char *read_line_from_git_path(const char *filename)
> static int split_commit_in_progress(struct wt_status *s)
> {
> int split_in_progress = 0;
> - char *head = read_line_from_git_path("HEAD");
> - char *orig_head = read_line_from_git_path("ORIG_HEAD");
> - char *rebase_amend = read_line_from_git_path("rebase-merge/amend");
> - char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
> -
> - if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
> - !s->branch || strcmp(s->branch, "HEAD")) {
> - free(head);
> - free(orig_head);
> - free(rebase_amend);
> - free(rebase_orig_head);
> - return split_in_progress;
> - }
> -
> - if (!strcmp(rebase_amend, rebase_orig_head)) {
> - if (strcmp(head, rebase_amend))
> - split_in_progress = 1;
> - } else if (strcmp(orig_head, rebase_orig_head)) {
> - split_in_progress = 1;
> - }
> + char *head, *orig_head, *rebase_amend, *rebase_orig_head;
> +
> + if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
> + !s->branch || strcmp(s->branch, "HEAD"))
> + return 0;
>
> - if (!s->amend && !s->nowarn && !s->workdir_dirty)
> - split_in_progress = 0;
> + head = read_line_from_git_path("HEAD");
> + orig_head = read_line_from_git_path("ORIG_HEAD");
> + rebase_amend = read_line_from_git_path("rebase-merge/amend");
> + rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");Further improvement idea (for a later series): Use rebase_path_amend() and rebase_path_orig_head() somehow, to build each path only once.
Accessing the files HEAD and ORIG_HEAD directly seems odd.
The second part of the function should probably be moved to sequencer.c.
> + > + if (!head || !orig_head || !rebase_amend || !rebase_orig_head) > + ; /* fall through, no split in progress */
You could set split_in_progress to zero here. Would save a comment without losing readability.
Show 10 quoted lines
> + else if (!strcmp(rebase_amend, rebase_orig_head)) > + split_in_progress = !!strcmp(head, rebase_amend); > + else if (strcmp(orig_head, rebase_orig_head)) > + split_in_progress = 1; > > free(head); > free(orig_head); > free(rebase_amend); > free(rebase_orig_head); > +
Isn't the patch big enough already without rearranging the else blocks and adding that blank line? :)
> return split_in_progress; > } > >