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

[John Keeping]

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.

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.

>  	}
>  	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?

>  	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