Re: [PATCH v3] setup: don't die if realpath(3) fails on getcwd(3)

From: Kevin Locke <kevin@kevinlocke.name>
Date: May 24, 2022, 18:00 UTC
Message-ID: <Yo0dS550l2NCmx9O@kevinlocke.name>
In-Reply-To: <dafc8178-d213-ffd0-9e2a-32ac88307b39@github.com>

On Tue, 2022-05-24 at 13:41 -0400, Derrick Stolee wrote:

Show 22 quoted lines

> On 5/24/2022 10:51 AM, Kevin Locke wrote:> -	/* Normalize the directory */
>> -	strbuf_realpath(&tmp, tmp_original_cwd, 1);
>> -	free((char*)tmp_original_cwd);
>> -	tmp_original_cwd = NULL;
>> -	startup_info->original_cwd = strbuf_detach(&tmp, NULL);
>> +	/* Try to normalize the directory. */
>> +	if (strbuf_realpath(&tmp, tmp_original_cwd, 0)) {
>> +		free((char*)tmp_original_cwd);
>> +		tmp_original_cwd = NULL;
>> +		startup_info->original_cwd = strbuf_detach(&tmp, NULL);
>> +	} else {
>> +		trace2_data_string("setup", the_repository,
>> +				   "realpath-path", tmp_original_cwd);
>> +		trace2_data_string("setup", the_repository,
>> +				   "realpath-failure", strerror(errno));
>> +		tmp_original_cwd = NULL;
> 
> I didn't catch this in v2, but should this line instead be
> 
> 		startup_info->original_cwd = NULL;
> 
> ? Especially based on this comment:

No worries. It's a good question. Setting startup_info->original_cwd to NULL is handled by the no_prevention_needed label. But I just realized it's not actually required in this case, since original_cwd is NULL when setup_original_cwd() is called. I should probably return rather than jumping to no_prevention_needed from the else block to avoid the unnecessary free(NULL) and assignment.

Your comment made me realize that v2 and later neglect to free tmp_original_cwd when strbuf_realpath() fails. How embarrassing.

I'll send an update to fix both those issues shortly.

Thanks again, Kevin