lkml.org 
[lkml]   [2014]   [Feb]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH 1/1] hfsplus: fix longname handling
From
Date
Hi Sougata,

On Mon, 2014-02-24 at 21:28 +0200, Sougata Santra wrote:
> -ENAMETOOLONG returned from hfsplus_asc2uni was not propaged to iops. This
> allowed hfsplus to create files/directories with HFSPLUS_MAX_STRLEN and
> incorrect keys, leaving the FS in an inconsistent state. This patch fixes
> this issue.
>

Good fix. Thank you.

I have some small remarks. Please, see below.

> Signed-off-by: Sougata Santra <sougata@tuxera.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/hfsplus/catalog.c | 89 ++++++++++++++++++++++++++++++++++++-------------
> fs/hfsplus/dir.c | 11 ++++--
> fs/hfsplus/hfsplus_fs.h | 4 ++-
> fs/hfsplus/super.c | 4 ++-
> 4 files changed, 79 insertions(+), 29 deletions(-)
>
> diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
> index 968ce41..389c474 100644
> --- a/fs/hfsplus/catalog.c
> +++ b/fs/hfsplus/catalog.c
> @@ -38,21 +38,30 @@ int hfsplus_cat_bin_cmp_key(const hfsplus_btree_key *k1,
> return hfsplus_strcmp(&k1->cat.name, &k2->cat.name);
> }
>
> -void hfsplus_cat_build_key(struct super_block *sb, hfsplus_btree_key *key,
> - u32 parent, struct qstr *str)
> +/* Generates key for catalog file/folders record. */
> +int hfsplus_cat_build_key(struct super_block *sb,
> + hfsplus_btree_key *key, u32 parent, struct qstr *str)
> {
> - int len;
> + int len, err;
>
> key->cat.parent = cpu_to_be32(parent);
> - if (str) {
> - hfsplus_asc2uni(sb, &key->cat.name, HFSPLUS_MAX_STRLEN,
> - str->name, str->len);
> - len = be16_to_cpu(key->cat.name.length);
> - } else {
> - key->cat.name.length = 0;
> - len = 0;
> - }
> + err = hfsplus_asc2uni(sb, &key->cat.name, HFSPLUS_MAX_STRLEN,
> + str->name, str->len);
> + if (unlikely(err < 0))
> + return err;
> +
> + len = be16_to_cpu(key->cat.name.length);
> key->key_len = cpu_to_be16(6 + 2 * len);

I think that maybe it is time to change hardcoded 6 on sensible named
constant. What do you think?

> + return 0;
> +}
> +
> +/* Generates key for catalog thread record. */
> +void hfsplus_cat_build_key_with_cnid(struct super_block *sb,
> + hfsplus_btree_key *key, u32 parent)
> +{
> + key->cat.parent = cpu_to_be32(parent);
> + key->cat.name.length = 0;
> + key->key_len = cpu_to_be16(6);

Ditto.

> }
>
> static void hfsplus_cat_build_key_uni(hfsplus_btree_key *key, u32 parent,

We have such code for hfsplus_cat_build_key_uni():

58 static void hfsplus_cat_build_key_uni(hfsplus_btree_key *key, u32 parent,
59 struct hfsplus_unistr *name)
60 {
61 int ustrlen;
62
63 ustrlen = be16_to_cpu(name->length);

I suppose that it makes sense to check name->length here and return
error. We can check possible volume corruption here.

64 key->cat.parent = cpu_to_be32(parent);
65 key->cat.name.length = cpu_to_be16(ustrlen);
66 ustrlen *= 2;
67 memcpy(key->cat.name.unicode, name->unicode, ustrlen);
68 key->key_len = cpu_to_be16(6 + ustrlen);
69 }

What do you think about such suggestion?

> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index 08846425b..66671c4 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -443,8 +443,10 @@ int hfsplus_cat_case_cmp_key(const hfsplus_btree_key *,
> const hfsplus_btree_key *);
> int hfsplus_cat_bin_cmp_key(const hfsplus_btree_key *,
> const hfsplus_btree_key *);
> -void hfsplus_cat_build_key(struct super_block *sb,
> +int hfsplus_cat_build_key(struct super_block *sb,
> hfsplus_btree_key *, u32, struct qstr *);
> +void hfsplus_cat_build_key_with_cnid(struct super_block *sb,

The whole style of the fix is good. But such mess looks not very good.
But it is not critical, of course. :)

Thanks,
Vyacheslav Dubeyko.




\
 
 \ /
  Last update: 2014-02-25 09:21    [W:0.078 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site