Messages in this thread Patch in this message |  | | Subject | Re: [PATCH 1/1] hfsplus: fix longname handling | From | Vyacheslav Dubeyko <> | Date | Tue, 25 Feb 2014 11:37:25 +0400 |
| |
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.
|  |