From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1ZPzfh-0004em-6s for mharc-grub-devel@gnu.org; Thu, 13 Aug 2015 17:04:21 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41248) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPzfd-0004dW-MA for grub-devel@gnu.org; Thu, 13 Aug 2015 17:04:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZPzfY-0004yp-Ts for grub-devel@gnu.org; Thu, 13 Aug 2015 17:04:17 -0400 Received: from complete.lackof.org ([198.49.126.79]:36899) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPzfY-0004yU-Nw for grub-devel@gnu.org; Thu, 13 Aug 2015 17:04:12 -0400 Received: from localhost (c-107-2-141-92.hsd1.co.comcast.net [107.2.141.92]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by complete.lackof.org (Postfix) with ESMTPSA id 580AF33E00C0; Thu, 13 Aug 2015 15:04:10 -0600 (MDT) Date: Thu, 13 Aug 2015 15:04:10 -0600 From: dann frazier To: Andrei Borzenkov Subject: Re: [PATCH] progress: Check for NULL filename Message-ID: <20150813210409.GA26195@fluid.dannf> References: <1439394805-8573-1-git-send-email-dann.frazier@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 198.49.126.79 Cc: The development of GNU GRUB X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 13 Aug 2015 21:04:19 -0000 On Thu, Aug 13, 2015 at 10:52:19AM +0300, Andrei Borzenkov wrote: > On Wed, Aug 12, 2015 at 6:53 PM, dann frazier > 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 > > --- > > 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