part_dos: allocate sector buffer dynamically

PATCH New Sergei Shtylyov 3 participants 6 messages Apr 12, 2011 – May 12, 2011

Share & tools

Sergei Shtylyov

author

2011-04-12 19:23 UTC #0 lore raw

PATCH 1/1 part_dos: allocate sector buffer dynamically

Apple iPod nanos have sector sizes of 2 or 4 KiB, which crashes U-Boot when it tries to read the MBR into 512-byte buffer situated on stack. Instead allocate this buffer dynamically to be safe with any large sector size.

Signed-off-by Sergei Shtylyov <[email protected]>

---
The same change is probably needed for disk/part_amiga.c but I'm not really sure if Amiga supports USB... :-)

 disk/part_dos.c |   59 +++++++++++++++++++++++++++++++++++++++-----------------
 disk/part_dos.h |    7 ------
 2 files changed, 42 insertions(+), 24 deletions(-)

disk/part_dos.c +42 ~~-17~~

Index: u-boot/disk/part_dos.c
===================================================================
@@ -32,6 +32,7 @@
 
 #include <common.h>
 #include <command.h>
+#include <exports.h>
 #include <ide.h>
 #include "part_dos.h"

@@ -87,14 +88,20 @@ static int test_block_type(unsigned char
 
 int test_part_dos (block_dev_desc_t *dev_desc)
 {
-	unsigned char buffer[DEFAULT_SECTOR_SIZE];
+	unsigned char *buffer;
+
+	buffer = malloc(dev_desc->blksz);
+	if (!buffer)
+		return -1;
 
 	if ((dev_desc->block_read(dev_desc->dev, 0, 1, (ulong *) buffer) != 1) ||
 	    (buffer[DOS_PART_MAGIC_OFFSET + 0] != 0x55) ||
 	    (buffer[DOS_PART_MAGIC_OFFSET + 1] != 0xaa) ) {
-		return (-1);
+		free(buffer);
+		return -1;
 	}
-	return (0);
+	free(buffer);
+	return 0;
 }
 
 /*  Print a partition that is relative to its Extended partition table

@@ -102,26 +109,32 @@ int test_part_dos (block_dev_desc_t *dev
 static void print_partition_extended (block_dev_desc_t *dev_desc, int ext_part_sector, int relative,
 							   int part_num)
 {
-	unsigned char buffer[DEFAULT_SECTOR_SIZE];
+	unsigned char *buffer;
 	dos_partition_t *pt;
 	int i;
 
+	buffer = malloc(dev_desc->blksz);
+	if (!buffer) {
+		printf ("** Can't allocate buffer **\n");
+		return;
+	}
+
 	if (dev_desc->block_read(dev_desc->dev, ext_part_sector, 1, (ulong *) buffer) != 1) {
 		printf ("** Can't read partition table on %d:%d **\n",
 			dev_desc->dev, ext_part_sector);
-		return;
+		goto exit;
 	}
 	i=test_block_type(buffer);
 	if(i==-1) {
 		printf ("bad MBR sector signature 0x%02x%02x\n",
 			buffer[DOS_PART_MAGIC_OFFSET],
 			buffer[DOS_PART_MAGIC_OFFSET + 1]);
-		return;
+		goto exit;
 	}
 	if(i==DOS_PBR) {
 		printf ("    1\t\t         0\t%10ld\t%2x\n",
 			dev_desc->lba, buffer[DOS_PBR_MEDIA_TYPE_OFFSET]);
-		return;
+		goto exit;
 	}
 	/* Print all primary/logical partitions */
 	pt = (dos_partition_t *) (buffer + DOS_PART_TBL_OFFSET);

@@ -156,7 +169,8 @@ static void print_partition_extended (bl
 		}
 	}
 
-	return;
+exit:
+	free(buffer);
 }

@@ -166,21 +180,27 @@ static int get_partition_info_extended (
 				 int relative, int part_num,
 				 int which_part, disk_partition_t *info)
 {
-	unsigned char buffer[DEFAULT_SECTOR_SIZE];
+	unsigned char *buffer;
 	dos_partition_t *pt;
-	int i;
+	int i, result = -1;
+
+	buffer = malloc(dev_desc->blksz);
+	if (!buffer) {
+		printf ("** Can't allocate buffer **\n");
+		return -1;
+	}
 
 	if (dev_desc->block_read (dev_desc->dev, ext_part_sector, 1, (ulong *) buffer) != 1) {
 		printf ("** Can't read partition table on %d:%d **\n",
 			dev_desc->dev, ext_part_sector);
-		return -1;
+		goto exit;
 	}
 	if (buffer[DOS_PART_MAGIC_OFFSET] != 0x55 ||
 		buffer[DOS_PART_MAGIC_OFFSET + 1] != 0xaa) {
 		printf ("bad MBR sector signature 0x%02x%02x\n",
 			buffer[DOS_PART_MAGIC_OFFSET],
 			buffer[DOS_PART_MAGIC_OFFSET + 1]);
-		return -1;
+		goto exit;
 	}
 
 	/* Print all primary/logical partitions */

@@ -223,7 +243,8 @@ static int get_partition_info_extended (
 			}
 			/* sprintf(info->type, "%d, pt->sys_ind); */
 			sprintf ((char *)info->type, "U-Boot");
-			return 0;
+			result = 0;
+			goto exit;
 		}
 
 		/* Reverse engr the fdisk part# assignment rule! */

