linux-kernel - Re: [PATCH] Fix wrong EOF compare

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <xa1ta9shm531.fsf@mina86.com>
Date:	Thu, 10 Jan 2013 16:26:58 +0100
From:	Michal Nazarewicz <mina86@...a86.com>
To:	Minchan Kim <minchan@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Cc:	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	Minchan Kim <minchan@...nel.org>, Mel Gorman <mgorman@...e.de>,
	Andy Whitcroft <apw@...dowen.org>,
	Alexander Nyberg <alexn@....su.se>
Subject: Re: [PATCH] Fix wrong EOF compare

On Thu, Jan 10 2013, Minchan Kim <minchan@...nel.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@...e.de>
> Cc: Andy Whitcroft <apw@...dowen.org>
> Cc: Alexander Nyberg <alexn@....su.se>
> Signed-off-by: Minchan Kim <minchan@...nel.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@...gle.com>--------------ooO--(_)--Ooo--

Content of type "application/pgp-signature" skipped