[PATCH] progress: Check for NULL filename

[PATCH] progress: Check for NULL filename

[dann frazier]

Avoid a NULL pointer dereference if the upper fs layer hasn't set the
file->name field. Files opened through the grub_net_fs interface currently do
not have this field set (though perhaps they should?).

Signed-off-by: dann frazier <dann.frazier@canonical.com>
---
 grub-core/lib/progress.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
index 63a0767..2775554 100644
--- a/grub-core/lib/progress.c
+++ b/grub-core/lib/progress.c
@@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)),
 	percent = grub_divmod64 (100 * file->progress_offset,
 				 file->size, 0);
 
-      partial_file_name = grub_strrchr (file->name, '/');
-      if (partial_file_name)
+      if (file->name && (partial_file_name = grub_strrchr (file->name, '/')))
 	partial_file_name++;
       else
 	partial_file_name = "";
-- 
2.5.0

Re: [PATCH] progress: Check for NULL filename

[Andrei Borzenkov]

On Wed, Aug 12, 2015 at 6:53 PM, dann frazier
<dann.frazier@canonical.com> wrote:
> Avoid a NULL pointer dereference if the upper fs layer hasn't set the
> file->name field. Files opened through the grub_net_fs interface currently do
> not have this field set (though perhaps they should?).
>

file->name is set in grub_file_open independently of any filesystem
used. How comes it becomes empty? Do you see it in current GIT master?

> Signed-off-by: dann frazier <dann.frazier@canonical.com>
> ---
>  grub-core/lib/progress.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
> index 63a0767..2775554 100644
> --- a/grub-core/lib/progress.c
> +++ b/grub-core/lib/progress.c
> @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)),
>         percent = grub_divmod64 (100 * file->progress_offset,
>                                  file->size, 0);
>
> -      partial_file_name = grub_strrchr (file->name, '/');
> -      if (partial_file_name)
> +      if (file->name && (partial_file_name = grub_strrchr (file->name, '/')))
>         partial_file_name++;
>        else
>         partial_file_name = "";
> --
> 2.5.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] progress: Check for NULL filename

[dann frazier]

On Thu, Aug 13, 2015 at 10:52:19AM +0300, Andrei Borzenkov wrote:
> On Wed, Aug 12, 2015 at 6:53 PM, dann frazier
> <dann.frazier@canonical.com> wrote:
> > Avoid a NULL pointer dereference if the upper fs layer hasn't set the
> > file->name field. Files opened through the grub_net_fs interface currently do
> > not have this field set (though perhaps they should?).
> >
> 
> file->name is set in grub_file_open independently of any filesystem
> used. How comes it becomes empty? Do you see it in current GIT
> master?

Yeah, I see it with current GIT master. Here's what I believe is happening.

grub_file_open() calls the fs->open callback, *before* it initializes
file->name. In the net_fs open callback (haven't checked others), it
makes a copy of the file structure and instantiates a bufio file
structure for it. It copies the bufio structure over the file
structure that was passed in.

Now we return to grub_file_open and set file->name in the (now bufio)
file structure. But the original file structure backing the bufio
still has a NULL name. When this bufio is read, it calls the read
method on this backing file, which causes the progress hook to run and
fall over.

Perhaps the fix here is just to make grub_file_open set file->name
before the fs_open callback?

diff --git a/grub-core/kern/file.c b/grub-core/kern/file.c
index 24da12b..4afa8c2 100644
--- a/grub-core/kern/file.c
+++ b/grub-core/kern/file.c
@@ -99,10 +99,11 @@ grub_file_open (const char *name)
        goto fail;
     }
 
+  file->name = grub_strdup (name);
+
   if ((file->fs->open) (file, file_name) != GRUB_ERR_NONE)
     goto fail;
 
