Re: [PATCH v4 10/27] stash: always have the owner of "stash_info" free it

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Apr 01 2022, Phillip Wood wrote:

> Hi Ævar
>
> On 31/03/2022 02:11, Ævar Arnfjörð Bjarmason wrote:
>> Change the initialization of the "revision" member of "struct
>> stash_info" to be initialized vi a macro, and more importantly that
>> that initializing function be tasked to free it, usually via a "goto
>> cleanup" pattern.
>> Despite the "revision" name (and the topic of the series containing
>> this commit) the "stash info" has nothing to do with the "struct
>> rev_info". I'm making this change because in the subsequent commit
>> when we do want to free the "struct rev_info" via a "goto cleanup"
>> pattern we'd otherwise free() uninitialized memory in some cases, as
>> we only strbuf_init() the string in get_stash_info().
>> So while it's the smallest possible change, let's convert all users
>> of
>> this pattern in the file while we're at it.
>> A good follow-up to this change would be to change all the "ret =
>> -1;
>> goto done;" in this file to instead use a "goto cleanup", and
>> initialize "int ret = -1" at the start of the relevant functions. That
>> would allow us to drop a lot of needless brace verbosity on two-line
>> "if" statements, but let's leave that alone for now.
>
> You seem to have made this change here.
>
>>[...]
>> @@ -861,10 +863,8 @@ static int show_stash(int argc, const char **argv, const char *prefix)
>>   			strvec_push(&revision_args, argv[i]);
>>   	}
>>   -	ret = get_stash_info(&info, stash_args.nr, stash_args.v);
>> -	strvec_clear(&stash_args);
>> -	if (ret)
>> -		return -1;
>> +	if (get_stash_info(&info, stash_args.nr, stash_args.v))
>> +		goto cleanup;
>>     	/*
>>   	 * The config settings are applied only if there are not passed
>> @@ -878,8 +878,8 @@ static int show_stash(int argc, const char **argv, const char *prefix)
>>   			rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
>>     		if (!show_stat && !show_patch) {
>> -			free_stash_info(&info);
>> -			return 0;
>> +			ret = 0;
>> +			goto cleanup;
>>   		}
>>   	}
>>   @@ -912,8 +912,11 @@ static int show_stash(int argc, const char
>> **argv, const char *prefix)
>>   	}
>>   	log_tree_diff_flush(&rev);
>>   +	ret = diff_result_code(&rev.diffopt, 0);;
>> +cleanup:
>> +	strvec_clear(&stash_args);
>
> This seems to be fixing a leak that's not mentioned in the commit message.

This is just moving the exsting strvec_clear() shown above to the
"cleanup" branch, as we change return to "goto cleanup".

>>   	free_stash_info(&info);
>> -	return diff_result_code(&rev.diffopt, 0);
>> +	return ret;
>>   }
>>[...]
>> @@ -1434,7 +1438,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
>>   			 int keep_index, int patch_mode, int include_untracked, int only_staged)
>>   {
>>   	int ret = 0;
>> -	struct stash_info info;
>> +	struct stash_info info = STASH_INFO_INIT;
>
> There doesn't seem to be a call to free_stash_info() in this function.

Hrm, I think you're right about that (but it was an existing leak). Will
fix.