2.02-beta3 remove attempt to free stack space and initialize variable before possible use

2.02-beta3 remove attempt to free stack space and initialize variable before possible use

[Aaron Luft]

Please consider these improvements to 2.02-beta3.
1) Remove the variable "oldname" which is attempting to free stack space.
2) Initialize the value of mdnobj to silence the compiler warning

In function 'grub_free',
    inlined from 'grub_iso9660_iterate_dir' at grub-core/fs/iso9660.c:764:15:
grub-core/kern/emu/mm.c:53:3: error: attempt to free a non-heap object 'name' [-Werror=free-nonheap-object]
   free (ptr);
   ^
lto1: all warnings being treated as errors
lto-wrapper: fatal error: x86_64-linux-gnu-gcc-5.3.0 returned 1 exit status

grub-core/fs/zfs/zfsinfo.c: In function 'grub_cmd_zfs_bootfs':
grub-core/fs/zfs/zfsinfo.c:401:10: error: 'mdnobj' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   bootfs = grub_xasprintf ("zfs-bootfs=%s/%llu%s%s%s%s%s%s",
          ^
grub-core/fs/zfs/zfsinfo.c:355:17: note: 'mdnobj' was declared here
   grub_uint64_t mdnobj;
                 ^
lto1: all warnings being treated as errors



diff -Naur grub-2.02-beta3.orig/grub-core/fs/iso9660.c grub-2.02-beta3/grub-core/fs/iso9660.c
--- grub-2.02-beta3.orig/grub-core/fs/iso9660.c 2016-02-28 02:07:41.000000000 +0000
+++ grub-2.02-beta3/grub-core/fs/iso9660.c      2016-03-12 01:17:26.581112809 +0000
@@ -750,19 +750,15 @@

         if (dir->data->joliet && !ctx.filename)
           {
-            char *oldname, *semicolon;
+            char *semicolon;

-            oldname = name;
             ctx.filename = grub_iso9660_convert_string
-                  ((grub_uint8_t *) oldname, dirent.namelen >> 1);
+                  ((grub_uint8_t *) name, dirent.namelen >> 1);

            semicolon = grub_strrchr (ctx.filename, ';');
            if (semicolon)
              *semicolon = '\0';

-            if (ctx.filename_alloc)
-              grub_free (oldname);
-
             ctx.filename_alloc = 1;
           }

diff -Naur grub-2.02-beta3.orig/grub-core/fs/zfs/zfsinfo.c grub-2.02-beta3/grub-core/fs/zfs/zfsinfo.c
--- grub-2.02-beta3.orig/grub-core/fs/zfs/zfsinfo.c     2016-02-28 02:07:41.000000000 +0000
+++ grub-2.02-beta3/grub-core/fs/zfs/zfsinfo.c  2016-03-12 01:18:00.504961950 +0000
@@ -352,7 +352,7 @@
   char *fsname;
   char *bootfs;
   char *poolname;
-  grub_uint64_t mdnobj;
+  grub_uint64_t mdnobj = 0;

   if (argc < 1)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("one argument expected"));

Re: 2.02-beta3 remove attempt to free stack space and initialize variable before possible use

[Andrei Borzenkov]

14.03.2016 17:37, Aaron Luft пишет:
> Please consider these improvements to 2.02-beta3.
> 1) Remove the variable "oldname" which is attempting to free stack space.
> 2) Initialize the value of mdnobj to silence the compiler warning
> 
> In function 'grub_free',
>     inlined from 'grub_iso9660_iterate_dir' at grub-core/fs/iso9660.c:764:15:
> grub-core/kern/emu/mm.c:53:3: error: attempt to free a non-heap object 'name' [-Werror=free-nonheap-object]
>    free (ptr);
>    ^
> lto1: all warnings being treated as errors
> lto-wrapper: fatal error: x86_64-linux-gnu-gcc-5.3.0 returned 1 exit status
> 
> grub-core/fs/zfs/zfsinfo.c: In function 'grub_cmd_zfs_bootfs':
> grub-core/fs/zfs/zfsinfo.c:401:10: error: 'mdnobj' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>    bootfs = grub_xasprintf ("zfs-bootfs=%s/%llu%s%s%s%s%s%s",
>           ^
> grub-core/fs/zfs/zfsinfo.c:355:17: note: 'mdnobj' was declared here
>    grub_uint64_t mdnobj;
>                  ^
> lto1: all warnings being treated as errors
> 

I cannot apply them due to whitespace changes.

> 
> 
> diff -Naur grub-2.02-beta3.orig/grub-core/fs/iso9660.c grub-2.02-beta3/grub-core/fs/iso9660.c
> --- grub-2.02-beta3.orig/grub-core/fs/iso9660.c 2016-02-28 02:07:41.000000000 +0000
> +++ grub-2.02-beta3/grub-core/fs/iso9660.c      2016-03-12 01:17:26.581112809 +0000
> @@ -750,19 +750,15 @@
> 
>          if (dir->data->joliet && !ctx.filename)
>            {
> -            char *oldname, *semicolon;
> +            char *semicolon;
> 
> -            oldname = name;
>              ctx.filename = grub_iso9660_convert_string
> -                  ((grub_uint8_t *) oldname, dirent.namelen >> 1);
> +                  ((grub_uint8_t *) name, dirent.namelen >> 1);
> 
>             semicolon = grub_strrchr (ctx.filename, ';');
>             if (semicolon)
>               *semicolon = '\0';
> 
> -            if (ctx.filename_alloc)
> -              grub_free (oldname);
> -
>              ctx.filename_alloc = 1;
>            }
> 
Yes, this is one correct. Please resend as attached patch generated by
"git format-patch" with suitable commit message. Do not expand tabs when
editing.


> diff -Naur grub-2.02-beta3.orig/grub-core/fs/zfs/zfsinfo.c grub-2.02-beta3/grub-core/fs/zfs/zfsinfo.c
> --- grub-2.02-beta3.orig/grub-core/fs/zfs/zfsinfo.c     2016-02-28 02:07:41.000000000 +0000
> +++ grub-2.02-beta3/grub-core/fs/zfs/zfsinfo.c  2016-03-12 01:18:00.504961950 +0000
> @@ -352,7 +352,7 @@
>    char *fsname;
>    char *bootfs;
>    char *poolname;
> -  grub_uint64_t mdnobj;
> +  grub_uint64_t mdnobj = 0;
> 
>    if (argc < 1)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("one argument expected"));
> 

