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

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

[andy]

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);
 	}
 	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];
 	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 */

[PATCH 2/2] gcc8.1: fix strcat warning

[andy]

../ui-ssdiff.c: In function ?replace_tabs?:
../ui-ssdiff.c:142:4: warning: ?strncat? output truncated copying between 1 and 8 bytes from a string of length 8 [-Wstringop-truncation]
    strncat(result, spaces, 8 - (strlen(result) % 8));
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Actually the strncat that was there before intends that its
stock of spaces gets truncated, and it's not a problem.

However gcc8.1 is also right, normally truncation is undesirable.

Make the code do the padding explicitly.

Signed-off-by: Andy Green <andy at warmcat.com>
---
 ui-ssdiff.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/ui-ssdiff.c b/ui-ssdiff.c
index 7f261ed..e520b95 100644
--- a/ui-ssdiff.c
+++ b/ui-ssdiff.c
@@ -118,7 +118,6 @@ static char *replace_tabs(char *line)
 	int n_tabs = 0;
 	int i;
 	char *result;
-	char *spaces = "        ";
 
 	if (linelen == 0) {
 		result = xmalloc(1);
@@ -138,8 +137,17 @@ static char *replace_tabs(char *line)
 			strcat(result, prev_buf);
 			break;
 		} else {
+			char *p;
+			int n;
+
 			strncat(result, prev_buf, cur_buf - prev_buf);
-			strncat(result, spaces, 8 - (strlen(result) % 8));
+
+			n = strlen(result);
+			p = result + n;
+			n = 8 - (n % 8);
+			while (n--)
+				*p++ = ' ';
+			*p = '\0';
 		}
 		prev_buf = cur_buf + 1;
 	}

[PATCH 2/2] gcc8.1: fix strcat warning

[john]

On Wed, Jun 13, 2018 at 07:34:07AM +0800, Andy Green wrote:
> ../ui-ssdiff.c: In function ?replace_tabs?:
> ../ui-ssdiff.c:142:4: warning: ?strncat? output truncated copying between 1 and 8 bytes from a string of length 8 [-Wstringop-truncation]
>     strncat(result, spaces, 8 - (strlen(result) % 8));
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Actually the strncat that was there before intends that its
> stock of spaces gets truncated, and it's not a problem.
> 
> However gcc8.1 is also right, normally truncation is undesirable.
> 
> Make the code do the padding explicitly.
> 
> Signed-off-by: Andy Green <andy at warmcat.com>

Reviewed-by: John Keeping <john at keeping.me.uk>

> ---
>  ui-ssdiff.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/ui-ssdiff.c b/ui-ssdiff.c
> index 7f261ed..e520b95 100644
> --- a/ui-ssdiff.c
> +++ b/ui-ssdiff.c
> @@ -118,7 +118,6 @@ static char *replace_tabs(char *line)
>  	int n_tabs = 0;
>  	int i;
>  	char *result;
> -	char *spaces = "        ";
>  
>  	if (linelen == 0) {
>  		result = xmalloc(1);
> @@ -138,8 +137,17 @@ static char *replace_tabs(char *line)
>  			strcat(result, prev_buf);
>  			break;
>  		} else {
> +			char *p;
> +			int n;
> +
>  			strncat(result, prev_buf, cur_buf - prev_buf);
> -			strncat(result, spaces, 8 - (strlen(result) % 8));
> +
> +			n = strlen(result);
> +			p = result + n;
> +			n = 8 - (n % 8);
> +			while (n--)
> +				*p++ = ' ';
> +			*p = '\0';
>  		}
>  		prev_buf = cur_buf + 1;
>  	}
> 

[PATCH 2/2] gcc8.1: fix strcat warning

[list]

John Keeping <john at keeping.me.uk> on Sat, 2018/06/16 14:11:
> On Wed, Jun 13, 2018 at 07:34:07AM +0800, Andy Green wrote:
> > ../ui-ssdiff.c: In function ?replace_tabs?:
> > ../ui-ssdiff.c:142:4: warning: ?strncat? output truncated copying between
> > 1 and 8 bytes from a string of length 8 [-Wstringop-truncation]
> > strncat(result, spaces, 8 - (strlen(result) % 8));
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Actually the strncat that was there before intends that its
> > stock of spaces gets truncated, and it's not a problem.
> > 
> > However gcc8.1 is also right, normally truncation is undesirable.
> > 
> > Make the code do the padding explicitly.
> > 
> > Signed-off-by: Andy Green <andy at warmcat.com>  
> 
> Reviewed-by: John Keeping <john at keeping.me.uk>

