* [PATCH 1/3] ieee1275: ofdisk dangling pointer
@ 2015-10-26 21:43 Eric Snowberg
2015-10-26 21:43 ` [PATCH 2/3] ieee1275: ofdisk memory leak Eric Snowberg
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Eric Snowberg @ 2015-10-26 21:43 UTC (permalink / raw)
To: grub-devel, eric.snowberg; +Cc: Eric Snowberg
Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 -
"Don't continue to query block-size if disk doesn't have it.”
a dangling pointer was introduced.
Fix dangling pointer issue in grub_ofdisk_open where devpath is freed
and then used again within the call to grub_ofdisk_get_block_size. This
solves many memory corruption issues we were seeing.
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
grub-core/disk/ieee1275/ofdisk.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
index 331769b..4a5632c 100644
--- a/grub-core/disk/ieee1275/ofdisk.c
+++ b/grub-core/disk/ieee1275/ofdisk.c
@@ -422,10 +422,11 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
op = ofdisk_hash_find (devpath);
if (!op)
op = ofdisk_hash_add (devpath, NULL);
- else
- grub_free (devpath);
if (!op)
- return grub_errno;
+ {
+ grub_free (devpath);
+ return grub_errno;
+ }
disk->id = (unsigned long) op;
disk->data = op->open_path;
--
1.7.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/3] ieee1275: ofdisk memory leak 2015-10-26 21:43 [PATCH 1/3] ieee1275: ofdisk dangling pointer Eric Snowberg @ 2015-10-26 21:43 ` Eric Snowberg 2015-10-26 22:03 ` Vladimir 'φ-coder/phcoder' Serbinenko 2015-10-26 21:43 ` [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it Eric Snowberg ` (2 subsequent siblings) 3 siblings, 1 reply; 9+ messages in thread From: Eric Snowberg @ 2015-10-26 21:43 UTC (permalink / raw) To: grub-devel, eric.snowberg; +Cc: Eric Snowberg Fix memory leak added within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue to query block-size if disk doesn't have it.” Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> --- grub-core/disk/ieee1275/ofdisk.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c index 4a5632c..297f058 100644 --- a/grub-core/disk/ieee1275/ofdisk.c +++ b/grub-core/disk/ieee1275/ofdisk.c @@ -432,7 +432,10 @@ grub_ofdisk_open (const char *name, grub_disk_t disk) err = grub_ofdisk_get_block_size (devpath, &block_size, op); if (err) - return err; + { + grub_free (devpath); + return err; + } if (block_size != 0) { for (disk->log_sector_size = 0; @@ -443,6 +446,7 @@ grub_ofdisk_open (const char *name, grub_disk_t disk) disk->log_sector_size = 9; } + grub_free (devpath); return 0; } -- 1.7.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] ieee1275: ofdisk memory leak 2015-10-26 21:43 ` [PATCH 2/3] ieee1275: ofdisk memory leak Eric Snowberg @ 2015-10-26 22:03 ` Vladimir 'φ-coder/phcoder' Serbinenko 0 siblings, 0 replies; 9+ messages in thread From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2015-10-26 22:03 UTC (permalink / raw) To: The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 1133 bytes --] On 26.10.2015 22:43, Eric Snowberg wrote: > Fix memory leak added within commit: > 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue to > query block-size if disk doesn't have it.” > Committed, thanks > Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> > --- > grub-core/disk/ieee1275/ofdisk.c | 6 +++++- > 1 files changed, 5 insertions(+), 1 deletions(-) > > diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c > index 4a5632c..297f058 100644 > --- a/grub-core/disk/ieee1275/ofdisk.c > +++ b/grub-core/disk/ieee1275/ofdisk.c > @@ -432,7 +432,10 @@ grub_ofdisk_open (const char *name, grub_disk_t disk) > > err = grub_ofdisk_get_block_size (devpath, &block_size, op); > if (err) > - return err; > + { > + grub_free (devpath); > + return err; > + } > if (block_size != 0) > { > for (disk->log_sector_size = 0; > @@ -443,6 +446,7 @@ grub_ofdisk_open (const char *name, grub_disk_t disk) > disk->log_sector_size = 9; > } > > + grub_free (devpath); > return 0; > } > > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 213 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it 2015-10-26 21:43 [PATCH 1/3] ieee1275: ofdisk dangling pointer Eric Snowberg 2015-10-26 21:43 ` [PATCH 2/3] ieee1275: ofdisk memory leak Eric Snowberg @ 2015-10-26 21:43 ` Eric Snowberg 2015-10-26 22:02 ` Vladimir 'φ-coder/phcoder' Serbinenko 2015-10-26 22:03 ` [PATCH 1/3] ieee1275: ofdisk dangling pointer Vladimir 'φ-coder/phcoder' Serbinenko 2015-10-29 14:48 ` Daniel Kiper 3 siblings, 1 reply; 9+ messages in thread From: Eric Snowberg @ 2015-10-26 21:43 UTC (permalink / raw) To: grub-devel, eric.snowberg; +Cc: Eric Snowberg Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue to query block-size if disk doesn't have it.” Disks that returned 0 to the block-size query, still get queried every time. Fix logic in grub_ofdisk_get_block_size so the block size is not requested upon each open since it will not change. Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> --- grub-core/disk/ieee1275/ofdisk.c | 30 +++++++++++++++++++++++------- 1 files changed, 23 insertions(+), 7 deletions(-) diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c index 297f058..a75ea51 100644 --- a/grub-core/disk/ieee1275/ofdisk.c +++ b/grub-core/disk/ieee1275/ofdisk.c @@ -35,7 +35,8 @@ struct ofdisk_hash_ent char *grub_devpath; int is_boot; int is_removable; - int block_size_fails; + int block_size_retries; + grub_uint32_t block_size; /* Pointer to shortest available name on nodes representing canonical names, otherwise NULL. */ const char *shortest; @@ -446,7 +447,17 @@ grub_ofdisk_open (const char *name, grub_disk_t disk) disk->log_sector_size = 9; } + if (last_ihandle) + grub_ieee1275_close (last_ihandle); + + last_ihandle = 0; + last_devpath = NULL; + grub_ieee1275_open (devpath, &last_ihandle); grub_free (devpath); + + if (! last_ihandle) + return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device"); + return 0; } @@ -619,6 +630,12 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size, grub_ieee1275_cell_t size2; } args_ieee1275; + if ((op->block_size_retries >= 2) || (op->block_size > 0)) + { + *block_size = op->block_size; + return GRUB_ERR_NONE; + } + if (last_ihandle) grub_ieee1275_close (last_ihandle); @@ -630,9 +647,7 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size, return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device"); *block_size = 0; - - if (op->block_size_fails >= 2) - return GRUB_ERR_NONE; + op->block_size_retries++; INIT_IEEE1275_COMMON (&args_ieee1275.common, "call-method", 2, 2); args_ieee1275.method = (grub_ieee1275_cell_t) "block-size"; @@ -642,21 +657,22 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size, if (IEEE1275_CALL_ENTRY_FN (&args_ieee1275) == -1) { grub_dprintf ("disk", "can't get block size: failed call-method\n"); - op->block_size_fails++; } else if (args_ieee1275.result) { grub_dprintf ("disk", "can't get block size: %lld\n", (long long) args_ieee1275.result); - op->block_size_fails++; } else if (args_ieee1275.size1 && !(args_ieee1275.size1 & (args_ieee1275.size1 - 1)) && args_ieee1275.size1 >= 512 && args_ieee1275.size1 <= 16384) { - op->block_size_fails = 0; + op->block_size = args_ieee1275.size1; *block_size = args_ieee1275.size1; } + grub_ieee1275_close (last_ihandle); + last_ihandle = 0; + return 0; } -- 1.7.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it 2015-10-26 21:43 ` [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it Eric Snowberg @ 2015-10-26 22:02 ` Vladimir 'φ-coder/phcoder' Serbinenko 2015-11-10 8:01 ` Andrei Borzenkov 0 siblings, 1 reply; 9+ messages in thread From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2015-10-26 22:02 UTC (permalink / raw) To: The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 3465 bytes --] On 26.10.2015 22:43, Eric Snowberg wrote: > Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue > to query block-size if disk doesn't have it.” Disks that returned 0 to the > block-size query, still get queried every time. > > Fix logic in grub_ofdisk_get_block_size so the block size is not requested > upon each open since it will not change. > Is it true for removable disks as well? What about someone unplugging USB stick and plugging-in 4K USB HDD? > Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> > --- > grub-core/disk/ieee1275/ofdisk.c | 30 +++++++++++++++++++++++------- > 1 files changed, 23 insertions(+), 7 deletions(-) > > diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c > index 297f058..a75ea51 100644 > --- a/grub-core/disk/ieee1275/ofdisk.c > +++ b/grub-core/disk/ieee1275/ofdisk.c > @@ -35,7 +35,8 @@ struct ofdisk_hash_ent > char *grub_devpath; > int is_boot; > int is_removable; > - int block_size_fails; > + int block_size_retries; > + grub_uint32_t block_size; > /* Pointer to shortest available name on nodes representing canonical names, > otherwise NULL. */ > const char *shortest; > @@ -446,7 +447,17 @@ grub_ofdisk_open (const char *name, grub_disk_t disk) > disk->log_sector_size = 9; > } > > + if (last_ihandle) > + grub_ieee1275_close (last_ihandle); > + > + last_ihandle = 0; > + last_devpath = NULL; > + grub_ieee1275_open (devpath, &last_ihandle); > grub_free (devpath); > + > + if (! last_ihandle) > + return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device"); > + > return 0; > } > > @@ -619,6 +630,12 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size, > grub_ieee1275_cell_t size2; > } args_ieee1275; > > + if ((op->block_size_retries >= 2) || (op->block_size > 0)) > + { > + *block_size = op->block_size; > + return GRUB_ERR_NONE; > + } > + > if (last_ihandle) > grub_ieee1275_close (last_ihandle); > > @@ -630,9 +647,7 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size, > return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device"); > > *block_size = 0; > - > - if (op->block_size_fails >= 2) > - return GRUB_ERR_NONE; > + op->block_size_retries++; > > INIT_IEEE1275_COMMON (&args_ieee1275.common, "call-method", 2, 2); > args_ieee1275.method = (grub_ieee1275_cell_t) "block-size"; > @@ -642,21 +657,22 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size, > if (IEEE1275_CALL_ENTRY_FN (&args_ieee1275) == -1) > { > grub_dprintf ("disk", "can't get block size: failed call-method\n"); > - op->block_size_fails++; > } > else if (args_ieee1275.result) > { > grub_dprintf ("disk", "can't get block size: %lld\n", > (long long) args_ieee1275.result); > - op->block_size_fails++; > } > else if (args_ieee1275.size1 > && !(args_ieee1275.size1 & (args_ieee1275.size1 - 1)) > && args_ieee1275.size1 >= 512 && args_ieee1275.size1 <= 16384) > { > - op->block_size_fails = 0; > + op->block_size = args_ieee1275.size1; > *block_size = args_ieee1275.size1; > } > > + grub_ieee1275_close (last_ihandle); > + last_ihandle = 0; > + > return 0; > } > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 213 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it 2015-10-26 22:02 ` Vladimir 'φ-coder/phcoder' Serbinenko @ 2015-11-10 8:01 ` Andrei Borzenkov 2015-11-10 17:45 ` Eric Snowberg 0 siblings, 1 reply; 9+ messages in thread From: Andrei Borzenkov @ 2015-11-10 8:01 UTC (permalink / raw) To: The development of GNU GRUB On Tue, Oct 27, 2015 at 1:02 AM, Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> wrote: > On 26.10.2015 22:43, Eric Snowberg wrote: >> Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue >> to query block-size if disk doesn't have it.” Disks that returned 0 to the >> block-size query, still get queried every time. >> >> Fix logic in grub_ofdisk_get_block_size so the block size is not requested >> upon each open since it will not change. >> > Is it true for removable disks as well? What about someone unplugging > USB stick and plugging-in 4K USB HDD? Does OBP alone have hotplug support until OS is running? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it 2015-11-10 8:01 ` Andrei Borzenkov @ 2015-11-10 17:45 ` Eric Snowberg 0 siblings, 0 replies; 9+ messages in thread From: Eric Snowberg @ 2015-11-10 17:45 UTC (permalink / raw) To: The development of GNU GRUB > On Nov 10, 2015, at 1:01 AM, Andrei Borzenkov <arvidjaar@gmail.com> wrote: > > On Tue, Oct 27, 2015 at 1:02 AM, Vladimir 'φ-coder/phcoder' Serbinenko > <phcoder@gmail.com> wrote: >> On 26.10.2015 22:43, Eric Snowberg wrote: >>> Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue >>> to query block-size if disk doesn't have it.” Disks that returned 0 to the >>> block-size query, still get queried every time. >>> >>> Fix logic in grub_ofdisk_get_block_size so the block size is not requested >>> upon each open since it will not change. >>> >> Is it true for removable disks as well? What about someone unplugging >> USB stick and plugging-in 4K USB HDD? > > Does OBP alone have hotplug support until OS is running? AFAIK, SPARC OBP does not have hot plug support. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] ieee1275: ofdisk dangling pointer 2015-10-26 21:43 [PATCH 1/3] ieee1275: ofdisk dangling pointer Eric Snowberg 2015-10-26 21:43 ` [PATCH 2/3] ieee1275: ofdisk memory leak Eric Snowberg 2015-10-26 21:43 ` [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it Eric Snowberg @ 2015-10-26 22:03 ` Vladimir 'φ-coder/phcoder' Serbinenko 2015-10-29 14:48 ` Daniel Kiper 3 siblings, 0 replies; 9+ messages in thread From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2015-10-26 22:03 UTC (permalink / raw) To: The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 1267 bytes --] On 26.10.2015 22:43, Eric Snowberg wrote: > Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 - > "Don't continue to query block-size if disk doesn't have it.” > a dangling pointer was introduced. > > Fix dangling pointer issue in grub_ofdisk_open where devpath is freed > and then used again within the call to grub_ofdisk_get_block_size. This > solves many memory corruption issues we were seeing. > Committed, thanks > Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> > --- > grub-core/disk/ieee1275/ofdisk.c | 7 ++++--- > 1 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c > index 331769b..4a5632c 100644 > --- a/grub-core/disk/ieee1275/ofdisk.c > +++ b/grub-core/disk/ieee1275/ofdisk.c > @@ -422,10 +422,11 @@ grub_ofdisk_open (const char *name, grub_disk_t disk) > op = ofdisk_hash_find (devpath); > if (!op) > op = ofdisk_hash_add (devpath, NULL); > - else > - grub_free (devpath); > if (!op) > - return grub_errno; > + { > + grub_free (devpath); > + return grub_errno; > + } > disk->id = (unsigned long) op; > disk->data = op->open_path; > > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 213 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] ieee1275: ofdisk dangling pointer 2015-10-26 21:43 [PATCH 1/3] ieee1275: ofdisk dangling pointer Eric Snowberg ` (2 preceding siblings ...) 2015-10-26 22:03 ` [PATCH 1/3] ieee1275: ofdisk dangling pointer Vladimir 'φ-coder/phcoder' Serbinenko @ 2015-10-29 14:48 ` Daniel Kiper 3 siblings, 0 replies; 9+ messages in thread From: Daniel Kiper @ 2015-10-29 14:48 UTC (permalink / raw) To: The development of GNU GRUB Cc: john.haxby, jose.marchesi, karl.volz, allen.pais On Mon, Oct 26, 2015 at 05:43:36PM -0400, Eric Snowberg wrote: > Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 - > "Don't continue to query block-size if disk doesn't have it.??? > a dangling pointer was introduced. > > Fix dangling pointer issue in grub_ofdisk_open where devpath is freed > and then used again within the call to grub_ofdisk_get_block_size. This > solves many memory corruption issues we were seeing. > > Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> Please CC me too if you repost patches on grub-devel. Daniel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-11-10 17:44 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-26 21:43 [PATCH 1/3] ieee1275: ofdisk dangling pointer Eric Snowberg 2015-10-26 21:43 ` [PATCH 2/3] ieee1275: ofdisk memory leak Eric Snowberg 2015-10-26 22:03 ` Vladimir 'φ-coder/phcoder' Serbinenko 2015-10-26 21:43 ` [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it Eric Snowberg 2015-10-26 22:02 ` Vladimir 'φ-coder/phcoder' Serbinenko 2015-11-10 8:01 ` Andrei Borzenkov 2015-11-10 17:45 ` Eric Snowberg 2015-10-26 22:03 ` [PATCH 1/3] ieee1275: ofdisk dangling pointer Vladimir 'φ-coder/phcoder' Serbinenko 2015-10-29 14:48 ` Daniel Kiper
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.