Messages in this thread | | | From | Arnaldo Carvalho de Melo <> | Date | Fri, 31 May 2019 09:05:30 -0300 | Subject | Re: [PATCH] perf jvmti: Fix gcc string overflow warning |
| |
Em Fri, May 31, 2019 at 10:03:07AM +0200, Jiri Olsa escreveu: > We are getting fake gcc warning when we compile with gcc9 (9.1.1): > > CC jvmti/libjvmti.o > In file included from /usr/include/string.h:494, > from jvmti/libjvmti.c:5: > In function ‘strncpy’, > inlined from ‘copy_class_filename.constprop’ at jvmti/libjvmti.c:166:3: > /usr/include/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ specified bound depends on the length of the source argument [-Werror=stringop-overflow=] > 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > jvmti/libjvmti.c: In function ‘copy_class_filename.constprop’: > jvmti/libjvmti.c:165:26: note: length computed here > 165 | size_t file_name_len = strlen(file_name); > | ^~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > > First I wanted to disable the check, but now I think the code > could be more straight forward. There's no need to check the > source size, strncpy will do that. We just need to make sure > the string is correctly terminated. > > Cc: Ben Gainey <ben.gainey@arm.com> > Cc: Stephane Eranian <eranian@google.com> > Link: http://lkml.kernel.org/n/tip-sve3b63c550wr907e6ui6gx5@git.kernel.org > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > tools/perf/jvmti/libjvmti.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/perf/jvmti/libjvmti.c b/tools/perf/jvmti/libjvmti.c > index aea7b1fe85aa..00fa0b7f1ad9 100644 > --- a/tools/perf/jvmti/libjvmti.c > +++ b/tools/perf/jvmti/libjvmti.c > @@ -162,8 +162,8 @@ copy_class_filename(const char * class_sign, const char * file_name, char * resu > result[i] = '\0'; > } else { > /* fallback case */ > - size_t file_name_len = strlen(file_name); > - strncpy(result, file_name, file_name_len < max_length ? file_name_len : max_length); > + strncpy(result, file_name, max_length - 1); > + result[max_length - 1] = 0;
The usual idiom here, if we stay with strncpy is:
strncpy(result, file_name, max_length - 1)[max_length - 1] = 0;
one line instead of two, but there are other possibilities, what I've done int these cases in tools/perf/ is to switch to strlcpy, so just flip that 'n' to a 'l' and it should be enough.
For that we just have a copy of the kernel's strlcpy() implementation in lib/string.c, and it has this doc:
/** * strlcpy - Copy a C-string into a sized buffer * @dest: Where to copy the string to * @src: Where to copy the string from * @size: size of destination buffer * * Compatible with ``*BSD``: the result is always a valid * NUL-terminated string that fits in the buffer (unless, * of course, the buffer size is zero). It does not pad * out the result like strncpy() does. */
The kernel folks moved beyond that and in lib/string.c we have:
/** * strscpy - Copy a C-string into a sized buffer * @dest: Where to copy the string to * @src: Where to copy the string from * @count: Size of destination buffer * * Copy the string, or as much of it as fits, into the dest buffer. The * behavior is undefined if the string buffers overlap. The destination * buffer is always NUL terminated, unless it's zero-sized. * * Preferred to strlcpy() since the API doesn't require reading memory * from the src string beyond the specified "count" bytes, and since * the return value is easier to error-check than strlcpy()'s. * In addition, the implementation is robust to the string changing out * from underneath it, unlike the current strlcpy() implementation. * * Preferred to strncpy() since it always returns a valid string, and * doesn't unnecessarily force the tail of the destination buffer to be * zeroed. If zeroing is desired please use strscpy_pad(). * * Return: The number of characters copied (not including the trailing * %NUL) or -E2BIG if the destination buffer wasn't big enough. */ ssize_t strscpy(char *dest, const char *src, size_t count)
I think for these needs flipping that 'n' into a 'l' is good enough.
- Arnaldo
> } > } > > -- > 2.21.0
--
- Arnaldo
| |