git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH 2/3] dir: make clear_directory() free all relevant memory
Date: Sun, 16 Aug 2020 04:54:06 -0400	[thread overview]
Message-ID: <20200816085406.GB1221900@coredump.intra.peff.net> (raw)
In-Reply-To: <068e097e22fa42b79e70b0346cc7460f1a3cbcff.1597561152.git.gitgitgadget@gmail.com>

On Sun, Aug 16, 2020 at 06:59:10AM +0000, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
> 
> The calling convention for the dir API is supposed to end with a call to
> clear_directory() to free up no longer needed memory.  However,
> clear_directory() didn't free dir->entries or dir->ignored.  I believe
> this was oversight, but a number of callers noticed memory leaks and
> started free'ing these, but often somewhat haphazardly (sometimes
> freeing the entries in the arrays, and sometimes only free'ing the
> arrays themselves).  This suggests the callers weren't trying to make
> sure any possible memory used might be free'd, but just the memory they
> noticed their usecase definitely had allocated.  This also caused the
> extra memory deallocations to be duplicated in many places.
> 
> Fix this mess by moving all the duplicated free'ing logic into
> clear_directory().

Makes sense. I don't know the dir.c code very well, so my worry would be
that some caller really wanted the other fields left untouched. But
looking over the callers, it seems they're all clearing it before the
struct goes out of scope. It's possible that they could have created
other pointer references, but it seems unlikely (and I'd argue they
should stop doing that and make their own copies of the data). E.g.,
wt_status_collect_untracked() sticks names into a string_list, but it
sets strdup_strings to make its own copy, so it's good.

> @@ -3034,6 +3031,13 @@ void clear_directory(struct dir_struct *dir)
>  		free(group->pl);
>  	}
>  
> +	for (i = 0; i < dir->ignored_nr; i++)
> +		free(dir->ignored[i]);
> +	for (i = 0; i < dir->nr; i++)
> +		free(dir->entries[i]);
> +	free(dir->ignored);
> +	free(dir->entries);
> +
>  	stk = dir->exclude_stack;
>  	while (stk) {
>  		struct exclude_stack *prev = stk->prev;

In most of our "clear" functions, the struct is ready for use again
(i.e., fields are set back to the initialized state). I don't think any
caller cares at this point, but it may be worth doing while we are here
as a least-surprise thing. That would mean setting these pointers to
NULL, and probably a few others that you aren't touching here.

Perhaps even easier would be to add a dir_init() call at the end after
your next patch adds that function.

-Peff

  reply	other threads:[~2020-08-16  8:54 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-16  6:59 [PATCH 0/3] Clean up some memory leaks in and around dir.c Elijah Newren via GitGitGadget
2020-08-16  6:59 ` [PATCH 1/3] dir: fix leak of parent_hashmap and recursive_hashmap Elijah Newren via GitGitGadget
2020-08-16  8:43   ` Jeff King
2020-08-17 16:57     ` Elijah Newren
2020-08-16  6:59 ` [PATCH 2/3] dir: make clear_directory() free all relevant memory Elijah Newren via GitGitGadget
2020-08-16  8:54   ` Jeff King [this message]
2020-08-17 16:58     ` Elijah Newren
2020-08-16  6:59 ` [PATCH 3/3] dir: fix problematic API to avoid memory leaks Elijah Newren via GitGitGadget
2020-08-16  9:11   ` Jeff King
2020-08-17 17:19     ` Elijah Newren
2020-08-17 19:00       ` Jeff King
2020-08-18 22:58 ` [PATCH v2 0/2] Clean up some memory leaks in and around dir.c Elijah Newren via GitGitGadget
2020-08-18 22:58   ` [PATCH v2 1/2] dir: make clear_directory() free all relevant memory Elijah Newren via GitGitGadget
2020-08-18 22:58   ` [PATCH v2 2/2] dir: fix problematic API to avoid memory leaks Elijah Newren via GitGitGadget
2020-08-19 13:51   ` [PATCH v2 0/2] Clean up some memory leaks in and around dir.c Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200816085406.GB1221900@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=newren@gmail.com \
    --subject='Re: [PATCH 2/3] dir: make clear_directory() free all relevant memory' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).