Messages in this thread | | | Date | Thu, 4 Oct 2018 13:45:35 +0100 | From | Daniel Thompson <> | Subject | Re: [PATCH v2] kdb: Use strscpy with destination buffer size |
| |
On Thu, Oct 04, 2018 at 08:25:30AM -0400, Prarit Bhargava wrote: > On 10/02/2018 11:53 AM, Daniel Thompson wrote: > > On 20/09/2018 13:59, Prarit Bhargava wrote: > >> gcc 8.1.0 warns with: > >> > >> kernel/debug/kdb/kdb_support.c: In function ‘kallsyms_symbol_next’: > >> kernel/debug/kdb/kdb_support.c:239:4: warning: ‘strncpy’ specified bound > >> depends on the length of the source argument [-Wstringop-overflow=] > >> strncpy(prefix_name, name, strlen(name)+1); > >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >> kernel/debug/kdb/kdb_support.c:239:31: note: length computed here > >> > >> Use strscpy() with the destination buffer size, and use ellipses when > >> displaying truncated symbols. > >> > >> v2: Use strscpy() > >> > >> Signed-off-by: Prarit Bhargava <prarit@redhat.com> > >> Cc: Jonathan Toppins <jtoppins@redhat.com> > >> Cc: Jason Wessel <jason.wessel@windriver.com> > >> Cc: Daniel Thompson <daniel.thompson@linaro.org> > >> Cc: kgdb-bugreport@lists.sourceforge.net > >> --- > >> kernel/debug/kdb/kdb_io.c | 15 +++++++++------ > >> kernel/debug/kdb/kdb_private.h | 2 +- > >> kernel/debug/kdb/kdb_support.c | 10 +++++----- > >> 3 files changed, 15 insertions(+), 12 deletions(-) > >> > >> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c > >> index ed5d34925ad0..6a4b41484afe 100644 > >> --- a/kernel/debug/kdb/kdb_io.c > >> +++ b/kernel/debug/kdb/kdb_io.c > >> @@ -216,7 +216,7 @@ static char *kdb_read(char *buffer, size_t bufsize) > >> int count; > >> int i; > >> int diag, dtab_count; > >> - int key; > >> + int key, buf_size, ret; > >> diag = kdbgetintenv("DTABCOUNT", &dtab_count); > >> @@ -336,9 +336,8 @@ static char *kdb_read(char *buffer, size_t bufsize) > >> else > >> p_tmp = tmpbuffer; > >> len = strlen(p_tmp); > >> - count = kallsyms_symbol_complete(p_tmp, > >> - sizeof(tmpbuffer) - > >> - (p_tmp - tmpbuffer)); > >> + buf_size = sizeof(tmpbuffer) - (p_tmp - tmpbuffer); > >> + count = kallsyms_symbol_complete(p_tmp, buf_size); > >> if (tab == 2 && count > 0) { > >> kdb_printf("\n%d symbols are found.", count); > >> if (count > dtab_count) { > >> @@ -350,9 +349,13 @@ static char *kdb_read(char *buffer, size_t bufsize) > >> } > >> kdb_printf("\n"); > >> for (i = 0; i < count; i++) { > >> - if (WARN_ON(!kallsyms_symbol_next(p_tmp, i))) > >> + ret = kallsyms_symbol_next(p_tmp, i, buf_size); > >> + if (WARN_ON(!ret)) > >> break; > > I'm getting confused by having two different branches on ret. > > > > Don't get a WARN_ON() when ret == -E2BIG? > > > > > > Should we WARN on a really long symbol? I don't think we should as we're > handling that by truncating the output and adding ellipses below.
It's OK. You describe the behaviour I expect but I was misreading the code (not realizing that kallsyms_symbol_next() had not become a 0 on success success function).
However after reviewing the code (properly this time) I wonder if the WARN_ON() should be improved to match the return value for the function:
WARN_ON(ret >= 0 && ret < len)
That said, checking the symbol length is pretty paranoid and getting close to nitpicking so I'll leave it up to you and, with or without, the change:
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Daniel.
| |