From: René Scharfe Date: Sat, 06 May 2017 17:13:40 GMT Subject: Re: [PATCH v3 12/25] split_commit_in_progress(): fix memory leak Message-ID: <49fad7f1-0bad-dbd2-89ff-5476f9b5bf91@web.de> In-Reply-To: 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 >>> --- >>> 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 > --- > 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; > } > >