[PATCH 0/4] UBIFS: Fix code issues detected by Fortify

[PATCH 0/4] UBIFS: Fix code issues detected by Fortify

[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

 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(-)

-- 
1.8.1.4

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

[hujianyang]

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 <hujianyang@huawei.com>
---
 fs/ubifs/sb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
index 4c37607..7ba1378 100644
--- a/fs/ubifs/sb.c
+++ b/fs/ubifs/sb.c
@@ -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;
-- 
1.8.1.4

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

[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 <hujianyang@huawei.com>
---
 fs/ubifs/lpt_commit.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/ubifs/lpt_commit.c b/fs/ubifs/lpt_commit.c
index 4b826ab..7e957b6 100644
--- a/fs/ubifs/lpt_commit.c
+++ b/fs/ubifs/lpt_commit.c
@@ -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);
-- 
1.8.1.4

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

[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 <hujianyang@huawei.com>
---
 fs/ubifs/lpt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ubifs/lpt.c b/fs/ubifs/lpt.c
index d46b19e..b4fb422 100644
--- a/fs/ubifs/lpt.c
+++ b/fs/ubifs/lpt.c
@@ -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;
-- 
1.8.1.4

[PATCH 4/4] UBIFS: Remove useless statements

[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 <hujianyang@huawei.com>
---
 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(-)

diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index 0e045e7..f68e088 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -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;
diff --git a/fs/ubifs/lpt.c b/fs/ubifs/lpt.c
index b4fb422..421bd0a 100644
--- a/fs/ubifs/lpt.c
+++ b/fs/ubifs/lpt.c
@@ -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);
diff --git a/fs/ubifs/lpt_commit.c b/fs/ubifs/lpt_commit.c
index 7e957b6..db08de4 100644
--- a/fs/ubifs/lpt_commit.c
+++ b/fs/ubifs/lpt_commit.c
@@ -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);
diff --git a/fs/ubifs/orphan.c b/fs/ubifs/orphan.c
index f1c3e5a1..4409f48 100644
--- a/fs/ubifs/orphan.c
+++ b/fs/ubifs/orphan.c
@@ -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)
diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
index 7ba1378..79c6dbb 100644
--- a/fs/ubifs/sb.c
+++ b/fs/ubifs/sb.c
@@ -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;
 	}
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 3904c85..3b0c2c0 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -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;
 	}
diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
index 8a40cf9..6793db0 100644
--- a/fs/ubifs/tnc.c
+++ b/fs/ubifs/tnc.c
@@ -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;
 	}
diff --git a/fs/ubifs/tnc_commit.c b/fs/ubifs/tnc_commit.c
index 52a6559..e570734 100644
--- a/fs/ubifs/tnc_commit.c
+++ b/fs/ubifs/tnc_commit.c
@@ -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++;
-- 
1.8.1.4

Re: [PATCH 0/4] UBIFS: Fix code issues detected by Fortify

[hujianyang]

ping?

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

[Artem Bityutskiy]

On Wed, 2014-06-11 at 10:42 +0800, hujianyang wrote:
> diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
> index 0e045e7..f68e088 100644
> --- a/fs/ubifs/journal.c
> +++ b/fs/ubifs/journal.c
> @@ -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!

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 0/4] UBIFS: Fix code issues detected by Fortify

[Artem Bityutskiy]

On Wed, 2014-06-11 at 10:37 +0800, hujianyang wrote:
> 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.

-- 
Best Regards,
Artem Bityutskiy

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

[hujianyang]

On 2014/7/1 20:37, Artem Bityutskiy wrote:
> On Wed, 2014-06-11 at 10:42 +0800, hujianyang wrote:
>> diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
>> index 0e045e7..f68e088 100644
>> --- a/fs/ubifs/journal.c
>> +++ b/fs/ubifs/journal.c
>> @@ -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 think error messages in 'dbg_check_lprops' is good enough.

> 
> 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.