[PATCH 1/2] gcc8.1: fix strncpy bounds warnings

[Andy Green]

On June 16, 2018 9:04:48 PM GMT+08:00, John Keeping <john at keeping.me.uk> wrote:
>On Wed, Jun 13, 2018 at 07:33:59AM +0800, Andy Green wrote:
>> These warnings are coming on default Fedora 28 build and probably
>others using gcc 8.1
>> 
>> ../shared.c: In function ‘expand_macro’:
>> ../shared.c:483:3: warning: ‘strncpy’ specified bound depends on the
>length of the source argument [-Wstringop-overflow=]
>>    strncpy(name, value, len);
>>    ^~~~~~~~~~~~~~~~~~~~~~~~~
>> ../shared.c:480:9: note: length computed here
>>    len = strlen(value);
>>          ^~~~~~~~~~~~~
>> 
>> strncpy with a computed length via strlen is usually
>> not the right thing.
>> 
>> ../ui-shared.c: In function ‘cgit_repobasename’:
>> ../ui-shared.c:135:2: warning: ‘strncpy’ specified bound 1024 equals
>destination size [-Wstringop-truncation]
>>   strncpy(rvbuf, reponame, sizeof(rvbuf));
>>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 
>> add one char of padding and adjust so the code does the same.
>> 
>> Signed-off-by: Andy Green <andy at warmcat.com>
>> ---
>>  shared.c    |    2 +-
>>  ui-shared.c |    7 ++++---
>>  2 files changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/shared.c b/shared.c
>> index 21ac8f4..477db0a 100644
>> --- a/shared.c
>> +++ b/shared.c
>> @@ -480,7 +480,7 @@ static char *expand_macro(char *name, int
>maxlength)
>>  		len = strlen(value);
>>  		if (len > maxlength)
>>  			len = maxlength;
>> -		strncpy(name, value, len);
>> +		memcpy(name, value, len);
>
>This is a change in behaviour because strncpy is guaranteed to null
>terminate the output (even writing one beyond len if necessary) whereas
>memcpy does not.

Eh... are you sure about that?  It's not my understanding, and --->

https://linux.die.net/man/3/strncpy

The strncpy() function is similar, except that at most n bytes of src are copied. Warning: If there is no null byte among the first n bytes of src, the string placed in dest will not be null-terminated. 

>But I think we can improve this by removing the fixed buffer completely
>and using struct strbuf to build the value (then return the allocated
>buffer and rely on the caller to free it).  I'll follow up with a
>couple
>of patches that make this change.

I just did the minimal change to resolve the warning.  If that's solution's better for larger reasons by all means.

>>  	}
>>  	return name + len;
>>  }
>> diff --git a/ui-shared.c b/ui-shared.c
>> index 9d8f66b..6656bd5 100644
>> --- a/ui-shared.c
>> +++ b/ui-shared.c
>> @@ -129,11 +129,12 @@ char *cgit_pageurl(const char *reponame, const
>char *pagename,
>>  const char *cgit_repobasename(const char *reponame)
>>  {
>>  	/* I assume we don't need to store more than one repo basename */
>> -	static char rvbuf[1024];
>> +	static char rvbuf[1025];
>
>This is just an arbitrary size, so I think it can stay at 1024.
>
>However, again, I think there's a better way to do this!  We don't need
>to copy the full reponame and modify it, why not figure out the start
>and end of the basename in reponame and then strncpy the relevant
>substring into rvbuf?

Same as above, if you prefer a larger refactor rather than just fix the warning, no worries.

-Andy

>>  	int p;
>>  	const char *rv;
>> -	strncpy(rvbuf, reponame, sizeof(rvbuf));
>> -	if (rvbuf[sizeof(rvbuf)-1])
>> +
>> +	strncpy(rvbuf, reponame, sizeof(rvbuf) - 1);
>> +	if (rvbuf[sizeof(rvbuf) - 2])
>>  		die("cgit_repobasename: truncated repository name '%s'",
>reponame);
>>  	p = strlen(rvbuf)-1;
>>  	/* strip trailing slashes */
>> 
>> _______________________________________________
>> CGit mailing list
>> CGit at lists.zx2c4.com
>> https://lists.zx2c4.com/mailman/listinfo/cgit