Re: [PATCH 1/1] gpg-interface: limit search for primary key fingerprint

From: Hans Jerry Illikainen <hji@dyntopia.com>
Date: Nov 21, 2019, 23:19 UTC
Message-ID: <87d0dkbyxb.hji@dyntopia.com>
In-Reply-To: <xmqqtv712145.fsf@gitster-ct.c.googlers.com>

On Mon, Nov 18 2019, Junio C Hamano wrote:

> I wonder if it is simpler to define it next to 'next'.  Yes, this
> new variable is used only within this block, but it also gets used
> only in conjunction with that other variable.

Done in v3 (sorry for the messed up versioning going from v0 to v2!).

Show 8 quoted lines

> A bigger question is, when this happens, what value do we want to
> leave in sigc->primary_key_fingerprint?  As we can see from the
> original code that makes sure the old value in the field will not
> leak by first free()ing, it seems that it is possible in this code
> that the field may not be NULL, but we just saw that on _our_
> signature verification system, the primary key is not available.
> Shouldn't we be nulling it out, after free()ing possibly leftover
> value in the field?

I investigated the code paths to `primary_key_fingerprint` and deduced that it's only ever touched when GPG_STATUS_FINGERPRINT is encountered and a primary fingerprint is extracted. However, v3 will NULL it even when no primary fingerprint is found.

Show 6 quoted lines

>> +							xmemdupz(line,
>> +								 next - line);
>> +					}
>
> Avoid such an unnatural line-wrapping that makes the result harder
> to read.

Sorry about that! I figured that some projects prefer to always trust in the code formatter; so I just left it be. Now I know that human decisions are allowed :)

Show 14 quoted lines

> A short helper
>
> 	static void replace_cstring(const char **field,
> 				    const char *line, const char *next)
> 	{
> 		free(*field);
> 		if (line && next)
> 			*field = xmemdupz(line, next - line);
> 		else
> 			*field = NULL;
> 	}
>
> may have quite a lot of uses in this function, not only for this
> field.

Implemented. I wasn't sure whether to do it in a separate commit or not, but #git-devel suggested that I do; so that's what I did.