From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1ZRgKU-0001gK-IU for mharc-grub-devel@gnu.org; Tue, 18 Aug 2015 08:49:26 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41493) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZRPTE-00073N-DB for grub-devel@gnu.org; Mon, 17 Aug 2015 14:49:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZRPTB-0007kA-5h for grub-devel@gnu.org; Mon, 17 Aug 2015 14:49:20 -0400 Received: from mail-lb0-x234.google.com ([2a00:1450:4010:c04::234]:33057) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZRPTA-0007jq-UY for grub-devel@gnu.org; Mon, 17 Aug 2015 14:49:17 -0400 Received: by lbbsx3 with SMTP id sx3so88161136lbb.0 for ; Mon, 17 Aug 2015 11:49:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=/7gFS2CBAj3MZwWnzNmVnqXEAL6fvNpTgtO71361g6I=; b=X0w/ZN/ZG6WHQDck8dJOtmkb4KenYcvSSKQmt3rl0MmyXE8paq3KMdeeR09u6OOtvk ojcqkSDK8pXLvfeWMiC4Wvi2j0CLjN0XVqjl/pJovVvJBorddXw+wkkqBo1YE6kI3eFH el4EXNrtCkT6Z7x1D8/4Gb3Kiif8EYZFVesOW/2/Tp0EOpY0uBturo3TticJcBwFvlVz JSJ/64gDqyu11HLPisAzV5DQytHPEbzwUYXyK0fALwf8DBMUbQvzAe7nwE6xF8AazZpf veVezWfsd8ybPvuK3QAN5Yj8n3LnSyXrveJpFV79U3FCdt7xJ3F9QMTRpD9jvIDJBxFX L3qQ== X-Received: by 10.112.77.168 with SMTP id t8mr2302271lbw.70.1439837355724; Mon, 17 Aug 2015 11:49:15 -0700 (PDT) Received: from [192.168.1.43] (ppp91-76-5-127.pppoe.mtu-net.ru. [91.76.5.127]) by smtp.gmail.com with ESMTPSA id t12sm4112829lbs.19.2015.08.17.11.49.14 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 17 Aug 2015 11:49:15 -0700 (PDT) Message-ID: <55D22CAA.3040200@gmail.com> Date: Mon, 17 Aug 2015 21:49:14 +0300 From: Andrei Borzenkov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: dann frazier Subject: Re: [PATCH] progress: Check for NULL filename References: <1439394805-8573-1-git-send-email-dann.frazier@canonical.com> <20150813210409.GA26195@fluid.dannf> In-Reply-To: <20150813210409.GA26195@fluid.dannf> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4010:c04::234 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: Tue, 18 Aug 2015 12:49:24 -0000 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 >> 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 >>> --- >>> 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