| Message ID | 97346d6f944e3587a08d96a5e1b4ead8df8a0bc0.1724159575.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.5) | expand |
Patrick Steinhardt <ps@pks.im> writes: > @@ -2802,6 +2809,7 @@ void remote_state_clear(struct remote_state *remote_state) > for (i = 0; i < remote_state->remotes_nr; i++) > remote_clear(remote_state->remotes[i]); > FREE_AND_NULL(remote_state->remotes); > + FREE_AND_NULL(remote_state->pushremote_name); > remote_state->remotes_alloc = 0; > remote_state->remotes_nr = 0; As remote_state has two extra structures embedded in it, I wonder if we should be clearing them in this function, but possibly it is cleared elsewhere or perhaps in a later series? As the focus of this step is about strings that we obtained from the config API, it is totally outside the scope of this topic, even if it turns out to be needed to clear them. Looking good. Thanks.
On Wed, Aug 21, 2024 at 10:58:55AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > @@ -2802,6 +2809,7 @@ void remote_state_clear(struct remote_state *remote_state) > > for (i = 0; i < remote_state->remotes_nr; i++) > > remote_clear(remote_state->remotes[i]); > > FREE_AND_NULL(remote_state->remotes); > > + FREE_AND_NULL(remote_state->pushremote_name); > > remote_state->remotes_alloc = 0; > > remote_state->remotes_nr = 0; > > As remote_state has two extra structures embedded in it, I wonder if > we should be clearing them in this function, but possibly it is > cleared elsewhere or perhaps in a later series? It is not yet part of any subsequent patch series, mostly because I didn't happen to stumble over such leaks yet. Both of the rewrites very much are leaky though, and would be hit when we use "insteadOf" or "pushInsteadOf" configs. The `struct branch` also needs handling and is being populated via "branch" configs. > As the focus of this step is about strings that we obtained from the > config API, it is totally outside the scope of this topic, even if > it turns out to be needed to clear them. Well, these are being populated via config strings. So I'd rather fix them in this commit, as well. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Wed, Aug 21, 2024 at 10:58:55AM -0700, Junio C Hamano wrote: >> Patrick Steinhardt <ps@pks.im> writes: >> >> > @@ -2802,6 +2809,7 @@ void remote_state_clear(struct remote_state *remote_state) >> > for (i = 0; i < remote_state->remotes_nr; i++) >> > remote_clear(remote_state->remotes[i]); >> > FREE_AND_NULL(remote_state->remotes); >> > + FREE_AND_NULL(remote_state->pushremote_name); >> > remote_state->remotes_alloc = 0; >> > remote_state->remotes_nr = 0; >> >> As remote_state has two extra structures embedded in it, I wonder if >> we should be clearing them in this function, but possibly it is >> cleared elsewhere or perhaps in a later series? > > It is not yet part of any subsequent patch series, mostly because I > didn't happen to stumble over such leaks yet. Both of the rewrites very > much are leaky though, and would be hit when we use "insteadOf" or > "pushInsteadOf" configs. Yes, I was wondering if our test coverage for the feature is lacking. In a sense, leaks from these insteadOf configuration variables are tiny and uninteresting, compared to how much it bothers me to find us not testing these often used features.
diff --git a/remote.c b/remote.c index 7d5b8f750d8..3087437bc61 100644 --- a/remote.c +++ b/remote.c @@ -373,8 +373,10 @@ static int handle_config(const char *key, const char *value, return -1; branch = make_branch(remote_state, name, namelen); if (!strcmp(subkey, "remote")) { + FREE_AND_NULL(branch->remote_name); return git_config_string(&branch->remote_name, key, value); } else if (!strcmp(subkey, "pushremote")) { + FREE_AND_NULL(branch->pushremote_name); return git_config_string(&branch->pushremote_name, key, value); } else if (!strcmp(subkey, "merge")) { if (!value) @@ -406,9 +408,11 @@ static int handle_config(const char *key, const char *value, return 0; /* Handle remote.* variables */ - if (!name && !strcmp(subkey, "pushdefault")) + if (!name && !strcmp(subkey, "pushdefault")) { + FREE_AND_NULL(remote_state->pushremote_name); return git_config_string(&remote_state->pushremote_name, key, value); + } if (!name) return 0; @@ -475,12 +479,15 @@ static int handle_config(const char *key, const char *value, else if (!strcmp(value, "--tags")) remote->fetch_tags = 2; } else if (!strcmp(subkey, "proxy")) { + FREE_AND_NULL(remote->http_proxy); return git_config_string(&remote->http_proxy, key, value); } else if (!strcmp(subkey, "proxyauthmethod")) { + FREE_AND_NULL(remote->http_proxy_authmethod); return git_config_string(&remote->http_proxy_authmethod, key, value); } else if (!strcmp(subkey, "vcs")) { + FREE_AND_NULL(remote->foreign_vcs); return git_config_string(&remote->foreign_vcs, key, value); } return 0; @@ -2802,6 +2809,7 @@ void remote_state_clear(struct remote_state *remote_state) for (i = 0; i < remote_state->remotes_nr; i++) remote_clear(remote_state->remotes[i]); FREE_AND_NULL(remote_state->remotes); + FREE_AND_NULL(remote_state->pushremote_name); remote_state->remotes_alloc = 0; remote_state->remotes_nr = 0;
We're leaking several config strings when assembling remotes, either because we do not free preceding values in case a config was set multiple times, or because we do not free them when releasing the remote state. Plug those leaks. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- remote.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)