Re: [PATCH 20/20] grep API: plug memory leaks by freeing "header_list"

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 28.12.22 um 19:00 schrieb Ævar Arnfjörð Bjarmason:
> When the "header_list" struct member was added in [1] it wasn't made
> to free the list using loop added for the adjacent "pattern_list"
> member, see [2] for when we started freeing it.
>
> Let's start doing o by splitting up the loop in free_grep_patterns()
> into a utility function. This makes e.g. this command leak-free when
> run on git.git:
>
> 	./git -P log -1 --color=always --author=A origin/master
>
> 1. 80235ba79ef ("log --author=me --grep=it" should find intersection,
>    not union, 2010-01-17)
> 2. b48fb5b6a95 (grep: free expressions and patterns when done.,
>    2006-09-27)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
>  grep.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index ca75514f8f6..c908535e0d8 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -769,11 +769,11 @@ static void free_pattern_expr(struct grep_expr *x)
>  	free(x);
>  }
>
> -void free_grep_patterns(struct grep_opt *opt)
> +static void free_grep_pat(struct grep_pat *pattern)
>  {
>  	struct grep_pat *p, *n;
>
> -	for (p = opt->pattern_list; p; p = n) {
> +	for (p = pattern; p; p = n) {
>  		n = p->next;
>  		switch (p->token) {
>  		case GREP_PATTERN: /* atom */
> @@ -790,6 +790,12 @@ void free_grep_patterns(struct grep_opt *opt)
>  		}
>  		free(p);
>  	}
> +}
> +
> +void free_grep_patterns(struct grep_opt *opt)
> +{
> +	free_grep_pat(opt->pattern_list);
> +	free_grep_pat(opt->header_list);

OK.  Patch 19 didn't help me understand this one, though, contrary to
the foreshadowing in its commit message.  It seems quite unrelated, in
fact.  It's a stylistic improvement, to be sure, but I expected more
structural relevance.

Factoring out free_grep_pat() first and then using it for header_list
in a separate patch would have made a difference by making the actual
leak fix being a one-liner.

Not necessarily worth a re-roll, though -- the patch is understandable
as-is, but it probably took me a moment longer with refactoring and
fix blended together like this.

>
>  	if (opt->pattern_expression)
>  		free_pattern_expr(opt->pattern_expression);




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux