Message ID | 20170421123320.7344-1-naveen.n.rao@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
On 04/21/2017 07:33 AM, Naveen N. Rao wrote: > Convert usage of strchr()/strncpy()/strncat() to > strnchr()/memcpy()/strlcat() for simpler and safer string manipulation. > > Reported-by: David Laight <David.Laight@ACULAB.COM> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > --- > Changes: Additionally convert the strchr(). > > > arch/powerpc/kernel/kprobes.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c > index 97b5eed1f76d..c73fb6e3b43f 100644 > --- a/arch/powerpc/kernel/kprobes.c > +++ b/arch/powerpc/kernel/kprobes.c > @@ -65,28 +65,27 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset) > char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN]; > const char *modsym; > bool dot_appended = false; > - if ((modsym = strchr(name, ':')) != NULL) { > + if ((modsym = strnchr(name, ':', MODULE_NAME_LEN)) != NULL) { > modsym++; > if (*modsym != '\0' && *modsym != '.') { > /* Convert to <module:.symbol> */ > - strncpy(dot_name, name, modsym - name); > + memcpy(dot_name, name, modsym - name); > dot_name[modsym - name] = '.'; > dot_name[modsym - name + 1] = '\0'; > - strncat(dot_name, modsym, > - sizeof(dot_name) - (modsym - name) - 2); > + strlcat(dot_name, modsym, sizeof(dot_name)); Would it be more efficient here to replace this: -- dot_name[modsym - name + 1] = '\0'; strlcat(dot_name, modsym, sizeof(dot_name)); -- with this: strncpy(&dot_name[modsym - name + 1], modsym, KSYM_NAME_LEN); (So you aren't rescanning dot_name to find the end, when you already know the end position?) > dot_appended = true; > } else { > dot_name[0] = '\0'; > - strncat(dot_name, name, sizeof(dot_name) - 1); > + strlcat(dot_name, name, sizeof(dot_name)); and here do: strncpy(dot_name, name, sizeof(dot_name)); (and remove the null termination immediately above) > } > } else if (name[0] != '.') { > dot_name[0] = '.'; > dot_name[1] = '\0'; > - strncat(dot_name, name, KSYM_NAME_LEN - 2); > + strlcat(dot_name, name, sizeof(dot_name)); and here do: strncpy(&dot_name[1], name, sizeof(dot_name)); (and remove the null termination immediately above) > dot_appended = true; > } else { > dot_name[0] = '\0'; > - strncat(dot_name, name, KSYM_NAME_LEN - 1); > + strlcat(dot_name, name, sizeof(dot_name)); and here do: strncpy(dot_name, name, sizeof(dot_name)); (and remove the null termination immediately above) > } PC
On 04/21/2017 08:33 AM, Paul Clarke wrote: > On 04/21/2017 07:33 AM, Naveen N. Rao wrote: >> } else if (name[0] != '.') { >> dot_name[0] = '.'; >> dot_name[1] = '\0'; >> - strncat(dot_name, name, KSYM_NAME_LEN - 2); >> + strlcat(dot_name, name, sizeof(dot_name)); > > and here do: > strncpy(&dot_name[1], name, sizeof(dot_name)); oops. s/sizeof(dot_name)/sizeof(dot_name) - 1/ PC
Sent too soon. The suggestions don't guarantee null termination. Refined, below. (Sorry for the noise.) On 04/21/2017 08:33 AM, Paul Clarke wrote: > On 04/21/2017 07:33 AM, Naveen N. Rao wrote: >> Convert usage of strchr()/strncpy()/strncat() to >> strnchr()/memcpy()/strlcat() for simpler and safer string manipulation. >> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c >> index 97b5eed1f76d..c73fb6e3b43f 100644 >> --- a/arch/powerpc/kernel/kprobes.c >> +++ b/arch/powerpc/kernel/kprobes.c >> @@ -65,28 +65,27 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset) >> char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN]; >> const char *modsym; >> bool dot_appended = false; >> - if ((modsym = strchr(name, ':')) != NULL) { >> + if ((modsym = strnchr(name, ':', MODULE_NAME_LEN)) != NULL) { >> modsym++; >> if (*modsym != '\0' && *modsym != '.') { >> /* Convert to <module:.symbol> */ >> - strncpy(dot_name, name, modsym - name); >> + memcpy(dot_name, name, modsym - name); >> dot_name[modsym - name] = '.'; >> dot_name[modsym - name + 1] = '\0'; >> - strncat(dot_name, modsym, >> - sizeof(dot_name) - (modsym - name) - 2); >> + strlcat(dot_name, modsym, sizeof(dot_name)); > > Would it be more efficient here to replace this: > -- > dot_name[modsym - name + 1] = '\0'; > strlcat(dot_name, modsym, sizeof(dot_name)); > -- > > with this: > strncpy(&dot_name[modsym - name + 1], modsym, KSYM_NAME_LEN); keep the null termination, and just adjust the starting point for strlcat: -- dot_name[modsym - name + 1] = '\0'; strlcat(&dot_name[modsym - name + 1], modsym, KSYM_NAME_LEN); -- > (So you aren't rescanning dot_name to find the end, when you already know the end position?) > >> dot_appended = true; >> } else { >> dot_name[0] = '\0'; >> - strncat(dot_name, name, sizeof(dot_name) - 1); >> + strlcat(dot_name, name, sizeof(dot_name)); > > and here do: > strncpy(dot_name, name, sizeof(dot_name)); > > (and remove the null termination immediately above) nevermind. :-) > >> } >> } else if (name[0] != '.') { >> dot_name[0] = '.'; >> dot_name[1] = '\0'; >> - strncat(dot_name, name, KSYM_NAME_LEN - 2); >> + strlcat(dot_name, name, sizeof(dot_name)); > > and here do: > strncpy(&dot_name[1], name, sizeof(dot_name)); > > (and remove the null termination immediately above) > nevermind. :-) >> dot_appended = true; >> } else { >> dot_name[0] = '\0'; >> - strncat(dot_name, name, KSYM_NAME_LEN - 1); >> + strlcat(dot_name, name, sizeof(dot_name)); > > and here do: > strncpy(dot_name, name, sizeof(dot_name)); > > (and remove the null termination immediately above) > >> } nevermind. :-) PC
Excerpts from Paul Clarke's message of April 21, 2017 19:22: > Sent too soon. The suggestions don't guarantee null termination. Refined, below. (Sorry for the noise.) Yeah, the string operations here are a bit of a minefield... > > On 04/21/2017 08:33 AM, Paul Clarke wrote: >> On 04/21/2017 07:33 AM, Naveen N. Rao wrote: >>> Convert usage of strchr()/strncpy()/strncat() to >>> strnchr()/memcpy()/strlcat() for simpler and safer string manipulation. > >>> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c >>> index 97b5eed1f76d..c73fb6e3b43f 100644 >>> --- a/arch/powerpc/kernel/kprobes.c >>> +++ b/arch/powerpc/kernel/kprobes.c >>> @@ -65,28 +65,27 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset) >>> char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN]; >>> const char *modsym; >>> bool dot_appended = false; >>> - if ((modsym = strchr(name, ':')) != NULL) { >>> + if ((modsym = strnchr(name, ':', MODULE_NAME_LEN)) != NULL) { ... as I just realized this is an epic FAIL! ;/ I will take my time to redo this. - Naveen
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c index 97b5eed1f76d..c73fb6e3b43f 100644 --- a/arch/powerpc/kernel/kprobes.c +++ b/arch/powerpc/kernel/kprobes.c @@ -65,28 +65,27 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset) char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN]; const char *modsym; bool dot_appended = false; - if ((modsym = strchr(name, ':')) != NULL) { + if ((modsym = strnchr(name, ':', MODULE_NAME_LEN)) != NULL) { modsym++; if (*modsym != '\0' && *modsym != '.') { /* Convert to <module:.symbol> */ - strncpy(dot_name, name, modsym - name); + memcpy(dot_name, name, modsym - name); dot_name[modsym - name] = '.'; dot_name[modsym - name + 1] = '\0'; - strncat(dot_name, modsym, - sizeof(dot_name) - (modsym - name) - 2); + strlcat(dot_name, modsym, sizeof(dot_name)); dot_appended = true; } else { dot_name[0] = '\0'; - strncat(dot_name, name, sizeof(dot_name) - 1); + strlcat(dot_name, name, sizeof(dot_name)); } } else if (name[0] != '.') { dot_name[0] = '.'; dot_name[1] = '\0'; - strncat(dot_name, name, KSYM_NAME_LEN - 2); + strlcat(dot_name, name, sizeof(dot_name)); dot_appended = true; } else { dot_name[0] = '\0'; - strncat(dot_name, name, KSYM_NAME_LEN - 1); + strlcat(dot_name, name, sizeof(dot_name)); } addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name); if (!addr && dot_appended) {
Convert usage of strchr()/strncpy()/strncat() to strnchr()/memcpy()/strlcat() for simpler and safer string manipulation. Reported-by: David Laight <David.Laight@ACULAB.COM> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> --- Changes: Additionally convert the strchr(). arch/powerpc/kernel/kprobes.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)