[PATCH 1/1 linux-next] udf: improve udf_get_filename() error management

[PATCH 1/1 linux-next] udf: improve udf_get_filename() error management

[Fabian Frederick]

Let udf_get_filename() return errors instead of 0.

udf_pc_to_char() now returns error accordingly.
udf_readdir() and udf_find_entry() process is done on
positive result.

Note that error propagation has still to be done in the
later functions.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
This is untested.

 fs/udf/dir.c     |  2 +-
 fs/udf/namei.c   |  3 ++-
 fs/udf/symlink.c |  3 +++
 fs/udf/unicode.c | 12 +++++++-----
 4 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/fs/udf/dir.c b/fs/udf/dir.c
index 05e90ed..98bcc27 100644
--- a/fs/udf/dir.c
+++ b/fs/udf/dir.c
@@ -169,7 +169,7 @@ static int udf_readdir(struct file *file, struct dir_context *ctx)
 		}
 
 		flen = udf_get_filename(sb, nameptr, lfi, fname, UDF_NAME_LEN);
-		if (!flen)
+		if (flen <= 0)
 			continue;
 
 		tloc = lelb_to_cpu(cfi.icb.extLocation);
diff --git a/fs/udf/namei.c b/fs/udf/namei.c
index 33b246b..bab34d1 100644
--- a/fs/udf/namei.c
+++ b/fs/udf/namei.c
@@ -235,7 +235,8 @@ static struct fileIdentDesc *udf_find_entry(struct inode *dir,
 			continue;
 
 		flen = udf_get_filename(sb, nameptr, lfi, fname, UDF_NAME_LEN);
-		if (flen && udf_match(flen, fname, child->len, child->name))
+		if ((flen > 0) && udf_match(flen, fname, child->len,
+					    child->name))
 			goto out_ok;
 	}
 
diff --git a/fs/udf/symlink.c b/fs/udf/symlink.c
index ac10ca9..a9f41fa 100644
--- a/fs/udf/symlink.c
+++ b/fs/udf/symlink.c
@@ -83,6 +83,9 @@ static int udf_pc_to_char(struct super_block *sb, unsigned char *from,
 			comp_len = udf_get_filename(sb, pc->componentIdent,
 						    pc->lengthComponentIdent,
 						    p, tolen);
+			if (comp_len < 0)
+				return comp_len;
+
 			p += comp_len;
 			tolen -= comp_len;
 			if (tolen == 0)
diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
index b84fee3..4911c1d 100644
--- a/fs/udf/unicode.c
+++ b/fs/udf/unicode.c
@@ -338,15 +338,17 @@ int udf_get_filename(struct super_block *sb, uint8_t *sname, int slen,
 		     uint8_t *dname, int dlen)
 {
 	struct ustr *filename, *unifilename;
-	int len = 0;
+	int ret = 0;
 
 	filename = kmalloc(sizeof(struct ustr), GFP_NOFS);
 	if (!filename)
-		return 0;
+		return -ENOMEM;
 
 	unifilename = kmalloc(sizeof(struct ustr), GFP_NOFS);
-	if (!unifilename)
+	if (!unifilename) {
+		ret = -ENOMEM;
 		goto out1;
+	}
 
 	if (udf_build_ustr_exact(unifilename, sname, slen))
 		goto out2;
@@ -367,14 +369,14 @@ int udf_get_filename(struct super_block *sb, uint8_t *sname, int slen,
 	} else
 		goto out2;
 
-	len = udf_translate_to_linux(dname, dlen,
+	ret = udf_translate_to_linux(dname, dlen,
 				     filename->u_name, filename->u_len,
 				     unifilename->u_name, unifilename->u_len);
 out2:
 	kfree(unifilename);
 out1:
 	kfree(filename);
-	return len;
+	return ret;
 }
 
 int udf_put_filename(struct super_block *sb, const uint8_t *sname,
-- 
2.1.0

Re: [PATCH 1/1 linux-next] udf: improve udf_get_filename() error management

[Jan Kara]

On Mon 16-03-15 21:20:23, Fabian Frederick wrote:
> Let udf_get_filename() return errors instead of 0.
> 
> udf_pc_to_char() now returns error accordingly.
> udf_readdir() and udf_find_entry() process is done on
> positive result.
> 
> Note that error propagation has still to be done in the
> later functions.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Fabian Frederick <fabf@skynet.be>
...
> diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
> index b84fee3..4911c1d 100644
> --- a/fs/udf/unicode.c
> +++ b/fs/udf/unicode.c
> @@ -338,15 +338,17 @@ int udf_get_filename(struct super_block *sb, uint8_t *sname, int slen,
>  		     uint8_t *dname, int dlen)
>  {
>  	struct ustr *filename, *unifilename;
> -	int len = 0;
> +	int ret = 0;
>  
>  	filename = kmalloc(sizeof(struct ustr), GFP_NOFS);
>  	if (!filename)
> -		return 0;
> +		return -ENOMEM;
>  
>  	unifilename = kmalloc(sizeof(struct ustr), GFP_NOFS);
> -	if (!unifilename)
> +	if (!unifilename) {
> +		ret = -ENOMEM;
>  		goto out1;
> +	}
>  
>  	if (udf_build_ustr_exact(unifilename, sname, slen))
>  		goto out2;
> @@ -367,14 +369,14 @@ int udf_get_filename(struct super_block *sb, uint8_t *sname, int slen,
>  	} else
>  		goto out2;
>  
> -	len = udf_translate_to_linux(dname, dlen,
> +	ret = udf_translate_to_linux(dname, dlen,
>  				     filename->u_name, filename->u_len,
>  				     unifilename->u_name, unifilename->u_len);
>  out2:
>  	kfree(unifilename);
>  out1:
>  	kfree(filename);
> -	return len;
> +	return ret;
>  }
  I think you need to do more work around udf_get_filename() if you want
error handling to function correctly.  The test in udf_build_ustr_exact()
which can make it fail is just bogus - remove it. udf_CS0toUTF8() and
udf_CS0toNLS() may fail as well - make them return error (-EINVAL?) and
propagate that up. The else branch in udf_get_filename() should be a BUG()
- mount code makes sure one of UDF_FLAG_NLS_MAP and UDF_FLAG_UTF8 is set.
That should deal with all the error paths so we really return value < 0 if
we fail and positive value if we succeed.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR