| Message ID | patch-02.10-050399cbfbf-20211004T002226Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | unpack-trees & dir APIs: fix memory leaks | expand |
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT > macro, but "merge-recursive.c" could not be converted to it since > it (re-)initializes a "struct unpack_trees_options" on the heap. > > In order to convert use the macro as the source of truth for > initialization we need to not only convert the initialization in > unpack_trees_start(), but also call the new > unpack_trees_options_init() just after the CALLOC_ARRAY() in > merge_start(). Um...why? > When we call merge_trees() we'll call merge_start() followed by > merge_trees_internal(), and it will call unpack_trees_start() before > it does much of anything. So it's covered by an initialization in > unpack_trees_start(). > > But when merge_recursive() is called it will call merge_start() > followed by merge_recursive_internal(), which won't call > unpack_trees_start() until its own call call to > merge_trees_internal(), but in the meantime it might end up using that > calloc'd "struct unpack_trees_options". Nothing in merge-recursive.c usings unpack_opts before unpack_trees_start() or after unpack_trees_finish(). If anyone attempts to use it elsewhere, that would itself be a bug. So, I'd replace the above three paragraphs with: "Change the initialization of opt->priv_unpack_opts from using memset to 0, with unpack_trees_options_init()." or something like that, and then drop your change to merge_start(). > This was OK before, as setup_unpack_trees_porcelain() would call > strvec_init(), and our "struct dir_struct" in turn is OK with being > NULL'd. Let's convert the former to macro initialization, we'll deal > with the latter in a subsequent commit. This is quite confusing; it's really hard to understand how this relates to the rest of the commit message. I have to read the code to see what you're doing, and then write my own commit message in my head because the wording in this paragraph still doesn't parse. I'd make the change strvec_init/STRVEC_INIT changes be a separate patch. I suspect it'll be easier to write the commit message for it as a standalone commit as well. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > merge-recursive.c | 3 ++- > unpack-trees.c | 8 ++++++-- > unpack-trees.h | 5 ++++- > 3 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/merge-recursive.c b/merge-recursive.c > index e594d4c3fa1..d24a4903f1d 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -405,7 +405,7 @@ static int unpack_trees_start(struct merge_options *opt, > struct tree_desc t[3]; > struct index_state tmp_index = { NULL }; > > - memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts)); > + unpack_trees_options_init(&opt->priv->unpack_opts); > if (opt->priv->call_depth) > opt->priv->unpack_opts.index_only = 1; > else > @@ -3704,6 +3704,7 @@ static int merge_start(struct merge_options *opt, struct tree *head) > > CALLOC_ARRAY(opt->priv, 1); > string_list_init_dup(&opt->priv->df_conflict_file_set); > + unpack_trees_options_init(&opt->priv->unpack_opts); Drop this hunk. > return 0; > } > > diff --git a/unpack-trees.c b/unpack-trees.c > index 8ea0a542da8..393c1f35a5d 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -108,8 +108,6 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, > const char **msgs = opts->msgs; > const char *msg; > > - strvec_init(&opts->msgs_to_free); > - > if (!strcmp(cmd, "checkout")) > msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE) > ? _("Your local changes to the following files would be overwritten by checkout:\n%%s" > @@ -189,6 +187,12 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, > opts->unpack_rejects[i].strdup_strings = 1; > } > > +void unpack_trees_options_init(struct unpack_trees_options *o) > +{ > + struct unpack_trees_options blank = UNPACK_TREES_OPTIONS_INIT; > + memcpy(o, &blank, sizeof(*o)); > +} > + > void clear_unpack_trees_porcelain(struct unpack_trees_options *opts) > { > strvec_clear(&opts->msgs_to_free); > diff --git a/unpack-trees.h b/unpack-trees.h > index ecf256cbcea..892b65ea623 100644 > --- a/unpack-trees.h > +++ b/unpack-trees.h > @@ -91,7 +91,10 @@ struct unpack_trees_options { > struct checkout_metadata meta; > }; > > -#define UNPACK_TREES_OPTIONS_INIT { 0 } > +#define UNPACK_TREES_OPTIONS_INIT { \ > + .msgs_to_free = STRVEC_INIT, \ > +} > +void unpack_trees_options_init(struct unpack_trees_options *o); > > int unpack_trees(unsigned n, struct tree_desc *t, > struct unpack_trees_options *options); > -- > 2.33.0.1404.g83021034c5d >
On Mon, Oct 04 2021, Elijah Newren wrote: > On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >> >> In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT >> macro, but "merge-recursive.c" could not be converted to it since >> it (re-)initializes a "struct unpack_trees_options" on the heap. >> >> In order to convert use the macro as the source of truth for >> initialization we need to not only convert the initialization in >> unpack_trees_start(), but also call the new >> unpack_trees_options_init() just after the CALLOC_ARRAY() in >> merge_start(). > > Um...why? Replied below. >> When we call merge_trees() we'll call merge_start() followed by >> merge_trees_internal(), and it will call unpack_trees_start() before >> it does much of anything. So it's covered by an initialization in >> unpack_trees_start(). >> >> But when merge_recursive() is called it will call merge_start() >> followed by merge_recursive_internal(), which won't call >> unpack_trees_start() until its own call call to >> merge_trees_internal(), but in the meantime it might end up using that >> calloc'd "struct unpack_trees_options". > > Nothing in merge-recursive.c usings unpack_opts before > unpack_trees_start() or after unpack_trees_finish(). If anyone > attempts to use it elsewhere, that would itself be a bug. So, I'd > replace the above three paragraphs with: > > "Change the initialization of opt->priv_unpack_opts from using memset > to 0, with unpack_trees_options_init()." > > or something like that, and then drop your change to merge_start(). Sure, I'll defer to you about being confident about that. I didn't want to leave a copy if it hanging without the proper initialization in case there were new callers. >> This was OK before, as setup_unpack_trees_porcelain() would call >> strvec_init(), and our "struct dir_struct" in turn is OK with being >> NULL'd. Let's convert the former to macro initialization, we'll deal >> with the latter in a subsequent commit. > > This is quite confusing; it's really hard to understand how this > relates to the rest of the commit message. I have to read the code to > see what you're doing, and then write my own commit message in my head > because the wording in this paragraph still doesn't parse. > > I'd make the change strvec_init/STRVEC_INIT changes be a separate > patch. I suspect it'll be easier to write the commit message for it > as a standalone commit as well. Sure, FWIW I had it split up locally, and figured it would be easier to explain if the API usage change came with the initialization change that made it possible. >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> merge-recursive.c | 3 ++- >> unpack-trees.c | 8 ++++++-- >> unpack-trees.h | 5 ++++- >> 3 files changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/merge-recursive.c b/merge-recursive.c >> index e594d4c3fa1..d24a4903f1d 100644 >> --- a/merge-recursive.c >> +++ b/merge-recursive.c >> @@ -405,7 +405,7 @@ static int unpack_trees_start(struct merge_options *opt, >> struct tree_desc t[3]; >> struct index_state tmp_index = { NULL }; >> >> - memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts)); >> + unpack_trees_options_init(&opt->priv->unpack_opts); >> if (opt->priv->call_depth) >> opt->priv->unpack_opts.index_only = 1; >> else >> @@ -3704,6 +3704,7 @@ static int merge_start(struct merge_options *opt, struct tree *head) >> >> CALLOC_ARRAY(opt->priv, 1); >> string_list_init_dup(&opt->priv->df_conflict_file_set); >> + unpack_trees_options_init(&opt->priv->unpack_opts); > > Drop this hunk. Speaking of splitting things up in more understandable ways: If we're going to hard rely on the interaction between merge_start() and unpack_trees_start() wouldn't it make sense to lead with a change that dropped that memset, which if this invariant holds is also redundant to the CALLOC() of opt->priv in merge_start() in the pre-image.
On Mon, Oct 4, 2021 at 7:49 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > On Mon, Oct 04 2021, Elijah Newren wrote: > > > On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > >> > >> In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT > >> macro, but "merge-recursive.c" could not be converted to it since > >> it (re-)initializes a "struct unpack_trees_options" on the heap. > >> > >> In order to convert use the macro as the source of truth for > >> initialization we need to not only convert the initialization in > >> unpack_trees_start(), but also call the new > >> unpack_trees_options_init() just after the CALLOC_ARRAY() in > >> merge_start(). > > > > Um...why? > > Replied below. > > >> When we call merge_trees() we'll call merge_start() followed by > >> merge_trees_internal(), and it will call unpack_trees_start() before > >> it does much of anything. So it's covered by an initialization in > >> unpack_trees_start(). > >> > >> But when merge_recursive() is called it will call merge_start() > >> followed by merge_recursive_internal(), which won't call > >> unpack_trees_start() until its own call call to > >> merge_trees_internal(), but in the meantime it might end up using that > >> calloc'd "struct unpack_trees_options". > > > > Nothing in merge-recursive.c usings unpack_opts before > > unpack_trees_start() or after unpack_trees_finish(). If anyone > > attempts to use it elsewhere, that would itself be a bug. So, I'd > > replace the above three paragraphs with: > > > > "Change the initialization of opt->priv_unpack_opts from using memset > > to 0, with unpack_trees_options_init()." > > > > or something like that, and then drop your change to merge_start(). > > Sure, I'll defer to you about being confident about that. I didn't want > to leave a copy if it hanging without the proper initialization in case > there were new callers. > > >> This was OK before, as setup_unpack_trees_porcelain() would call > >> strvec_init(), and our "struct dir_struct" in turn is OK with being > >> NULL'd. Let's convert the former to macro initialization, we'll deal > >> with the latter in a subsequent commit. > > > > This is quite confusing; it's really hard to understand how this > > relates to the rest of the commit message. I have to read the code to > > see what you're doing, and then write my own commit message in my head > > because the wording in this paragraph still doesn't parse. > > > > I'd make the change strvec_init/STRVEC_INIT changes be a separate > > patch. I suspect it'll be easier to write the commit message for it > > as a standalone commit as well. > > Sure, FWIW I had it split up locally, and figured it would be easier to > explain if the API usage change came with the initialization change that > made it possible. > > >> > >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > >> --- > >> merge-recursive.c | 3 ++- > >> unpack-trees.c | 8 ++++++-- > >> unpack-trees.h | 5 ++++- > >> 3 files changed, 12 insertions(+), 4 deletions(-) > >> > >> diff --git a/merge-recursive.c b/merge-recursive.c > >> index e594d4c3fa1..d24a4903f1d 100644 > >> --- a/merge-recursive.c > >> +++ b/merge-recursive.c > >> @@ -405,7 +405,7 @@ static int unpack_trees_start(struct merge_options *opt, > >> struct tree_desc t[3]; > >> struct index_state tmp_index = { NULL }; > >> > >> - memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts)); > >> + unpack_trees_options_init(&opt->priv->unpack_opts); > >> if (opt->priv->call_depth) > >> opt->priv->unpack_opts.index_only = 1; > >> else > >> @@ -3704,6 +3704,7 @@ static int merge_start(struct merge_options *opt, struct tree *head) > >> > >> CALLOC_ARRAY(opt->priv, 1); > >> string_list_init_dup(&opt->priv->df_conflict_file_set); > >> + unpack_trees_options_init(&opt->priv->unpack_opts); > > > > Drop this hunk. > > Speaking of splitting things up in more understandable ways: If we're > going to hard rely on the interaction between merge_start() and > unpack_trees_start() wouldn't it make sense to lead with a change that > dropped that memset, which if this invariant holds is also redundant to > the CALLOC() of opt->priv in merge_start() in the pre-image. It is not redundant, and that would be a very confusing change. merge-recursive is so named because its main driving function contains recursive calls to itself. merge_start() is not involved in that recursion. For readability, we should initialize unpack_opts before each use.
diff --git a/merge-recursive.c b/merge-recursive.c index e594d4c3fa1..d24a4903f1d 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -405,7 +405,7 @@ static int unpack_trees_start(struct merge_options *opt, struct tree_desc t[3]; struct index_state tmp_index = { NULL }; - memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts)); + unpack_trees_options_init(&opt->priv->unpack_opts); if (opt->priv->call_depth) opt->priv->unpack_opts.index_only = 1; else @@ -3704,6 +3704,7 @@ static int merge_start(struct merge_options *opt, struct tree *head) CALLOC_ARRAY(opt->priv, 1); string_list_init_dup(&opt->priv->df_conflict_file_set); + unpack_trees_options_init(&opt->priv->unpack_opts); return 0; } diff --git a/unpack-trees.c b/unpack-trees.c index 8ea0a542da8..393c1f35a5d 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -108,8 +108,6 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, const char **msgs = opts->msgs; const char *msg; - strvec_init(&opts->msgs_to_free); - if (!strcmp(cmd, "checkout")) msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE) ? _("Your local changes to the following files would be overwritten by checkout:\n%%s" @@ -189,6 +187,12 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, opts->unpack_rejects[i].strdup_strings = 1; } +void unpack_trees_options_init(struct unpack_trees_options *o) +{ + struct unpack_trees_options blank = UNPACK_TREES_OPTIONS_INIT; + memcpy(o, &blank, sizeof(*o)); +} + void clear_unpack_trees_porcelain(struct unpack_trees_options *opts) { strvec_clear(&opts->msgs_to_free); diff --git a/unpack-trees.h b/unpack-trees.h index ecf256cbcea..892b65ea623 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -91,7 +91,10 @@ struct unpack_trees_options { struct checkout_metadata meta; }; -#define UNPACK_TREES_OPTIONS_INIT { 0 } +#define UNPACK_TREES_OPTIONS_INIT { \ + .msgs_to_free = STRVEC_INIT, \ +} +void unpack_trees_options_init(struct unpack_trees_options *o); int unpack_trees(unsigned n, struct tree_desc *t, struct unpack_trees_options *options);
In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT macro, but "merge-recursive.c" could not be converted to it since it (re-)initializes a "struct unpack_trees_options" on the heap. In order to convert use the macro as the source of truth for initialization we need to not only convert the initialization in unpack_trees_start(), but also call the new unpack_trees_options_init() just after the CALLOC_ARRAY() in merge_start(). When we call merge_trees() we'll call merge_start() followed by merge_trees_internal(), and it will call unpack_trees_start() before it does much of anything. So it's covered by an initialization in unpack_trees_start(). But when merge_recursive() is called it will call merge_start() followed by merge_recursive_internal(), which won't call unpack_trees_start() until its own call call to merge_trees_internal(), but in the meantime it might end up using that calloc'd "struct unpack_trees_options". This was OK before, as setup_unpack_trees_porcelain() would call strvec_init(), and our "struct dir_struct" in turn is OK with being NULL'd. Let's convert the former to macro initialization, we'll deal with the latter in a subsequent commit. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- merge-recursive.c | 3 ++- unpack-trees.c | 8 ++++++-- unpack-trees.h | 5 ++++- 3 files changed, 12 insertions(+), 4 deletions(-)