Skip to contentSkip to conversation

UBIFS: Fix code issues detected by Fortify

PATCH In review hujianyang 2 participants 9 messages Jun 11, 2014 – Jul 7, 2014

Subsystems (2)
Memory technology devices (MTD) UBI file system (UBIFS)
lore
Share & tools
lore

hujianyang

author
#0 lore raw

cover UBIFS: Fix code issues detected by Fortify

With the help of Fortify, I found some problems in source code. I would like to fix them in this series of patches.

hujianyang (4):
UBIFS: Add missing error handling in create_default_filesystem()
UBIFS: Add missing error handling in dump_lpt_leb()
UBIFS: Add missing break in dbg_chk_pnode()
UBIFS: Remove useless statements

 fs/ubifs/journal.c    | 3 +++
 fs/ubifs/lpt.c        | 5 ++---
 fs/ubifs/lpt_commit.c | 7 +++++--
 fs/ubifs/orphan.c     | 1 -
 fs/ubifs/sb.c         | 4 +++-
 fs/ubifs/super.c      | 2 +-
 fs/ubifs/tnc.c        | 1 -
 fs/ubifs/tnc_commit.c | 1 -
 8 files changed, 14 insertions(+), 10 deletions(-)
Signature · 1 line
1.8.1.4
author
#1 lore raw

PATCH 1/4 UBIFS: Add missing error handling in create_default_filesystem()

In the end of create_default_filesystem(), we need to check the return value of ubifs_write_node() to ensure if we have successfully written the cs_node.

Signed-off-by hujianyang <[email protected]>
---
 fs/ubifs/sb.c | 2 ++
 1 file changed, 2 insertions(+)
fs/ubifs/sb.c +2 -0
index 4c37607..7ba1378 100644
@@ -332,6 +332,8 @@ static int create_default_filesystem(struct ubifs_info *c)
 	cs->ch.node_type = UBIFS_CS_NODE;
 	err = ubifs_write_node(c, cs, UBIFS_CS_NODE_SZ, UBIFS_LOG_LNUM, 0);
 	kfree(cs);
+	if (err)
+		return err;
 
 	ubifs_msg("default file-system created");
 	return 0;
Signature · 1 line
1.8.1.4
author
#2 lore raw

PATCH 2/4 UBIFS: Add missing error handling in dump_lpt_leb()

In reply to hujianyang

This patch checks the return value of ubifs_unpack_nnode(). If this function returns an error, @nnode may not be initialized, so just print an error mesg and break.

Signed-off-by hujianyang <[email protected]>
---
 fs/ubifs/lpt_commit.c | 5 +++++
 1 file changed, 5 insertions(+)
fs/ubifs/lpt_commit.c +5 -0
index 4b826ab..7e957b6 100644
@@ -1941,6 +1941,11 @@ static void dump_lpt_leb(const struct ubifs_info *c, int lnum)
 				pr_err("LEB %d:%d, nnode, ",
 				       lnum, offs);
 			err = ubifs_unpack_nnode(c, p, &nnode);
+			if (err) {
+				pr_err("failed to unpack_node, error %d\n",
+				       err);
+				break;
+			}
 			for (i = 0; i < UBIFS_LPT_FANOUT; i++) {
 				pr_cont("%d:%d", nnode.nbranch[i].lnum,
 				       nnode.nbranch[i].offs);
Signature · 1 line
1.8.1.4
author
#3 lore raw

PATCH 3/4 UBIFS: Add missing break in dbg_chk_pnode()

In reply to hujianyang

This is a minor fix. These two branches in dbg_chk_pnode are dealing with different conditions. Although there is no fault in current state, I think adding "break"s in each end of branch is better.

Signed-off-by hujianyang <[email protected]>
---
 fs/ubifs/lpt.c | 2 ++
 1 file changed, 2 insertions(+)
fs/ubifs/lpt.c +2 -0
index d46b19e..b4fb422 100644
@@ -2198,6 +2198,7 @@ static int dbg_chk_pnode(struct ubifs_info *c, struct ubifs_pnode *pnode,
 					  lprops->dirty);
 				return -EINVAL;
 			}
+			break;
 		case LPROPS_FREEABLE:
 		case LPROPS_FRDI_IDX:
 			if (lprops->free + lprops->dirty != c->leb_size) {

@@ -2206,6 +2207,7 @@ static int dbg_chk_pnode(struct ubifs_info *c, struct ubifs_pnode *pnode,
 					  lprops->dirty);
 				return -EINVAL;
 			}
+			break;
 		}
 	}
 	return 0;