-  file->name = grub_strdup (name);
   grub_errno = GRUB_ERR_NONE;
 
   for (filter = 0; file && filter < ARRAY_SIZE (grub_file_filters_enabled);
@@ -126,6 +127,7 @@ grub_file_open (const char *name)
 
   /* if (net) grub_net_close (net);  */
 
+  grub_free (file->name);
   grub_free (file);
 
   grub_memcpy (grub_file_filters_enabled, grub_file_filters_all,


> 
> > Signed-off-by: dann frazier <dann.frazier@canonical.com>
> > ---
> >  grub-core/lib/progress.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
> > index 63a0767..2775554 100644
> > --- a/grub-core/lib/progress.c
> > +++ b/grub-core/lib/progress.c
> > @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)),
> >         percent = grub_divmod64 (100 * file->progress_offset,
> >                                  file->size, 0);
> >
> > -      partial_file_name = grub_strrchr (file->name, '/');
> > -      if (partial_file_name)
> > +      if (file->name && (partial_file_name = grub_strrchr (file->name, '/')))
> >         partial_file_name++;
> >        else
> >         partial_file_name = "";
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] progress: Check for NULL filename

[Andrei Borzenkov]

14.08.2015 00:04, dann frazier пишет:
> On Thu, Aug 13, 2015 at 10:52:19AM +0300, Andrei Borzenkov wrote:
>> On Wed, Aug 12, 2015 at 6:53 PM, dann frazier
>> <dann.frazier@canonical.com> wrote:
>>> Avoid a NULL pointer dereference if the upper fs layer hasn't set the
>>> file->name field. Files opened through the grub_net_fs interface currently do
>>> not have this field set (though perhaps they should?).
>>>
>>
>> file->name is set in grub_file_open independently of any filesystem
>> used. How comes it becomes empty? Do you see it in current GIT
>> master?
>
> Yeah, I see it with current GIT master. Here's what I believe is happening.
>
> grub_file_open() calls the fs->open callback, *before* it initializes
> file->name. In the net_fs open callback (haven't checked others), it
> makes a copy of the file structure and instantiates a bufio file
> structure for it. It copies the bufio structure over the file
> structure that was passed in.
>
> Now we return to grub_file_open and set file->name in the (now bufio)
> file structure. But the original file structure backing the bufio
> still has a NULL name. When this bufio is read, it calls the read
> method on this backing file, which causes the progress hook to run and
> fall over.
>
> Perhaps the fix here is just to make grub_file_open set file->name
> before the fs_open callback?
>

Then grub_net_fs_open needs to duplicate file->name, otherwise it will 
result in double free of file->name (first by grub_file_close in 
grub_bufio_close and then by original grub_file_close).

And grub_file_open ignores NULL return from grub_strdup so we need check 
in progress anyway.

I wish we could simply use grub_buffile_open, but I do not see easy way.

Could you make combined patch?

> diff --git a/grub-core/kern/file.c b/grub-core/kern/file.c
> index 24da12b..4afa8c2 100644
> --- a/grub-core/kern/file.c
> +++ b/grub-core/kern/file.c
> @@ -99,10 +99,11 @@ grub_file_open (const char *name)
>          goto fail;
>       }
>
> +  file->name = grub_strdup (name);
> +
>     if ((file->fs->open) (file, file_name) != GRUB_ERR_NONE)
>       goto fail;
>
> -  file->name = grub_strdup (name);
>     grub_errno = GRUB_ERR_NONE;
>
>     for (filter = 0; file && filter < ARRAY_SIZE (grub_file_filters_enabled);
> @@ -126,6 +127,7 @@ grub_file_open (const char *name)
>
>     /* if (net) grub_net_close (net);  */
>
> +  grub_free (file->name);
>     grub_free (file);
>
>     grub_memcpy (grub_file_filters_enabled, grub_file_filters_all,
>
>
>>
>>> Signed-off-by: dann frazier <dann.frazier@canonical.com>
>>> ---
>>>   grub-core/lib/progress.c | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
>>> index 63a0767..2775554 100644
>>> --- a/grub-core/lib/progress.c
>>> +++ b/grub-core/lib/progress.c
>>> @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)),
>>>          percent = grub_divmod64 (100 * file->progress_offset,
>>>                                   file->size, 0);
>>>
>>> -      partial_file_name = grub_strrchr (file->name, '/');
>>> -      if (partial_file_name)
>>> +      if (file->name && (partial_file_name = grub_strrchr (file->name, '/')))
>>>          partial_file_name++;
>>>         else
>>>          partial_file_name = "";
>>>
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH] Avoid NULL pointer dereference in progress module.

