| Message ID | 20121108163348.GA7729@merkur.ravnborg.org |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
于 2012年11月09日 00:33, Sam Ravnborg 写道: > Hi Cheng. > On Thu, Nov 08, 2012 at 11:41:39AM +0800, Chen Gang wrote: >> >> when cp >= barg_buf + BARG_LEN-2, it only break internel loop (while) >> but outside loop (for) still has effect, and "*cp++ = ' '" repeating >> so need additional checking for it. >> >> >> Signed-off-by: Chen Gang <gang.chen@asianux.com> > > I wonder how you found this bug?!?! I only find it through "code review". A) I grep all "strcpy" in kernel wide (about 2943 lines) B) I check them one by one. i) when I check strcpy, also reading all relative source code. ii) my goal is finding issues. iii) if I think it is valuable to continue reading, I will do. C) when find arch/sparc, I find this bug (although it is not relative with strcpy). It seems just starting (I have only finished checking 10 lines strcpy of 2943 lines), > Anyway please consider this alternative fix: > > diff --git a/arch/sparc/prom/bootstr_32.c b/arch/sparc/prom/bootstr_32.c > index f5ec32e..4ce602f 100644 > --- a/arch/sparc/prom/bootstr_32.c > +++ b/arch/sparc/prom/bootstr_32.c > @@ -31,14 +31,10 @@ prom_getbootargs(void) > arg = (*(romvec->pv_v0bootargs))->argv[iter]; > if (arg == NULL) > break; > - while(*arg != 0) { > - /* Leave place for space and null. */ > - if(cp >= barg_buf + BARG_LEN-2){ > - /* We might issue a warning here. */ > - break; > - } > + while (*arg != 0 && cp < (barg_buf + BARG_LEN - 2)) > *cp++ = *arg++; > - } > + > + /* Append trailing space + null */ > *cp++ = ' '; > } > *cp = 0; > > > Adding the conditional inside the while loop makes > the logic simpler. And the patch actually deletes more lines than it adds. > And please take care to follow coding style too. In particular spaces around operators. > > The old code does not follow coding style - but this is no excuse. > I agree with the contents above. > Note - the above is not even build tested! > A) I think, it will be better to give a test (although it seems obviously) B) but sorry for I have no relative environments now. i) if testing is necessary; ii) also if you have no environments, either. iii) I should try. (please tell me) > If you use the above code-snippet you can add my: > Acked-by: Sam Ravnborg <sam@ravnborg.org> > I think it is necessary, you can do it, automatically. if truly need I do, please tell me.
于 2012年11月13日 11:25, Chen Gang 写道: > 于 2012年11月09日 00:33, Sam Ravnborg 写道: >> Anyway please consider this alternative fix: >> >> diff --git a/arch/sparc/prom/bootstr_32.c b/arch/sparc/prom/bootstr_32.c >> index f5ec32e..4ce602f 100644 >> --- a/arch/sparc/prom/bootstr_32.c >> +++ b/arch/sparc/prom/bootstr_32.c >> @@ -31,14 +31,10 @@ prom_getbootargs(void) >> arg = (*(romvec->pv_v0bootargs))->argv[iter]; >> if (arg == NULL) >> break; >> - while(*arg != 0) { >> - /* Leave place for space and null. */ >> - if(cp >= barg_buf + BARG_LEN-2){ >> - /* We might issue a warning here. */ >> - break; >> - } >> + while (*arg != 0 && cp < (barg_buf + BARG_LEN - 2)) >> *cp++ = *arg++; >> - } >> + >> + /* Append trailing space + null */ >> *cp++ = ' '; >> } >> *cp = 0; >> >> >> Adding the conditional inside the while loop makes >> the logic simpler. And the patch actually deletes more lines than it adds. >> And please take care to follow coding style too. In particular spaces around operators. >> >> The old code does not follow coding style - but this is no excuse. >> > > I agree with the contents above. > 1) although I agree with what you said. 2) but for the patch code: after "*cp++ = ' ';", also need "break checking" for "for (; ;)" outside of "while()" 3) please check, and still it will be better to notice coding style, too.
From: Chen Gang <gang.chen@asianux.com> Date: Tue, 13 Nov 2012 12:46:17 +0800 > 1) although I agree with what you said. > > 2) but for the patch code: > after "*cp++ = ' ';", also need "break checking" for "for (; ;)" outside of "while()" > > 3) please check, and still it will be better to notice coding style, too. Why is every single one of your replies a set of 3 bullet points? You aren't giving a PowerPoint presentation. Just talk and discuss things normally, using sentences and paragraphs for your structure. Everything you say and do looks very unnatural, awkward, and forced. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/sparc/prom/bootstr_32.c b/arch/sparc/prom/bootstr_32.c index f5ec32e..4ce602f 100644 --- a/arch/sparc/prom/bootstr_32.c +++ b/arch/sparc/prom/bootstr_32.c @@ -31,14 +31,10 @@ prom_getbootargs(void) arg = (*(romvec->pv_v0bootargs))->argv[iter]; if (arg == NULL) break; - while(*arg != 0) { - /* Leave place for space and null. */ - if(cp >= barg_buf + BARG_LEN-2){ - /* We might issue a warning here. */ - break; - } + while (*arg != 0 && cp < (barg_buf + BARG_LEN - 2)) *cp++ = *arg++; - } + + /* Append trailing space + null */ *cp++ = ' '; } *cp = 0;