[PATCH] Fix wrong EOF compare

[PATCH] Fix wrong EOF compare

[Minchan Kim]

getc returns "int" so EOF could be -1 but storing getc's return
value to char directly makes the vaule to 255 so below condition
is always false.

It happens in my ARM system so loop is not ended, then segfaulted.
This patch fixes it.

                *curr = getc(fin); // *curr = 255
                if (*curr == EOF) return -1; // if ( 255 == -1)

Cc: Mel Gorman <mgorman@suse.de>
Cc: Andy Whitcroft <apw@shadowen.org>
Cc: Alexander Nyberg <alexn@dsv.su.se>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 Documentation/page_owner.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/page_owner.c b/Documentation/page_owner.c
index f0156e1..b777fb6 100644
--- a/Documentation/page_owner.c
+++ b/Documentation/page_owner.c
@@ -32,12 +32,14 @@ int read_block(char *buf, FILE *fin)
 {
 	int ret = 0;
 	int hit = 0;
+	int vaule;
 	char *curr = buf;
 
 	for (;;) {
-		*curr = getc(fin);
-		if (*curr == EOF) return -1;
+		value = getc(fin);
+		if (value == EOF) return -1;
 
+		*curr = value;
 		ret++;
 		if (*curr == '\n' && hit == 1)
 			return ret - 1;
-- 
1.7.9.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Fix wrong EOF compare

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 2226 bytes --]

On Thu, Jan 10 2013, Minchan Kim <minchan@kernel.org> wrote:
> getc returns "int" so EOF could be -1 but storing getc's return
> value to char directly makes the vaule to 255 so below condition
> is always false.

Technically, this is implementation defined and I believe on many
systems char is signed thus the loop will end on EOF or byte 255.

Either way, my point is the patch is correct, but the comment is not. ;)

Of course, even better if the function just used fgets(), ie. something
like:

int read_block(char *buf, int buf_size, FILE *fin)
{
	char *curr = buf, *const buf_end = buf + buf_size;

	while (buf_end - curr > 1 && fgets(curr, buf_end - curr, fin)) {
		if (*curr == '\n') /* empty line */
			return curr - buf;
		curr += strlen(curr);
	}

	return -1; /* EOF or no space left in buf. */
}

which is much shorter and does not have buffer overflow issues.

> It happens in my ARM system so loop is not ended, then segfaulted.
> This patch fixes it.
>
>                 *curr = getc(fin); // *curr = 255
>                 if (*curr == EOF) return -1; // if ( 255 == -1)
>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Andy Whitcroft <apw@shadowen.org>
> Cc: Alexander Nyberg <alexn@dsv.su.se>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  Documentation/page_owner.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/page_owner.c b/Documentation/page_owner.c
> index f0156e1..b777fb6 100644
> --- a/Documentation/page_owner.c
> +++ b/Documentation/page_owner.c
> @@ -32,12 +32,14 @@ int read_block(char *buf, FILE *fin)
>  {
>  	int ret = 0;
>  	int hit = 0;
> +	int vaule;
>  	char *curr = buf;
>  
>  	for (;;) {
> -		*curr = getc(fin);
> -		if (*curr == EOF) return -1;
> +		value = getc(fin);
> +		if (value == EOF) return -1;
>  
> +		*curr = value;
>  		ret++;
>  		if (*curr == '\n' && hit == 1)
>  			return ret - 1;

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] Fix wrong EOF compare

[Randy Dunlap]

On 01/10/13 07:26, Michal Nazarewicz wrote:
> On Thu, Jan 10 2013, Minchan Kim <minchan@kernel.org> wrote:
>> getc returns "int" so EOF could be -1 but storing getc's return
>> value to char directly makes the vaule to 255 so below condition
>> is always false.
> 
> Technically, this is implementation defined and I believe on many
> systems char is signed thus the loop will end on EOF or byte 255.
> 
> Either way, my point is the patch is correct, but the comment is not. ;)

and change spelling of 'vaule' to 'value'
and test build it please.