[dann frazier]

grub_net_fs_open() saves off a copy of the file structure it gets passed and
uses it to create a bufio structure. It then overwrites the passed in file
structure with this new bufio structure. Since file->name doesn't get set
until we return back to grub_file_open(), it means that only the bufio
structure gets a valid file->name. The "real" file's name is left
uninitialized. This leads to a crash when the progress module hook is called
on it. Fix this by adding a copy of the filename during the switcheroo.
grub_file_close() will free that string in the success case, we only need
to handle the free in an error. While we're at it, also fix a leaked file
structure in the case that grub_strdup() fails when setting
file->device->net->name.

Finally, make progress more robust by checking for NULL before reading the
name. Andrei noticed that this could occur if grub_strdup() fails in
grub_file_open().

Signed-off-by: dann frazier <dann.frazier@canonical.com>
---
 grub-core/lib/progress.c |  3 +--
 grub-core/net/net.c      | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
index 63a0767..2775554 100644
--- a/grub-core/lib/progress.c
+++ b/grub-core/lib/progress.c
@@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)),
 	percent = grub_divmod64 (100 * file->progress_offset,
 				 file->size, 0);
 
-      partial_file_name = grub_strrchr (file->name, '/');
-      if (partial_file_name)
+      if (file->name && (partial_file_name = grub_strrchr (file->name, '/')))
 	partial_file_name++;
       else
 	partial_file_name = "";
diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index 21a4e94..9f83765 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const char *name)
   file->device->net->packs.last = NULL;
   file->device->net->name = grub_strdup (name);
   if (!file->device->net->name)
-    return grub_errno;
+    {
+      grub_free (file);
+      return grub_errno;
+    }
+  file->name = grub_strdup (name);
+  if (!file->name)
+    {
+      grub_free (file->device->net->name);
+      grub_free (file);
+      return grub_errno;
+    }
 
   err = file->device->net->protocol->open (file, name);
   if (err)
@@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name)
 	  grub_netbuff_free (file->device->net->packs.first->nb);
 	  grub_net_remove_packet (file->device->net->packs.first);
 	}
+      grub_free (file->name);
       grub_free (file->device->net->name);
       grub_free (file);
       return err;
@@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name)
 	  grub_net_remove_packet (file->device->net->packs.first);
 	}
       file->device->net->protocol->close (file);
+      grub_free (file->name);
       grub_free (file->device->net->name);
       grub_free (file);
       return grub_errno;
-- 
2.5.0

Re: [PATCH] Avoid NULL pointer dereference in progress module.

[Andrei Borzenkov]

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

18.08.2015 00:57, dann frazier пишет:
> grub_net_fs_open() saves off a copy of the file structure it gets passed and
> uses it to create a bufio structure. It then overwrites the passed in file
> structure with this new bufio structure. Since file->name doesn't get set
> until we return back to grub_file_open(), it means that only the bufio
> structure gets a valid file->name. The "real" file's name is left
> uninitialized. This leads to a crash when the progress module hook is called
> on it. Fix this by adding a copy of the filename during the switcheroo.

Actually name was already saved off as device->net-name. What about 
attached patch? I'll commit leak fix separately.

