arch/sparc: additional len check in loop for prom_getbootargs
PATCH 1/1 arch/sparc: additional len check in loop for prom_getbootargs
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.
--- arch/sparc/prom/bootstr_32.c | 4 ++++ 1 file changed, 4 insertions(+)
arch/sparc/prom/bootstr_32.c +4 -0
@@ -40,6 +40,10 @@ prom_getbootargs(void) *cp++ = *arg++; } *cp++ =' ' ; +if (cp >= barg_buf +BARG_LEN -1 ){ +/* We might issue a warning here. */ +break ; + } } *cp =0 ;break ;
Signature · 1 line
1.7.9.5
Hi Cheng.
Quoted 7 lines · Chen Gang
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 <[email protected]>
I wonder how you found this bug?!?!
Anyway please consider this alternative fix:
arch/sparc/prom/bootstr_32.c +3 -7
@@ -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.
Note - the above is not even build tested!
If you use the above code-snippet you can add my:
Sam
Hell David Miller:
1) Is this patch applied ? or need some discussion ? or suspended ?
2) If possible:
A) can you tell me about our patch processing work flow "
B) or some reference links (or document location) ?
thanks.
Quoted 27 lines · Chen Gang
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 <[email protected]>--- arch/sparc/prom/bootstr_32.c | 4 ++++ 1 file changed, 4 insertions(+)arch/sparc/prom/bootstr_32.c +4
-0@@ -40,6 +40,10 @@ prom_getbootargs(void) *cp++ = *arg++; } *cp++ =' ' ; +if (cp >= barg_buf +BARG_LEN -1 ){ +/* We might issue a warning here. */ +break ; + } } *cp =0 ;break ;
Signature · 7 lines
Chen Gang Asianux Corporation -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller
maintainerI'm backlogged and often without internet access, and therefore concentrating my limited time on kernel networking and much more important sparc changes such as bug fixes for problems people will actually hit.
So you'll just need to be very patient.
Quoted 9 lines · David Miller
I'm backlogged and often without internet access, and therefore concentrating my limited time on kernel networking and much more important sparc changes such as bug fixes for problems people will actually hit.
So you'll just need to be very patient.
ok, thanks, that is truly what I shall be.
and I suggest:
1) can write a document for patch work flow and time waiting document
2) put it in a suitable location (can be easily found by new members).
3) I think this document is useful for the new members.
thanks
Signature · 3 lines
Chen Gang Asianux Corporation
Hi Chen,
Quoted 7 lines · Chen Gang
Hell David Miller:
1) Is this patch applied ? or need some discussion ? or suspended ?
2) If possible:
A) can you tell me about our patch processing work flow "
B) or some reference links (or document location) ?
In regards to patches, you should assume that maintainers are:
1. grumpy
2. overworked
3. looking at your patch as you email them about it.
(Note that only #2 is true, but it saves everyone time and aggravation if you assume the rest)
11 hours is not long enough for David to get to your patch, review it and apply it. I don't believe that there are any maintainers which would get to a patch that quickly.
If David hadn't applied the patch within a few days of it being posted, then you might want to gently ask what's up. However, as Sam reviewed the patch and had some comments about it, it's unlikely that David will even look at it until you've addressed Sam's feedback.
The best way to get a patch into the kernel is to follow the rules in Documentation/SubmittingPatches which you have, respond quickly to any feedback, be patient, be prepared to explain why you're doing what you're doing, and be prepared to change and resubmit your patch many times until everyone is happy with it.
I'm sure that this is all written down in a document somewhere, but I can't recall what or where it is.
Thanks,
Signature · 5 lines
Julian Calaby Email: [email protected] Profile: http://www.google.com/profiles/julian.calaby/ .Plan: http://sites.google.com/site/juliancalaby/
Hi Julian Calaby:
thank you for your reply, firstly.
1) I think what you reply are valuable (at least, for me, it is true).
2) I have read Documents/SubmittingPatches (which Eric Dumazet suggested)
3) I also think, it will be better, if put part contents of your reply into Documents/SubmittingPatches as completion.
thanks.
gchen.
Quoted 39 lines · Julian Calaby
Hi Chen,
Quoted 7 lines · Chen Gang
Hell David Miller:
1) Is this patch applied ? or need some discussion ? or suspended ?
2) If possible:
A) can you tell me about our patch processing work flow "
B) or some reference links (or document location) ?In regards to patches, you should assume that maintainers are:
1. grumpy
2. overworked
3. looking at your patch as you email them about it.(Note that only #2 is true, but it saves everyone time and aggravation if you assume the rest)
11 hours is not long enough for David to get to your patch, review it and apply it. I don't believe that there are any maintainers which would get to a patch that quickly.
If David hadn't applied the patch within a few days of it being posted, then you might want to gently ask what's up. However, as Sam reviewed the patch and had some comments about it, it's unlikely that David will even look at it until you've addressed Sam's feedback.
The best way to get a patch into the kernel is to follow the rules in
Documentation/SubmittingPatcheswhich you have, respond quickly to any feedback, be patient, be prepared to explain why you're doing what you're doing, and be prepared to change and resubmit your patch many times until everyone is happy with it.I'm sure that this is all written down in a document somewhere, but I can't recall what or where it is.
Thanks,
Signature · 3 lines
Chen Gang Asianux Corporation
Hello David Miller, Julian Calaby:
1) I am sorry for original mails to you:
A) at 2012-11-09 00:33 Sam Ravnborg has already replied to me (need
additional discussion)
B) but I did not see it, it is my fault.
C) at 2012-11-09 11:15 I sent mail to want to get reply, and you
replied immediately.
(the Time Location is at Beijing China)
2) sorry again, for bothering you, and also thank Sam Ravnborg's reply.
3) next time, I shall notice, and check my own mail carefully.
Chen Gang
Quoted 11 lines · Sam Ravnborg
Hi Cheng.
Quoted 7 lines · Chen Gang
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 <[email protected]>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),
Quoted 26 lines
Anyway please consider this alternative fix:
arch/sparc/prom/bootstr_32.c +3
-7@@ -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 ;
Quoted 6 lines
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.
Quoted 2 lines
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)
Quoted 3 lines
If you use the above code-snippet you can add my:
Acked-by Sam Ravnborg <[email protected]>
I think it is necessary, you can do it, automatically. if truly need I do, please tell me.
Signature · 3 lines
Chen Gang Asianux Corporation
Quoted 27 lines · Chen Gang
Quoted 26 lines · Sam Ravnborg
Anyway please consider this alternative fix:
arch/sparc/prom/bootstr_32.c +3
-7@@ -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 ;
Quoted 9 lines
Quoted 6 lines
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.
Signature · 3 lines
Chen Gang Asianux Corporation
David Miller
maintainerFrom: Chen Gang <[email protected]>
Date: Tue, 13 Nov 2012 12:46:17 +0800
Quoted 6 lines
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.
Quoted 10 lines · David Miller
From: Chen Gang <[email protected]>
Date: Tue, 13 Nov 2012 12:46:17 +0800Quoted 6 lines
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.
Quoted 9 lines
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.
Excuse me,, my English is not quite well, just as you said (I need improving)
after "*cp++ = ' ';" at line 42, also need "break checking" for "for (; ;)" outside of "while()"
the reply from sam not check it, (the patch what I sent check it).
this is the original source code of function prom_getbootargs in arch/sparc/prom/bootstr_32.c.
------------------------------------------------------------------------------------------------
27 case PROM_V0:
28 cp = barg_buf;
29 /* Start from 1 and go over fd(0,0,0)kernel */
30 for(iter = 1; iter < 8; iter++) {
31 arg = (*(romvec->pv_v0bootargs))->argv[iter];
32 if (arg = NULL)
33 break;
34 while(*arg != 0) {
35 /* Leave place for space and null. */
36 if(cp >= barg_buf + BARG_LEN-2){
37 /* We might issue a warning here. */
38 break;
39 }
40 *cp++ = *arg++;
41 }
42 *cp++ = ' ';
43 }
44 *cp = 0;
45 break;---------------------------------------------------------------------------------------------------
this is the original reply from sam:
--------------------------------------------------------------------------------------------------
Hi Cheng.
Quoted 7 lines · Chen Gang
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 <[email protected]>
I wonder how you found this bug?!?!
Anyway please consider this alternative fix:
arch/sparc/prom/bootstr_32.c +3 -7
@@ -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.
Note - the above is not even build tested!
If you use the above code-snippet you can add my:
Sam
------------------------------------------------------------------------------------------
Signature · 3 lines
Chen Gang Asianux Corporation