@@ -239,12 +260,16 @@ static int get_partition_info_extended (
 		if (is_extended (pt->sys_ind)) {
 			int lba_start = le32_to_int (pt->start4) + relative;
 
-			return get_partition_info_extended (dev_desc, lba_start,
-				 ext_part_sector == 0 ? lba_start : relative,
-				 part_num, which_part, info);
+			result = get_partition_info_extended(dev_desc,
+					lba_start, ext_part_sector == 0 ?
+						lba_start : relative,
+					part_num, which_part, info);
+			goto exit;
 		}
 	}
-	return -1;
+exit:
+	free(buffer);
+	return result;
 }
 
 void print_part_dos (block_dev_desc_t *dev_desc)

disk/part_dos.h +0 -7

Index: u-boot/disk/part_dos.h
===================================================================
@@ -25,13 +25,6 @@
 #define _DISK_PART_DOS_H
 
 
-#ifdef CONFIG_ISO_PARTITION
-/* Make the buffers bigger if ISO partition support is enabled -- CD-ROMS
-   have 2048 byte blocks */
-#define DEFAULT_SECTOR_SIZE	2048
-#else
-#define DEFAULT_SECTOR_SIZE	512
-#endif
 #define DOS_PART_TBL_OFFSET	0x1be
 #define DOS_PART_MAGIC_OFFSET	0x1fe
 #define DOS_PBR_FSTYPE_OFFSET	0x36

Wolfgang Denk

2011-04-30 19:14 UTC #1 lore raw

PATCH 1/1 part_dos: allocate sector buffer dynamically

Dear Sergei Shtylyov,

Quoted 5 lines · In message <[email protected]> you wrote:

Apple iPod nanos have sector sizes of 2 or 4 KiB, which crashes U-Boot when it tries to read the MBR into 512-byte buffer situated on stack. Instead allocate this buffer dynamically to be safe with any large sector size.
Signed-off-by Sergei Shtylyov <[email protected]>

Can we please keep the buffer on the stack as before? There is no need to use malloc() here. It just makes the code slower and more complicated and error prone without neeed.

Best regards,

Wolfgang Denk

Signature · 6 lines

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
"Where shall I begin, please your Majesty?" he asked. "Begin  at  the
beginning,"  the  King said, gravely, "and go on till you come to the
end: then stop."    - Alice's Adventures in Wonderland, Lewis Carroll

Sergei Shtylyov

2011-05-03 12:20 UTC #2 lore raw

PATCH 1/1 part_dos: allocate sector buffer dynamically

Hello.

Quoted 7 lines · Wolfgang Denk

Quoted 5 lines
Apple iPod nanos have sector sizes of 2 or 4 KiB, which crashes U-Boot when it tries to read the MBR into 512-byte buffer situated on stack. Instead allocate this buffer dynamically to be safe with any large sector size.
Signed-off-by Sergei Shtylyov<[email protected]>
Can we please keep the buffer on the stack as before?

It will be unsafe. We can't really predict the size of the buffer (unless
we postulate that the buffer size won't ever exceed e.g. 4K).

Quoted 1 line

There is no need to use malloc() here.

No, there *is* a need I think.

Quoted 2 lines

It just makes the code slower and more
complicated and error prone without neeed.

I think using stack variables makes the code much more error prone, to the
point that U-Boot just crashes when the sector size happens to exceed our buffer size.

Quoted 3 lines

Best regards,
Wolfgang Denk

WBR, Sergei

Wolfgang Denk

2011-05-03 12:34 UTC #3 lore raw

PATCH 1/1 part_dos: allocate sector buffer dynamically

Dear Sergei Shtylyov,

Quoted 5 lines · In message <[email protected]> you wrote:

Quoted 2 lines
Can we please keep the buffer on the stack as before?
It will be unsafe. We can't really predict the size of the buffer (unless
we postulate that the buffer size won't ever exceed e.g. 4K).

In which way will a buffer allocated on the stack be less safe than one allocated using malloc()? Changes to do things wrong (like forgetting to free the array on return or freeing a bad pointer) are much higher with malloc(), it seems.

Quoted 3 lines

I think using stack variables makes the code much more error prone, to the
point that U-Boot just crashes when the sector size happens to exceed our buffer size.

This statement makes no sense to me. Wether the sector size exceeds the buffer size or not is in no way dependent on where or how you allocate the buffer - be it on the stack or using malloc().

Umm... you _are_ aware that you can put dynamically sized arrays on the stack, aren't you?

Best regards,

Wolfgang Denk

Signature · 4 lines

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
Disc space - the final frontier!

Sergei Shtylyov

2011-05-03 12:50 UTC #4 lore raw

PATCH 1/1 part_dos: allocate sector buffer dynamically

Hello.

Quoted 2 lines · Wolfgang Denk

Umm... you _are_ aware that you can put dynamically sized arrays on the stack, aren't you?

No, it seems I'm not. Is it a standard C now?

Quoted 3 lines

Best regards,
Wolfgang Denk

WBR, Sergei

Wolfgang Denk

2011-05-12 17:35 UTC #5 lore raw

PATCH 1/1 part_dos: allocate sector buffer dynamically

Dear Sergei Shtylyov,

Quoted 5 lines · In message <[email protected]> you wrote:

Quoted 3 lines
Umm... you _are_ aware that you can put dynamically sized arrays on the stack, aren't you?
No, it seems I'm not. Is it a standard C now?

It's been a GCC extension forever (well, I have to admit that I don't remember before GCC v1.39 or so).

Best regards,

Wolfgang Denk

Signature · 6 lines

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
When a man sits with a pretty girl for  an  hour,  it  seems  like  a
minute.  But let him sit on a hot stove for a minute -- and it's lon-
ger than any hour. That's relativity.              -- Albert Einstein