> grub_file_close() will free that string in the success case, we only need
> to handle the free in an error. While we're at it, also fix a leaked file
> structure in the case that grub_strdup() fails when setting
> file->device->net->name.
>
> Finally, make progress more robust by checking for NULL before reading the
> name. Andrei noticed that this could occur if grub_strdup() fails in
> grub_file_open().
>
> Signed-off-by: dann frazier <dann.frazier@canonical.com>
> ---
>   grub-core/lib/progress.c |  3 +--
>   grub-core/net/net.c      | 14 +++++++++++++-
>   2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
> index 63a0767..2775554 100644
> --- a/grub-core/lib/progress.c
> +++ b/grub-core/lib/progress.c
> @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)),
>   	percent = grub_divmod64 (100 * file->progress_offset,
>   				 file->size, 0);
>
> -      partial_file_name = grub_strrchr (file->name, '/');
> -      if (partial_file_name)
> +      if (file->name && (partial_file_name = grub_strrchr (file->name, '/')))
>   	partial_file_name++;
>         else
>   	partial_file_name = "";
> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index 21a4e94..9f83765 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const char *name)
>     file->device->net->packs.last = NULL;
>     file->device->net->name = grub_strdup (name);
>     if (!file->device->net->name)
> -    return grub_errno;
> +    {
> +      grub_free (file);
> +      return grub_errno;
> +    }
> +  file->name = grub_strdup (name);
> +  if (!file->name)
> +    {
> +      grub_free (file->device->net->name);
> +      grub_free (file);
> +      return grub_errno;
> +    }
>
>     err = file->device->net->protocol->open (file, name);
>     if (err)
> @@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name)
>   	  grub_netbuff_free (file->device->net->packs.first->nb);
>   	  grub_net_remove_packet (file->device->net->packs.first);
>   	}
> +      grub_free (file->name);
>         grub_free (file->device->net->name);
>         grub_free (file);
>         return err;
> @@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name)
>   	  grub_net_remove_packet (file->device->net->packs.first);
>   	}
>         file->device->net->protocol->close (file);
> +      grub_free (file->name);
>         grub_free (file->device->net->name);
>         grub_free (file);
>         return grub_errno;
>


