Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value

From: Anders Waldenborg <anders@0x63.nu>
Date: Jan 29, 2019, 06:45 UTC
Message-ID: <87tvhsklpb.fsf@0x63.nu>
In-Reply-To: <xmqq8sz49zm1.fsf@gitster-ct.c.googlers.com>

Junio C Hamano writes:

Show 24 quoted lines

>> ...
>> +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,
>> +				      const char **end, int *val)
>> +{
>> +	char buf[8];
>> +	const char *strval;
>> +	size_t len;
>> +	int v;
>> +
>> +	if (!match_placeholder_arg_value(to_parse, candidate, end, &strval, &len))
>> +		return 0;
>> +
>> +	if (!strval) {
>> +		*val = 1;
>> +		return 1;
>> +	}
>> +
>> +	strlcpy(buf, strval, sizeof(buf));
>> +	if (len < sizeof(buf))
>> +		buf[len] = 0;
>
> Doesn't strlcpy() terminate buf[len] if len is short enough?
> Even if the strval is longer than buf[], strlcpy() would truncate
> and make sure buf[] is NUL terminated, no?

Yes, but no. strval is not NUL-terminated at len. E.g strval would point to "false,something=true". `buf[len] = 0` makes sure it becomes "false".

> Instead of using "char buf[8]", just using a strbuf and avoidng
> strlcpy() would make the code much better, I would think.

Yes, taking the heap allocation hit would most likely make the intent clearer.