| Message ID | 20240924215539.GH1143820@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 05372c28bedbd2ae067e695805590b7eea947e10 |
| Headers | show |
| Series | leak fixes for http fetch/push | expand |
On Tue, Sep 24, 2024 at 05:55:39PM -0400, Jeff King wrote: > diff --git a/remote.c b/remote.c > index 390a03c264..e291e8ff5c 100644 > --- a/remote.c > +++ b/remote.c > @@ -2544,7 +2544,7 @@ struct ref *get_stale_heads(struct refspec *rs, struct ref *fetch_map) > /* > * Compare-and-swap > */ > -static void clear_cas_option(struct push_cas_option *cas) > +void clear_cas_option(struct push_cas_option *cas) > { > int i; > > diff --git a/remote.h b/remote.h > index a58713f20a..ad4513f639 100644 > --- a/remote.h > +++ b/remote.h > @@ -409,6 +409,7 @@ struct push_cas_option { > }; > > int parseopt_push_cas_option(const struct option *, const char *arg, int unset); > +void clear_cas_option(struct push_cas_option *); Nit: I was wondering whether we'd also want to fix up this functions name to conform to our style guide, which says this should be called `push_cas_option_clear()` instead. But I don't mind it much, so please feel free to ignore this nit. Patrick
On Thu, Sep 26, 2024 at 03:50:09PM +0200, Patrick Steinhardt wrote: > On Tue, Sep 24, 2024 at 05:55:39PM -0400, Jeff King wrote: > > diff --git a/remote.c b/remote.c > > index 390a03c264..e291e8ff5c 100644 > > --- a/remote.c > > +++ b/remote.c > > @@ -2544,7 +2544,7 @@ struct ref *get_stale_heads(struct refspec *rs, struct ref *fetch_map) > > /* > > * Compare-and-swap > > */ > > -static void clear_cas_option(struct push_cas_option *cas) > > +void clear_cas_option(struct push_cas_option *cas) > > { > > int i; > > > > diff --git a/remote.h b/remote.h > > index a58713f20a..ad4513f639 100644 > > --- a/remote.h > > +++ b/remote.h > > @@ -409,6 +409,7 @@ struct push_cas_option { > > }; > > > > int parseopt_push_cas_option(const struct option *, const char *arg, int unset); > > +void clear_cas_option(struct push_cas_option *); > > Nit: I was wondering whether we'd also want to fix up this functions > name to conform to our style guide, which says this should be called > `push_cas_option_clear()` instead. But I don't mind it much, so please > feel free to ignore this nit. I'd prefer to punt on that for now, as the whole suite of "methods" for this struct would need to be renamed to match that style. If we were making a too-short name into a public symbol, I'd be worried about addressing that now, but I think this is purely about style and can wait. -Peff
diff --git a/builtin/push.c b/builtin/push.c index e6f48969b8..59d4485603 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -669,6 +669,7 @@ int cmd_push(int argc, rc = do_push(flags, push_options, remote); string_list_clear(&push_options_cmdline, 0); string_list_clear(&push_options_config, 0); + clear_cas_option(&cas); if (rc == -1) usage_with_options(push_usage, options); else diff --git a/builtin/send-pack.c b/builtin/send-pack.c index c49fe6c53c..8b1d46e79a 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -344,5 +344,6 @@ int cmd_send_pack(int argc, free_refs(local_refs); refspec_clear(&rs); oid_array_clear(&shallow); + clear_cas_option(&cas); return ret; } diff --git a/remote.c b/remote.c index 390a03c264..e291e8ff5c 100644 --- a/remote.c +++ b/remote.c @@ -2544,7 +2544,7 @@ struct ref *get_stale_heads(struct refspec *rs, struct ref *fetch_map) /* * Compare-and-swap */ -static void clear_cas_option(struct push_cas_option *cas) +void clear_cas_option(struct push_cas_option *cas) { int i; diff --git a/remote.h b/remote.h index a58713f20a..ad4513f639 100644 --- a/remote.h +++ b/remote.h @@ -409,6 +409,7 @@ struct push_cas_option { }; int parseopt_push_cas_option(const struct option *, const char *arg, int unset); +void clear_cas_option(struct push_cas_option *); int is_empty_cas(const struct push_cas_option *); void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);
The send-pack --force-with-lease option populates a push_cas_option struct with allocated strings. Exiting without cleaning this up will cause leak-checkers to complain. We can fix this by calling clear_cas_option(), after making it publicly available. Previously it was used only for resetting the list when we saw --no-force-with-lease. The git-push command has the same "leak", though in this case it won't trigger a leak-checker since it stores the push_cas_option struct as a global rather than on the stack (and is thus reachable even after main() exits). I've added cleanup for it here anyway, though, as future-proofing. The leak is triggered by t5541 (it tests --force-with-lease over http, which requires a separate send-pack process under the hood), but we can't mark it as leak-free yet. Signed-off-by: Jeff King <peff@peff.net> --- builtin/push.c | 1 + builtin/send-pack.c | 1 + remote.c | 2 +- remote.h | 1 + 4 files changed, 4 insertions(+), 1 deletion(-)