[-- Attachment #2: proress-net.patch --]
[-- Type: text/x-patch, Size: 1906 bytes --]

From: Andrei Borzenkov <arvidjaar@gmail.com>
Subject: [PATCH] progress: avoid NULL dereference for net files

From original patch by dann frazier <dann.frazier@canonical.com>:

  grub_net_fs_open() saves off a copy of the file structure it gets passed and
  uses it to create a bufio structure. It then overwrites the passed in file
  structure with this new bufio structure. Since file->name doesn't get set
  until we return back to grub_file_open(), it means that only the bufio
  structure gets a valid file->name. The "real" file's name is left
  uninitialized. This leads to a crash when the progress module hook is called
  on it.

grub_net_fs_open() already saved copy of file name as ->net->name, so change
progress module to use it.

Also, grub_file_open may leave file->name as NULL if grub_strdup fails. Check
for it.

Also-By: dann frazier <dann.frazier@canonical.com>

---
 grub-core/lib/progress.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
index 63a0767..7c723eb 100644
--- a/grub-core/lib/progress.c
+++ b/grub-core/lib/progress.c
@@ -23,6 +23,7 @@
 #include <grub/dl.h>
 #include <grub/misc.h>
 #include <grub/normal.h>
+#include <grub/net.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -70,7 +71,12 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)),
 	percent = grub_divmod64 (100 * file->progress_offset,
 				 file->size, 0);
 
-      partial_file_name = grub_strrchr (file->name, '/');
+      if (file->device->net)
+	partial_file_name = grub_strrchr (file->device->net->name, '/');
+      else if (file->name) /* grub_file_open() may leave it as NULL */
+	partial_file_name = grub_strrchr (file->name, '/');
+      else
+	partial_file_name = NULL;
       if (partial_file_name)
 	partial_file_name++;
       else
-- 
tg: (ba218c1..) u/progress-net-file (depends on: master)

Re: [PATCH] Avoid NULL pointer dereference in progress module.

[Dann Frazier]

On Thu, Aug 20, 2015 at 11:55 AM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> 18.08.2015 00:57, dann frazier пишет:
>>
>> grub_net_fs_open() saves off a copy of the file structure it gets passed
>> and
>> uses it to create a bufio structure. It then overwrites the passed in file
>> structure with this new bufio structure. Since file->name doesn't get set
>> until we return back to grub_file_open(), it means that only the bufio
>> structure gets a valid file->name. The "real" file's name is left
>> uninitialized. This leads to a crash when the progress module hook is
>> called
>> on it. Fix this by adding a copy of the filename during the switcheroo.
>
>
> Actually name was already saved off as device->net-name. What about attached
> patch? I'll commit leak fix separately.

It does seem like it pokes a hole in the fs abstraction, but it works for me.

   -dann

>
>> grub_file_close() will free that string in the success case, we only need
>> to handle the free in an error. While we're at it, also fix a leaked file
>> structure in the case that grub_strdup() fails when setting
>> file->device->net->name.
>>
>> Finally, make progress more robust by checking for NULL before reading the
>> name. Andrei noticed that this could occur if grub_strdup() fails in
>> grub_file_open().
>>
>> Signed-off-by: dann frazier <dann.frazier@canonical.com>
>> ---
>>   grub-core/lib/progress.c |  3 +--
>>   grub-core/net/net.c      | 14 +++++++++++++-
>>   2 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
>> index 63a0767..2775554 100644
>> --- a/grub-core/lib/progress.c
>> +++ b/grub-core/lib/progress.c
>> @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector
>> __attribute__ ((unused)),
>>         percent = grub_divmod64 (100 * file->progress_offset,
>>                                  file->size, 0);
>>
>> -      partial_file_name = grub_strrchr (file->name, '/');
>> -      if (partial_file_name)
>> +      if (file->name && (partial_file_name = grub_strrchr (file->name,
>> '/')))
>>         partial_file_name++;
>>         else
>>         partial_file_name = "";
>> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
>> index 21a4e94..9f83765 100644
>> --- a/grub-core/net/net.c
>> +++ b/grub-core/net/net.c
>> @@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const
>> char *name)
>>     file->device->net->packs.last = NULL;
>>     file->device->net->name = grub_strdup (name);
>>     if (!file->device->net->name)
>> -    return grub_errno;
>> +    {
>> +      grub_free (file);
>> +      return grub_errno;
>> +    }
>> +  file->name = grub_strdup (name);
>> +  if (!file->name)
>> +    {
>> +      grub_free (file->device->net->name);
>> +      grub_free (file);
>> +      return grub_errno;
>> +    }
>>
>>     err = file->device->net->protocol->open (file, name);
>>     if (err)
>> @@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const
>> char *name)
>>           grub_netbuff_free (file->device->net->packs.first->nb);
>>           grub_net_remove_packet (file->device->net->packs.first);
>>         }
>> +      grub_free (file->name);
>>         grub_free (file->device->net->name);
>>         grub_free (file);
>>         return err;
>> @@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const
>> char *name)
>>           grub_net_remove_packet (file->device->net->packs.first);
>>         }
>>         file->device->net->protocol->close (file);
>> +      grub_free (file->name);
>>         grub_free (file->device->net->name);
>>         grub_free (file);
>>         return grub_errno;
>>
>

Re: [PATCH] Avoid NULL pointer dereference in progress module.

[Dann Frazier]