Well ... it cannot really reach code where mdnobj is used if
grub_zfs_getmdnobj() fails but static analyzer may not know it.

How do you compile it? I cannot reproduce it using gcc 5.3.1 nor did it
fail previously. Do you use non-standard compiler flags?

Re: 2.02-beta3 remove attempt to free stack space and initialize variable before possible use

[Aaron Luft]

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


> On Mar 14, 2016, at 1:35 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> Yes, this is one correct. Please resend as attached patch generated by
> "git format-patch" with suitable commit message. Do not expand tabs when
> editing.
> 
Ok, attaching the patch to this mail.

> 
> Well ... it cannot really reach code where mdnobj is used if
> grub_zfs_getmdnobj() fails but static analyzer may not know it.
> 
> How do you compile it? I cannot reproduce it using gcc 5.3.1 nor did it
> fail previously. Do you use non-standard compiler flags?

Yes, non-standard command line.
I found an extra -Wall that the standard build doesn't have.

Appreciate your comments and time,
Aaron


[-- Attachment #2: grub-dont-free-stack-plus-one.patch --]
[-- Type: application/octet-stream, Size: 1943 bytes --]

From ff7dc2550f289f0ead3e4ee6a74003be7c98152f Mon Sep 17 00:00:00 2001
From: Aaron Luft <aluft@lifesize.com>
Date: Mon, 14 Mar 2016 16:48:33 -0400
Subject: [PATCH 1/2] Remove the variable oldname which is attempting to free
 stack space.

---
 grub-core/fs/iso9660.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index 67a67cf..c9c8374 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -750,19 +750,15 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
 
         if (dir->data->joliet && !ctx.filename)
           {
-            char *oldname, *semicolon;
+            char *semicolon;
 
-            oldname = name;
             ctx.filename = grub_iso9660_convert_string
-                  ((grub_uint8_t *) oldname, dirent.namelen >> 1);
+                  ((grub_uint8_t *) name, dirent.namelen >> 1);
 
 	    semicolon = grub_strrchr (ctx.filename, ';');
 	    if (semicolon)
 	      *semicolon = '\0';
 
-            if (ctx.filename_alloc)
-              grub_free (oldname);
-
             ctx.filename_alloc = 1;
           }
 
-- 
2.5.0


From c50f4167c59adefd30a6179fc967fecf33c7a895 Mon Sep 17 00:00:00 2001
From: Aaron Luft <aluft@lifesize.com>
Date: Mon, 14 Mar 2016 16:48:56 -0400
Subject: [PATCH 2/2] Initialize the value of mdnobj to silence the compiler
 warning

---
 grub-core/fs/zfs/zfsinfo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/fs/zfs/zfsinfo.c b/grub-core/fs/zfs/zfsinfo.c
index c8a28ac..926f966 100644
--- a/grub-core/fs/zfs/zfsinfo.c
+++ b/grub-core/fs/zfs/zfsinfo.c
@@ -352,7 +352,7 @@ grub_cmd_zfs_bootfs (grub_command_t cmd __attribute__ ((unused)), int argc,
   char *fsname;
   char *bootfs;
   char *poolname;
-  grub_uint64_t mdnobj;
+  grub_uint64_t mdnobj = 0;
 
   if (argc < 1)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("one argument expected"));
-- 
2.5.0

Re: 2.02-beta3 remove attempt to free stack space and initialize variable before possible use

[Andrei Borzenkov]

15.03.2016 00:21, Aaron Luft пишет:
> 
>> On Mar 14, 2016, at 1:35 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>> Yes, this is one correct. Please resend as attached patch generated by
>> "git format-patch" with suitable commit message. Do not expand tabs when
>> editing.
>>
> Ok, attaching the patch to this mail.
> 

I committed the iso9660 patch adding commit message with some explanation.

>>
>> Well ... it cannot really reach code where mdnobj is used if
>> grub_zfs_getmdnobj() fails but static analyzer may not know it.
>>
>> How do you compile it? I cannot reproduce it using gcc 5.3.1 nor did it
>> fail previously. Do you use non-standard compiler flags?
> 
> Yes, non-standard command line.
> I found an extra -Wall that the standard build doesn't have.
> 

We use -Wall always. The problem is apparently caused by using LTO.
Could you upload somewhere full log of configure && make with this error?