Re: [RFC PATCH 13/20] hfsplus: Do not use broken utf8 NLS table for iocharset=utf8 mount option
From: Viacheslav Dubeyko <slava@dubeyko.com>
Date: 2021-08-09 17:42:10
Also in:
linux-fsdevel, lkml
quoted hunk ↗ jump to hunk
On Aug 8, 2021, at 9:24 AM, Pali Rohár [off-list ref] wrote: NLS table for utf8 is broken and cannot be fixed. So instead of broken utf8 nls functions char2uni() and uni2char() use functions utf8_to_utf32() and utf32_to_utf8() which implements correct encoding and decoding between Unicode code points and UTF-8 sequence. Note that this fs driver does not support full Unicode range, specially UTF-16 surrogate pairs are unsupported. This patch does not change this limitation and support for UTF-16 surrogate pairs stay unimplemented. When iochatset=utf8 is used then set sbi->nls to NULL and use it for distinguish between the fact if NLS table or native UTF-8 functions should be used. Signed-off-by: Pali Rohár <pali@kernel.org> --- fs/hfsplus/dir.c | 6 ++++-- fs/hfsplus/options.c | 32 ++++++++++++++++++-------------- fs/hfsplus/super.c | 7 +------ fs/hfsplus/unicode.c | 31 ++++++++++++++++++++++++++++--- fs/hfsplus/xattr.c | 14 +++++++++----- fs/hfsplus/xattr_security.c | 3 ++- 6 files changed, 62 insertions(+), 31 deletions(-)diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c index 84714bbccc12..2caf0cd82221 100644 --- a/fs/hfsplus/dir.c +++ b/fs/hfsplus/dir.c@@ -144,7 +144,8 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd); if (err) return err; - strbuf = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN + 1, GFP_KERNEL); + strbuf = kmalloc((HFSPLUS_SB(sb)->nls ? NLS_MAX_CHARSET_SIZE : 4) * + HFSPLUS_MAX_STRLEN + 1, GFP_KERNEL);
Maybe, introduce some variable that will contain the length calculation?
quoted hunk ↗ jump to hunk
if (!strbuf) { err = -ENOMEM; goto out;@@ -203,7 +204,8 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)hfs_bnode_read(fd.bnode, &entry, fd.entryoffset, fd.entrylength); type = be16_to_cpu(entry.type); - len = NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN; + len = (HFSPLUS_SB(sb)->nls ? NLS_MAX_CHARSET_SIZE : 4) * + HFSPLUS_MAX_STRLEN; err = hfsplus_uni2asc(sb, &fd.key->cat.name, strbuf, &len); if (err) goto out;diff --git a/fs/hfsplus/options.c b/fs/hfsplus/options.c index a975548f6b91..16c08cb5c4f8 100644 --- a/fs/hfsplus/options.c +++ b/fs/hfsplus/options.c@@ -104,6 +104,9 @@ int hfsplus_parse_options(char *input, struct hfsplus_sb_info *sbi)char *p; substring_t args[MAX_OPT_ARGS]; int tmp, token; + int have_iocharset; + + have_iocharset = 0;
What’s about boolean type and to use true/false?
quoted hunk ↗ jump to hunk
if (!input) goto done;@@ -171,20 +174,24 @@ int hfsplus_parse_options(char *input, struct hfsplus_sb_info *sbi)pr_warn("option nls= is deprecated, use iocharset=\n"); /* fallthrough */ case opt_iocharset: - if (sbi->nls) { + if (have_iocharset) { pr_err("unable to change nls mapping\n"); return 0; } p = match_strdup(&args[0]); - if (p) - sbi->nls = load_nls(p); - if (!sbi->nls) { - pr_err("unable to load nls mapping \"%s\"\n", - p); - kfree(p); + if (!p) return 0; + if (strcmp(p, "utf8") != 0) { + sbi->nls = load_nls(p); + if (!sbi->nls) { + pr_err("unable to load nls mapping " + "\"%s\"\n", p); + kfree(p); + return 0; + } } kfree(p); + have_iocharset = 1;
Ditto. What’s about true here?
quoted hunk ↗ jump to hunk
break; case opt_decompose: clear_bit(HFSPLUS_SB_NODECOMPOSE, &sbi->flags);@@ -207,13 +214,10 @@ int hfsplus_parse_options(char *input, struct hfsplus_sb_info *sbi)} done: - if (!sbi->nls) { - /* try utf8 first, as this is the old default behaviour */ - sbi->nls = load_nls("utf8"); - if (!sbi->nls) - sbi->nls = load_nls_default(); - if (!sbi->nls) - return 0; + if (!have_iocharset) { + /* use utf8, as this is the old default behaviour */ + pr_debug("using native UTF-8 without nls\n"); + /* no sbi->nls means that native UTF-8 code is used */ } return 1;diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c index b9e3db3f855f..985662451bfc 100644 --- a/fs/hfsplus/super.c +++ b/fs/hfsplus/super.c@@ -403,11 +403,7 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)/* temporarily use utf8 to correctly find the hidden dir below */ nls = sbi->nls; - sbi->nls = load_nls("utf8"); - if (!sbi->nls) { - pr_err("unable to load nls for utf8\n"); - goto out_unload_nls; - } + sbi->nls = NULL; /* Grab the volume header */ if (hfsplus_read_wrapper(sb)) {@@ -585,7 +581,6 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)} } - unload_nls(sbi->nls); sbi->nls = nls; return 0;diff --git a/fs/hfsplus/unicode.c b/fs/hfsplus/unicode.c index 73342c925a4b..1d8c31c5126f 100644 --- a/fs/hfsplus/unicode.c +++ b/fs/hfsplus/unicode.c@@ -190,7 +190,12 @@ int hfsplus_uni2asc(struct super_block *sb,c0 = ':'; break; } - res = nls->uni2char(c0, op, len); + if (nls) + res = nls->uni2char(c0, op, len); + else (len > 0) + res = utf32_to_utf8(c0, op, len); + else + res = -ENAMETOOLONG; if (res < 0) { if (res == -ENAMETOOLONG) goto out;@@ -233,7 +238,12 @@ int hfsplus_uni2asc(struct super_block *sb,cc = c0; } done: - res = nls->uni2char(cc, op, len); + if (nls) + res = nls->uni2char(cc, op, len); + else (len > 0) + res = utf32_to_utf8(cc, op, len); + else + res = -ENAMETOOLONG; if (res < 0) { if (res == -ENAMETOOLONG) goto out;@@ -256,7 +266,22 @@ int hfsplus_uni2asc(struct super_block *sb,static inline int asc2unichar(struct super_block *sb, const char *astr, int len, wchar_t *uc) { - int size = HFSPLUS_SB(sb)->nls->char2uni(astr, len, uc); + struct nls_table *nls = HFSPLUS_SB(sb)->nls; + unicode_t u; + int size; + + if (nls) + size = nls->char2uni(astr, len, uc); + else { + size = utf8_to_utf32(astr, len, &u); + if (size >= 0) { + /* TODO: Add support for UTF-16 surrogate pairs */
Have you forgot to delete this string? Or do you plan to implement this?
quoted hunk ↗ jump to hunk
+ if (u <= MAX_WCHAR_T) + *uc = u; + else + size = -EINVAL; + } + } if (size <= 0) { *uc = '?'; size = 1;diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c index e2855ceefd39..9b2653f08a5f 100644 --- a/fs/hfsplus/xattr.c +++ b/fs/hfsplus/xattr.c@@ -425,7 +425,8 @@ int hfsplus_setxattr(struct inode *inode, const char *name,char *xattr_name; int res; - xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1, + xattr_name = kmalloc((HFSPLUS_SB(sb)->nls ? NLS_MAX_CHARSET_SIZE : 4) * + HFSPLUS_ATTR_MAX_STRLEN + 1, GFP_KERNEL);
What’s about to introduce a variable for length calculation?
quoted hunk ↗ jump to hunk
if (!xattr_name) return -ENOMEM;@@ -579,7 +580,8 @@ ssize_t hfsplus_getxattr(struct inode *inode, const char *name,int res; char *xattr_name; - xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1, + xattr_name = kmalloc((HFSPLUS_SB(sb)->nls ? NLS_MAX_CHARSET_SIZE : 4) * + HFSPLUS_ATTR_MAX_STRLEN + 1, GFP_KERNEL);
Ditto. What’s about to introduce a variable for length calculation?
quoted hunk ↗ jump to hunk
if (!xattr_name) return -ENOMEM;@@ -699,8 +701,9 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)return err; } - strbuf = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + - XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL); + strbuf = kmalloc((HFSPLUS_SB(sb)->nls ? NLS_MAX_CHARSET_SIZE : 4) * + HFSPLUS_ATTR_MAX_STRLEN + XATTR_MAC_OSX_PREFIX_LEN + 1, + GFP_KERNEL);
Ditto. What’s about to introduce a variable for length calculation?
quoted hunk ↗ jump to hunk
if (!strbuf) { res = -ENOMEM; goto out;@@ -732,7 +735,8 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)if (be32_to_cpu(attr_key.cnid) != inode->i_ino) goto end_listxattr; - xattr_name_len = NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN; + xattr_name_len = (HFSPLUS_SB(sb)->nls ? NLS_MAX_CHARSET_SIZE : 4) + * HFSPLUS_ATTR_MAX_STRLEN; if (hfsplus_uni2asc(inode->i_sb, (const struct hfsplus_unistr *)&fd.key->attr.key_name, strbuf, &xattr_name_len)) {diff --git a/fs/hfsplus/xattr_security.c b/fs/hfsplus/xattr_security.c index c1c7a16cbf21..438ebcd1359b 100644 --- a/fs/hfsplus/xattr_security.c +++ b/fs/hfsplus/xattr_security.c@@ -41,7 +41,8 @@ static int hfsplus_initxattrs(struct inode *inode,char *xattr_name; int err = 0; - xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1, + xattr_name = kmalloc((HFSPLUS_SB(sb)->nls ? NLS_MAX_CHARSET_SIZE : 4) * + HFSPLUS_ATTR_MAX_STRLEN + 1, GFP_KERNEL);
Ditto. What’s about to introduce a variable for length calculation? Thanks, Slava.
if (!xattr_name) return -ENOMEM; -- 2.20.1