| Message ID | RFC-patch-01.15-b3a678d934a-20220603T183608Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix GCC -fanalyzer warnings & add -fanalyzer DEVOPTS mode | expand |
Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason: > Fix a bug in fd3cb0501e1 (remote: move static variables into > per-repository struct, 2021-11-17) where we'd free(remote->pushurl[i]) > after having NULL'd out remote->pushurl. itself. > > While we're at it let's get rid of the redundant braces per the > CodingGuidelines, which also serves to show in the diff context that > we were doing a FREE_AND_NULL(remote->pushurl) afterwards too, let's > keep that one. The extended context is helping, but the brace removal makes this change harder to read. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > remote.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/remote.c b/remote.c > index 930fdc9c2f6..61240562df1 100644 > --- a/remote.c > +++ b/remote.c > @@ -144,14 +144,10 @@ static void remote_clear(struct remote *remote) > free((char *)remote->name); > free((char *)remote->foreign_vcs); > > - for (i = 0; i < remote->url_nr; i++) { > + for (i = 0; i < remote->url_nr; i++) > free((char *)remote->url[i]); > - } > - FREE_AND_NULL(remote->pushurl); So you remove the redundant release of the pushurl array, but the url array, which should be freed here after its elements are released, still leaks. The duplicate FREE_AND_NULL perhaps resulted from a copy&paste error. > - > - for (i = 0; i < remote->pushurl_nr; i++) { > + for (i = 0; i < remote->pushurl_nr; i++) > free((char *)remote->pushurl[i]); > - } > FREE_AND_NULL(remote->pushurl); Why set pushurl to NULL after release? This results in an invalid state unless pushurl_nr und pushurl_alloc are reset to zero. Same goes for the url array above -- either a simple free(3) call suffices or url_nr and url_alloc need to be cleared as well. > free((char *)remote->receivepack); > free((char *)remote->uploadpack);
René Scharfe <l.s.r@web.de> writes: >> - for (i = 0; i < remote->pushurl_nr; i++) { >> + for (i = 0; i < remote->pushurl_nr; i++) >> free((char *)remote->pushurl[i]); >> - } >> FREE_AND_NULL(remote->pushurl); > > Why set pushurl to NULL after release? This results in an invalid state > unless pushurl_nr und pushurl_alloc are reset to zero. Same goes for > the url array above -- either a simple free(3) call suffices or url_nr > and url_alloc need to be cleared as well. We probably should give a huge warning next to FREE_AND_NULL() about this. It also is an effective way to hide an existing bug under the rug. diff_options.pathspec might be freed prematurely which may be noticed by a use-after-free if left to use free(), but FREE_AND_NULL() will mislead the use-after-free caller into thinkig that "ah there is no pathspec to be used" and produce nonsense result without crashing. Thanks.
René Scharfe <l.s.r@web.de> writes: > Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason: >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> remote.c | 8 ++------ >> 1 file changed, 2 insertions(+), 6 deletions(-) >> >> diff --git a/remote.c b/remote.c >> index 930fdc9c2f6..61240562df1 100644 >> --- a/remote.c >> +++ b/remote.c >> @@ -144,14 +144,10 @@ static void remote_clear(struct remote *remote) >> free((char *)remote->name); >> free((char *)remote->foreign_vcs); >> >> - for (i = 0; i < remote->url_nr; i++) { >> + for (i = 0; i < remote->url_nr; i++) >> free((char *)remote->url[i]); >> - } >> - FREE_AND_NULL(remote->pushurl); > > So you remove the redundant release of the pushurl array, but the url > array, which should be freed here after its elements are released, still > leaks. The duplicate FREE_AND_NULL perhaps resulted from a copy&paste > error. This seems like the most likely explanation (even I don't recall how I ended up introducing this bug..). I probably meant to change pushurl -> url, but forgot somehow. >> - >> - for (i = 0; i < remote->pushurl_nr; i++) { >> + for (i = 0; i < remote->pushurl_nr; i++) >> free((char *)remote->pushurl[i]); >> - } >> FREE_AND_NULL(remote->pushurl); > > Why set pushurl to NULL after release? This results in an invalid state > unless pushurl_nr und pushurl_alloc are reset to zero. Same goes for > the url array above -- either a simple free(3) call suffices or url_nr > and url_alloc need to be cleared as well. True, thanks for catching that. We'd probably never notice it because we never reuse the clear-ed `struct remote` (since we only ever call this when we clean up `struct repository`), but it's better to do the 'right thing'. I didn't consider that when I chose to use FREE_AND_NULL() instead of free(), and there wasn't any particular reason why I chose one over the other.
> On 03 June 2022 at 22:07 René Scharfe <l.s.r@web.de> wrote: > > > Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason: > > Fix a bug in fd3cb0501e1 (remote: move static variables into > > per-repository struct, 2021-11-17) where we'd free(remote->pushurl[i]) > > after having NULL'd out remote->pushurl. itself. > > > > While we're at it let's get rid of the redundant braces per the > > CodingGuidelines, which also serves to show in the diff context that > > we were doing a FREE_AND_NULL(remote->pushurl) afterwards too, let's > > keep that one. > > The extended context is helping, but the brace removal makes this change > harder to read. Indeed, a small style fix in a larger change is one thing but here at least 80% of the changed lines are unrelated to the bugfix. I'm afraid my heart has started to sink when I see the phrase "while we're at it" in a commit message. > [...] > > FREE_AND_NULL(remote->pushurl); > > Why set pushurl to NULL after release? This results in an invalid state > unless pushurl_nr und pushurl_alloc are reset to zero. Same goes for > the url array above -- either a simple free(3) call suffices or url_nr > and url_alloc need to be cleared as well. That's a good point Best Wishes Phillip
On Sat, Jun 04 2022, Phillip Wood wrote: >> On 03 June 2022 at 22:07 René Scharfe <l.s.r@web.de> wrote: >> >> >> Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason: >> > Fix a bug in fd3cb0501e1 (remote: move static variables into >> > per-repository struct, 2021-11-17) where we'd free(remote->pushurl[i]) >> > after having NULL'd out remote->pushurl. itself. >> > >> > While we're at it let's get rid of the redundant braces per the >> > CodingGuidelines, which also serves to show in the diff context that >> > we were doing a FREE_AND_NULL(remote->pushurl) afterwards too, let's >> > keep that one. >> >> The extended context is helping, but the brace removal makes this change >> harder to read. > > Indeed, a small style fix in a larger change is one thing but here at > least 80% of the changed lines are unrelated to the bugfix. I'm afraid > my heart has started to sink when I see the phrase "while we're at it" > in a commit message. I'm happy to omit it, I thought it was helpful to force the context to be shown, will change that.
diff --git a/remote.c b/remote.c index 930fdc9c2f6..61240562df1 100644 --- a/remote.c +++ b/remote.c @@ -144,14 +144,10 @@ static void remote_clear(struct remote *remote) free((char *)remote->name); free((char *)remote->foreign_vcs); - for (i = 0; i < remote->url_nr; i++) { + for (i = 0; i < remote->url_nr; i++) free((char *)remote->url[i]); - } - FREE_AND_NULL(remote->pushurl); - - for (i = 0; i < remote->pushurl_nr; i++) { + for (i = 0; i < remote->pushurl_nr; i++) free((char *)remote->pushurl[i]); - } FREE_AND_NULL(remote->pushurl); free((char *)remote->receivepack); free((char *)remote->uploadpack);
Fix a bug in fd3cb0501e1 (remote: move static variables into per-repository struct, 2021-11-17) where we'd free(remote->pushurl[i]) after having NULL'd out remote->pushurl. itself. While we're at it let's get rid of the redundant braces per the CodingGuidelines, which also serves to show in the diff context that we were doing a FREE_AND_NULL(remote->pushurl) afterwards too, let's keep that one. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)