From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753326AbcADJz7 (ORCPT ); Mon, 4 Jan 2016 04:55:59 -0500 Received: from mx2.suse.de ([195.135.220.15]:47524 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753294AbcADJz5 (ORCPT ); Mon, 4 Jan 2016 04:55:57 -0500 Date: Mon, 4 Jan 2016 10:56:05 +0100 From: Jan Kara To: Arnd Bergmann Cc: linux-arm-kernel@lists.infradead.org, Jan Kara , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] udf: avoid uninitialized variable use Message-ID: <20160104095605.GC3888@quack.suse.cz> References: <9221187.T0u8gszVyj@wuerfel> <5491623.oZlI0zcRmi@wuerfel> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="9jxsPFA5p3P2qPhR" Content-Disposition: inline In-Reply-To: <5491623.oZlI0zcRmi@wuerfel> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --9jxsPFA5p3P2qPhR Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri 01-01-16 15:21:54, Arnd Bergmann wrote: > A new warning about a real bug has come up from a recent cleanup: > > fs/udf/inode.c: In function 'udf_setup_indirect_aext': > fs/udf/inode.c:1927:28: warning: 'adsize' may be used uninitialized in this function [-Wmaybe-uninitialized] > > If the alloc_type is neither ICBTAG_FLAG_AD_SHORT nor ICBTAG_FLAG_AD_LONG, > the value of adsize is undefined. This changes the code to use zero for adsize > in that case, which may be the correct solution, though I have not looked > at the code in enough detail to know if it should be something else instead. > > Signed-off-by: Arnd Bergmann > Fixes: fcea62babc81 ("udf: Factor out code for creating indirect extent") > --- > sorry for missing another instance the first time around. The warning is > a bit unreliable and it seems in my first configuration I got it only > for one of the two instances that show it in other configuration. > > After checking the remaining functions in this file for the same possible > problem, I found that the other functions use either 'BUG()' or 'return -EIO' > in the 'else' path, so I assume the two functions here should one of those > as well, but I don't know which. Callers of these functions make sure alloc_type is one of the two valid ones. However for future-proofing you're right that probably we should handle the invalid case as well. Setting adsize to zero is problematic - not sure what the code would actually do but it wouldn't definitely work. I'd just return -EIO. Attached is the patch I have merged. Honza > > diff --git a/fs/udf/inode.c b/fs/udf/inode.c > index 5b83351041a4..42f68dd7e6ef 100644 > --- a/fs/udf/inode.c > +++ b/fs/udf/inode.c > @@ -1890,6 +1890,8 @@ int udf_setup_indirect_aext(struct inode *inode, int block, > adsize = sizeof(struct short_ad); > else if (UDF_I(inode)->i_alloc_type == ICBTAG_FLAG_AD_LONG) > adsize = sizeof(struct long_ad); > + else > + adsize = 0; > > neloc.logicalBlockNum = block; > neloc.partitionReferenceNum = epos->block.partitionReferenceNum; > @@ -1963,6 +1965,8 @@ int __udf_add_aext(struct inode *inode, struct extent_position *epos, > adsize = sizeof(struct short_ad); > else if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_LONG) > adsize = sizeof(struct long_ad); > + else > + adsize = 0; > > if (!epos->bh) { > WARN_ON(iinfo->i_lenAlloc != > -- Jan Kara SUSE Labs, CR --9jxsPFA5p3P2qPhR Content-Type: text/x-patch; charset=us-ascii Content-Disposition: attachment; filename="0001-udf-avoid-uninitialized-variable-use.patch" >>From 4f1b1519f7bec44ded3c2c4d46a2594c01446dc8 Mon Sep 17 00:00:00 2001 From: Arnd Bergmann Date: Fri, 1 Jan 2016 15:21:54 +0100 Subject: [PATCH] udf: avoid uninitialized variable use A new warning has come up from a recent cleanup: fs/udf/inode.c: In function 'udf_setup_indirect_aext': fs/udf/inode.c:1927:28: warning: 'adsize' may be used uninitialized in this function [-Wmaybe-uninitialized] If the alloc_type is neither ICBTAG_FLAG_AD_SHORT nor ICBTAG_FLAG_AD_LONG, the value of adsize is undefined. Currently, callers of these functions make sure alloc_type is one of the two valid ones but for future proofing make sure we handle the case of invalid alloc type as well. This changes the code to return -EIOin that case. Signed-off-by: Arnd Bergmann Fixes: fcea62babc81 ("udf: Factor out code for creating indirect extent") Signed-off-by: Jan Kara --- fs/udf/inode.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/fs/udf/inode.c b/fs/udf/inode.c index 846294891925..91d8fa9d87a4 100644 --- a/fs/udf/inode.c +++ b/fs/udf/inode.c @@ -1889,6 +1889,8 @@ int udf_setup_indirect_aext(struct inode *inode, int block, adsize = sizeof(struct short_ad); else if (UDF_I(inode)->i_alloc_type == ICBTAG_FLAG_AD_LONG) adsize = sizeof(struct long_ad); + else + return -EIO; neloc.logicalBlockNum = block; neloc.partitionReferenceNum = epos->block.partitionReferenceNum; @@ -1962,6 +1964,8 @@ int __udf_add_aext(struct inode *inode, struct extent_position *epos, adsize = sizeof(struct short_ad); else if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_LONG) adsize = sizeof(struct long_ad); + else + return -EIO; if (!epos->bh) { WARN_ON(iinfo->i_lenAlloc != -- 2.6.2 --9jxsPFA5p3P2qPhR--