On Fri, Aug 21, 2015 at 8:24 AM, Dann Frazier
<dann.frazier@canonical.com> wrote:
> On Thu, Aug 20, 2015 at 11:55 AM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>> 18.08.2015 00:57, dann frazier пишет:
>>>
>>> grub_net_fs_open() saves off a copy of the file structure it gets passed
>>> and
>>> uses it to create a bufio structure. It then overwrites the passed in file
>>> structure with this new bufio structure. Since file->name doesn't get set
>>> until we return back to grub_file_open(), it means that only the bufio
>>> structure gets a valid file->name. The "real" file's name is left
>>> uninitialized. This leads to a crash when the progress module hook is
>>> called
>>> on it. Fix this by adding a copy of the filename during the switcheroo.
>>
>>
>> Actually name was already saved off as device->net-name. What about attached
>> patch? I'll commit leak fix separately.
>
> It does seem like it pokes a hole in the fs abstraction, but it works for me.

hey Andrei,
 Did you need anything more from me wrt this fix?

 -dann

>>
>>> grub_file_close() will free that string in the success case, we only need
>>> to handle the free in an error. While we're at it, also fix a leaked file
>>> structure in the case that grub_strdup() fails when setting
>>> file->device->net->name.
>>>
>>> Finally, make progress more robust by checking for NULL before reading the
>>> name. Andrei noticed that this could occur if grub_strdup() fails in
>>> grub_file_open().
>>>
>>> Signed-off-by: dann frazier <dann.frazier@canonical.com>
>>> ---
>>>   grub-core/lib/progress.c |  3 +--
>>>   grub-core/net/net.c      | 14 +++++++++++++-
>>>   2 files changed, 14 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
>>> index 63a0767..2775554 100644
>>> --- a/grub-core/lib/progress.c
>>> +++ b/grub-core/lib/progress.c
>>> @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector
>>> __attribute__ ((unused)),
>>>         percent = grub_divmod64 (100 * file->progress_offset,
>>>                                  file->size, 0);
>>>
>>> -      partial_file_name = grub_strrchr (file->name, '/');
>>> -      if (partial_file_name)
>>> +      if (file->name && (partial_file_name = grub_strrchr (file->name,
>>> '/')))
>>>         partial_file_name++;
>>>         else
>>>         partial_file_name = "";
>>> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
>>> index 21a4e94..9f83765 100644
>>> --- a/grub-core/net/net.c
>>> +++ b/grub-core/net/net.c
>>> @@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const
>>> char *name)
>>>     file->device->net->packs.last = NULL;
>>>     file->device->net->name = grub_strdup (name);
>>>     if (!file->device->net->name)
>>> -    return grub_errno;
>>> +    {
>>> +      grub_free (file);
>>> +      return grub_errno;
>>> +    }
>>> +  file->name = grub_strdup (name);
>>> +  if (!file->name)
>>> +    {
>>> +      grub_free (file->device->net->name);
>>> +      grub_free (file);
>>> +      return grub_errno;
>>> +    }
>>>
>>>     err = file->device->net->protocol->open (file, name);
>>>     if (err)
>>> @@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const
>>> char *name)
>>>           grub_netbuff_free (file->device->net->packs.first->nb);
>>>           grub_net_remove_packet (file->device->net->packs.first);
>>>         }
>>> +      grub_free (file->name);
>>>         grub_free (file->device->net->name);
>>>         grub_free (file);
>>>         return err;
>>> @@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const
>>> char *name)
>>>           grub_net_remove_packet (file->device->net->packs.first);
>>>         }
>>>         file->device->net->protocol->close (file);
>>> +      grub_free (file->name);
>>>         grub_free (file->device->net->name);
>>>         grub_free (file);
>>>         return grub_errno;
>>>
>>

Re: [PATCH] Avoid NULL pointer dereference in progress module.

[Andrei Borzenkov]

