| Message ID | 20240531113545.GJ428814@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leak fixes for sparse-checkout code | expand |
On Fri, May 31, 2024 at 07:35:45AM -0400, Jeff King wrote: > We allocate a heap buffer via get_sparse_checkout_filename(). Most calls > remember to free it, but sparse_checkout_init() forgets to, causing a > leak. Ironically, it remembers to do so in the error return paths, but > not in the path that makes it all the way to the function end! > > Fixing this clears up 6 leaks from t1091. > > Signed-off-by: Jeff King <peff@peff.net> > --- > builtin/sparse-checkout.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c > index 356b7349f9..3af9fec1fb 100644 > --- a/builtin/sparse-checkout.c > +++ b/builtin/sparse-checkout.c > @@ -500,6 +500,8 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix) > return 0; > } > > + free(sparse_filename); > + I wonder whether it would make sense to merge this patch and patch 4 and then refactor the code to have a common exit path. Patrick
On Tue, Jun 04, 2024 at 09:42:57AM +0200, Patrick Steinhardt wrote: > > diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c > > index 356b7349f9..3af9fec1fb 100644 > > --- a/builtin/sparse-checkout.c > > +++ b/builtin/sparse-checkout.c > > @@ -500,6 +500,8 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix) > > return 0; > > } > > > > + free(sparse_filename); > > + > > I wonder whether it would make sense to merge this patch and patch 4 > and then refactor the code to have a common exit path. I thought about that, too, but it doesn't quite work. In the non-error exit path we _don't_ clean up the pattern_list, because we tail-call into write_patterns_and_update(), which frees it itself. If we refactored that function to _not_ free, and then switched here to a "ret" variable, like: ... ret = write_patterns_and_update(&pl); out: clear_pattern_list(&pl); free(sparse_filename); return ret; it could work. I mostly tried to err on the side of minimizing refactoring, though. -Peff
On Tue, Jun 04, 2024 at 06:01:42AM -0400, Jeff King wrote: > On Tue, Jun 04, 2024 at 09:42:57AM +0200, Patrick Steinhardt wrote: > > > > diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c > > > index 356b7349f9..3af9fec1fb 100644 > > > --- a/builtin/sparse-checkout.c > > > +++ b/builtin/sparse-checkout.c > > > @@ -500,6 +500,8 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix) > > > return 0; > > > } > > > > > > + free(sparse_filename); > > > + > > > > I wonder whether it would make sense to merge this patch and patch 4 > > and then refactor the code to have a common exit path. > > I thought about that, too, but it doesn't quite work. In the non-error > exit path we _don't_ clean up the pattern_list, because we tail-call > into write_patterns_and_update(), which frees it itself. > > If we refactored that function to _not_ free, and then switched here to > a "ret" variable, like: > > ... > ret = write_patterns_and_update(&pl); > out: > clear_pattern_list(&pl); > free(sparse_filename); > return ret; > > it could work. I mostly tried to err on the side of minimizing > refactoring, though. Fair enough, thanks for explaining. Patrick
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index 356b7349f9..3af9fec1fb 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -500,6 +500,8 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix) return 0; } + free(sparse_filename); + add_pattern("/*", empty_base, 0, &pl, 0); add_pattern("!/*/", empty_base, 0, &pl, 0); pl.use_cone_patterns = init_opts.cone_mode;
We allocate a heap buffer via get_sparse_checkout_filename(). Most calls remember to free it, but sparse_checkout_init() forgets to, causing a leak. Ironically, it remembers to do so in the error return paths, but not in the path that makes it all the way to the function end! Fixing this clears up 6 leaks from t1091. Signed-off-by: Jeff King <peff@peff.net> --- builtin/sparse-checkout.c | 2 ++ 1 file changed, 2 insertions(+)