Agreed, except the typo in commit message. This is about strncat, not strcat.
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20180616/ba8141ee/attachment.asc>

[PATCH 2/2] gcc8.1: fix strcat warning

[andy]

On 06/17/2018 05:52 AM, Christian Hesse wrote:
> John Keeping <john at keeping.me.uk> on Sat, 2018/06/16 14:11:
>> On Wed, Jun 13, 2018 at 07:34:07AM +0800, Andy Green wrote:
>>> ../ui-ssdiff.c: In function ?replace_tabs?:
>>> ../ui-ssdiff.c:142:4: warning: ?strncat? output truncated copying between
>>> 1 and 8 bytes from a string of length 8 [-Wstringop-truncation]
>>> strncat(result, spaces, 8 - (strlen(result) % 8));
>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Actually the strncat that was there before intends that its
>>> stock of spaces gets truncated, and it's not a problem.
>>>
>>> However gcc8.1 is also right, normally truncation is undesirable.
>>>
>>> Make the code do the padding explicitly.
>>>
>>> Signed-off-by: Andy Green <andy at warmcat.com>
>>
>> Reviewed-by: John Keeping <john at keeping.me.uk>
> 
> Agreed, except the typo in commit message. This is about strncat, not strcat.
> 

OK... I fixed that and dropped the first patch.

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

[john]

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

[PATCH 1/2] shared: allocate return value from expand_macros()

[john]

In preparation for switching the implementation of expand_macros away
from a static buffer, return an allocated buffer and update all callers
to release it.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cgit.c      | 33 +++++++++++++++++++++++----------
 shared.c    |  5 ++---
 ui-shared.c |  9 ++++++---
 3 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/cgit.c b/cgit.c
index bd9cb3f..ca1bc15 100644
--- a/cgit.c
+++ b/cgit.c
@@ -190,7 +190,7 @@ static void config_cb(const char *name, const char *value)
 	else if (!strcmp(name, "cache-size"))
 		ctx.cfg.cache_size = atoi(value);
 	else if (!strcmp(name, "cache-root"))
-		ctx.cfg.cache_root = xstrdup(expand_macros(value));
+		ctx.cfg.cache_root = expand_macros(value);
 	else if (!strcmp(name, "cache-root-ttl"))
 		ctx.cfg.cache_root_ttl = atoi(value);
 	else if (!strcmp(name, "cache-repo-ttl"))
@@ -232,16 +232,20 @@ static void config_cb(const char *name, const char *value)
 	else if (!strcmp(name, "max-commit-count"))
 		ctx.cfg.max_commit_count = atoi(value);
 	else if (!strcmp(name, "project-list"))
-		ctx.cfg.project_list = xstrdup(expand_macros(value));
-	else if (!strcmp(name, "scan-path"))
+		ctx.cfg.project_list = expand_macros(value);
+	else if (!strcmp(name, "scan-path")) {
+		char *expanded = expand_macros(value);
+
 		if (!ctx.cfg.nocache && ctx.cfg.cache_size)
-			process_cached_repolist(expand_macros(value));
+			process_cached_repolist(expanded);
 		else if (ctx.cfg.project_list)
-			scan_projects(expand_macros(value),
+			scan_projects(expanded,
 				      ctx.cfg.project_list, repo_config);
 		else
-			scan_tree(expand_macros(value), repo_config);
-	else if (!strcmp(name, "scan-hidden-path"))
+			scan_tree(expanded, repo_config);
+
+		free(expanded);
+	} else if (!strcmp(name, "scan-hidden-path"))
 		ctx.cfg.scan_hidden_path = atoi(value);
 	else if (!strcmp(name, "section-from-path"))
 		ctx.cfg.section_from_path = atoi(value);
@@ -287,8 +291,12 @@ static void config_cb(const char *name, const char *value)
 			ctx.cfg.branch_sort = 0;
 	} else if (skip_prefix(name, "mimetype.", &arg))
 		add_mimetype(arg, value);
-	else if (!strcmp(name, "include"))
-		parse_configfile(expand_macros(value), config_cb);
+	else if (!strcmp(name, "include")) {
+		char *expanded = expand_macros(value);
+
+		parse_configfile(expanded, config_cb);
+		free(expanded);
+	}
 }
 
 static void querystring_cb(const char *name, const char *value)
