[PATCH] nandwrite - handle situation when read returns less bytes the expected

[PATCH] nandwrite - handle situation when read returns less bytes the expected

[Hai Zaar]

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

Good day!
I've found a bug: nandwrite does not handle the situation when read
syscall returns less bytes when expected. Instead of requesting more
input, nandwrite just aborts with error.
The bug is especially triggered when input comes from stdin[1] which
is filled from remote host.

[1] http://lists.infradead.org/pipermail/linux-mtd/2008-September/022913.html.
BTW - is it going to be applied?

P.S. I'm not on the list, so please CC me.
-- 
Zaar

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mtd-utils-1.2.0-all-read-fix.patch --]
[-- Type: text/x-diff; name=mtd-utils-1.2.0-all-read-fix.patch, Size: 1152 bytes --]

read syscall can return less bytes then requested - handle this.
This bug is triggered when reading from stdin that comes from remote host

Author: Hai Zaar <haizaar@haizaar.com>

--- mtd-utils-1.2.0/nandwrite.c.orig	2008-12-14 14:56:11.000000000 +0200
+++ mtd-utils-1.2.0/nandwrite.c	2008-12-14 15:11:56.000000000 +0200
@@ -218,6 +218,7 @@
 	int ret, readlen;
 	int oobinfochanged = 0;
 	struct nand_oobinfo old_oobinfo;
+	int readcnt = 0;
 
 	process_options(argc, argv);
 
@@ -405,12 +406,22 @@
 		}
 
 		/* Read Page Data from input file */