Signature · 1 line
1.8.1.4
author
#4 lore raw

PATCH 4/4 UBIFS: Remove useless statements

In reply to hujianyang

This patch removes useless and duplicate statements. Also add a simple err mesg in journal.c to avoid a value-never-read error.

*journal.c
Add return value check.

*lpt.c & lpt_commit
No need to set @shft and @done_ltab, value never read after these statements.

*orphan.c
Remove duplicate code.

*sb.c & super.c
The types of @default_compr and @compr_type are
unsigned int, should never less than zero.

*tnc.c & tnc_commit.c
No need to set @err.

Signed-off-by hujianyang <[email protected]>
---
 fs/ubifs/journal.c    | 3 +++
 fs/ubifs/lpt.c        | 3 ---
 fs/ubifs/lpt_commit.c | 2 --
 fs/ubifs/orphan.c     | 1 -
 fs/ubifs/sb.c         | 2 +-
 fs/ubifs/super.c      | 2 +-
 fs/ubifs/tnc.c        | 1 -
 fs/ubifs/tnc_commit.c | 1 -
 8 files changed, 5 insertions(+), 10 deletions(-)
fs/ubifs/journal.c +3 -0
index 0e045e7..f68e088 100644
@@ -389,6 +389,9 @@ out:
 		ubifs_dump_budg(c, &c->bi);
 		ubifs_dump_lprops(c);
 		cmt_retries = dbg_check_lprops(c);
+		if (cmt_retries)
+			ubifs_err("fail to check lprops, error %d",
+				  cmt_retries);
 		up_write(&c->commit_sem);
 	}
 	return err;
fs/ubifs/lpt.c +0 -3
index b4fb422..421bd0a 100644
@@ -1464,7 +1464,6 @@ struct ubifs_lprops *ubifs_lpt_lookup(struct ubifs_info *c, int lnum)
 			return ERR_CAST(nnode);
 	}
 	iip = ((i >> shft) & (UBIFS_LPT_FANOUT - 1));
-	shft -= UBIFS_LPT_FANOUT_SHIFT;
 	pnode = ubifs_get_pnode(c, nnode, iip);
 	if (IS_ERR(pnode))
 		return ERR_CAST(pnode);

@@ -1604,7 +1603,6 @@ struct ubifs_lprops *ubifs_lpt_lookup_dirty(struct ubifs_info *c, int lnum)
 			return ERR_CAST(nnode);
 	}
 	iip = ((i >> shft) & (UBIFS_LPT_FANOUT - 1));
-	shft -= UBIFS_LPT_FANOUT_SHIFT;
 	pnode = ubifs_get_pnode(c, nnode, iip);
 	if (IS_ERR(pnode))
 		return ERR_CAST(pnode);

@@ -1964,7 +1962,6 @@ again:
 		}
 	}
 	iip = ((i >> shft) & (UBIFS_LPT_FANOUT - 1));
-	shft -= UBIFS_LPT_FANOUT_SHIFT;
 	pnode = scan_get_pnode(c, path + h, nnode, iip);
 	if (IS_ERR(pnode)) {
 		err = PTR_ERR(pnode);
fs/ubifs/lpt_commit.c +0 -2
index 7e957b6..db08de4 100644
@@ -304,7 +304,6 @@ static int layout_cnodes(struct ubifs_info *c)
 			ubifs_assert(lnum >= c->lpt_first &&
 				     lnum <= c->lpt_last);
 		}
-		done_ltab = 1;
 		c->ltab_lnum = lnum;
 		c->ltab_offs = offs;
 		offs += c->ltab_sz;

@@ -514,7 +513,6 @@ static int write_cnodes(struct ubifs_info *c)
 			if (err)
 				return err;
 		}
-		done_ltab = 1;
 		ubifs_pack_ltab(c, buf + offs, c->ltab_cmt);
 		offs += c->ltab_sz;
 		dbg_chk_lpt_sz(c, 1, c->ltab_sz);
fs/ubifs/orphan.c +0 -1
index f1c3e5a1..4409f48 100644
@@ -346,7 +346,6 @@ static int write_orph_nodes(struct ubifs_info *c, int atomic)
 		int lnum;
 
 		/* Unmap any unused LEBs after consolidation */
-		lnum = c->ohead_lnum + 1;
 		for (lnum = c->ohead_lnum + 1; lnum <= c->orph_last; lnum++) {
 			err = ubifs_leb_unmap(c, lnum);
 			if (err)
fs/ubifs/sb.c +1 -1
index 7ba1378..79c6dbb 100644
@@ -449,7 +449,7 @@ static int validate_sb(struct ubifs_info *c, struct ubifs_sb_node *sup)
 		goto failed;
 	}
 
