Re: [PATCH v3 12/25] split_commit_in_progress(): fix memory leak

["René Scharfe" <l.s.r@web.de>]

Am 04.05.2017 um 12:59 schrieb Johannes Schindelin:
> 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.

> 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.

> -- 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.

> +	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;
>   }
>   
>