From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1aprzB-00079J-5r for mharc-grub-devel@gnu.org; Tue, 12 Apr 2016 02:39:41 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52212) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aprz8-00076i-To for grub-devel@gnu.org; Tue, 12 Apr 2016 02:39:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aprz5-0004PV-NL for grub-devel@gnu.org; Tue, 12 Apr 2016 02:39:38 -0400 Received: from smtp.nue.novell.com ([195.135.221.5]:38087) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aprz5-0004PL-Dx for grub-devel@gnu.org; Tue, 12 Apr 2016 02:39:35 -0400 Received: from nwb-ext-pat.microfocus.com ([10.120.13.103]) by smtp.nue.novell.com with ESMTP (TLS encrypted); Tue, 12 Apr 2016 08:39:33 +0200 Received: from linux-9gqx.suse (nwb-a10-snat.microfocus.com [10.120.13.202]) by nwb-ext-pat.microfocus.com with ESMTP (TLS encrypted); Tue, 12 Apr 2016 07:39:28 +0100 Date: Tue, 12 Apr 2016 14:39:21 +0800 From: Michael Chang To: The development of GNU GRUB Subject: Re: [PATCH] grub-file: fix segmentation fault Message-ID: <20160412063921.GA17027@linux-9gqx.suse> Mail-Followup-To: The development of GNU GRUB References: <20160408064322.GA15761@linux-9gqx.suse> <57087EAE.6070903@gmail.com> <20160411040002.GA4508@linux-9gqx.suse> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 195.135.221.5 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Apr 2016 06:39:40 -0000 On Mon, Apr 11, 2016 at 12:28:35PM +0300, Andrei Borzenkov wrote: > On Mon, Apr 11, 2016 at 7:00 AM, Michael Chang wrote: > > On Sat, Apr 09, 2016 at 07:01:50AM +0300, Andrei Borzenkov wrote: > >> 08.04.2016 09:43, Michael Chang пишет: > >> > In grub_file_open the file handle returned by file filters has no file->name > >> > set which leads to segmentation fault later referenced by grub_elf_file. We > >> > move the file->name value assignment after file filters to make sure it will be > >> > set and returned. > >> > > >> > >> This now makes filename unavailable to progress module (which gets the > >> last grub_file in a chain) and it still does not cover corner case of > >> failing grub_strdup in grub_file_open. > > > > I don't get why the filename would, in the other way round to this patch trying > > to fix, become unavailable to progress module? As far as I see the file > > progress read hook in grub_file_read would use the file handle returned > > from grub_file_open and do not hold another chaining of opened files .. > > > > porogress module for disk IO relies on file hooks; what we have is > > if (!file->read_hook) > { > file->read_hook = grub_file_progress_hook; > file->read_hook_data = file; > file->progress_offset = file->offset; > } > > Each filter is implemented as pseudo-fs with own ->read function. This > function calls lower fs or filter as needed. So we basically have > > grub_file_read (filter1) > filter1->read > grub_file_read (filter2) > filter2->read > grub_file_read (real_file) > real_file->read > disk_read Thanks, I modified the send v2 patch to keep the original file->name as io handle for filters. It will now only assign file->name if it's emtpy in returned handle from filters to make sure it's available to user. > > Each grub_file_read will re-set file hooks to progress on its > argument, but only the very last disk_read will actually interpret > them. And it will get as data pointer to real_file and will fetch file > name from there. > > Note that network code explicitly calls progress and does not rely on > above framework at all. > > Oh ... and it looks like if user explicitly set file hooks on top > level filter, these hooks are not propagated at all. Which may or may > not be intentional. > > This is a mess, and needs cleanup. It is really into something that I do not consider or expect to fix, it is too far from fixing the segmentation fault problem. If we forced into that field instead of a trivial fix that's really sad to me. Thanks, Michael > > > About covering the grub_strdup failure, the patch didn't do because it's not > > the cause for the segfault so leaving it as it is, if you think it necessary we > > can handle the error by returning null handle of course. > > I do not know if this is intentional. Vladimir? Personally I'd say > that if we could not allocate several bytes for file name, we unlikely > can continue to do anything useful. > > >> > >> Fixing the former requires some redesign. But as long as we allow > >> filename to remain empty in grub_file_open every user must explicitly > >> check for it being NULL. > > > > For what reason the filename returned by grub_file_open would be empty and how > > to know it reasonable from the user ? Adding the check is fine, but still a bug > > to me a filename is provided during grub_file_open but get ditched in returned > > handle without a reason. > > > > Thanks, > > Michael > > > > _______________________________________________ > > Grub-devel mailing list > > Grub-devel@gnu.org > > https://lists.gnu.org/mailman/listinfo/grub-devel > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel