Re: [ammarfaizi2-block:rostedt/linux-trace/ftrace/core 12/13] include/linux/fortify-string.h:47:30: error: '__builtin_strncat' output truncated before terminating nul copying as many bytes from a string as its length

[Steven Rostedt]

On Mon, 14 Feb 2022 06:42:59 +0800
kernel test robot <lkp@xxxxxxxxx> wrote:

> All errors (new ones prefixed by >>):
> 
>    In file included from include/linux/string.h:253,
>                     from arch/x86/include/asm/page_32.h:22,
>                     from arch/x86/include/asm/page.h:14,
>                     from arch/x86/include/asm/processor.h:19,
>                     from arch/x86/include/asm/timex.h:5,
>                     from include/linux/timex.h:65,
>                     from include/linux/time32.h:13,
>                     from include/linux/time.h:60,
>                     from include/linux/stat.h:19,
>                     from include/linux/module.h:13,
>                     from kernel/trace/trace_events_hist.c:8:
>    In function 'strncat',
>        inlined from 'last_cmd_set' at
> kernel/trace/trace_events_hist.c:759:2:
> >> include/linux/fortify-string.h:47:30: error: '__builtin_strncat'
> >> output truncated before terminating nul copying as many bytes from a
> >> string as its length [-Werror=stringop-truncation]  
>       47 | #define __underlying_strncat __builtin_strncat
>          |                              ^
>    include/linux/fortify-string.h:191:10: note: in expansion of macro
> '__underlying_strncat' 191 |   return __underlying_strncat(p, q, count);
>          |          ^~~~~~~~~~~~~~~~~~~~
>    kernel/trace/trace_events_hist.c: In function 'last_cmd_set':
>    include/linux/fortify-string.h:46:29: note: length computed here
>       46 | #define __underlying_strlen __builtin_strlen
>          |                             ^
>    include/linux/fortify-string.h:102:10: note: in expansion of macro
> '__underlying_strlen' 102 |   return __underlying_strlen(p);
>          |          ^~~~~~~~~~~~~~~~~~~
>    cc1: all warnings being treated as errors

I have no idea what the above riddle is saying. But looking at the code, I
can figure out a few issues with it.

> static void last_cmd_set(struct trace_event_file *file, char *str)
> {
> 	const char *system = NULL, *name = NULL;
> 	struct trace_event_call *call;
> 	int len = 0;
> 
> 	if (!str)
> 		return;
> 
> 	len += sizeof(HIST_PREFIX) + strlen(str) + 1;

Nit, I have no idea why len has += and is initialized to zero. Looks like
it could have just been:

	len = sizeof(HIST_PREFIX) + strlen(str) + 1;

> 	kfree(last_cmd);
> 	last_cmd = kzalloc(len, GFP_KERNEL);
> 	if (!last_cmd)
> 		return;
> 
> 	strcpy(last_cmd, HIST_PREFIX);
> 	strncat(last_cmd, str, len - sizeof(HIST_PREFIX));

OK, I think the issue here is that "len" contains the "+1" for the nul
byte. According to the man page for strncat(), it states that "dest" needs
to be at least strlen(dest)+n+1. And since len has + 1 in it already, it
can't have it in the strncat().

Perhaps we need to change this to:

	len -= sizeof(HIST_PREFIX) + 1;
	strncat(last_cmd, str, len);

-- Steve


> 
> 	if (file) {
> 		call = file->event_call;
> 		system = call->class->system;
> 		if (system) {
> 			name = trace_event_name(call);
> 			if (!name)
> 				system = NULL;
> 		}
> 	}
> 
> 	if (system)
> 		snprintf(last_cmd_loc, MAX_FILTER_STR_VAL, HIST_PREFIX "%s:%s", system, name); }