@@ -1041,6 +1049,7 @@ static int calc_ttl(void)
 int cmd_main(int argc, const char **argv)
 {
 	const char *path;
+	char *expanded;
 	int err, ttl;
 
 	cgit_init_filters();
@@ -1052,7 +1061,11 @@ int cmd_main(int argc, const char **argv)
 	cgit_repolist.repos = NULL;
 
 	cgit_parse_args(argc, argv);
-	parse_configfile(expand_macros(ctx.env.cgit_config), config_cb);
+
+	expanded = expand_macros(ctx.env.cgit_config);
+	parse_configfile(expanded, config_cb);
+	free(expanded);
+
 	ctx.repo = NULL;
 	http_parse_querystring(ctx.qry.raw, querystring_cb);
 
diff --git a/shared.c b/shared.c
index 21ac8f4..32d0f46 100644
--- a/shared.c
+++ b/shared.c
@@ -489,8 +489,7 @@ static char *expand_macro(char *name, int maxlength)
 
 /* Replace all tokens prefixed by '$' in the specified text with the
  * value of the named environment variable.
- * NB: the return value is a static buffer, i.e. it must be strdup'd
- * by the caller.
+ * NB: the return value is allocated, it must be freed by the caller.
  */
 char *expand_macros(const char *txt)
 {
@@ -530,7 +529,7 @@ char *expand_macros(const char *txt)
 		p = expand_macro(start, len);
 		*p = '\0';
 	}
-	return result;
+	return xstrdup(result);
 }
 
 char *get_mimetype_for_filename(const char *filename)
diff --git a/ui-shared.c b/ui-shared.c
index 9d8f66b..c48f422 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -838,9 +838,12 @@ static void add_clone_urls(void (*fn)(const char *), char *txt, char *suffix)
 
 void cgit_add_clone_urls(void (*fn)(const char *))
 {
-	if (ctx.repo->clone_url)
-		add_clone_urls(fn, expand_macros(ctx.repo->clone_url), NULL);
-	else if (ctx.cfg.clone_prefix)
+	if (ctx.repo->clone_url) {
+		char *expanded = expand_macros(ctx.repo->clone_url);
+
+		add_clone_urls(fn, expanded, NULL);
+		free(expanded);
+	} else if (ctx.cfg.clone_prefix)
 		add_clone_urls(fn, ctx.cfg.clone_prefix, ctx.repo->url);
 }
 
-- 
2.17.1

[PATCH 2/2] shared: use strbuf for expanding macros

[john]

Avoid a fixed size buffer by using struct strbuf to build the expanded
value.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 shared.c | 73 +++++++++++++++++++++-----------------------------------
 1 file changed, 27 insertions(+), 46 deletions(-)

diff --git a/shared.c b/shared.c
index 32d0f46..6ce3990 100644
--- a/shared.c
+++ b/shared.c
@@ -467,69 +467,50 @@ static int is_token_char(char c)
 	return isalnum(c) || c == '_';
 }
 
-/* Replace name with getenv(name), return pointer to zero-terminating char
+/* Replace name with getenv(name).
  */
-static char *expand_macro(char *name, int maxlength)
+static void expand_macro(struct strbuf *buf, ssize_t start)
 {
-	char *value;
-	int len;
+	const char *name = buf->buf + start;
+	const char *value = getenv(name);
 
-	len = 0;
-	value = getenv(name);
-	if (value) {
-		len = strlen(value);
-		if (len > maxlength)
-			len = maxlength;
-		strncpy(name, value, len);
-	}
-	return name + len;
+	/* Truncate output to remove variable name. */
+	strbuf_setlen(buf, start);
+
+	if (value)
+		strbuf_addstr(buf, value);
 }
 