> 
> Of course, even better if the function just used fgets(), ie. something
> like:
> 
> int read_block(char *buf, int buf_size, FILE *fin)
> {
> 	char *curr = buf, *const buf_end = buf + buf_size;
> 
> 	while (buf_end - curr > 1 && fgets(curr, buf_end - curr, fin)) {
> 		if (*curr == '\n') /* empty line */
> 			return curr - buf;
> 		curr += strlen(curr);
> 	}
> 
> 	return -1; /* EOF or no space left in buf. */
> }
> 
> which is much shorter and does not have buffer overflow issues.
> 
>> It happens in my ARM system so loop is not ended, then segfaulted.
>> This patch fixes it.
>>
>>                 *curr = getc(fin); // *curr = 255
>>                 if (*curr == EOF) return -1; // if ( 255 == -1)
>>
>> Cc: Mel Gorman <mgorman@suse.de>
>> Cc: Andy Whitcroft <apw@shadowen.org>
>> Cc: Alexander Nyberg <alexn@dsv.su.se>
>> Signed-off-by: Minchan Kim <minchan@kernel.org>
>> ---
>>  Documentation/page_owner.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/page_owner.c b/Documentation/page_owner.c
>> index f0156e1..b777fb6 100644
>> --- a/Documentation/page_owner.c
>> +++ b/Documentation/page_owner.c
>> @@ -32,12 +32,14 @@ int read_block(char *buf, FILE *fin)
>>  {
>>  	int ret = 0;
>>  	int hit = 0;
>> +	int vaule;
>>  	char *curr = buf;
>>  
>>  	for (;;) {
>> -		*curr = getc(fin);
>> -		if (*curr == EOF) return -1;
>> +		value = getc(fin);
>> +		if (value == EOF) return -1;
>>  
>> +		*curr = value;
>>  		ret++;
>>  		if (*curr == '\n' && hit == 1)
>>  			return ret - 1;
> 
> 
> 


-- 
~Randy

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Fix wrong EOF compare

[Minchan Kim]

Hi Randy,

On Thu, Jan 10, 2013 at 08:12:24AM -0800, Randy Dunlap wrote:
> On 01/10/13 07:26, Michal Nazarewicz wrote:
> > On Thu, Jan 10 2013, Minchan Kim <minchan@kernel.org> wrote:
> >> getc returns "int" so EOF could be -1 but storing getc's return
> >> value to char directly makes the vaule to 255 so below condition
> >> is always false.
> > 
> > Technically, this is implementation defined and I believe on many
> > systems char is signed thus the loop will end on EOF or byte 255.
> > 
> > Either way, my point is the patch is correct, but the comment is not. ;)
> 
> and change spelling of 'vaule' to 'value'

Oops. It was typo when I cooked the patch after testing in my ARM system.

> and test build it please.

Keep in mind. Thanks!

> 
> > 
> > Of course, even better if the function just used fgets(), ie. something
> > like:
> > 
> > int read_block(char *buf, int buf_size, FILE *fin)
> > {
> > 	char *curr = buf, *const buf_end = buf + buf_size;
> > 
> > 	while (buf_end - curr > 1 && fgets(curr, buf_end - curr, fin)) {
> > 		if (*curr == '\n') /* empty line */
> > 			return curr - buf;
> > 		curr += strlen(curr);
> > 	}
> > 
> > 	return -1; /* EOF or no space left in buf. */
> > }
> > 
> > which is much shorter and does not have buffer overflow issues.
> > 
> >> It happens in my ARM system so loop is not ended, then segfaulted.
> >> This patch fixes it.
> >>
> >>                 *curr = getc(fin); // *curr = 255
> >>                 if (*curr == EOF) return -1; // if ( 255 == -1)
> >>
> >> Cc: Mel Gorman <mgorman@suse.de>
> >> Cc: Andy Whitcroft <apw@shadowen.org>
> >> Cc: Alexander Nyberg <alexn@dsv.su.se>
> >> Signed-off-by: Minchan Kim <minchan@kernel.org>
> >> ---
> >>  Documentation/page_owner.c |    6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/page_owner.c b/Documentation/page_owner.c
> >> index f0156e1..b777fb6 100644
> >> --- a/Documentation/page_owner.c
> >> +++ b/Documentation/page_owner.c
> >> @@ -32,12 +32,14 @@ int read_block(char *buf, FILE *fin)
> >>  {
> >>  	int ret = 0;
> >>  	int hit = 0;
> >> +	int vaule;
> >>  	char *curr = buf;
> >>  
> >>  	for (;;) {
> >> -		*curr = getc(fin);
> >> -		if (*curr == EOF) return -1;
> >> +		value = getc(fin);
> >> +		if (value == EOF) return -1;
> >>  
> >> +		*curr = value;
> >>  		ret++;
> >>  		if (*curr == '\n' && hit == 1)
> >>  			return ret - 1;
> > 
> > 
> > 
> 
> 
> -- 
> ~Randy
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Fix wrong EOF compare

[Minchan Kim]

Hello Michal,

On Thu, Jan 10, 2013 at 04:26:58PM +0100, Michal Nazarewicz wrote:
> On Thu, Jan 10 2013, Minchan Kim <minchan@kernel.org> wrote:
> > getc returns "int" so EOF could be -1 but storing getc's return
> > value to char directly makes the vaule to 255 so below condition
> > is always false.
> 
> Technically, this is implementation defined and I believe on many
> systems char is signed thus the loop will end on EOF or byte 255.

True. That's why there is no problem in x86.
The problem would happens on ARM and PowerPC which defined char as unsigned
in GCC.

> 
> Either way, my point is the patch is correct, but the comment is not. ;)

Yeb. It was not elegant :(
How about this?

The C standards allows the character type char to be singed or unsinged,
depending on the platform and compiler. Most of systems uses signed char,
but those based on PowerPC and ARM processors typically use unsigned char.
This can lead to unexpected results when the variable is used to compare
with EOF(-1).

This patch fixes the problem.

> 
> Of course, even better if the function just used fgets(), ie. something
> like:
> 
> int read_block(char *buf, int buf_size, FILE *fin)
> {
> 	char *curr = buf, *const buf_end = buf + buf_size;
> 
> 	while (buf_end - curr > 1 && fgets(curr, buf_end - curr, fin)) {
> 		if (*curr == '\n') /* empty line */
> 			return curr - buf;
> 		curr += strlen(curr);
> 	}
> 
> 	return -1; /* EOF or no space left in buf. */
> }
> 
> which is much shorter and does not have buffer overflow issues.

Looks better. It is bug fix + code clean up + performance enhance.
Although it's very straightforward, I would like to separate simple
bug fix with others so I will resend two patches.

Thanks for the review!

> 
> > It happens in my ARM system so loop is not ended, then segfaulted.
> > This patch fixes it.
> >
> >                 *curr = getc(fin); // *curr = 255
> >                 if (*curr == EOF) return -1; // if ( 255 == -1)
> >
> > Cc: Mel Gorman <mgorman@suse.de>
> > Cc: Andy Whitcroft <apw@shadowen.org>
> > Cc: Alexander Nyberg <alexn@dsv.su.se>
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  Documentation/page_owner.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/page_owner.c b/Documentation/page_owner.c
> > index f0156e1..b777fb6 100644
> > --- a/Documentation/page_owner.c
> > +++ b/Documentation/page_owner.c
> > @@ -32,12 +32,14 @@ int read_block(char *buf, FILE *fin)
> >  {
> >  	int ret = 0;
> >  	int hit = 0;
> > +	int vaule;
> >  	char *curr = buf;
> >  
> >  	for (;;) {
> > -		*curr = getc(fin);
> > -		if (*curr == EOF) return -1;
> > +		value = getc(fin);
> > +		if (value == EOF) return -1;
> >  
> > +		*curr = value;
> >  		ret++;
> >  		if (*curr == '\n' && hit == 1)
> >  			return ret - 1;
> 
> -- 
> Best regards,                                         _     _
> .o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
> ..o | Computer Science,  MichaA? a??mina86a?? Nazarewicz    (o o)
> ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--





-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>