Non-static variables and nested function pointers [bug #28392]

Non-static variables and nested function pointers [bug #28392]

[Grégoire Sutre]

[-- Attachment #1: Type: text/plain, Size: 809 bytes --]

Hi,

I am trying to add NetBSD specific code to util/hostdisk.c in order to 
make grub-probe work.  This part is almost finished.  However, I had a 
hard time dealing with segfaults in callbacks (hook function pointers) 
in a number of places of the vanilla code.  Actually, I get segfaults in 
grub-probe with the vanilla trunk code (see bug report #28392).  This is 
on NetBSD 5.0 i386.

In the end, these segfaults were fixed by making sure that all variables 
accessed by pointers to nested functions are declared static.  I attach 
a patch that fixes these segfaults on my NetBSD box (this patch is also 
included in the bug report).

However, I am not a C expert, and I must be missing something as the 
code (obviously) works well on other systems.

Thanks for your help,

Grégoire

[-- Attachment #2: patch-nested-functions.diff --]
[-- Type: text/x-patch, Size: 7659 bytes --]

diff -Naur grub2_2009-12-22/fs/ext2.c grub2/fs/ext2.c
--- grub2_2009-12-22/fs/ext2.c	2009-12-22 17:53:27.000000000 +0100
+++ grub2/fs/ext2.c	2009-12-23 19:05:36.000000000 +0100
@@ -789,9 +789,12 @@
 	       int (*hook) (const char *filename,
 			    const struct grub_dirhook_info *info))
 {
-  struct grub_ext2_data *data = 0;
+  static struct grub_ext2_data *data = 0;
   struct grub_fshelp_node *fdiro = 0;
 
+  static int (*myhook) (const char *filename,
+                        const struct grub_dirhook_info *info);
+
   auto int NESTED_FUNC_ATTR iterate (const char *filename,
 				     enum grub_fshelp_filetype filetype,
 				     grub_fshelp_node_t node);
@@ -817,9 +820,11 @@
 
       info.dir = ((filetype & GRUB_FSHELP_TYPE_MASK) == GRUB_FSHELP_DIR);
       grub_free (node);
-      return hook (filename, &info);
+      return myhook (filename, &info);
     }
 
+  myhook = hook;
+
   grub_dl_ref (my_mod);
 
   data = grub_ext2_mount (device->disk);
diff -Naur grub2_2009-12-22/fs/fat.c grub2/fs/fat.c
--- grub2_2009-12-22/fs/fat.c	2009-12-22 17:53:27.000000000 +0100
+++ grub2/fs/fat.c	2009-12-23 17:35:00.000000000 +0100
@@ -606,9 +606,13 @@
 		   int (*hook) (const char *filename,
 				const struct grub_dirhook_info *info))
 {
-  char *dirname, *dirp;
-  int call_hook;
-  int found = 0;
+  static char *dirname;
+  char *dirp;
+  static int call_hook;
+  static int found = 0;
+  static struct grub_fat_data *mydata;
+  static int (*myhook) (const char *filename,
+                        const struct grub_dirhook_info *info);
 
   auto int iter_hook (const char *filename, struct grub_fat_dir_entry *dir);
   int iter_hook (const char *filename, struct grub_fat_dir_entry *dir)
@@ -622,25 +626,28 @@
     if (dir->attr & GRUB_FAT_ATTR_VOLUME_ID)
       return 0;
     if (*dirname == '\0' && call_hook)
-      return hook (filename, &info);
+      return myhook (filename, &info);
 
     if (grub_strcasecmp (dirname, filename) == 0)
       {
 	found = 1;
-	data->attr = dir->attr;
-	data->file_size = grub_le_to_cpu32 (dir->file_size);
-	data->file_cluster = ((grub_le_to_cpu16 (dir->first_cluster_high) << 16)
+	mydata->attr = dir->attr;
+	mydata->file_size = grub_le_to_cpu32 (dir->file_size);
+	mydata->file_cluster = ((grub_le_to_cpu16 (dir->first_cluster_high) << 16)
 			      | grub_le_to_cpu16 (dir->first_cluster_low));
-	data->cur_cluster_num = ~0U;
+	mydata->cur_cluster_num = ~0U;
 
 	if (call_hook)
-	  hook (filename, &info);
+	  myhook (filename, &info);
 
 	return 1;
       }
     return 0;
   }
 
+  mydata = data;
+  myhook = hook;
+
   if (! (data->attr & GRUB_FAT_ATTR_DIRECTORY))
     {
       grub_error (GRUB_ERR_BAD_FILE_TYPE, "not a directory");
diff -Naur grub2_2009-12-22/kern/device.c grub2/kern/device.c
--- grub2_2009-12-22/kern/device.c	2009-12-22 17:53:30.000000000 +0100
+++ grub2/kern/device.c	2009-12-23 18:43:21.000000000 +0100
@@ -83,17 +83,45 @@
   auto int iterate_partition (grub_disk_t disk,
 			      const grub_partition_t partition);
 
-  struct part_ent
+  static struct part_ent
   {
     struct part_ent *next;
     char name[0];
   } *ents;
 
+  static int (*myhook) (const char *name);
+
+  int iterate_partition (grub_disk_t disk, const grub_partition_t partition)
+    {
+      char *partition_name;
+      struct part_ent *p;
+
+      partition_name = grub_partition_get_name (partition);
+      if (! partition_name)
+	return 1;
+
+      p = grub_malloc (sizeof (p->next) + grub_strlen (disk->name) + 1 +
+		       grub_strlen (partition_name) + 1);
+      if (!p)
+	{
+	  grub_free (partition_name);
+	  return 1;
+	}
+
+      grub_sprintf (p->name, "%s,%s", disk->name, partition_name);
+      grub_free (partition_name);
+
+      p->next = ents;
+      ents = p;
+
+      return 0;
+    }
+
   int iterate_disk (const char *disk_name)
     {
       grub_device_t dev;
 
-      if (hook (disk_name))
+      if (myhook (disk_name))
 	return 1;
 
       dev = grub_device_open (disk_name);
@@ -117,7 +145,7 @@
 	      struct part_ent *next = p->next;
 
 	      if (!ret)
-		ret = hook (p->name);
+		ret = myhook (p->name);
 	      grub_free (p);
 	      p = next;
 	    }
@@ -129,31 +157,7 @@
       return 0;
     }
 
-  int iterate_partition (grub_disk_t disk, const grub_partition_t partition)
-    {
-      char *partition_name;
-      struct part_ent *p;
-
-      partition_name = grub_partition_get_name (partition);
-      if (! partition_name)
-	return 1;
-
-      p = grub_malloc (sizeof (p->next) + grub_strlen (disk->name) + 1 +
-		       grub_strlen (partition_name) + 1);
-      if (!p)
-	{
-	  grub_free (partition_name);
-	  return 1;
-	}
-
-      grub_sprintf (p->name, "%s,%s", disk->name, partition_name);
-      grub_free (partition_name);
-
-      p->next = ents;
-      ents = p;
-
-      return 0;
-    }
+  myhook = hook;
 
   /* Only disk devices are supported at the moment.  */
   return grub_disk_dev_iterate (iterate_disk);
diff -Naur grub2_2009-12-22/kern/partition.c grub2/kern/partition.c
--- grub2_2009-12-22/kern/partition.c	2009-12-22 17:53:30.000000000 +0100
+++ grub2/kern/partition.c	2009-12-23 17:52:43.000000000 +0100
@@ -57,13 +57,15 @@
 grub_partition_t
 grub_partition_probe (struct grub_disk *disk, const char *str)
 {
-  grub_partition_t part = 0;
+  static grub_partition_t part = 0;
+  static struct grub_disk *mydisk;
+  static const char *mystr;
 
   auto int part_map_probe (const grub_partition_map_t partmap);
 
   int part_map_probe (const grub_partition_map_t partmap)
     {
-      part = partmap->probe (disk, str);
+      part = partmap->probe (mydisk, mystr);
       if (part)
 	return 1;
 
@@ -77,6 +79,9 @@
       return 1;
     }
 
+  mydisk = disk;
+  mystr = str;
+
   /* Use the first partition map type found.  */
   grub_partition_map_iterate (part_map_probe);
 
@@ -88,8 +93,9 @@
 			int (*hook) (grub_disk_t disk,
 				     const grub_partition_t partition))
 {
-  grub_partition_map_t partmap = 0;
+  static grub_partition_map_t partmap = 0;
   int ret = 0;
+  static struct grub_disk *mydisk;
 
   auto int part_map_iterate (const grub_partition_map_t p);
   auto int part_map_iterate_hook (grub_disk_t d,
@@ -104,7 +110,7 @@
   int part_map_iterate (const grub_partition_map_t p)
     {
       grub_dprintf ("partition", "Detecting %s...\n", p->name);
-      p->iterate (disk, part_map_iterate_hook);
+      p->iterate (mydisk, part_map_iterate_hook);
 
       if (grub_errno != GRUB_ERR_NONE)
 	{
@@ -119,6 +125,8 @@
       return 1;
     }
 
+  mydisk = disk;
+
   grub_partition_map_iterate (part_map_iterate);
   if (partmap)
     ret = partmap->iterate (disk, hook);
diff -Naur grub2_2009-12-22/partmap/msdos.c grub2/partmap/msdos.c
--- grub2_2009-12-22/partmap/msdos.c	2009-12-22 17:53:31.000000000 +0100
+++ grub2/partmap/msdos.c	2009-12-23 14:19:35.000000000 +0100
@@ -251,8 +251,8 @@
 static grub_partition_t
 pc_partition_map_probe (grub_disk_t disk, const char *str)
 {
-  grub_partition_t p;
-  struct grub_msdos_partition *pcdata;
+  static grub_partition_t p;
+  static struct grub_msdos_partition *pcdata;
 
   auto int find_func (grub_disk_t d, const grub_partition_t partition);
 
diff -Naur grub2_2009-12-22/partmap/sun.c grub2/partmap/sun.c
--- grub2_2009-12-22/partmap/sun.c	2009-12-22 17:53:31.000000000 +0100
+++ grub2/partmap/sun.c	2009-12-23 17:44:40.000000000 +0100
@@ -141,8 +141,8 @@
 static grub_partition_t
 sun_partition_map_probe (grub_disk_t disk, const char *str)
 {
-  grub_partition_t p = 0;
-  int partnum = 0;
+  static grub_partition_t p = 0;
+  static int partnum = 0;
   char *s = (char *) str;
 
   auto int find_func (grub_disk_t d, const grub_partition_t partition);

Re: Non-static variables and nested function pointers [bug #28392]

[Seth Goldberg]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1217 bytes --]

Hi,

   Your problem is probably lack of executable stack support, or at least you 
haven't linked your application with a linker mapfile that specifies an 
executable stack -- the callbacks require the use of trampolines to access 
local variables, which require an executable stack.

  --S

Quoting Grégoire Sutre, who wrote the following on Wed, 23 Dec 2009:

> Hi,
>
> I am trying to add NetBSD specific code to util/hostdisk.c in order to make 
> grub-probe work.  This part is almost finished.  However, I had a hard time 
> dealing with segfaults in callbacks (hook function pointers) in a number of 
> places of the vanilla code.  Actually, I get segfaults in grub-probe with the 
> vanilla trunk code (see bug report #28392).  This is on NetBSD 5.0 i386.
>
> In the end, these segfaults were fixed by making sure that all variables 
> accessed by pointers to nested functions are declared static.  I attach a 
> patch that fixes these segfaults on my NetBSD box (this patch is also 
> included in the bug report).
>
> However, I am not a C expert, and I must be missing something as the code 
> (obviously) works well on other systems.
>
> Thanks for your help,
>
> Grégoire
>

Re: Non-static variables and nested function pointers [bug #28392]

[Grégoire Sutre]

Seth Goldberg wrote:

>   Your problem is probably lack of executable stack support, or at least 
> you haven't linked your application with a linker mapfile that specifies 
> an executable stack -- the callbacks require the use of trampolines to 
> access local variables, which require an executable stack.

Thanks a lot for your answer ! That explains it :-)

I tried linking with -Wl,-z,execstack, but with no success, even though 
readelf shows a new header with this option:

Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RWE 0x4

But the problem may come from NetBSD's default policy of non-executable 
stack on i386 (http://www.netbsd.org/docs/kernel/non-exec.html).

I'll have to dig further...  Thanks again,

Grégoire

Re: Non-static variables and nested function pointers [bug #28392]

[Seth Goldberg]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1060 bytes --]



Quoting Grégoire Sutre, who wrote the following on Thu, 24 Dec 2009:

> Seth Goldberg wrote:
>
>>   Your problem is probably lack of executable stack support, or at least 
>> you haven't linked your application with a linker mapfile that specifies an 
>> executable stack -- the callbacks require the use of trampolines to access 
>> local variables, which require an executable stack.
>
> Thanks a lot for your answer ! That explains it :-)
>
> I tried linking with -Wl,-z,execstack, but with no success, even though 
> readelf shows a new header with this option:
>
> Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
> GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RWE 0x4
>
> But the problem may come from NetBSD's default policy of non-executable stack 
> on i386 (http://www.netbsd.org/docs/kernel/non-exec.html).

  Exactly -- the presence of the execstack attribute in the segment is merely a 
request -- the kernel is free to discard it, and many OSes do, as you've found 
:).

  --S

Re: Non-static variables and nested function pointers [bug #28392]

[Grégoire Sutre]

Seth Goldberg wrote:

>  Exactly -- the presence of the execstack attribute in the segment is 
> merely a request -- the kernel is free to discard it, and many OSes do, 
> as you've found :).

The problem is more complex: I tried a simple example with a pointer to 
a nested function, and it runs without any segfault on NetBSD/i386.
This would suggest that, by default, the stack is executable -- at least 
if trampolines are used?

The same example segfaults on Debian/amd64 if compiled with 
-Wl,-z,noexecstack (and does not segfault otherwise).

After some digging, I found threads in the archives of the mailing list 
on the problem of executable stack on NetBSD. This led to a patch that 
is now part of trunk if I'm not mistaken.

http://lists.gnu.org/archive/html/grub-devel/2008-02/msg00095.html

I will try to test on NetBSD/amd64 and report here (in a few days), as 
NetBSD's support for non-executable stack and heap seems better on amd64.

Grégoire


#include <stdio.h>

int apply(void (*hook) (int *))
{
    int a = 0;
    hook(&a);
    hook(&a);
    return a;
}

int main (int argc, char *argv[])
{
    int j = 5;
    int res;

    void hook(int *n)
    {
       *n = *n + j;
       j--;
    }

    res = apply(hook);
    printf("result: %d, j=%d\n", res, j);
    return 0;
}

Re: Non-static variables and nested function pointers [bug #28392]

[Seth Goldberg]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 779 bytes --]



Quoting Grégoire Sutre, who wrote the following on Thu, 24 Dec 2009:

> Seth Goldberg wrote:
>
>>  Exactly -- the presence of the execstack attribute in the segment is 
>> merely a request -- the kernel is free to discard it, and many OSes do, as 
>> you've found :).
>
> The problem is more complex: I tried a simple example with a pointer to a 
> nested function, and it runs without any segfault on NetBSD/i386.
> This would suggest that, by default, the stack is executable -- at least if 
> trampolines are used?

  It depends how the page tables are configured on that kernel -- if the NX bit 
isn't being set in page table entries on that platform, then the hardware 
support for execute-protection isn't enabled and you won't get any faults.

  --S

Re: Non-static variables and nested function pointers [bug #28392]

[Seth Goldberg]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 738 bytes --]


Quoting Grégoire Sutre, who wrote the following on Thu, 24 Dec 2009:

> Seth Goldberg wrote:
>
>>  Exactly -- the presence of the execstack attribute in the segment is 
>> merely a request -- the kernel is free to discard it, and many OSes do, as 
>> you've found :).
>
> The problem is more complex: I tried a simple example with a pointer to a 
> nested function, and it runs without any segfault on NetBSD/i386.
> This would suggest that, by default, the stack is executable -- at least if 
> trampolines are used?
>

   I forgot to add: the NX bit is only supported when PAE mode is enabled on a 
32-bit kernel, so if NetBSD/i386 isn't using PAE mode (for whatever reason), 
the NX bit won't be supported.

  --S

Re: Non-static variables and nested function pointers [bug #28392]

[Robert Millan]

On Wed, Dec 23, 2009 at 02:17:06PM -0800, Seth Goldberg wrote:
> Hi,
>
>   Your problem is probably lack of executable stack support, or at least 
> you haven't linked your application with a linker mapfile that specifies 
> an executable stack -- the callbacks require the use of trampolines to 
> access local variables, which require an executable stack.

The following snippet (kern/misc.c) comes to mind:

#ifdef NEED_ENABLE_EXECUTE_STACK
/* Some gcc versions generate a call to this function
   in trampolines for nested functions.  */
void __enable_execute_stack (void *addr __attribute__ ((unused)))
{
}
#endif

We added this for NetBSD in fact.  In that platform, GCC generates references
to this function, which are usually satisfied by libc, but we don't link with
libc, so we made it happy with an empty stub.

But this is only supposed to happen when building real GRUB.  For util/
stuff, we should use the libc facility instead.  Maybe that's not the case?

-- 
Robert Millan

  "Be the change you want to see in the world" -- Gandhi

Re: Non-static variables and nested function pointers [bug #28392]

[Grégoire Sutre]

[-- Attachment #1: Type: text/plain, Size: 2576 bytes --]

Robert Millan wrote:
> 
> The following snippet (kern/misc.c) comes to mind:
> 
> #ifdef NEED_ENABLE_EXECUTE_STACK
> /* Some gcc versions generate a call to this function
>    in trampolines for nested functions.  */
> void __enable_execute_stack (void *addr __attribute__ ((unused)))
> {
> }
> #endif

I was away from my email, sorry for the delay.  I hunted this
segfault down in the meantime, and I came to the same
conclusion.  If I remove `grub_CHECK_ENABLE_EXECUTE_STACK' from
configure.ac then the segfault in grub-probe disappear.


> We added this for NetBSD in fact.  In that platform, GCC generates references
> to this function, which are usually satisfied by libc, but we don't link with
> libc, so we made it happy with an empty stub.

Yes, I realized that this was NetBSD specific from the archives of the 
mailing list -- kind of ironic ;-)

On my NetBSD/i386 5.0 system, if I compile the attached example, the 
symbol __enable_execute_stack does indeed appear in the executable 
(according to nm).  This holds for gcc 4.1.3 (the system's default 
compiler) and for gcc 4.4.1.  And the symbol __enable_execute_stack is 
found in libgcc_s.so.1.


> But this is only supposed to happen when building real GRUB.  For util/
> stuff, we should use the libc facility instead.  Maybe that's not the case?

I guess that it's not the case: util programs that are linked with 
kern/misc.c do not use the libc's __enable_execute_stack (if 
NEED_ENABLE_EXECUTE_STACK is defined in config.h).


In case that helps, and to continue the discussion regarding
executable stack on NetBSD, I made a few experiments on i386 and
amd64 on a simplified example (see below).  From what I could
read, NetBSD implements memory protection with PaX.

* NetBSD/i386:

                  | with dummy __enable_execute_stack |
                  |       no        |       yes       |
-----------------+-----------------+-----------------+
PaX MPROTECT off |       ok        |    segfault     |
-----------------+-----------------+-----------------+
PaX MPROTECT on  |    segfault     |    segfault     |
-----------------+-----------------+-----------------+

However, in all four cases, there is no segfault if the variable
j in main is declared static.  And in that case, the generated 
executable does not contain the symbol __enable_execute_stack (according 
to nm).

* NetBSD/amd64: in all four cases, no segfault.

Also, on NetBSD/amd64, the segfault problem I experienced in grub-probe 
completely disappears.

Grégoire

[-- Attachment #2: nested.c --]
[-- Type: text/x-csrc, Size: 513 bytes --]

#include <stdio.h>

#ifdef BAD
/* Some gcc versions generate a call to this function
   in trampolines for nested functions.  */
void __enable_execute_stack (void *addr __attribute__ ((unused)))
{
}
#endif

int apply(void (*hook) (int *))
{
   int a = 0;
   hook(&a);
   hook(&a);
   return a;
}

int main (int argc, char *argv[])
{
   /* static */ int j = 5;
   int res;

   void hook(int *n)
   {
      *n = *n + j;
      j--;
   }

   res = apply(hook);
   printf("result: %d, j=%d\n", res, j);
   return 0;
}