-#define EXPBUFSIZE (1024 * 8)
-
 /* Replace all tokens prefixed by '$' in the specified text with the
  * value of the named environment variable.
  * NB: the return value is allocated, it must be freed by the caller.
  */
 char *expand_macros(const char *txt)
 {
-	static char result[EXPBUFSIZE];
-	char *p, *start;
-	int len;
+	struct strbuf result = STRBUF_INIT;
+	ssize_t start = -1;
 
-	p = result;
-	start = NULL;
-	while (p < result + EXPBUFSIZE - 1 && txt && *txt) {
-		*p = *txt;
-		if (start) {
+	while (txt && *txt) {
+		if (start >= 0) {
 			if (!is_token_char(*txt)) {
-				if (p - start > 0) {
-					*p = '\0';
-					len = result + EXPBUFSIZE - start - 1;
-					p = expand_macro(start, len) - 1;
-				}
-				start = NULL;
-				txt--;
+				if (result.len - start >= 0)
+					expand_macro(&result, start);
+				start = -1;
 			}
-			p++;
-			txt++;
-			continue;
 		}
-		if (*txt == '$') {
-			start = p;
-			txt++;
-			continue;
-		}
-		p++;
+
+		if (*txt == '$')
+			start = result.len;
+		else
+			strbuf_addch(&result, *txt);
+
 		txt++;
 	}
-	*p = '\0';
-	if (start && p - start > 0) {
-		len = result + EXPBUFSIZE - start - 1;
-		p = expand_macro(start, len);
-		*p = '\0';
-	}
-	return xstrdup(result);
+
+	if (start >= 0 && result.len - start > 0)
+		expand_macro(&result, start);
+
+	return strbuf_detach(&result, NULL);
 }
 
 char *get_mimetype_for_filename(const char *filename)
-- 
2.17.1

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

[andy]

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

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

[john]

On Sat, Jun 16, 2018 at 09:12:08PM +0800, Andy Green wrote:
> 
> 
> 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. 

Yes, I'm getting it confused with strncat.  And in this case we do
ensure that the output is null terminated separately.

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

[andy]

These patches fix warnings blown by gcc8.1 about strncpy usage.

The first patch is the first part of v1 that got a confusing
reception the first time.  But I think in the end no objection
to it.

In the second version, I follow John's suggestion and go further
refactoring the affected functions.  Obviously the warning can
be fixed without all this as I did the first time.

Tested on libwebsockets.org.

Tree with these patches on latest for-jason pieces

https://warmcat.com/git/cgit/

---

Andy Green (2):
      gcc8.1: fix strncpy bounds warnings
      cgit_repobasename: convert to allocated result


 cgit.h        |    2 --
 shared.c      |    2 +-
 ui-shared.c   |   53 +++++++++++++++++++++++++++++------------------------
 ui-shared.h   |    3 ++-
 ui-snapshot.c |   21 ++++++++++++++++++---
 5 files changed, 50 insertions(+), 31 deletions(-)

--
Signature

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

[andy]

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.

Signed-off-by: Andy Green <andy at warmcat.com>
---
 shared.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/shared.c b/shared.c
index d7c7636..6cda79e 100644
--- a/shared.c
+++ b/shared.c
@@ -483,7 +483,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);
 	}
 	return name + len;
 }

[PATCH v2 2/2] cgit_repobasename: convert to allocated result

[andy]

cgit_repobasename has one user also in ui-shared.c.  Make it static
and remove the declaration from cgit.h.

Instead of the gnarly return pointer to now deallocated stack,
compute the valid part of the string using the incoming pointer,
then just allocate the right amount and copy it in.  Drop the
const on the return type now it's allocated.

Cover the fact the input may be garbage by returning NULL if so.

Comment the function at the start that the result may be NULL or
must be freed now.

Convert the only user, cgit_snapshot_prefix(), to the same return
convention and also comment him at the start that the result may
be NULL or must be freed.  Also change the return type to char *.

Convert his only users, get_ref_from_filename() and
cgit_print_snapshot()in ui-snapshot.c, to deal with the new
result convention.  cgit_print_snapshot() already did an
xstrdup() on him anyway, just remove it and check for NULL.

The reason triggering all this was

../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));
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

+ comment from John Keeping.

Signed-off-by: Andy Green <andy at warmcat.com>
---
 cgit.h        |    2 --
 ui-shared.c   |   53 +++++++++++++++++++++++++++++------------------------
 ui-shared.h   |    3 ++-
 ui-snapshot.c |   21 ++++++++++++++++++---
 4 files changed, 49 insertions(+), 30 deletions(-)

diff --git a/cgit.h b/cgit.h
index 6feca68..6e6750c 100644
--- a/cgit.h
+++ b/cgit.h
@@ -369,8 +369,6 @@ extern struct commitinfo *cgit_parse_commit(struct commit *commit);
 extern struct taginfo *cgit_parse_tag(struct tag *tag);
 extern void cgit_parse_url(const char *url);
 
-extern const char *cgit_repobasename(const char *reponame);
-
 extern int cgit_parse_snapshots_mask(const char *str);
 extern const struct object_id *cgit_snapshot_get_sig(const char *ref,
 						     const struct cgit_snapshot_format *f);
diff --git a/ui-shared.c b/ui-shared.c
index ba88106..54d428f 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -127,35 +127,40 @@ char *cgit_pageurl(const char *reponame, const char *pagename,
 	return cgit_fileurl(reponame, pagename, NULL, query);
 }
 
