linux-kernel - Re: [PATCH][next] erofs: fix uninitialized variable i used in a while-loop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b56a44542a338583279893870ec819d4b1b4e23b.camel@perches.com>
Date:   Tue, 06 Apr 2021 20:38:44 -0700
From:   Joe Perches <joe@...ches.com>
To:     Gao Xiang <hsiangkao@...hat.com>,
        Colin King <colin.king@...onical.com>
Cc:     Gao Xiang <xiang@...nel.org>, Chao Yu <chao@...nel.org>,
        linux-erofs@...ts.ozlabs.org, kernel-janitors@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH][next] erofs: fix uninitialized variable i used in a
 while-loop

On Wed, 2021-04-07 at 07:54 +0800, Gao Xiang wrote:
> Hi Colin,
> 
> On Tue, Apr 06, 2021 at 05:27:18PM +0100, Colin King wrote:
> > From: Colin Ian King <colin.king@...onical.com>
> > 
> > The while-loop iterates until src is non-null or i is 3, however, the
> > loop counter i is not intinitialied to zero, causing incorrect iteration
> > counts.  Fix this by initializing it to zero.
> > 
> > Addresses-Coverity: ("Uninitialized scalar variable")
> > Fixes: 1aa5f2e2feed ("erofs: support decompress big pcluster for lz4 backend")
> > Signed-off-by: Colin Ian King <colin.king@...onical.com>
> 
> Thank you very much for catching this! It looks good to me,
> Reviewed-by: Gao Xiang <hsiangkao@...hat.com>
> 
> (btw, may I fold this into the original patchset? since such big pcluster
>  patchset is just applied to for-next for further integration testing, and
>  the commit id is not stable yet..)
> 
> Thanks,
> Gao Xiang

I think this code is odd and would be more intelligible using
a for loop like:
---
 fs/erofs/decompressor.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
index 27aa6a99b371..5a64f4649414 100644
--- a/fs/erofs/decompressor.c
+++ b/fs/erofs/decompressor.c
@@ -286,28 +286,24 @@ static int z_erofs_decompress_generic(struct z_erofs_decompress_req *rq,
 	}
 
 	ret = alg->prepare_destpages(rq, pagepool);
-	if (ret < 0) {
+	if (ret < 0)
 		return ret;
-	} else if (ret) {
+	if (ret) {
 		dst = page_address(*rq->out);
 		dst_maptype = 1;
 		goto dstmap_out;
 	}
 
-	i = 0;
-	while (1) {
+	for (i = 0; i < 3; i++) {
 		dst = vm_map_ram(rq->out, nrpages_out, -1);
-
+		if (dst) {
+			dst_maptype = 2;
+			goto dstmap_out;
+		}
 		/* retry two more times (totally 3 times) */
-		if (dst || ++i >= 3)
-			break;
 		vm_unmap_aliases();
 	}
-
-	if (!dst)
-		return -ENOMEM;
-
-	dst_maptype = 2;
+	return -ENOMEM;
 
 dstmap_out:
 	ret = alg->decompress(rq, dst + rq->pageofs_out);