[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Andrei Borzenkov
Subject: Re: [PATCH] Avoid NULL pointer dereference in progress module.
Date: Sat, 10 Oct 2015 11:44:07 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0

21.09.2015 18:11, Dann Frazier пишет:
On Fri, Aug 21, 2015 at 8:24 AM, Dann Frazier
<address@hidden> wrote:
On Thu, Aug 20, 2015 at 11:55 AM, Andrei Borzenkov <address@hidden> wrote:
18.08.2015 00:57, dann frazier пишет:

grub_net_fs_open() saves off a copy of the file structure it gets passed
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
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.


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

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

Signed-off-by: dann frazier <address@hidden>
   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 = "";
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;

reply via email to

[Prev in Thread] Current Thread [Next in Thread]