Re: [PATCH v2 2/4] help.c::exclude_cmds: realloc() before copy, plug a leak

[Junio C Hamano <gitster@pobox.com>]

Tay Ray Chuan <rctay89@gmail.com> writes:

> Copying with structural assignment may not take into account that the
> LHS struct has sufficient memory, especially since the cmdname->name
> member is nonfixed in size. Be unambiguous about it by realloc()'ing it
> to be of sufficient size.

If the original code were

	*(cmd->names[cj++]) = *(cmd->names[ci++]);

there may be a structural assignment involved, but

	cmds->names[dst] = cmd->names[src]

just copies the pointer that points at a struct cmdname that records
the src command name to another slot of cmds->names[] array, whose
elements are pointers, no?  What's there to realloc?

> @@ -58,20 +69,25 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)
>  {
>  	int ci, cj, ei;
>  	int cmp;
> +	int last_cj;
>  
>  	ci = cj = ei = 0;
>  	while (ci < cmds->cnt && ei < excludes->cnt) {
>  		cmp = strcmp(cmds->names[ci]->name, excludes->names[ei]->name);
>  		if (cmp < 0)
> -			cmds->names[cj++] = cmds->names[ci++];
> +			copy_cmdname(&cmds->names[cj++], cmds->names[ci++]);
>  		else if (cmp == 0)
>  			ci++, ei++;
>  		else if (cmp > 0)
>  			ei++;
>  	}
> +	last_cj = cj;
>  
>  	while (ci < cmds->cnt)
> -		cmds->names[cj++] = cmds->names[ci++];
> +		copy_cmdname(&cmds->names[cj++], cmds->names[ci++]);
> +
> +	while (last_cj < cmds->cnt)
> +		free(cmds->names[last_cj++]);
>  
>  	cmds->cnt = cj;
>  }

We shifted cmds->names[] array to skip entries that appear in
excludes.  If original cmds->names[] had "0", "1", "2", "3", ...
and excludes had "0" and "1", cmds->names[] would contain "2", "3",
"2", "3"; the first two are copied over "0" and "1" that are
excluded, and the latter two are leftover beyond last_cj.  The
corresponding names share the same structure (cmds->names[] is an
array of pointers).  Doesn't freeing cmds->names[2] free the
structure that is used by both cmds->names[0] and cmds->names[2]?

Confused.

The function drops cmds->names[ci] when it appears in excludes, so
you may want to free it when it happens, though.

 help.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/help.c b/help.c
index 6991492..cae389b 100644
--- a/help.c
+++ b/help.c
@@ -64,9 +64,10 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)
 		cmp = strcmp(cmds->names[ci]->name, excludes->names[ei]->name);
 		if (cmp < 0)
 			cmds->names[cj++] = cmds->names[ci++];
-		else if (cmp == 0)
-			ci++, ei++;
-		else if (cmp > 0)
+		else if (cmp == 0) {
+			ei++;
+			free(cmd->names[ci++]);
+		} else if (cmp > 0)
 			ei++;
 	}