-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.
Signed-off-by: Sougata Santra <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
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);
+ 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);
}
static void hfsplus_cat_build_key_uni(hfsplus_btree_key *key, u32 parent,
@@ -165,11 +174,16 @@ static int hfsplus_fill_cat_thread(struct super_block *sb,
hfsplus_cat_entry *entry, int type,
u32 parentid, struct qstr *str)
{
+ int err;
+
entry->type = cpu_to_be16(type);
entry->thread.reserved = 0;
entry->thread.parentID = cpu_to_be32(parentid);
- hfsplus_asc2uni(sb, &entry->thread.nodeName, HFSPLUS_MAX_STRLEN,
+ err = hfsplus_asc2uni(sb, &entry->thread.nodeName, HFSPLUS_MAX_STRLEN,
str->name, str->len);
+ if (unlikely(err < 0))
+ return err;
+
return 10 + be16_to_cpu(entry->thread.nodeName.length) * 2;
}
@@ -181,7 +195,7 @@ int hfsplus_find_cat(struct super_block *sb, u32 cnid,
int err;
u16 type;
- hfsplus_cat_build_key(sb, fd->search_key, cnid, NULL);
+ hfsplus_cat_build_key_with_cnid(sb, fd->search_key, cnid);
err = hfs_brec_read(fd, &tmp, sizeof(hfsplus_cat_entry));
if (err)
return err;
@@ -218,11 +232,16 @@ int hfsplus_create_cat(u32 cnid, struct inode *dir,
if (err)
return err;
- hfsplus_cat_build_key(sb, fd.search_key, cnid, NULL);
+ hfsplus_cat_build_key_with_cnid(sb, fd.search_key, cnid);
entry_size = hfsplus_fill_cat_thread(sb, &entry,
S_ISDIR(inode->i_mode) ?
HFSPLUS_FOLDER_THREAD : HFSPLUS_FILE_THREAD,
dir->i_ino, str);
+ if (unlikely(entry_size < 0)) {
+ err = entry_size;
+ goto err2;
+ }
+
err = hfs_brec_find(&fd, hfs_find_rec_by_key);
if (err != -ENOENT) {
if (!err)
@@ -233,7 +252,10 @@ int hfsplus_create_cat(u32 cnid, struct inode *dir,
if (err)
goto err2;
- hfsplus_cat_build_key(sb, fd.search_key, dir->i_ino, str);
+ err = hfsplus_cat_build_key(sb, fd.search_key, dir->i_ino, str);
+ if (unlikely(err))
+ goto err1;
+
entry_size = hfsplus_cat_build_record(&entry, cnid, inode);
err = hfs_brec_find(&fd, hfs_find_rec_by_key);
if (err != -ENOENT) {
@@ -254,7 +276,7 @@ int hfsplus_create_cat(u32 cnid, struct inode *dir,
return 0;
err1:
- hfsplus_cat_build_key(sb, fd.search_key, cnid, NULL);
+ hfsplus_cat_build_key_with_cnid(sb, fd.search_key, cnid);
if (!hfs_brec_find(&fd, hfs_find_rec_by_key))
hfs_brec_remove(&fd);
err2:
@@ -279,7 +301,7 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str)
if (!str) {
int len;
- hfsplus_cat_build_key(sb, fd.search_key, cnid, NULL);
+ hfsplus_cat_build_key_with_cnid(sb, fd.search_key, cnid);
err = hfs_brec_find(&fd, hfs_find_rec_by_key);
if (err)
goto out;
@@ -295,7 +317,9 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str)
off + 2, len);
fd.search_key->key_len = cpu_to_be16(6 + len);
} else
- hfsplus_cat_build_key(sb, fd.search_key, dir->i_ino, str);
+ err = hfsplus_cat_build_key(sb, fd.search_key, dir->i_ino, str);
+ if (unlikely(err))
+ goto out;
err = hfs_brec_find(&fd, hfs_find_rec_by_key);
if (err)
@@ -326,7 +350,7 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str)
if (err)
goto out;
- hfsplus_cat_build_key(sb, fd.search_key, cnid, NULL);
+ hfsplus_cat_build_key_with_cnid(sb, fd.search_key, cnid);
err = hfs_brec_find(&fd, hfs_find_rec_by_key);
if (err)
goto out;
@@ -369,7 +393,11 @@ int hfsplus_rename_cat(u32 cnid,
dst_fd = src_fd;
/* find the old dir entry and read the data */
- hfsplus_cat_build_key(sb, src_fd.search_key, src_dir->i_ino, src_name);
+ err = hfsplus_cat_build_key(sb, src_fd.search_key,
+ src_dir->i_ino, src_name);
+ if (unlikely(err))
+ goto out;
+
err = hfs_brec_find(&src_fd, hfs_find_rec_by_key);
if (err)
goto out;
@@ -382,7 +410,11 @@ int hfsplus_rename_cat(u32 cnid,
src_fd.entrylength);
/* create new dir entry with the data from the old entry */
- hfsplus_cat_build_key(sb, dst_fd.search_key, dst_dir->i_ino, dst_name);
+ err = hfsplus_cat_build_key(sb, dst_fd.search_key,
+ dst_dir->i_ino, dst_name);
+ if (unlikely(err))
+ goto out;
+
err = hfs_brec_find(&dst_fd, hfs_find_rec_by_key);
if (err != -ENOENT) {
if (!err)
@@ -397,7 +429,11 @@ int hfsplus_rename_cat(u32 cnid,
dst_dir->i_mtime = dst_dir->i_ctime = CURRENT_TIME_SEC;
/* finally remove the old entry */
- hfsplus_cat_build_key(sb, src_fd.search_key, src_dir->i_ino, src_name);
+ err = hfsplus_cat_build_key(sb, src_fd.search_key,
+ src_dir->i_ino, src_name);
+ if (unlikely(err))
+ goto out;
+
err = hfs_brec_find(&src_fd, hfs_find_rec_by_key);
if (err)
goto out;
@@ -408,7 +444,7 @@ int hfsplus_rename_cat(u32 cnid,
src_dir->i_mtime = src_dir->i_ctime = CURRENT_TIME_SEC;
/* remove old thread entry */
- hfsplus_cat_build_key(sb, src_fd.search_key, cnid, NULL);
+ hfsplus_cat_build_key_with_cnid(sb, src_fd.search_key, cnid);
err = hfs_brec_find(&src_fd, hfs_find_rec_by_key);
if (err)
goto out;
@@ -418,9 +454,14 @@ int hfsplus_rename_cat(u32 cnid,
goto out;
/* create new thread entry */
- hfsplus_cat_build_key(sb, dst_fd.search_key, cnid, NULL);
+ hfsplus_cat_build_key_with_cnid(sb, dst_fd.search_key, cnid);
entry_size = hfsplus_fill_cat_thread(sb, &entry, type,
dst_dir->i_ino, dst_name);
+ if (unlikely(entry_size < 0)) {
+ err = entry_size;
+ goto out;
+ }
+
err = hfs_brec_find(&dst_fd, hfs_find_rec_by_key);
if (err != -ENOENT) {
if (!err)
diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index bdec665..b306b66 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -43,7 +43,10 @@ static struct dentry *hfsplus_lookup(struct inode *dir, struct dentry *dentry,
err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
if (err)
return ERR_PTR(err);
- hfsplus_cat_build_key(sb, fd.search_key, dir->i_ino, &dentry->d_name);
+ err = hfsplus_cat_build_key(sb, fd.search_key, dir->i_ino,
+ &dentry->d_name);
+ if (unlikely(err < 0))
+ goto fail;
again:
err = hfs_brec_read(&fd, &entry, sizeof(entry));
if (err) {
@@ -96,9 +99,11 @@ again:
be32_to_cpu(entry.file.permissions.dev);
str.len = sprintf(name, "iNode%d", linkid);
str.name = name;
- hfsplus_cat_build_key(sb, fd.search_key,
+ err = hfsplus_cat_build_key(sb, fd.search_key,
HFSPLUS_SB(sb)->hidden_dir->i_ino,
&str);
+ if (unlikely(err < 0))
+ goto fail;
goto again;
}
} else if (!dentry->d_fsdata)
@@ -139,7 +144,7 @@ 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;
- hfsplus_cat_build_key(sb, fd.search_key, inode->i_ino, NULL);
+ hfsplus_cat_build_key_with_cnid(sb, fd.search_key, inode->i_ino);
err = hfs_brec_find(&fd, hfs_find_rec_by_key);
if (err)
goto out;
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,
+ hfsplus_btree_key *, u32);
int hfsplus_find_cat(struct super_block *, u32, struct hfs_find_data *);
int hfsplus_create_cat(u32, struct inode *, struct qstr *, struct inode *);
int hfsplus_delete_cat(u32, struct inode *, struct qstr *);
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 80875aa..4fdf0ca 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -513,7 +513,9 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
err = hfs_find_init(sbi->cat_tree, &fd);
if (err)
goto out_put_root;
- hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
+ err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
+ if (unlikely(err < 0))
+ goto out_put_root;
if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
hfs_find_exit(&fd);
if (entry.type != cpu_to_be16(HFSPLUS_FOLDER))
--
1.8.5.3
On Mon, 24 Feb 2014 21:28:27 +0200 Sougata Santra <[email protected]> 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.
>
> ...
>
> --- 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,
> + hfsplus_btree_key *, u32);
> int hfsplus_find_cat(struct super_block *, u32, struct hfs_find_data *);
> int hfsplus_create_cat(u32, struct inode *, struct qstr *, struct inode *);
> int hfsplus_delete_cat(u32, struct inode *, struct qstr *);
grumble. Omitting the argument names from declarations makes them
unreadable and generally useless. I mean, a bare u32?
And including the names of some arguments but omitting others is
downright bizarre.
However this isn't a thing which can or should be addressed within this
patch.
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 <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
> ---
> 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.
On 02/25/2014 09:37 AM, Vyacheslav Dubeyko wrote:
> 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.
Thank you for taking time to look into it.
>
> I have some small remarks. Please, see below.
>
>> Signed-off-by: Sougata Santra <[email protected]>
>> Reviewed-by: Christoph Hellwig <[email protected]>
>> ---
>> 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?
I agree, although I think this should he done in a separate patch. Also
there are other instances of hard-coding. We can clean them with a patch. ?
>
>> + 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.
I looked into it while writing the patch. I think this was already
handled before. Please see. catalog.c#hfsplus_find_cat the only place it
is called.
{code}
if (be16_to_cpu(tmp.thread.nodeName.length) > 255) {
pr_err("catalog name length corrupted\n");
return -EIO;
}
{code}
>
> 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. :)
Thank you for taking time to read it.
>
> Thanks,
> Vyacheslav Dubeyko.
>
>
Best regards,
Sougata
On Tue, 2014-02-25 at 12:05 +0200, sougata santra wrote:
> 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?
>
> I agree, although I think this should he done in a separate patch. Also
> there are other instances of hard-coding. We can clean them with a patch. ?
>
Yes, I think so too. It will be great.
> > 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.
>
> I looked into it while writing the patch. I think this was already
> handled before. Please see. catalog.c#hfsplus_find_cat the only place it
> is called.
>
> {code}
> if (be16_to_cpu(tmp.thread.nodeName.length) > 255) {
> pr_err("catalog name length corrupted\n");
> return -EIO;
> }
> {code}
>
OK. I agree that my suggestion is not necessary.
Thanks,
Vyacheslav Dubeyko.
On 02/24/2014 11:48 PM, Andrew Morton wrote:
> On Mon, 24 Feb 2014 21:28:27 +0200 Sougata Santra<[email protected]> 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.
>>
>> ...
>>
>> --- 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,
>> + hfsplus_btree_key *, u32);
>> int hfsplus_find_cat(struct super_block *, u32, struct hfs_find_data *);
>> int hfsplus_create_cat(u32, struct inode *, struct qstr *, struct inode *);
>> int hfsplus_delete_cat(u32, struct inode *, struct qstr *);
>
> grumble. Omitting the argument names from declarations makes them
> unreadable and generally useless. I mean, a bare u32?
>
> And including the names of some arguments but omitting others is
> downright bizarre.
>
> However this isn't a thing which can or should be addressed within this
> patch.
Also in hfsplus_fs.h there are:
====> snip <=====
#define hfs_btree_open hfsplus_btree_open
#define hfs_btree_close hfsplus_btree_close
#define hfs_btree_write hfsplus_btree_write
#define hfs_bmap_alloc hfsplus_bmap_alloc
#define hfs_bmap_free hfsplus_bmap_free
#define hfs_bnode_read hfsplus_bnode_read
...
====> snap <=====
Does these exist for some legacy reason or does it serve any other
purpose ?. The only usage I see is that they change function names after
pre-processing.
Thanks a lot,
Best regards,
Sougata.
On Tue, Feb 25, 2014 at 08:01:16PM +0200, sougata wrote:
> ====> snip <=====
> #define hfs_btree_open hfsplus_btree_open
> #define hfs_btree_close hfsplus_btree_close
> #define hfs_btree_write hfsplus_btree_write
> #define hfs_bmap_alloc hfsplus_bmap_alloc
> #define hfs_bmap_free hfsplus_bmap_free
> #define hfs_bnode_read hfsplus_bnode_read
As far as I understand the history is that some code was at some point
mostly identical between hfsplus and hfs and this made diffing easier,
but Brad would have to answert that. I don't think there's much point
today where hfs is basically stale legacy code and hfsplus gets all the
development.