-const char *cgit_repobasename(const char *reponame)
+/* result is NULL or must be freed */
+static char *cgit_repobasename(const char *reponame)
 {
-	/* I assume we don't need to store more than one repo basename */
-	static char rvbuf[1024];
-	int p;
-	const char *rv;
-	strncpy(rvbuf, reponame, sizeof(rvbuf));
-	if (rvbuf[sizeof(rvbuf)-1])
-		die("cgit_repobasename: truncated repository name '%s'", reponame);
-	p = strlen(rvbuf)-1;
-	/* strip trailing slashes */
-	while (p && rvbuf[p] == '/') rvbuf[p--] = 0;
-	/* strip trailing .git */
-	if (p >= 3 && starts_with(&rvbuf[p-3], ".git")) {
-		p -= 3; rvbuf[p--] = 0;
-	}
-	/* strip more trailing slashes if any */
-	while ( p && rvbuf[p] == '/') rvbuf[p--] = 0;
-	/* find last slash in the remaining string */
-	rv = strrchr(rvbuf,'/');
-	if (rv)
-		return ++rv;
-	return rvbuf;
+	int last = strlen(reponame) - 1, n;
+	char *rv;
+
+	if (last < 1)
+		return NULL;
+
+	while (last && reponame[last] == '/')
+		last--;
+
+	if (last >= 3 && !strncmp(&reponame[last - 3], ".git", 3))
+		last -= 3;
+
+	while (last && reponame[last] == '/')
+		last--;
+
+	n = last;
+	while (n && reponame[n] != '/')
+		n--;
+
+	rv = xmalloc(last - n + 2);
+	strncpy(rv, &reponame[n], last - n + 1);
+	rv[last - n + 1] = '\0';
+
+	return rv;
 }
 
-const char *cgit_snapshot_prefix(const struct cgit_repo *repo)
+/* result is NULL or must be freed */
+char *cgit_snapshot_prefix(const struct cgit_repo *repo)
 {
 	if (repo->snapshot_prefix)
-		return repo->snapshot_prefix;
+		return xstrdup(repo->snapshot_prefix);
 
 	return cgit_repobasename(repo->url);
 }
diff --git a/ui-shared.h b/ui-shared.h
index 4d5978b..49c11fc 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -78,7 +78,8 @@ extern void cgit_compose_snapshot_prefix(struct strbuf *filename,
 					 const char *base, const char *ref);
 extern void cgit_print_snapshot_links(const struct cgit_repo *repo,
 				      const char *ref, const char *separator);
-extern const char *cgit_snapshot_prefix(const struct cgit_repo *repo);
+/* result is NULL or must be freed */
+extern char *cgit_snapshot_prefix(const struct cgit_repo *repo);
 extern void cgit_add_hidden_formfields(int incl_head, int incl_search,
 				       const char *page);
 
diff --git a/ui-snapshot.c b/ui-snapshot.c
index 4f7c21a..c7c945d 100644
--- a/ui-snapshot.c
+++ b/ui-snapshot.c
@@ -206,7 +206,7 @@ static const char *get_ref_from_filename(const struct cgit_repo *repo,
 					 const char *filename,
 					 const struct cgit_snapshot_format *format)
 {
-	const char *reponame;
+	char *reponame = NULL;
 	struct object_id oid;
 	struct strbuf snapshot = STRBUF_INIT;
 	int result = 1;
@@ -215,9 +215,12 @@ static const char *get_ref_from_filename(const struct cgit_repo *repo,
 	strbuf_setlen(&snapshot, snapshot.len - strlen(format->suffix));
 
 	if (get_oid(snapshot.buf, &oid) == 0)
-		goto out;
+		goto out1;
 
 	reponame = cgit_snapshot_prefix(repo);
+	if (!reponame)
+		goto out1;
+
 	if (starts_with(snapshot.buf, reponame)) {
 		const char *new_start = snapshot.buf;
 		new_start += strlen(reponame);
@@ -241,6 +244,8 @@ static const char *get_ref_from_filename(const struct cgit_repo *repo,
 	strbuf_release(&snapshot);
 
 out:
+	free(reponame);
+out1:
 	return result ? strbuf_detach(&snapshot, NULL) : NULL;
 }
 
@@ -288,7 +293,15 @@ void cgit_print_snapshot(const char *head, const char *hex,
 		hex = head;
 
 	if (!prefix)
-		prefix = xstrdup(cgit_snapshot_prefix(ctx.repo));
+		prefix = cgit_snapshot_prefix(ctx.repo);
+
+	if (!prefix) {
+		cgit_print_error_page(500, "Internal Server Error",
+				"Bad repo name");
+
+		goto out1;
+	}
+
 
 	if (sig_filename)
 		write_sig(f, hex, filename, sig_filename);
@@ -296,5 +309,7 @@ void cgit_print_snapshot(const char *head, const char *hex,
 		make_snapshot(f, hex, prefix, filename);
 
 	free(prefix);
+
+out1:
 	free(adj_filename);
 }