21.09.2015 18:11, Dann Frazier пишет:
> On Fri, Aug 21, 2015 at 8:24 AM, Dann Frazier
> <dann.frazier@canonical.com> wrote:
>> On Thu, Aug 20, 2015 at 11:55 AM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>>> 18.08.2015 00:57, dann frazier пишет:
>>>>
>>>> grub_net_fs_open() saves off a copy of the file structure it gets passed
>>>> and
>>>> uses it to create a bufio structure. It then overwrites the passed in file
>>>> structure with this new bufio structure. Since file->name doesn't get set
>>>> until we return back to grub_file_open(), it means that only the bufio
>>>> structure gets a valid file->name. The "real" file's name is left
>>>> uninitialized. This leads to a crash when the progress module hook is
>>>> called
>>>> on it. Fix this by adding a copy of the filename during the switcheroo.
>>>
>>>
>>> Actually name was already saved off as device->net-name. What about attached
>>> patch? I'll commit leak fix separately.
>>
>> It does seem like it pokes a hole in the fs abstraction, but it works for me.
>
> hey Andrei,
>   Did you need anything more from me wrt this fix?
>

Sorry for delay. We already need to differentiate between disk and net 
files in other places anyway. So I think it is OK as a simple fix. I 
committed it.

May be bufio needs to be converted to filter but this is too intrusive 
for this.

>   -dann
>
>>>
>>>> grub_file_close() will free that string in the success case, we only need
>>>> to handle the free in an error. While we're at it, also fix a leaked file
>>>> structure in the case that grub_strdup() fails when setting
>>>> file->device->net->name.
>>>>
>>>> Finally, make progress more robust by checking for NULL before reading the
>>>> name. Andrei noticed that this could occur if grub_strdup() fails in
>>>> grub_file_open().
>>>>
>>>> Signed-off-by: dann frazier <dann.frazier@canonical.com>
>>>> ---
>>>>    grub-core/lib/progress.c |  3 +--
>>>>    grub-core/net/net.c      | 14 +++++++++++++-
>>>>    2 files changed, 14 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
>>>> index 63a0767..2775554 100644
>>>> --- a/grub-core/lib/progress.c
>>>> +++ b/grub-core/lib/progress.c
>>>> @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector
>>>> __attribute__ ((unused)),
>>>>          percent = grub_divmod64 (100 * file->progress_offset,
>>>>                                   file->size, 0);
>>>>
>>>> -      partial_file_name = grub_strrchr (file->name, '/');
>>>> -      if (partial_file_name)
>>>> +      if (file->name && (partial_file_name = grub_strrchr (file->name,
>>>> '/')))
>>>>          partial_file_name++;
>>>>          else
>>>>          partial_file_name = "";
>>>> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
>>>> index 21a4e94..9f83765 100644
>>>> --- a/grub-core/net/net.c
>>>> +++ b/grub-core/net/net.c
>>>> @@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const
>>>> char *name)
>>>>      file->device->net->packs.last = NULL;
>>>>      file->device->net->name = grub_strdup (name);
>>>>      if (!file->device->net->name)
>>>> -    return grub_errno;
>>>> +    {
>>>> +      grub_free (file);
>>>> +      return grub_errno;
>>>> +    }
>>>> +  file->name = grub_strdup (name);
>>>> +  if (!file->name)
>>>> +    {
>>>> +      grub_free (file->device->net->name);
>>>> +      grub_free (file);
>>>> +      return grub_errno;
>>>> +    }
>>>>
>>>>      err = file->device->net->protocol->open (file, name);
>>>>      if (err)
>>>> @@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const
>>>> char *name)
>>>>            grub_netbuff_free (file->device->net->packs.first->nb);
>>>>            grub_net_remove_packet (file->device->net->packs.first);
>>>>          }
>>>> +      grub_free (file->name);
>>>>          grub_free (file->device->net->name);
>>>>          grub_free (file);
>>>>          return err;
>>>> @@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const
>>>> char *name)
>>>>            grub_net_remove_packet (file->device->net->packs.first);
>>>>          }
>>>>          file->device->net->protocol->close (file);
>>>> +      grub_free (file->name);
>>>>          grub_free (file->device->net->name);
>>>>          grub_free (file);
>>>>          return grub_errno;
>>>>
>>>