-	if (c->default_compr < 0 || c->default_compr >= UBIFS_COMPR_TYPES_CNT) {
+	if (c->default_compr >= UBIFS_COMPR_TYPES_CNT) {
 		err = 13;
 		goto failed;
 	}
fs/ubifs/super.c +1 -1
index 3904c85..3b0c2c0 100644
@@ -75,7 +75,7 @@ static int validate_inode(struct ubifs_info *c, const struct inode *inode)
 		return 1;
 	}
 
-	if (ui->compr_type < 0 || ui->compr_type >= UBIFS_COMPR_TYPES_CNT) {
+	if (ui->compr_type >= UBIFS_COMPR_TYPES_CNT) {
 		ubifs_err("unknown compression type %d", ui->compr_type);
 		return 2;
 	}
fs/ubifs/tnc.c +0 -1
index 8a40cf9..6793db0 100644
@@ -3294,7 +3294,6 @@ int dbg_check_inode_size(struct ubifs_info *c, const struct inode *inode,
 		goto out_unlock;
 
 	if (err) {
-		err = -EINVAL;
 		key = &from_key;
 		goto out_dump;
 	}
fs/ubifs/tnc_commit.c +0 -1
index 52a6559..e570734 100644
@@ -389,7 +389,6 @@ static int layout_in_gaps(struct ubifs_info *c, int cnt)
 				ubifs_dump_lprops(c);
 			}
 			/* Try to commit anyway */
-			err = 0;
 			break;
 		}
 		p++;
Signature · 1 line
1.8.1.4
#5 lore raw

Re: [PATCH 4/4] UBIFS: Remove useless statements

fs/ubifs/journal.c out: · line 395

Quoted 12 lines · hujianyang
fs/ubifs/journal.c +3 -0
index 0e045e7..f68e088 100644
@@ -389,6 +389,9 @@ out:
 		ubifs_dump_budg(c, &c->bi);
 		ubifs_dump_lprops(c);
 		cmt_retries = dbg_check_lprops(c);
+		if (cmt_retries)
+			ubifs_err("fail to check lprops, error %d",
+				  cmt_retries);
 		up_write(&c->commit_sem);

The general approach we took in UBIFS is that the function prints about its own problems itself. We tend to avoid not make calling function print an error message for the called function.

Please, add an error message to 'dbg_check_lprops()' instead.

I've dropped this change. The other ones look OK, so I applied them.

Thanks!

Signature · 2 lines
Best Regards,
Artem Bityutskiy
author
#6 lore raw

Re: [PATCH 4/4] UBIFS: Remove useless statements

Quoted 18 lines · Artem Bityutskiy

fs/ubifs/journal.c out: · line 395

Quoted 12 lines · hujianyang
fs/ubifs/journal.c +3 -0
index 0e045e7..f68e088 100644
@@ -389,6 +389,9 @@ out:
 		ubifs_dump_budg(c, &c->bi);
 		ubifs_dump_lprops(c);
 		cmt_retries = dbg_check_lprops(c);
+		if (cmt_retries)
+			ubifs_err("fail to check lprops, error %d",
+				  cmt_retries);
 		up_write(&c->commit_sem);

The general approach we took in UBIFS is that the function prints about its own problems itself. We tend to avoid not make calling function print an error message for the called function.

Quoted 1 line

Please, add an error message to 'dbg_check_lprops()' instead.

I think error messages in 'dbg_check_lprops' is good enough.

Quoted 5 lines

I've dropped this change. The other ones look OK, so I applied them.

Thanks!

I add the return value check because a value-never-read warn. We set this cmt_retries and never read it. But it is really no nead check it as you said.

I think this is better.

- cmt_retries = dbg_check_lprops(c);
+ dbg_check_lprops(c);

There is not need to change this by a separate patch. Just leave it behind.

#8 lore raw
In reply to hujianyang
Quoted 8 lines · hujianyang

With the help of Fortify, I found some problems in source code. I would like to fix them in this series of patches.

hujianyang (4):
UBIFS: Add missing error handling in create_default_filesystem()
UBIFS: Add missing error handling in dump_lpt_leb()
UBIFS: Add missing break in dbg_chk_pnode()
UBIFS: Remove useless statements

Pushed patches 1-3 as is, but removed one change from patch 4, and pushed too.

Signature · 2 lines
Best Regards,
Artem Bityutskiy