diff mbox

[U-Boot] part_dos: allocate sector buffer dynamically

Message ID 201104122323.59105.sshtylyov@ru.mvista.com
State Changes Requested
Headers show

Commit Message

Sergei Shtylyov April 12, 2011, 7:23 p.m. UTC 
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 <sshtylyov@ru.mvista.com>

---
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(-)

Comments

Wolfgang Denk April 30, 2011, 7:14 p.m. UTC | #1 
Dear Sergei Shtylyov,

In message <201104122323.59105.sshtylyov@ru.mvista.com> 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 <sshtylyov@ru.mvista.com>

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
Sergei Shtylyov May 3, 2011, 12:20 p.m. UTC | #2 
Hello.

On 30-04-2011 23:14, Wolfgang Denk 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<sshtylyov@ru.mvista.com>

> 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).

> There is no need to use malloc() here.

    No, there *is* a need I think.

> 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.

> Best regards,

> Wolfgang Denk

WBR, Sergei
Wolfgang Denk May 3, 2011, 12:34 p.m. UTC | #3 
Dear Sergei Shtylyov,

In message <4DBFF300.9010906@mvista.com> you wrote:
> 
> > 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.

>     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
Sergei Shtylyov May 3, 2011, 12:50 p.m. UTC | #4 
Hello.

On 03-05-2011 16:34, Wolfgang Denk wrote:

> 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?

> Best regards,

> Wolfgang Denk

WBR, Sergei
Wolfgang Denk May 12, 2011, 5:35 p.m. UTC | #5 
Dear Sergei Shtylyov,

In message <4DBFF9FD.1070907@mvista.com> you wrote:
> 
> > 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
diff mbox

Patch

Index: u-boot/disk/part_dos.c
===================================================================
--- u-boot.orig/disk/part_dos.c
+++ 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)
Index: u-boot/disk/part_dos.h
===================================================================
--- u-boot.orig/disk/part_dos.h
+++ 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