[Fwd: Re: Bug#495949: grub-common: grub-probe segfaults]

[Fwd: Re: Bug#495949: grub-common: grub-probe segfaults]

[Felix Zielcke]

And another bug forward
Anyone has an idea why a dm-crypt/lvm leads to a segfault in the strcmp
here:
 grub_partition_iterate (dest_dev->disk, (strcmp (dest_partmap, "pc_partition_map") ?
                                           find_usable_region_gpt : find_usable_region_msdos));


-------- Weitergeleitete Nachricht --------
> Von: Jean-Luc Coulon (f5ibh) <jean-luc.coulon@wanadoo.fr>
> An: Felix Zielcke <fzielcke@z-51.de>
> Kopie: 495949@bugs.debian.org
> Betreff: Re: Bug#495949: grub-common: grub-probe segfaults
> Datum: Fri, 24 Jul 2009 20:40:03 +0200
> 
> Hi Felix,
> 
> >> Jean-Luc
> >
> >Oh not good.
> >Then please do again a debug compile with `DEB_BUILD_OPTIONS="nostrip
> >noopt debug" dpkg-buildpackage -b'
> >and get a backtrace with gdb.
> >See [0] for my previous mail but note the spaces now in
> >DEB_BUILD_OPTIONS instead of the commas. I was wrong.
> >
> >[0] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=495949#30
> >
> 
> So, I rebuilt grub.
> Then as I don't know how to debug directly the shell script (grub-
> install), I laaunched it with set -x and got the failing line:
> 
> --->8---------->8------------->8----------->8------------>8----------
> ...
> + /usr/bin/grub-mkimage --output=/boot/grub/core.img '--prefix=(md0)/
> grub' biosdisk ext2 pc raid mdraid
> + /usr/sbin/grub-setup --force --directory=/boot/grub --device-map=/
> boot/grub/device.map /dev/md0
> ./grub-install: line 323:  4663 Erreur de segmentation  $grub_setup 
> ${setup_verbose} ${setup_force} --directory=${grubdir} --device-map=
> ${device_map} ${install_device}
> + exit 1
> --->8---------->8------------->8----------->8------------>8----------
> 
> So I launched gdb as follos, I don't know if it is the right process... 
> but I got a (small) backtrace):
> 
> --->8---------->8------------->8----------->8------------>8----------
> [jean-luc@tangerine] % sudio gdb --args /usr/sbin/grub-setup --force --
> directory=/boot/grub --device-map=/boot/grub/device.map /dev/md0
> GNU gdb (GDB) 6.8.50.20090628-cvs-debian
> Copyright (C) 2009 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/
> gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show 
> copying"
> and "show warranty" for details.
> This GDB was configured as "x86_64-linux-gnu".
> For bug reporting instructions, please see:
> <http://www.gnu.org/software/gdb/bugs/>...
> (gdb) run
> Starting program: /usr/sbin/grub-setup --force --directory=/boot/grub 
> --device-map=/boot/grub/device.map /dev/md0
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x00007ffff7b08c60 in strcmp () from /lib/libc.so.6
> (gdb) bt
> #0  0x00007ffff7b08c60 in strcmp () from /lib/libc.so.6
> #1  0x0000000000401cb1 in setup (dir=0x63e010 "/boot/grub", 
>     boot_file=0x4279f4 "boot.img", core_file=0x4279eb "core.img", 
>     root=0x64b9c0 "md0", dest=0x0, must_embed=1, force=1)
>     at /usr/local/src/tmp/grub/grub2-1.96+20090721/util/i386/pc/grub-
> setup.c:335
> #2  0x0000000000402cea in main (argc=5, argv=0x7fffffffe8a8)
>     at /usr/local/src/tmp/grub/grub2-1.96+20090721/util/i386/pc/grub-
> setup.c:774
> (gdb) 
> --->8---------->8------------->8----------->8------------>8----------
> 
> Best regards
> 
> Jean-Luc


-- 
Felix Zielcke

Re: [Fwd: Re: Bug#495949: grub-common: grub-probe segfaults]

[Pavel Roskin]

On Fri, 2009-07-24 at 20:46 +0200, Felix Zielcke wrote:
> And another bug forward
> Anyone has an idea why a dm-crypt/lvm leads to a segfault in the strcmp
> here:
>  grub_partition_iterate (dest_dev->disk, (strcmp (dest_partmap, "pc_partition_map") ?
>                                            find_usable_region_gpt : find_usable_region_msdos));

dest_partmap is only assigned a value in identify_partmap.  If
grub_partition_iterate() doesn't find any partitions, dest_partmap
remains a random pointer.

The fix would be probably to initialize dest_partmap with NULL.  If it
becomes "pc_partition_map", iterate with find_usable_region_msdos, if it
becomes "gpt_partition_map", iterate with find_usable_region_gpt.  If
it's NULL or another string, exit with a warning.

-- 
Regards,
Pavel Roskin

Re: [Fwd: Re: Bug#495949: grub-common: grub-probe segfaults]

[Pavel Roskin]

On Fri, 2009-07-24 at 16:09 -0400, Pavel Roskin wrote:
> On Fri, 2009-07-24 at 20:46 +0200, Felix Zielcke wrote:
> > And another bug forward
> > Anyone has an idea why a dm-crypt/lvm leads to a segfault in the strcmp
> > here:
> >  grub_partition_iterate (dest_dev->disk, (strcmp (dest_partmap, "pc_partition_map") ?
> >                                            find_usable_region_gpt : find_usable_region_msdos));
> 
> dest_partmap is only assigned a value in identify_partmap.  If
> grub_partition_iterate() doesn't find any partitions, dest_partmap
> remains a random pointer.
> 
> The fix would be probably to initialize dest_partmap with NULL.  If it
> becomes "pc_partition_map", iterate with find_usable_region_msdos, if it
> becomes "gpt_partition_map", iterate with find_usable_region_gpt.  If
> it's NULL or another string, exit with a warning.

How about this?

Require positive identification of PC or GPT partition for embedding

ChangeLog:

	* util/i386/pc/grub-setup.c (setup): Initialize dest_partmap
	before iteration.  Don't allow embedding unless dest_partmap is
	"pc_partition_map" or "gpt_partition_map".
---
 util/i386/pc/grub-setup.c |   37 ++++++++++++++++++++++++++++++-------
 1 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index 5a51964..7ac5ace 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -329,16 +329,39 @@ setup (const char *dir,
       dest_partmap = p->partmap->name;
       return 1;
     }
+
+  dest_partmap = NULL;
   grub_partition_iterate (dest_dev->disk, identify_partmap);
 
-  grub_partition_iterate (dest_dev->disk, (strcmp (dest_partmap, "pc_partition_map") ?
-					   find_usable_region_gpt : find_usable_region_msdos));
-  if (embed_region.end == embed_region.start)
+  if (! dest_partmap)
     {
-      if (! strcmp (dest_partmap, "pc_partition_map"))
-	grub_util_warn ("This msdos-style partition label has no post-MBR gap; embedding won't be possible!");
-      else
-	grub_util_warn ("This GPT partition label has no BIOS Boot Partition; embedding won't be possible!");
+      grub_util_warn ("Cannot identify partition map.");
+      goto unable_to_embed;
+    }
+  else if (strcmp (dest_partmap, "pc_partition_map") == 0)
+    {
+      grub_partition_iterate (dest_dev->disk, find_usable_region_msdos);
+      if (embed_region.end == embed_region.start)
+	{
+	  grub_util_warn ("This msdos-style partition label has no post-MBR "
+			  "gap; embedding won't be possible!");
+	  goto unable_to_embed;
+	}
+    }
+  else if (strcmp (dest_partmap, "gpt_partition_map") == 0)
+    {
+      grub_partition_iterate (dest_dev->disk, find_usable_region_gpt);
+      if (embed_region.end == embed_region.start)
+	{
+	  grub_util_warn ("This GPT partition label has no BIOS Boot "
+			  "Partition; embedding won't be possible!");
+	  goto unable_to_embed;
+	}
+    }
+  else
+    {
+      grub_util_warn ("Embedding on partition type %s is unsupported",
+		      dest_partmap);
       goto unable_to_embed;
     }
 

-- 
Regards,
Pavel Roskin

Re: [Fwd: Re: Bug#495949: grub-common: grub-probe segfaults]

[Vladimir 'phcoder' Serbinenko]

On Fri, Jul 24, 2009 at 10:09 PM, Pavel Roskin<proski@gnu.org> wrote:
> On Fri, 2009-07-24 at 20:46 +0200, Felix Zielcke wrote:
>> And another bug forward
>> Anyone has an idea why a dm-crypt/lvm leads to a segfault in the strcmp
>> here:
>>  grub_partition_iterate (dest_dev->disk, (strcmp (dest_partmap, "pc_partition_map") ?
>>                                            find_usable_region_gpt : find_usable_region_msdos));
>
> dest_partmap is only assigned a value in identify_partmap.  If
> grub_partition_iterate() doesn't find any partitions, dest_partmap
> remains a random pointer.
>
> The fix would be probably to initialize dest_partmap with NULL.  If it
> becomes "pc_partition_map", iterate with find_usable_region_msdos, if it
> becomes "gpt_partition_map", iterate with find_usable_region_gpt.  If
> it's NULL or another string, exit with a warning.
Actualy I already fixed this in my patch for installing on
partitionless devices. Since there were no oppositions I'll commit it
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [Fwd: Re: Bug#495949: grub-common: grub-probe segfaults]

[Pavel Roskin]

On Fri, 2009-07-24 at 22:37 +0200, Vladimir 'phcoder' Serbinenko wrote:

> Actualy I already fixed this in my patch for installing on
> partitionless devices. Since there were no oppositions I'll commit it

OK, go ahead.  I just realized that your patch is a subset of my patch,
so it would be fair if you apply your part under your name.  Just please
use a better description in the ChangeLog.

-- 
Regards,
Pavel Roskin

Re: [Fwd: Re: Bug#495949: grub-common: grub-probe segfaults]

[Vladimir 'phcoder' Serbinenko]

> -      if (! strcmp (dest_partmap, "pc_partition_map"))
> -       grub_util_warn ("This msdos-style partition label has no post-MBR gap; embedding won't be possible!");
> -      else
> -       grub_util_warn ("This GPT partition label has no BIOS Boot Partition; embedding won't be possible!");
> +      grub_util_warn ("Cannot identify partition map.");
This message will appear on partionless disks. Perhaps more or more
precise information is better (we're in core.img here)
> +      grub_util_warn ("Embedding on partition type %s is unsupported",
> +                     dest_partmap);
>       goto unable_to_embed;
>     }
This should be a harder error. Only few partition schemes have a place
for bootsector. Writing to "bootsector" is likely to destroy partition
map. If more partitions are sane for install (e.g. sun_pc I recently
implemented for solaris is) then I prefer to add them manually
>
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [Fwd: Re: Bug#495949: grub-common: grub-probe segfaults]

[Vladimir 'phcoder' Serbinenko]

On Fri, Jul 24, 2009 at 10:56 PM, Pavel Roskin<proski@gnu.org> wrote:
> On Fri, 2009-07-24 at 22:37 +0200, Vladimir 'phcoder' Serbinenko wrote:
>
>> Actualy I already fixed this in my patch for installing on
>> partitionless devices. Since there were no oppositions I'll commit it
>
> OK, go ahead.  I just realized that your patch is a subset of my patch,
> so it would be fair if you apply your part under your name.
I'm not that credit-seeker. I comitted my patch before I saw your
message and the goal was to avoid work duplication
>  Just please
> use a better description in the ChangeLog.
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [Fwd: Re: Bug#495949: grub-common: grub-probe segfaults]

[Felix Zielcke]

Am Freitag, den 24.07.2009, 22:37 +0200 schrieb Vladimir 'phcoder'
Serbinenko:
> On Fri, Jul 24, 2009 at 10:09 PM, Pavel Roskin<proski@gnu.org> wrote:
> > On Fri, 2009-07-24 at 20:46 +0200, Felix Zielcke wrote:
> >> And another bug forward
> >> Anyone has an idea why a dm-crypt/lvm leads to a segfault in the strcmp
> >> here:
> >>  grub_partition_iterate (dest_dev->disk, (strcmp (dest_partmap, "pc_partition_map") ?
> >>                                            find_usable_region_gpt : find_usable_region_msdos));
> >
> > dest_partmap is only assigned a value in identify_partmap.  If
> > grub_partition_iterate() doesn't find any partitions, dest_partmap
> > remains a random pointer.
> >
> > The fix would be probably to initialize dest_partmap with NULL.  If it
> > becomes "pc_partition_map", iterate with find_usable_region_msdos, if it
> > becomes "gpt_partition_map", iterate with find_usable_region_gpt.  If
> > it's NULL or another string, exit with a warning.
> Actualy I already fixed this in my patch for installing on
> partitionless devices. Since there were no oppositions I'll commit it

Thanks, it fixed the segfault for Jean-Luc.

-- 
Felix Zielcke
Proud Debian Maintainer

Re: [Fwd: Re: Bug#495949: grub-common: grub-probe segfaults]

[Robert Millan]

On Fri, Jul 24, 2009 at 04:33:44PM -0400, Pavel Roskin wrote:
> +  else
> +    {
> +      grub_util_warn ("Embedding on partition type %s is unsupported",
> +		      dest_partmap);
>        goto unable_to_embed;
>      }

This extends blocklist support for label types other than MSDOS or GPT,
which aren't designed with BIOS-based boot in mind.

I think we should refuse to install on them, unless there's a compelling
reason to support those.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [Fwd: Re: Bug#495949: grub-common: grub-probe segfaults]

[Robert Millan]

On Fri, Jul 24, 2009 at 11:17:08PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Fri, Jul 24, 2009 at 10:56 PM, Pavel Roskin<proski@gnu.org> wrote:
> > On Fri, 2009-07-24 at 22:37 +0200, Vladimir 'phcoder' Serbinenko wrote:
> >
> >> Actualy I already fixed this in my patch for installing on
> >> partitionless devices. Since there were no oppositions I'll commit it
> >
> > OK, go ahead.  I just realized that your patch is a subset of my patch,
> > so it would be fair if you apply your part under your name.
> I'm not that credit-seeker. I comitted my patch before I saw your
> message and the goal was to avoid work duplication

Next time, please wait a bit more to gather input, unless something is
absolutely obvious.  This thread is less than 24h old and I just sent a
comment now that I had a chance to read it.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."