[parted-devel] [PATCH 04/14] do not discard bootcode from extended partition

[Joel Granados]

On Tue, Jun 09, 2009 at 11:06:28AM +0200, Jim Meyering wrote:
> Joel Granados wrote:
> 
> > On Tue, Jun 09, 2009 at 10:10:16AM +0200, Joel Granados wrote:
> >> On Fri, Jun 05, 2009 at 03:30:52PM +0200, Jim Meyering wrote:
> >> > Joel Granados Moreno wrote:
> >> > > From: Petr Uzel <petr.uzel at suse.cz>
> >> > >
> >> > > * libparted/labels/dos.c (write_ext_table): Do not discard
> >> > > bootcode from extended partition on msdos label when some of
> >> > > the logical partitions are changed.
> >> > >
> >> > > Signed-off-by: Petr Uzel <petr.uzel at suse.cz>
> >> > > ---
> >> > >  libparted/labels/dos.c |    6 ++++--
> >> > >  1 files changed, 4 insertions(+), 2 deletions(-)
> >> > >
> >> > > diff --git a/libparted/labels/dos.c b/libparted/labels/dos.c
> >> > > index f219e7d..4e308fe 100644
> >> > > --- a/libparted/labels/dos.c
> >> > > +++ b/libparted/labels/dos.c
> >> > > @@ -1060,7 +1060,8 @@ write_ext_table (const PedDisk* disk,
> >> > >
> >> > >  	lba_offset = ped_disk_extended_partition (disk)->geom.start;
> >> > >
> >> > > -	memset (&table, 0, sizeof (DosRawTable));
> >> > > +	ped_device_read (disk->dev, &table, sector, 1);
> >> > > +	memset (&(table.partitions), 0, 4 * sizeof(DosRawPartition));
> >> > >  	table.magic = PED_CPU_TO_LE16 (MSDOS_MAGIC);
> >> > >
> >> > >  	if (!fill_raw_part (&table.partitions[0], logical, sector))
> >> > > @@ -1094,7 +1095,8 @@ write_empty_table (const PedDisk* disk, PedSector sector)
> >> > >
> >> > >  	PED_ASSERT (disk != NULL, return 0);
> >> > >
> >> > > -	memset (&table, 0, sizeof (DosRawTable));
> >> > > +	ped_device_read (disk->dev, &table, sector, 1);
> >> > > +	memset (&(table.partitions), 0, 4 * sizeof(DosRawPartition));
> >> > >  	table.magic = PED_CPU_TO_LE16 (MSDOS_MAGIC);
> >> > >
> >> > >  	return ped_device_write (disk->dev, (void*) &table, sector, 1);
> >> >
> >> > This has the same problem I mentioned for 1/14:
> >> > it introduces new code that depends on fixed-size (512-byte) sectors.
> >> > Thus it conflicts with changes on "next" that eliminated the 512-byte
> >> > limitation.
> >> >
> >> > How about putting it on that branch instead?
> >>
> >>
> >> Same answer as above....
> >
> > Wait!!! How does this conflict with changes in the next branch?
> 
> Take a look, or try to merge and then run the tests,
> including with valgrind.
> 
> There may not be an official version-control conflict,
> but that change would certainly introduce a nasty bug.
> 
> Quick answer: table is declared like this:
> 
>     DosRawTable table;
> 
> yet your added ped_device_read call may write >512 bytes
> (i.e., sector size) into that buffer, thus clobbering the stack.

Aha.  I see,  I thought you were only refering to merge conflicts.

> 
> > Are you talking about your local changes?
> 
> Of course not.  Those would be private.  Besides, I have none.

:)

> 
> > If you are pls commit so I can
> > address this.  Additionally, how does this depend on the sector size?
> > Does the ped_device_read call change in your local next branch?  And
> 
> No, but it must work for sector sizes larger than 512 bytes.
> 
> > "memset (&(table.partitions), 0, 4 * sizeof(DosRawPartition))" will be
> > valid for all secto sizes.  I mean, the RawPartition definition will not
> > change even though the sector size does.

I'll let Ptr address this issue...


-- 
Joel Andres Granados
Brno, Czech Republic, Red Hat.