-		if ((cnt = read(ifd, writebuf, readlen)) != readlen) {
-			if (cnt == 0)	// EOF
-				break;
-			perror ("File I/O error on input file");
-			goto closeall;
+		readcnt = 0;
+		while (readcnt < readlen) {
+			if ((cnt = read(ifd, writebuf + readcnt, readlen - readcnt)) < 0) {
+				perror ("File I/O error on input file");
+				goto closeall;
+			} else {
+				if ((cnt == 0) && (readcnt == 0)) {	// EOF
+					break;
+				} else {
+					readcnt += cnt;
+				}
+			}
+
 		}
+		if ((cnt == 0) && (readcnt == 0))	// EOF
+			break;
 
 		if (writeoob) {
 			/* Read OOB data from input file, exit on failure */

Re: [PATCH] nandwrite - handle situation when read returns less bytes the expected

[Artem Bityutskiy]

On Sun, 2008-12-14 at 15:59 +0200, Hai Zaar wrote:
> Good day!
> I've found a bug: nandwrite does not handle the situation when read
> syscall returns less bytes when expected. Instead of requesting more
> input, nandwrite just aborts with error.
> The bug is especially triggered when input comes from stdin[1] which
> is filled from remote host.
> 
> [1] http://lists.infradead.org/pipermail/linux-mtd/2008-September/022913.html.
> BTW - is it going to be applied?
> 
> P.S. I'm not on the list, so please CC me.

Your patch does not apply. It looks like it is not against the latest
mtd-utils. How about this patch?

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Date: Tue, 16 Dec 2008 10:02:16 +0200
Subject: [PATCH] nandwrite: correct data reading

The "read" syscall does not necessarily return all the requested
data, in which case the caller has to try again and read more.
Take this into account when reading input data.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 nandwrite.c |   31 +++++++++++++++++++++++--------
 1 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index fc23e85..0b2a9ee 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -260,6 +260,7 @@ int main(int argc, char * const argv[])
 	int ret, readlen;
 	int oobinfochanged = 0;
 	struct nand_oobinfo old_oobinfo;
+	int readcnt = 0;
 
 	process_options(argc, argv);
 
@@ -477,6 +478,8 @@ int main(int argc, char * const argv[])
 		readlen = meminfo.writesize;
 
 		if (ifd != STDIN_FILENO) {
+			int tinycnt = 0;
+
 			if (pad && (imglen < readlen))
 			{
 				readlen = imglen;
@@ -484,11 +487,15 @@ int main(int argc, char * const argv[])
 			}
 
 			/* Read Page Data from input file */
-			if ((cnt = read(ifd, writebuf, readlen)) != readlen) {
-				if (cnt == 0)	// EOF
+			while(tinycnt < readlen) {
+				cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
+				if (cnt == 0) { // EOF
 					break;
-				perror ("File I/O error on input file");
-				goto closeall;
+				} else if (cnt < 0) {
+					perror ("File I/O error on input file");
+					goto closeall;
+				}
+				tinycnt += cnt;
 			}
 		} else {
 			int tinycnt = 0;
@@ -522,11 +529,19 @@ int main(int argc, char * const argv[])
 		}
 
 		if (writeoob) {
-			/* Read OOB data from input file, exit on failure */
-			if ((cnt = read(ifd, oobreadbuf, meminfo.oobsize)) != meminfo.oobsize) {
-				perror ("File I/O error on input file");
-				goto closeall;
+			int tinycnt = 0;
+
+			while(tinycnt < readlen) {
+				cnt = read(ifd, oobreadbuf + tinycnt, meminfo.oobsize - tinycnt);
+				if (cnt == 0) { // EOF
+					break;
+				} else if (cnt < 0) {
+					perror ("File I/O error on input file");
+					goto closeall;
+				}
+				tinycnt += cnt;
 			}
+
 			if (!noecc) {
 				int i, start, len;
 				/*
-- 
1.5.4.3

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH] nandwrite - handle situation when read returns less bytes the expected

[Hai Zaar]

On Tue, Dec 16, 2008 at 8:28 AM, Artem Bityutskiy
<dedekind@infradead.org> wrote:
>
> Your patch does not apply. It looks like it is not against the latest
> mtd-utils. How about this patch?
Yes, my patch is against mtd-utils-1.2.0. Yours is surely better since
it covers oob reading as well.

>
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> Date: Tue, 16 Dec 2008 10:02:16 +0200
> Subject: [PATCH] nandwrite: correct data reading
>
> The "read" syscall does not necessarily return all the requested
> data, in which case the caller has to try again and read more.
> Take this into account when reading input data.
>
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> ---
>  nandwrite.c |   31 +++++++++++++++++++++++--------
>  1 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/nandwrite.c b/nandwrite.c
> index fc23e85..0b2a9ee 100644
> --- a/nandwrite.c
> +++ b/nandwrite.c
> @@ -260,6 +260,7 @@ int main(int argc, char * const argv[])
>        int ret, readlen;
>        int oobinfochanged = 0;
>        struct nand_oobinfo old_oobinfo;
> +       int readcnt = 0;
>
>        process_options(argc, argv);
>
> @@ -477,6 +478,8 @@ int main(int argc, char * const argv[])
>                readlen = meminfo.writesize;
>
>                if (ifd != STDIN_FILENO) {
> +                       int tinycnt = 0;
> +
>                        if (pad && (imglen < readlen))
>                        {
>                                readlen = imglen;
> @@ -484,11 +487,15 @@ int main(int argc, char * const argv[])
>                        }
>
>                        /* Read Page Data from input file */
> -                       if ((cnt = read(ifd, writebuf, readlen)) != readlen) {
> -                               if (cnt == 0)   // EOF
> +                       while(tinycnt < readlen) {
> +                               cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
> +                               if (cnt == 0) { // EOF
>                                        break;
> -                               perror ("File I/O error on input file");
> -                               goto closeall;
> +                               } else if (cnt < 0) {
> +                                       perror ("File I/O error on input file");
> +                                       goto closeall;
> +                               }
> +                               tinycnt += cnt;
>                        }
>                } else {
>                        int tinycnt = 0;
> @@ -522,11 +529,19 @@ int main(int argc, char * const argv[])
>                }
>
>                if (writeoob) {
> -                       /* Read OOB data from input file, exit on failure */
> -                       if ((cnt = read(ifd, oobreadbuf, meminfo.oobsize)) != meminfo.oobsize) {
> -                               perror ("File I/O error on input file");
> -                               goto closeall;
> +                       int tinycnt = 0;
> +
> +                       while(tinycnt < readlen) {
> +                               cnt = read(ifd, oobreadbuf + tinycnt, meminfo.oobsize - tinycnt);
> +                               if (cnt == 0) { // EOF
> +                                       break;
> +                               } else if (cnt < 0) {
> +                                       perror ("File I/O error on input file");
> +                                       goto closeall;
> +                               }
> +                               tinycnt += cnt;
>                        }
> +
>                        if (!noecc) {
>                                int i, start, len;
>                                /*
> --
> 1.5.4.3
>
> --
> Best regards,
> Artem Bityutskiy (Битюцкий Артём)
>
>



-- 
Zaar

Re: [PATCH] nandwrite - handle situation when read returns less bytes the expected

[Artem Bityutskiy]

On Tue, 2008-12-16 at 10:40 +0200, Hai Zaar wrote:
> On Tue, Dec 16, 2008 at 8:28 AM, Artem Bityutskiy
> <dedekind@infradead.org> wrote:
> >
> > Your patch does not apply. It looks like it is not against the latest
> > mtd-utils. How about this patch?
> Yes, my patch is against mtd-utils-1.2.0. Yours is surely better since
> it covers oob reading as well.

Would be nice if you tried my patch and confirmed it works, because I
did not test it.

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Subject: [PATCH] nandwrite: correct data reading

The "read" syscall does not necessarily return all the requested
data, in which case the caller has to try again and read more.
Take this into account when reading input data.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 nandwrite.c |   32 +++++++++++++++++++++-----------
 1 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index fc23e85..b4bc871 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -262,7 +262,6 @@ int main(int argc, char * const argv[])
 	struct nand_oobinfo old_oobinfo;
 
 	process_options(argc, argv);
-
 	erase_buffer(oobbuf, sizeof(oobbuf));
 
 	if (pad && writeoob) {
@@ -438,6 +437,8 @@ int main(int argc, char * const argv[])
 	 * length or zero.
 	 */
 	while (imglen && (mtdoffset < meminfo.size)) {
+		int tinycnt = 0;
+
 		// new eraseblock , check for bad block(s)
 		// Stay in the loop to be sure if the mtdoffset changes because
 		// of a bad block, that the next block that will be written to
@@ -484,15 +485,17 @@ int main(int argc, char * const argv[])
 			}
 
 			/* Read Page Data from input file */
-			if ((cnt = read(ifd, writebuf, readlen)) != readlen) {
-				if (cnt == 0)	// EOF
+			while(tinycnt < readlen) {
+				cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
+				if (cnt == 0) { // EOF
 					break;
-				perror ("File I/O error on input file");
-				goto closeall;
+				} else if (cnt < 0) {
+					perror ("File I/O error on input file");
+					goto closeall;
+				}
+				tinycnt += cnt;
 			}
 		} else {
-			int tinycnt = 0;
-
 			while(tinycnt < readlen) {
 				cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
 				if (cnt == 0) { // EOF
@@ -522,11 +525,18 @@ int main(int argc, char * const argv[])
 		}
 
 		if (writeoob) {
-			/* Read OOB data from input file, exit on failure */
-			if ((cnt = read(ifd, oobreadbuf, meminfo.oobsize)) != meminfo.oobsize) {
-				perror ("File I/O error on input file");
-				goto closeall;
+			tinycnt = 0;
+			while(tinycnt < readlen) {
+				cnt = read(ifd, oobreadbuf + tinycnt, meminfo.oobsize - tinycnt);
+				if (cnt == 0) { // EOF
+					break;
+				} else if (cnt < 0) {
+					perror ("File I/O error on input file");
+					goto closeall;
+				}
+				tinycnt += cnt;
 			}
+
 			if (!noecc) {
 				int i, start, len;
 				/*
-- 
1.5.4.3

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH] nandwrite - handle situation when read returns less bytes the expected

[Hai Zaar]

On Tue, Dec 16, 2008 at 12:18 PM, Artem Bityutskiy
<dedekind@infradead.org> wrote:
> On Tue, 2008-12-16 at 10:40 +0200, Hai Zaar wrote:
>> On Tue, Dec 16, 2008 at 8:28 AM, Artem Bityutskiy
>> <dedekind@infradead.org> wrote:
>> >
>> > Your patch does not apply. It looks like it is not against the latest
>> > mtd-utils. How about this patch?
>> Yes, my patch is against mtd-utils-1.2.0. Yours is surely better since
>> it covers oob reading as well.
>
> Would be nice if you tried my patch and confirmed it works, because I
> did not test it.
I've tried the patch with the latest git version of mtd-utils and I
confirm that it works on ARM at91sam9260 board.
Thanks!

>
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> Subject: [PATCH] nandwrite: correct data reading
>
> The "read" syscall does not necessarily return all the requested
> data, in which case the caller has to try again and read more.
> Take this into account when reading input data.
>
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> ---
>  nandwrite.c |   32 +++++++++++++++++++++-----------
>  1 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/nandwrite.c b/nandwrite.c
> index fc23e85..b4bc871 100644
> --- a/nandwrite.c
> +++ b/nandwrite.c
> @@ -262,7 +262,6 @@ int main(int argc, char * const argv[])
>        struct nand_oobinfo old_oobinfo;
>
>        process_options(argc, argv);
> -
>        erase_buffer(oobbuf, sizeof(oobbuf));
>
>        if (pad && writeoob) {
> @@ -438,6 +437,8 @@ int main(int argc, char * const argv[])
>         * length or zero.
>         */
>        while (imglen && (mtdoffset < meminfo.size)) {
> +               int tinycnt = 0;
> +
>                // new eraseblock , check for bad block(s)
>                // Stay in the loop to be sure if the mtdoffset changes because
>                // of a bad block, that the next block that will be written to
> @@ -484,15 +485,17 @@ int main(int argc, char * const argv[])
>                        }
>
>                        /* Read Page Data from input file */
> -                       if ((cnt = read(ifd, writebuf, readlen)) != readlen) {
> -                               if (cnt == 0)   // EOF
> +                       while(tinycnt < readlen) {
> +                               cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
> +                               if (cnt == 0) { // EOF
>                                        break;
> -                               perror ("File I/O error on input file");
> -                               goto closeall;
> +                               } else if (cnt < 0) {
> +                                       perror ("File I/O error on input file");
> +                                       goto closeall;
> +                               }
> +                               tinycnt += cnt;
>                        }
>                } else {
> -                       int tinycnt = 0;
> -
>                        while(tinycnt < readlen) {
>                                cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
>                                if (cnt == 0) { // EOF
> @@ -522,11 +525,18 @@ int main(int argc, char * const argv[])
>                }
>
>                if (writeoob) {
> -                       /* Read OOB data from input file, exit on failure */
> -                       if ((cnt = read(ifd, oobreadbuf, meminfo.oobsize)) != meminfo.oobsize) {
> -                               perror ("File I/O error on input file");
> -                               goto closeall;
> +                       tinycnt = 0;
> +                       while(tinycnt < readlen) {
> +                               cnt = read(ifd, oobreadbuf + tinycnt, meminfo.oobsize - tinycnt);
> +                               if (cnt == 0) { // EOF
> +                                       break;
> +                               } else if (cnt < 0) {
> +                                       perror ("File I/O error on input file");
> +                                       goto closeall;
> +                               }
> +                               tinycnt += cnt;
>                        }
> +
>                        if (!noecc) {
>                                int i, start, len;
>                                /*
> --
> 1.5.4.3
>
> --
> Best regards,
> Artem Bityutskiy (Битюцкий Артём)
>
>



-- 
Zaar

Re: [PATCH] nandwrite - handle situation when read returns less bytes the expected

[Artem Bityutskiy]

On Wed, 2008-12-24 at 16:48 +0200, Hai Zaar wrote:
> > Would be nice if you tried my patch and confirmed it works, because I
> > did not test it.
> I've tried the patch with the latest git version of mtd-utils and I
> confirm that it works on ARM at91sam9260 board.
> Thanks!

Pushed, thank you.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)