Hi Hin-Tak,
I think you are taking a sub-optimal approach! Think about it: you are now performing the hfsplus_asc2uni() twice for each name being handled! Once to check if the length is ok and once to get the actual result. This is a waste of time.
Instead, just do the conversion. If the name is longer than allowed, hfsplus_asc2uni() will return -ENAMETOOLONG and at that point you can bail out. There is no need to do a "test conversion" first the result of which you through away (and even worse you do a kmalloc() twice - once for the "test" and once for the real thing)...
In fact you do it correctly in your proposed patch in hfsplus_attr_build_key(): You thrown out the length check and replace it with a call to hfsplus_asc2uni() that returns -ENAMETOOLONG if the name is too long and thus hfsplus_attr_build_key() returns -ENAMETOOLONG. All you need to do is change the code to do the Right Thing when it gets back -ENAMETOOLONG and you are done (and you throw away all the bogus length checks in the upper layers).
If you definitely want to do checking in advance, then that is fine, too - but don't do it with a test conversion - instead, convert it, check the length of the converted string and then use the converted name from there onwards.
Both my suggestions are perhaps more involved changes than what you are doing but I think they are much more worthwhile as they do not introduce double the allocations and conversions.
Best regards,
Anton
On 12 Apr 2014, at 04:26, Hin-Tak Leung <hintak.leung@xxxxxxxxx> wrote:
> From: Hin-Tak Leung <htl10@xxxxxxxxxxxxxxxxxxxxx>
>
> HFSPLUS_ATTR_MAX_STRLEN (=127) is a limit for the number of unicode character
> (UTF-16BE) storable in the HFS+ file system. Almost all the current
> usage of it is wrong in relation to NLS to on-disk conversion.
>
> Except for one use calling hfsplus_asc2uni (which should stay the same) and
> its uses in calling hfsplus_uni2asc (which was changed in an earlier patch),
> all the other uses are of the forms:
>
> - char buffer[size]
>
> - bound check: "if (input_length > size) return failure;"
>
> Conversion between on-disk and NLS char strings (in whichever direction)
> always needs to be in the worst, so all char buffers of that size
> need to have a NLS_MAX_CHARSET_SIZE x .
>
> The bound checks are all wrong, since they all compare
> nls_length to unicode_length_limit.
>
> A new function, hfsplus_try_asc2uni(), is introduced to
> try the conversion, to do the bound-check instead. (After review this
> will be splitted off as a separate earlier patch).
>
> Before this patch, trying to set a 55 CJK character (in a UTF-8 locale)
> + user prefix fails with:
>
> $ setfattr -n user.`cat testing-string` -v `cat testing-string` testing-string
> setfattr: testing-string: Operation not supported
>
> and getting long attributes is particular ugly(!):
>
> find /mnt/* -type f -exec getfattr -d {} \;
> getfattr: /mnt/testing-string: Input/output error
>
> with console log:
> [268008.389781] hfsplus: unicode conversion failed
>
> After the patch, both of the above works.
>
> However, I was not able to set double the size (110 + 5 is still under 127):
>
> $setfattr -n user.`cat testing-string testing-string` -v `cat testing-string testing-string` testing-string
> setfattr: testing-string: Numerical result out of range
>
> But this also fails on ext2, so this failure is not HFS+ specific. It seems
> that either the command line or the system call itself has a 256-byte limit.
> (110 CJK char in UTF-8 is 330 bytes).
>
> Signed-off-by: Hin-Tak Leung <htl10@xxxxxxxxxxxxxxxxxxxxx>
> ---
> fs/hfsplus/attributes.c | 11 ++++-----
> fs/hfsplus/hfsplus_fs.h | 7 ++++++
> fs/hfsplus/unicode.c | 18 +++++++++++++++
> fs/hfsplus/xattr.c | 40 ++++++++++++++++++++++----------
> fs/hfsplus/xattr_security.c | 56 ++++++++++++++++++++++++++++++---------------
> fs/hfsplus/xattr_trusted.c | 36 +++++++++++++++++++++--------
> fs/hfsplus/xattr_user.c | 36 +++++++++++++++++++++--------
> 7 files changed, 147 insertions(+), 57 deletions(-)
>
> diff --git a/fs/hfsplus/attributes.c b/fs/hfsplus/attributes.c
> index caf89a7..f3345c0 100644
> --- a/fs/hfsplus/attributes.c
> +++ b/fs/hfsplus/attributes.c
> @@ -54,14 +54,11 @@ int hfsplus_attr_build_key(struct super_block *sb, hfsplus_btree_key *key,
> memset(key, 0, sizeof(struct hfsplus_attr_key));
> key->attr.cnid = cpu_to_be32(cnid);
> if (name) {
> - len = strlen(name);
> - if (len > HFSPLUS_ATTR_MAX_STRLEN) {
> - pr_err("invalid xattr name's length\n");
> - return -EINVAL;
> - }
> - hfsplus_asc2uni(sb,
> + int res = hfsplus_asc2uni(sb,
> (struct hfsplus_unistr *)&key->attr.key_name,
> - HFSPLUS_ATTR_MAX_STRLEN, name, len);
> + HFSPLUS_ATTR_MAX_STRLEN, name, strlen(name));
> + if (res)
> + return res;
> len = be16_to_cpu(key->attr.key_name.length);
> } else {
> key->attr.key_name.length = 0;
> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index 83dc292..80257d0 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -507,6 +507,13 @@ int hfsplus_uni2asc(struct super_block *,
> const struct hfsplus_unistr *, char *, int *);
> int hfsplus_asc2uni(struct super_block *,
> struct hfsplus_unistr *, int, const char *, int);
> +int hfsplus_try_asc2uni_sb(struct super_block *sb,
> + const char *nls_name, int unilimit);
> +static inline int hfsplus_try_asc2uni(const struct dentry *dentry,
> + const char *nls_name, int unilimit)
> +{
> + return hfsplus_try_asc2uni_sb(dentry->d_sb, nls_name, unilimit);
> +}
> int hfsplus_hash_dentry(const struct dentry *dentry, struct qstr *str);
> int hfsplus_compare_dentry(const struct dentry *parent, const struct dentry *dentry,
> unsigned int len, const char *str, const struct qstr *name);
> diff --git a/fs/hfsplus/unicode.c b/fs/hfsplus/unicode.c
> index e8ef121..cd00bd2 100644
> --- a/fs/hfsplus/unicode.c
> +++ b/fs/hfsplus/unicode.c
> @@ -330,6 +330,24 @@ int hfsplus_asc2uni(struct super_block *sb,
> }
>
> /*
> + * Try encoding an nls_name within a unicode size limit,
> + * and see whether it will fit.
> + */
> +int hfsplus_try_asc2uni_sb(struct super_block *sb,
> + const char *nls_name, int unilimit)
> +{
> + int res = 0;
> + struct hfsplus_unistr *ustr = kmalloc(sizeof(*ustr), GFP_KERNEL);
> + if (!ustr)
> + return -ENOMEM;
> +
> + res = hfsplus_asc2uni(sb,
> + ustr, unilimit, nls_name, strlen(nls_name));
> + kfree(ustr);
> + return res;
> +}
> +
> +/*
> * Hash a string to an integer as appropriate for the HFS+ filesystem.
> * Composed unicode characters are decomposed and case-folding is performed
> * if the appropriate bits are (un)set on the superblock.
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 40c0a63..a05cab9 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -805,15 +805,15 @@ end_removexattr:
> static int hfsplus_osx_getxattr(struct dentry *dentry, const char *name,
> void *buffer, size_t size, int type)
> {
> - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN +
> - XATTR_MAC_OSX_PREFIX_LEN + 1] = {0};
> - size_t len = strlen(name);
> + char *xattr_name;
> + int res;
>
> if (!strcmp(name, ""))
> return -EINVAL;
>
> - if (len > HFSPLUS_ATTR_MAX_STRLEN)
> - return -EOPNOTSUPP;
> + res = hfsplus_try_asc2uni(dentry, name, HFSPLUS_ATTR_MAX_STRLEN);
> + if (res)
> + return res;
>
> /*
> * Don't allow retrieving properly prefixed attributes
> @@ -821,22 +821,30 @@ static int hfsplus_osx_getxattr(struct dentry *dentry, const char *name,
> */
> if (is_known_namespace(name))
> return -EOPNOTSUPP;
> + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN
> + + XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL);
> + if (!xattr_name)
> + return -ENOMEM;
> + strcpy(xattr_name, XATTR_MAC_OSX_PREFIX);
> + strcpy(xattr_name + XATTR_MAC_OSX_PREFIX_LEN, name);
>
> - return hfsplus_getxattr(dentry, xattr_name, buffer, size);
> + res = hfsplus_getxattr(dentry, xattr_name, buffer, size);
> + kfree(xattr_name);
> + return res;
> }
>
> static int hfsplus_osx_setxattr(struct dentry *dentry, const char *name,
> const void *buffer, size_t size, int flags, int type)
> {
> - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN +
> - XATTR_MAC_OSX_PREFIX_LEN + 1] = {0};
> - size_t len = strlen(name);
> + char *xattr_name;
> + int res;
>
> if (!strcmp(name, ""))
> return -EINVAL;
>
> - if (len > HFSPLUS_ATTR_MAX_STRLEN)
> - return -EOPNOTSUPP;
> + res = hfsplus_try_asc2uni(dentry, name, HFSPLUS_ATTR_MAX_STRLEN);
> + if (res)
> + return res;
>
> /*
> * Don't allow setting properly prefixed attributes
> @@ -844,8 +852,16 @@ static int hfsplus_osx_setxattr(struct dentry *dentry, const char *name,
> */
> if (is_known_namespace(name))
> return -EOPNOTSUPP;
> + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN
> + + XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL);
> + if (!xattr_name)
> + return -ENOMEM;
> + strcpy(xattr_name, XATTR_MAC_OSX_PREFIX);
> + strcpy(xattr_name + XATTR_MAC_OSX_PREFIX_LEN, name);
>
> - return hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
> + res = hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
> + kfree(xattr_name);
> + return res;
> }
>
> static size_t hfsplus_osx_listxattr(struct dentry *dentry, char *list,
> diff --git a/fs/hfsplus/xattr_security.c b/fs/hfsplus/xattr_security.c
> index 0072276..f4993e0 100644
> --- a/fs/hfsplus/xattr_security.c
> +++ b/fs/hfsplus/xattr_security.c
> @@ -14,37 +14,53 @@
> static int hfsplus_security_getxattr(struct dentry *dentry, const char *name,
> void *buffer, size_t size, int type)
> {
> - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
> - size_t len = strlen(name);
> + char *xattr_name;
> + int res;
>
> if (!strcmp(name, ""))
> return -EINVAL;
>
> - if (len + XATTR_SECURITY_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
> - return -EOPNOTSUPP;
> + res = hfsplus_try_asc2uni(dentry, name,
> + HFSPLUS_ATTR_MAX_STRLEN - XATTR_SECURITY_PREFIX_LEN);
> + if (res)
> + return res;
>
> + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
> + GFP_KERNEL);
> + if (!xattr_name)
> + return -ENOMEM;
> strcpy(xattr_name, XATTR_SECURITY_PREFIX);
> strcpy(xattr_name + XATTR_SECURITY_PREFIX_LEN, name);
>
> - return hfsplus_getxattr(dentry, xattr_name, buffer, size);
> + res = hfsplus_getxattr(dentry, xattr_name, buffer, size);
> + kfree(xattr_name);
> + return res;
> }
>
> static int hfsplus_security_setxattr(struct dentry *dentry, const char *name,
> const void *buffer, size_t size, int flags, int type)
> {
> - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
> - size_t len = strlen(name);
> + char *xattr_name;
> + int res;
>
> if (!strcmp(name, ""))
> return -EINVAL;
>
> - if (len + XATTR_SECURITY_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
> - return -EOPNOTSUPP;
> + res = hfsplus_try_asc2uni(dentry, name,
> + HFSPLUS_ATTR_MAX_STRLEN - XATTR_SECURITY_PREFIX_LEN);
> + if (res)
> + return res;
>
> + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
> + GFP_KERNEL);
> + if (!xattr_name)
> + return -ENOMEM;
> strcpy(xattr_name, XATTR_SECURITY_PREFIX);
> strcpy(xattr_name + XATTR_SECURITY_PREFIX_LEN, name);
>
> - return hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
> + res = hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
> + kfree(xattr_name);
> + return res;
> }
>
> static size_t hfsplus_security_listxattr(struct dentry *dentry, char *list,
> @@ -62,31 +78,35 @@ static int hfsplus_initxattrs(struct inode *inode,
> void *fs_info)
> {
> const struct xattr *xattr;
> - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
> - size_t xattr_name_len;
> + char *xattr_name;
> int err = 0;
>
> + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
> + GFP_KERNEL);
> + if (!xattr_name)
> + return -ENOMEM;
> for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> - xattr_name_len = strlen(xattr->name);
>
> - if (xattr_name_len == 0)
> + if (!strcmp(xattr->name, ""))
> continue;
>
> - if (xattr_name_len + XATTR_SECURITY_PREFIX_LEN >
> - HFSPLUS_ATTR_MAX_STRLEN)
> - return -EOPNOTSUPP;
> + err = hfsplus_try_asc2uni_sb(inode->i_sb, xattr->name,
> + HFSPLUS_ATTR_MAX_STRLEN - XATTR_SECURITY_PREFIX_LEN);
> + if (err)
> + break;
>
> strcpy(xattr_name, XATTR_SECURITY_PREFIX);
> strcpy(xattr_name +
> XATTR_SECURITY_PREFIX_LEN, xattr->name);
> memset(xattr_name +
> - XATTR_SECURITY_PREFIX_LEN + xattr_name_len, 0, 1);
> + XATTR_SECURITY_PREFIX_LEN + strlen(xattr->name), 0, 1);
>
> err = __hfsplus_setxattr(inode, xattr_name,
> xattr->value, xattr->value_len, 0);
> if (err)
> break;
> }
> + kfree(xattr_name);
> return err;
> }
>
> diff --git a/fs/hfsplus/xattr_trusted.c b/fs/hfsplus/xattr_trusted.c
> index 426cee2..36716af 100644
> --- a/fs/hfsplus/xattr_trusted.c
> +++ b/fs/hfsplus/xattr_trusted.c
> @@ -12,37 +12,53 @@
> static int hfsplus_trusted_getxattr(struct dentry *dentry, const char *name,
> void *buffer, size_t size, int type)
> {
> - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
> - size_t len = strlen(name);
> + char *xattr_name;
> + int res;
>
> if (!strcmp(name, ""))
> return -EINVAL;
>
> - if (len + XATTR_TRUSTED_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
> - return -EOPNOTSUPP;
> + res = hfsplus_try_asc2uni(dentry, name,
> + HFSPLUS_ATTR_MAX_STRLEN - XATTR_TRUSTED_PREFIX_LEN);
> + if (res)
> + return res;
>
> + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
> + GFP_KERNEL);
> + if (!xattr_name)
> + return -ENOMEM;
> strcpy(xattr_name, XATTR_TRUSTED_PREFIX);
> strcpy(xattr_name + XATTR_TRUSTED_PREFIX_LEN, name);
>
> - return hfsplus_getxattr(dentry, xattr_name, buffer, size);
> + res = hfsplus_getxattr(dentry, xattr_name, buffer, size);
> + kfree(xattr_name);
> + return res;
> }
>
> static int hfsplus_trusted_setxattr(struct dentry *dentry, const char *name,
> const void *buffer, size_t size, int flags, int type)
> {
> - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
> - size_t len = strlen(name);
> + char *xattr_name;
> + int res;
>
> if (!strcmp(name, ""))
> return -EINVAL;
>
> - if (len + XATTR_TRUSTED_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
> - return -EOPNOTSUPP;
> + res = hfsplus_try_asc2uni(dentry, name,
> + HFSPLUS_ATTR_MAX_STRLEN - XATTR_TRUSTED_PREFIX_LEN);
> + if (res)
> + return res;
>
> + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
> + GFP_KERNEL);
> + if (!xattr_name)
> + return -ENOMEM;
> strcpy(xattr_name, XATTR_TRUSTED_PREFIX);
> strcpy(xattr_name + XATTR_TRUSTED_PREFIX_LEN, name);
>
> - return hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
> + res = hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
> + kfree(xattr_name);
> + return res;
> }
>
> static size_t hfsplus_trusted_listxattr(struct dentry *dentry, char *list,
> diff --git a/fs/hfsplus/xattr_user.c b/fs/hfsplus/xattr_user.c
> index e340165..26b832c 100644
> --- a/fs/hfsplus/xattr_user.c
> +++ b/fs/hfsplus/xattr_user.c
> @@ -12,37 +12,53 @@
> static int hfsplus_user_getxattr(struct dentry *dentry, const char *name,
> void *buffer, size_t size, int type)
> {
> - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
> - size_t len = strlen(name);
> + char *xattr_name;
> + int res;
>
> if (!strcmp(name, ""))
> return -EINVAL;
>
> - if (len + XATTR_USER_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
> - return -EOPNOTSUPP;
> + res = hfsplus_try_asc2uni(dentry, name,
> + HFSPLUS_ATTR_MAX_STRLEN - XATTR_USER_PREFIX_LEN);
> + if (res)
> + return res;
>
> + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
> + GFP_KERNEL);
> + if (!xattr_name)
> + return -ENOMEM;
> strcpy(xattr_name, XATTR_USER_PREFIX);
> strcpy(xattr_name + XATTR_USER_PREFIX_LEN, name);
>
> - return hfsplus_getxattr(dentry, xattr_name, buffer, size);
> + res = hfsplus_getxattr(dentry, xattr_name, buffer, size);
> + kfree(xattr_name);
> + return res;
> }
>
> static int hfsplus_user_setxattr(struct dentry *dentry, const char *name,
> const void *buffer, size_t size, int flags, int type)
> {
> - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
> - size_t len = strlen(name);
> + char *xattr_name;
> + int res;
>
> if (!strcmp(name, ""))
> return -EINVAL;
>
> - if (len + XATTR_USER_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
> - return -EOPNOTSUPP;
> + res = hfsplus_try_asc2uni(dentry, name,
> + HFSPLUS_ATTR_MAX_STRLEN - XATTR_USER_PREFIX_LEN);
> + if (res)
> + return res;
>
> + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
> + GFP_KERNEL);
> + if (!xattr_name)
> + return -ENOMEM;
> strcpy(xattr_name, XATTR_USER_PREFIX);
> strcpy(xattr_name + XATTR_USER_PREFIX_LEN, name);
>
> - return hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
> + res = hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
> + kfree(xattr_name);
> + return res;
> }
>
> static size_t hfsplus_user_listxattr(struct dentry *dentry, char *list,
> --
> 1.9.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Best regards,
Anton
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
