Patchwork arch/sparc: additional len check in loop for prom_getbootargs

login
register
mail settings
Submitter Sam Ravnborg
Date Nov. 8, 2012, 4:33 p.m.
Message ID <20121108163348.GA7729@merkur.ravnborg.org>
Download mbox | patch
Permalink /patch/197856/
State RFC
Delegated to: David Miller
Headers show

Comments

Sam Ravnborg - Nov. 8, 2012, 4:33 p.m.
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?!?!
Anyway please consider this alternative fix:



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:
Acked-by: Sam Ravnborg <sam@ravnborg.org>

	Sam
--
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
Chen Gang - Nov. 13, 2012, 3:25 a.m.
于 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.
Chen Gang - Nov. 13, 2012, 4:46 a.m.
于 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.
David Miller - Nov. 13, 2012, 5:06 a.m.
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

Patch

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;