Re: [PATCH v4 1/3] merge-ort: fix massive leak
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Jan 24, 2021, 19:11 UTC
- Message-ID
- <7e1c184f-4745-530d-8aca-879319786845@gmail.com>
- In-Reply-To
- <20210124060112.1258291-2-newren@gmail.com>
On 1/24/2021 1:01 AM, Elijah Newren wrote:
Show 34 quoted lines
> When a series of merges was performed (such as for a rebase or series of
> cherry-picks), only the data structures allocated by the final merge
> operation were being freed. The problem was that while picking out
> pieces of merge-ort to upstream, I previously misread a certain section
> of merge_start() and assumed it was associated with a later
> optimization. Include that section now, which ensures that if there was
> a previous merge operation, that we clear out result->priv and then
> re-use it for opt->priv, and otherwise we allocate opt->priv.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> merge-ort.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 05c6b2e0dc..b5845ff6e9 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -3227,11 +3227,28 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
> assert(opt->obuf.len == 0);
>
> assert(opt->priv == NULL);
> + if (result->priv) {
> + opt->priv = result->priv;
> + result->priv = NULL;
> + /*
> + * opt->priv non-NULL means we had results from a previous
> + * run; do a few sanity checks that user didn't mess with
> + * it in an obvious fashion.
> + */
> + assert(opt->priv->call_depth == 0);
> + assert(!opt->priv->toplevel_dir ||
> + 0 == strlen(opt->priv->toplevel_dir));
> + }So instead of simply leaking result->priv, we re-use the data for the next round.
Show 11 quoted lines
>
> /* Default to histogram diff. Actually, just hardcode it...for now. */
> opt->xdl_opts = DIFF_WITH_ALG(opt, HISTOGRAM_DIFF);
>
> /* Initialization of opt->priv, our internal merge data */
> + if (opt->priv) {
> + clear_or_reinit_internal_opts(opt->priv, 1);
> + trace2_region_leave("merge", "allocate/init", opt->repo);
> + return;
> + }
> opt->priv = xcalloc(1, sizeof(*opt->priv));and here you reset the data instead of reallocating it. OK.
-Stolee