[U-Boot] [PATCH 3/4 v2] UBIFS: Change ubifsload to not read beyond the requested size

[U-Boot] [PATCH 3/4 v2] UBIFS: Change ubifsload to not read beyond the requested size

[Stefan Roese]

Until now ubifsload pads the destination with 0 up to a multiple of
UBIFS_BLOCK_SIZE (4KiB) while reading a file to memory. This patch
changes this behaviour to only read to the requested length. This
is either the file length or the length/size provided as parameter
to the ubifsload command.

Signed-off-by: Stefan Roese <sr@denx.de>
---
v2:
- Added error checking to malloc and read_block as suggested
  by Wolfgang
- Changed comment issues as suggested by Ben

 fs/ubifs/ubifs.c |   71 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
index 1cc31a9..d16d2b0 100644
--- a/fs/ubifs/ubifs.c
+++ b/fs/ubifs/ubifs.c
@@ -3,7 +3,7 @@
  *
  * Copyright (C) 2006-2008 Nokia Corporation.
  *
- * (C) Copyright 2008-2009
+ * (C) Copyright 2008-2010
  * Stefan Roese, DENX Software Engineering, sr at denx.de.
  *
  * This program is free software; you can redistribute it and/or modify it
@@ -567,7 +567,8 @@ dump:
 	return -EINVAL;
 }
 
-static int do_readpage(struct ubifs_info *c, struct inode *inode, struct page *page)
+static int do_readpage(struct ubifs_info *c, struct inode *inode,
+		       struct page *page, int last_block_size)
 {
 	void *addr;
 	int err = 0, i;
@@ -601,17 +602,54 @@ static int do_readpage(struct ubifs_info *c, struct inode *inode, struct page *p
 			err = -ENOENT;
 			memset(addr, 0, UBIFS_BLOCK_SIZE);
 		} else {
-			ret = read_block(inode, addr, block, dn);
-			if (ret) {
-				err = ret;
-				if (err != -ENOENT)
+			/*
+			 * Reading last block? Make sure to not write beyond
+			 * the requested size in the destination buffer.
+			 */
+			if (((block + 1) == beyond) || last_block_size) {
+				void *buff;
+				int dlen;
+
+				/*
+				 * We need to buffer the data locally for the
+				 * last block. This is to not pad the
+				 * destination area to a multiple of
+				 * UBIFS_BLOCK_SIZE.
+				 */
+				buff = malloc(UBIFS_BLOCK_SIZE);
+				if (!buff) {
+					printf("%s: Error, malloc fails!\n",
+					       __func__);
+					err = -ENOMEM;
 					break;
-			} else if (block + 1 == beyond) {
-				int dlen = le32_to_cpu(dn->size);
-				int ilen = i_size & (UBIFS_BLOCK_SIZE - 1);
-
-				if (ilen && ilen < dlen)
-					memset(addr + ilen, 0, dlen - ilen);
+				}
+
+				/* Read block-size into temp buffer */
+				ret = read_block(inode, buff, block, dn);
+				if (ret) {
+					err = ret;
+					if (err != -ENOENT) {
+						free(buff);
+						break;
+					}
+				}
+
+				if (last_block_size)
+					dlen = last_block_size;
+				else
+					dlen = le32_to_cpu(dn->size);
+
+				/* Now copy required size back to dest */
+				memcpy(addr, buff, dlen);
+
+				free(buff);
+			} else {
+				ret = read_block(inode, addr, block, dn);
+				if (ret) {
+					err = ret;
+					if (err != -ENOENT)
+						break;
+				}
 			}
 		}
 		if (++i >= UBIFS_BLOCKS_PER_PAGE)
@@ -649,6 +687,7 @@ int ubifs_load(char *filename, u32 addr, u32 size)
 	int err = 0;
 	int i;
 	int count;
+	int last_block_size = 0;
 
 	c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, UBI_READONLY);
 	/* ubifs_findfile will resolve symlinks, so we know that we get
@@ -684,7 +723,13 @@ int ubifs_load(char *filename, u32 addr, u32 size)
 	page.index = 0;
 	page.inode = inode;
 	for (i = 0; i < count; i++) {
-		err = do_readpage(c, inode, &page);
+		/*
+		 * Make sure to not read beyond the requested size
+		 */
+		if (((i + 1) == count) && (size < inode->i_size))
+			last_block_size = size - (i * PAGE_SIZE);
+
+		err = do_readpage(c, inode, &page, last_block_size);
 		if (err)
 			break;
 
-- 
1.7.3.2

[U-Boot] [PATCH 3/4 v2] UBIFS: Change ubifsload to not read beyond the requested size

[Wolfgang Denk]

Dear Stefan Roese,

In message <1288628880-6910-1-git-send-email-sr@denx.de> you wrote:
>
> +				/* Read block-size into temp buffer */
> +				ret = read_block(inode, buff, block, dn);
> +				if (ret) {
> +					err = ret;
> +					if (err != -ENOENT) {
> +						free(buff);
> +						break;
> +					}
> +				}
...
> +				ret = read_block(inode, addr, block, dn);
> +				if (ret) {
> +					err = ret;
> +					if (err != -ENOENT)
> +						break;
> +				}

I still wonder what's the logic behind this code. When will
read_block() return -ENOENT (aka "No such file or directory") ?
What are the other possible error conditions, and why would it make
sense to continue reading after these other errors?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You don't have to worry about me. I might have been born yesterday...
but I stayed up all night.

[U-Boot] [PATCH 3/4 v2] UBIFS: Change ubifsload to not read beyond the requested size

[Stefan Roese]

Hi Wolfgang,

sorry for the late reply. I just "stumbled" again over this mail.

On Monday 01 November 2010 20:05:00 Wolfgang Denk wrote:
> I still wonder what's the logic behind this code. When will
> read_block() return -ENOENT (aka "No such file or directory") ?
> What are the other possible error conditions, and why would it make
> sense to continue reading after these other errors?

As it seems, ENOENT is used to mark "a hole" in the file system. Meaning space 
that will be filled with zeros but does not occupy space (other than in the 
index). So we should keep the existing logic intact.

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

[U-Boot] [PATCH 3/4 v2] UBIFS: Change ubifsload to not read beyond the requested size

[Stefan Roese]

On Monday 01 November 2010 17:28:00 Stefan Roese wrote:
> Until now ubifsload pads the destination with 0 up to a multiple of
> UBIFS_BLOCK_SIZE (4KiB) while reading a file to memory. This patch
> changes this behaviour to only read to the requested length. This
> is either the file length or the length/size provided as parameter
> to the ubifsload command.

Applied to u-boot-ubi/master. Thanks.

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de