| Message ID | xmqqlevl4ysk.fsf@gitster.g (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6dfadc8981a3f2fd3fb552eb956fe12a542f8ee8 |
| Headers | show |
| Series | clone: plug a miniscule leak | expand |
On 5/1/2022 1:17 AM, Junio C Hamano wrote: > Perhaps a Coccinelle rule like this might have caught similar > leaks: > > @@ > expression E; > expression V; > @@ > - if (E) > - V = xstrdup(E); > + if (E) { > + free(V); > + V = xstrdup(E); > + } > > The fact that the result of xstrdup() is assigned to V is that V > is meant to hold a pointer to an allocated piece of memory. > > With the preimage of the above semantic patch, it is reasonable > to expect that V may be initialized to NULL or may be holding a > pointer to a piece of allocated memory when the control reaches > here, because otherwise, V will be either need to be freed (when > E was not NULL, in which case we assigned the result of > xstrdup() to it) or V has garbage that cannot be freed later. Initially, I did think "what if V is not initialized to NULL?" but you are right that the code would already be broken in that case. > - if (option_origin != NULL) This technically wouldn't hit your rule, since "E" isn't just the variable name, as we typically do with our style. Is that something that Coccinelle automatically simplifies? > + if (option_origin != NULL) { Do you want to take this opportunity to drop the "!= NULL" here? > + free(remote_name); > remote_name = xstrdup(option_origin); > + } > > if (remote_name == NULL) Or do you want to keep similar style from the surrounding code? Either way, looks good to me. Thanks, -Stolee
Derrick Stolee <derrickstolee@github.com> writes: >> - if (option_origin != NULL) > > This technically wouldn't hit your rule, since "E" isn't just the > variable name, as we typically do with our style. Is that something > that Coccinelle automatically simplifies? > >> + if (option_origin != NULL) { > > Do you want to take this opportunity to drop the "!= NULL" here? > >> + free(remote_name); >> remote_name = xstrdup(option_origin); >> + } >> > if (remote_name == NULL) > > Or do you want to keep similar style from the surrounding code? I think that it is better to leave that particular clean-up to the equals-null.cocci topic started by Elia; I know having them separate would cause a merge conflict, but even if I change them here, it will result in the same merge conflict anyway ;-)
CC'ing Elia as per the mention. Thread https://lore.kernel.org/git/xmqqzgjzzwnd.fsf@gitster.g/t/#u. On 02/05/2022 18:12, Junio C Hamano wrote: <xmqqzgjzzwnd.fsf@gitster.g> > Derrick Stolee <derrickstolee@github.com> writes: > >>> - if (option_origin != NULL) >> This technically wouldn't hit your rule, since "E" isn't just the >> variable name, as we typically do with our style. Is that something >> that Coccinelle automatically simplifies? >> >>> + if (option_origin != NULL) { >> Do you want to take this opportunity to drop the "!= NULL" here? >> >>> + free(remote_name); >>> remote_name = xstrdup(option_origin); >>> + } >>>> if (remote_name == NULL) >> Or do you want to keep similar style from the surrounding code? > I think that it is better to leave that particular clean-up to > the equals-null.cocci topic started by Elia; I know having them > separate would cause a merge conflict, but even if I change them > here, it will result in the same merge conflict anyway ;-)
diff --git c/builtin/clone.c w/builtin/clone.c index 5231656379..194d50f75f 100644 --- c/builtin/clone.c +++ w/builtin/clone.c @@ -1106,8 +1106,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix) * apply the remote name provided by --origin only after this second * call to git_config, to ensure it overrides all config-based values. */ - if (option_origin != NULL) + if (option_origin != NULL) { + free(remote_name); remote_name = xstrdup(option_origin); + } if (remote_name == NULL) remote_name = xstrdup("origin");
The remote_name variable is first assigned a copy of the value of the "clone.defaultremotename" configuration variable and then by the value of the "--origin" command line option. The former is prepared to see multiple instances of the variable by freeing the current value of the variable before a copy of the newly discovered value gets assigned to it. The latter blindly assigned a copy of the new value to it, thereby leaking the value read from the configuration. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- This was discovered by a recently removed bogus coccinelle rewrite rule---if you apply an incorrect change suggested by the bogus rewrite rule to unconditionally assign to remote_name a copy of option_origin, or NULL, the value read from the configuration would be lost and never be used, thereby breaking a test to ensure the configuration is used, instead of the default remote nickname "origin". Perhaps a Coccinelle rule like this might have caught similar leaks: @@ expression E; expression V; @@ - if (E) - V = xstrdup(E); + if (E) { + free(V); + V = xstrdup(E); + } The fact that the result of xstrdup() is assigned to V is that V is meant to hold a pointer to an allocated piece of memory. With the preimage of the above semantic patch, it is reasonable to expect that V may be initialized to NULL or may be holding a pointer to a piece of allocated memory when the control reaches here, because otherwise, V will be either need to be freed (when E was not NULL, in which case we assigned the result of xstrdup() to it) or V has garbage that cannot be freed later. builtin/clone.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)