Re: [PATCH 06/22] sequencer: release memory that was allocated when reading options
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 31, 2016, 08:07 UTC
- Message-ID
- <alpine.DEB.2.20.1608311005210.129229@virtualbox>
- In-Reply-To
- <xmqqfupm3ya1.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 30 Aug 2016, Junio C Hamano wrote:
Show 28 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > @@ -811,13 +813,18 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
> > opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
> > else if (!strcmp(key, "options.mainline"))
> > opts->mainline = git_config_int(key, value);
> > - else if (!strcmp(key, "options.strategy"))
> > + else if (!strcmp(key, "options.strategy")) {
> > git_config_string(&opts->strategy, key, value);
> > - else if (!strcmp(key, "options.gpg-sign"))
> > + sequencer_entrust(opts, (char *) opts->strategy);
> > + }
> > + else if (!strcmp(key, "options.gpg-sign")) {
> > git_config_string(&opts->gpg_sign, key, value);
> > + sequencer_entrust(opts, (char *) opts->gpg_sign);
> > + }
> > else if (!strcmp(key, "options.strategy-option")) {
> > ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
> > - opts->xopts[opts->xopts_nr++] = xstrdup(value);
> > + opts->xopts[opts->xopts_nr++] =
> > + sequencer_entrust(opts, xstrdup(value));
> > } else
> > return error(_("Invalid key: %s"), key);
>
> Hmm.
>
> I would have expected a call to sequencer_opts_clear(&opts) once the
> machinery is done with the options structure,It is part of sequencer_remove_state().
> and among these places where an old value in opts->field is overwritten > by a new one would get > > free(opt->field); opt->field = ... new value ...;
That is *exactly* what we *cannot* do.
You see, the replay_opts struct is *not necessarily* populated by read_populate_opts(). So we really cannot tell whether we are free to free(opt->field) or whether some other code still wants to reference that memory in the end (or for that matter, whether it was malloc()ed).
That is the *precise* reason for the existence of sequencer_entrust().
Ciao, Dscho