Re: [PATCH] hfsplus: fixes worst-case unicode to char conversion of file names

Re: [PATCH] hfsplus: fixes worst-case unicode to char conversion of file names

[Hin-Tak Leung]

Hi Anton,

------------------------------
On Sat, Apr 5, 2014 9:37 PM BST Anton Altaparmakov wrote:

>Hi Hin-Tak,
>
>On 4 Apr 2014, at 23:11, Hin-Tak Leung <htl10@users.sourceforge.net> wrote:
>> On Fri, Apr 4, 2014 10:24 PM BST Anton Altaparmakov wrote:
>> 
>> On 4 Apr 2014, at 20:46, Hin-Tak Leung <hintak.leung@gmail.com> wrote:
>>> From: Hin-Tak Leung <htl10@users.sourceforge.net>
>>> 
>>> The HFS Plus Volume Format specification (TN1150) states that
>>> file names are stored internally as a maximum of 255 unicode
>>> characters, as defined by The Unicode Standard, Version 2.0
>>> [Unicode, Inc. ISBN 0-201-48345-9]. File names are converted by
>>> the NLS system on Linux before presented to the user.
>>> 
>>> Though it is rare, the worst-case is 255 CJK characters converting
>>> to UTF-8 with 1 unicode character to 3 bytes. Surrogate pairs are
>>> no worse. The receiver buffer needs to be 255 x 3 bytes,
>>> not 255 bytes as the code has always been.
>> 
>> You are correct that that buffer is too small.  However:
>> 
>> 1) The correct size for the buffer is NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN + 1 and not using a magic constant "3" (which is actually not big enough in case the string is storing UTF-16 rather than UCS-2 Unicode which I have observed happen on NTFS written to by asian versions of Windows but I see no reason why it could not happen on OS X, too, especially on a HFS+ volume that has been written to by a Windows HFS+ driver - even if native OS X driver would not normally do it - I have not looked at it I admit).  That reliable source of information Wikipedia suggests Mac OS X also uses UTF-16 as of OS X 10.3 at least in userspace so chances are it either also uses it in the kernel or if not yet it might well do in future:
>> 
>>     http://en.wikipedia.org/wiki/Comparison_of_Unicode_encodings
>> 
>> 2) You are now allocating a huge buffer on the stack.  This is not a good thing to do in the kernel (think 4k stack kernel config - that single variable is consuming about a quarter of available stack).  You need to allocate the buffer dynamically.  As you only need to do the allocation on entry to hfsplus_readdir() and deallocate it on exit it is not a problem as it could be if you had to allocate/free for every filename.
>> 
>> 
>> Hi Anton,
>> 
>> Thanks for the comments.
>
>You are welcome.
>
>> NLS_MAX_CHARSET_SIZE is 6 include/linux/nls.h but I think it is too generous in this case. It is correct that a unicode character needs at worst 6 bytes to code, but those in the upper range of that when encoded in UTF-16 would require a surrogate pair - i.e. it goes from *two* UTF-16 units to 6 bytes. So that's still x3, not x6. Also Unicode 2.0 covers only the first supplementary plane, and only requires up to 4 bytes. So that's what my "Surrogate pairs are no worse." part of the message was about. Please correct me if this reasoning is wrong.
>
>Yes, I think as things stand you are correct.  However, using NLS_MAX_CHARSET_SIZE has the advantage of being future proof - any changes to character handling in the kernel and/or to Unicode standards will automatically be fixed in HFS+ when using this constant.  As this is a transient buffer only allocated for the duration of the system call it really does not matter whether it is 766 or 1532 bytes.  In either case kmalloc() will allocate it from the respective slab (assuming slab allocator in use) and as they are both smaller than the smallest PAGE_SIZE on any architecture there is no increase in memory pressure or anything else by allocating the bigger buffer so whilst you are technically correct if I were writing the code I would definitely use NLS_MAX_CHARSET_SIZE.
>
>If you decide to definitely use "3" I suggest you at least give make a #define with some sensible name of your choice and also add a comment to say that that may need increasing in future if NLS handling in the kernel and/or Unicode standard changes...
>

Thanks for the comments. If I knew of the constant, I'd have used it :-). Actually I think the comments around NLS_MAX_CHARSET_SIZE is strictly speaking, wrong/misleading. HFS+ internally is definitely UTF-16 BE, so the question is just what is the worst of UTF16-BE to any encoding. The worst case of 6 bytes with UTF-8 is only achievable through surrogate pairs (i.e. two int16 units), but I think UTF-8 may not be the worst. GB18030 covers the whole of unicode, but at different code points, which means it may be possible that some code points in the basic plane of unicode maps to a higher plane in GB18030. If that's the case, 4 bytes is needed, rather than 3.

I think I'll use the constant, but put a comment in that it is somewhat wasteful as it is almost certain that at most half or 2/3 of it is needed. I'll
also switch to dynamic allocation also, and since this is simple enough, will possibly just add the related change in extended attribute which   
Vyacheslav points out. And will prepare v2 of a patch.

Hin-Tak
P.S. until a few months ago, I had an @*.cam e-mail address also, though not on hermes.

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hfsplus: fixes worst-case unicode to char conversion of file names

[Anton Altaparmakov]

Hi Hin-Tak,

On 4 Apr 2014, at 23:11, Hin-Tak Leung <htl10@users.sourceforge.net> wrote:
> On Fri, Apr 4, 2014 10:24 PM BST Anton Altaparmakov wrote:
>> 
>> On 4 Apr 2014, at 20:46, Hin-Tak Leung <hintak.leung@gmail.com> wrote:
>>> From: Hin-Tak Leung <htl10@users.sourceforge.net>
>>> 
>>> The HFS Plus Volume Format specification (TN1150) states that
>>> file names are stored internally as a maximum of 255 unicode
>>> characters, as defined by The Unicode Standard, Version 2.0
>>> [Unicode, Inc. ISBN 0-201-48345-9]. File names are converted by
>>> the NLS system on Linux before presented to the user.
>>> 
>>> Though it is rare, the worst-case is 255 CJK characters converting
>>> to UTF-8 with 1 unicode character to 3 bytes. Surrogate pairs are
>>> no worse. The receiver buffer needs to be 255 x 3 bytes,
>>> not 255 bytes as the code has always been.
>> 
>> You are correct that that buffer is too small.  However:
>> 
>> 1) The correct size for the buffer is NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN + 1 and not using a magic constant "3" (which is actually not big enough in case the string is storing UTF-16 rather than UCS-2 Unicode which I have observed happen on NTFS written to by asian versions of Windows but I see no reason why it could not happen on OS X, too, especially on a HFS+ volume that has been written to by a Windows HFS+ driver - even if native OS X driver would not normally do it - I have not looked at it I admit).  That reliable source of information Wikipedia suggests Mac OS X also uses UTF-16 as of OS X 10.3 at least in userspace so chances are it either also uses it in the kernel or if not yet it might well do in future:
>> 
>>     http://en.wikipedia.org/wiki/Comparison_of_Unicode_encodings
>> 
>> 2) You are now allocating a huge buffer on the stack.  This is not a good thing to do in the kernel (think 4k stack kernel config - that single variable is consuming about a quarter of available stack).  You need to allocate the buffer dynamically.  As you only need to do the allocation on entry to hfsplus_readdir() and deallocate it on exit it is not a problem as it could be if you had to allocate/free for every filename.
>> 
> 
> Hi Anton,
> 
> Thanks for the comments.

You are welcome.

> NLS_MAX_CHARSET_SIZE is 6 include/linux/nls.h but I think it is too generous in this case. It is correct that a unicode character needs at worst 6 bytes to code, but those in the upper range of that when encoded in UTF-16 would require a surrogate pair - i.e. it goes from *two* UTF-16 units to 6 bytes. So that's still x3, not x6. Also Unicode 2.0 covers only the first supplementary plane, and only requires up to 4 bytes. So that's what my "Surrogate pairs are no worse." part of the message was about. Please correct me if this reasoning is wrong.

Yes, I think as things stand you are correct.  However, using NLS_MAX_CHARSET_SIZE has the advantage of being future proof - any changes to character handling in the kernel and/or to Unicode standards will automatically be fixed in HFS+ when using this constant.  As this is a transient buffer only allocated for the duration of the system call it really does not matter whether it is 766 or 1532 bytes.  In either case kmalloc() will allocate it from the respective slab (assuming slab allocator in use) and as they are both smaller than the smallest PAGE_SIZE on any architecture there is no increase in memory pressure or anything else by allocating the bigger buffer so whilst you are technically correct if I were writing the code I would definitely use NLS_MAX_CHARSET_SIZE.

If you decide to definitely use "3" I suggest you at least give make a #define with some sensible name of your choice and also add a comment to say that that may need increasing in future if NLS handling in the kernel and/or Unicode standard changes...

Best regards,

	Anton

> I'll switch to dynamic allocation and prepare a revised patch, after further discussion on the x3 vs x6 issue.
> 
> Hin-Tak

-- 
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

Re: [PATCH] hfsplus: fixes worst-case unicode to char conversion of file names

[Vyacheslav Dubeyko]

Hi Hin-Tak,

On Apr 5, 2014, at 2:11 AM, Hin-Tak Leung wrote:

> ------------------------------
> On Fri, Apr 4, 2014 10:24 PM BST Anton Altaparmakov wrote:
> 
>> Hi,
>> 
>> On 4 Apr 2014, at 20:46, Hin-Tak Leung <hintak.leung@gmail.com> wrote:
>>> From: Hin-Tak Leung <htl10@users.sourceforge.net>
>>> 
>>> The HFS Plus Volume Format specification (TN1150) states that
>>> file names are stored internally as a maximum of 255 unicode
>>> characters, as defined by The Unicode Standard, Version 2.0
>>> [Unicode, Inc. ISBN 0-201-48345-9]. File names are converted by
>>> the NLS system on Linux before presented to the user.
>>> 
>>> Though it is rare, the worst-case is 255 CJK characters converting
>>> to UTF-8 with 1 unicode character to 3 bytes. Surrogate pairs are
>>> no worse. The receiver buffer needs to be 255 x 3 bytes,
>>> not 255 bytes as the code has always been.
>> 
>> You are correct that that buffer is too small.  However:
>> 
>> 1) The correct size for the buffer is NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN + 1 and not using a magic constant "3" (which is actually not big enough in case the string is storing UTF-16 rather than UCS-2 Unicode which I have observed happen on NTFS written to by asian versions of Windows but I see no reason why it could not happen on OS X, too, especially on a HFS+ volume that has been written to by a Windows HFS+ driver - even if native OS X driver would not normally do it - I have not looked at it I admit).  That reliable source of information Wikipedia suggests Mac OS X also uses UTF-16 as of OS X 10.3 at least in userspace so chances are it either also uses it in the kernel or if not yet it might well do in future:
>> 
>>     http://en.wikipedia.org/wiki/Comparison_of_Unicode_encodings
>> 
>> 2) You are now allocating a huge buffer on the stack.  This is not a good thing to do in the kernel (think 4k stack kernel config - that single variable is consuming about a quarter of available stack).  You need to allocate the buffer dynamically.  As you only need to do the allocation on entry to hfsplus_readdir() and deallocate it on exit it is not a problem as it could be if you had to allocate/free for every filename.
>> 
> 
> Hi Anton,
> 
> Thanks for the comments. NLS_MAX_CHARSET_SIZE is 6 include/linux/nls.h but I think it is too generous in this case. It is correct that a unicode character needs at worst 6 bytes to code, but those in the upper range of that when encoded in UTF-16 would require a surrogate pair - i.e. it goes from *two* UTF-16 units to 6 bytes. So that's still x3, not x6. Also Unicode 2.0 covers only the first supplementary plane, and only requires up to 4 bytes. So that's what my "Surrogate pairs are no worse." part of the message was about. Please correct me if this reasoning is wrong.
> 
> I'll switch to dynamic allocation and prepare a revised patch, after further discussion on the x3 vs x6 issue.
> 

Thank you for the patch. I have some additional remarks.

(1) At first I think that it makes sense to describe the reproducing path and resulting crash
(if it is possible, of course). Such description can be useful for distinguishing user's issues.

(2) I think that magic "3" is not good way too. :) Named constant is much better.

(3) Now buffer will be big in size. So, to allocate it on the stack is dangerous way. I agree
with it. It needs to use dynamic allocation (kmalloc/kfree) or look-aside cache
(kmem_cache_alloc/kmem_cache_free).

(4) Unfortunately, we have the same issue and for extended attributes case
(HFSPLUS_ATTR_MAX_STRLEN). It needs to fix it too. Could you fix it?

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH] hfsplus: fixes worst-case unicode to char conversion of file names

[Hin-Tak Leung]

------------------------------
On Fri, Apr 4, 2014 10:24 PM BST Anton Altaparmakov wrote:

>Hi,
>
>On 4 Apr 2014, at 20:46, Hin-Tak Leung <hintak.leung@gmail.com> wrote:
>> From: Hin-Tak Leung <htl10@users.sourceforge.net>
>> 
>> The HFS Plus Volume Format specification (TN1150) states that
>> file names are stored internally as a maximum of 255 unicode
>> characters, as defined by The Unicode Standard, Version 2.0
>> [Unicode, Inc. ISBN 0-201-48345-9]. File names are converted by
>> the NLS system on Linux before presented to the user.
>> 
>> Though it is rare, the worst-case is 255 CJK characters converting
>> to UTF-8 with 1 unicode character to 3 bytes. Surrogate pairs are
>> no worse. The receiver buffer needs to be 255 x 3 bytes,
>> not 255 bytes as the code has always been.
>
>You are correct that that buffer is too small.  However:
>
>1) The correct size for the buffer is NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN + 1 and not using a magic constant "3" (which is actually not big enough in case the string is storing UTF-16 rather than UCS-2 Unicode which I have observed happen on NTFS written to by asian versions of Windows but I see no reason why it could not happen on OS X, too, especially on a HFS+ volume that has been written to by a Windows HFS+ driver - even if native OS X driver would not normally do it - I have not looked at it I admit).  That reliable source of information Wikipedia suggests Mac OS X also uses UTF-16 as of OS X 10.3 at least in userspace so chances are it either also uses it in the kernel or if not yet it might well do in future:
>
>    http://en.wikipedia.org/wiki/Comparison_of_Unicode_encodings
>
>2) You are now allocating a huge buffer on the stack.  This is not a good thing to do in the kernel (think 4k stack kernel config - that single variable is consuming about a quarter of available stack).  You need to allocate the buffer dynamically.  As you only need to do the allocation on entry to hfsplus_readdir() and deallocate it on exit it is not a problem as it could be if you had to allocate/free for every filename.
>

Hi Anton,

Thanks for the comments. NLS_MAX_CHARSET_SIZE is 6 include/linux/nls.h but I think it is too generous in this case. It is correct that a unicode character needs at worst 6 bytes to code, but those in the upper range of that when encoded in UTF-16 would require a surrogate pair - i.e. it goes from *two* UTF-16 units to 6 bytes. So that's still x3, not x6. Also Unicode 2.0 covers only the first supplementary plane, and only requires up to 4 bytes. So that's what my "Surrogate pairs are no worse." part of the message was about. Please correct me if this reasoning is wrong.

I'll switch to dynamic allocation and prepare a revised patch, after further discussion on the x3 vs x6 issue.

Hin-Tak
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hfsplus: fixes worst-case unicode to char conversion of file names

[Anton Altaparmakov]

Hi,

On 4 Apr 2014, at 20:46, Hin-Tak Leung <hintak.leung@gmail.com> wrote:
> From: Hin-Tak Leung <htl10@users.sourceforge.net>
> 
> The HFS Plus Volume Format specification (TN1150) states that
> file names are stored internally as a maximum of 255 unicode
> characters, as defined by The Unicode Standard, Version 2.0
> [Unicode, Inc. ISBN 0-201-48345-9]. File names are converted by
> the NLS system on Linux before presented to the user.
> 
> Though it is rare, the worst-case is 255 CJK characters converting
> to UTF-8 with 1 unicode character to 3 bytes. Surrogate pairs are
> no worse. The receiver buffer needs to be 255 x 3 bytes,
> not 255 bytes as the code has always been.

You are correct that that buffer is too small.  However:

1) The correct size for the buffer is NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN + 1 and not using a magic constant "3" (which is actually not big enough in case the string is storing UTF-16 rather than UCS-2 Unicode which I have observed happen on NTFS written to by asian versions of Windows but I see no reason why it could not happen on OS X, too, especially on a HFS+ volume that has been written to by a Windows HFS+ driver - even if native OS X driver would not normally do it - I have not looked at it I admit).  That reliable source of information Wikipedia suggests Mac OS X also uses UTF-16 as of OS X 10.3 at least in userspace so chances are it either also uses it in the kernel or if not yet it might well do in future:

	http://en.wikipedia.org/wiki/Comparison_of_Unicode_encodings

2) You are now allocating a huge buffer on the stack.  This is not a good thing to do in the kernel (think 4k stack kernel config - that single variable is consuming about a quarter of available stack).  You need to allocate the buffer dynamically.  As you only need to do the allocation on entry to hfsplus_readdir() and deallocate it on exit it is not a problem as it could be if you had to allocate/free for every filename.

Best regards,

	Anton

> Signed-off-by: Hin-Tak Leung <htl10@users.sourceforge.net>
> CC: Vyacheslav Dubeyko <slava@dubeyko.com>
> CC: Al Viro <viro@zeniv.linux.org.uk>
> CC: Christoph Hellwig <hch@infradead.org>
> ---
> fs/hfsplus/dir.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
> index bdec665..381c668 100644
> --- a/fs/hfsplus/dir.c
> +++ b/fs/hfsplus/dir.c
> @@ -127,7 +127,7 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
> 	struct inode *inode = file_inode(file);
> 	struct super_block *sb = inode->i_sb;
> 	int len, err;
> -	char strbuf[HFSPLUS_MAX_STRLEN + 1];
> +	char strbuf[3 * HFSPLUS_MAX_STRLEN + 1];
> 	hfsplus_cat_entry entry;
> 	struct hfs_find_data fd;
> 	struct hfsplus_readdir_data *rd;
> @@ -193,7 +193,7 @@ 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 = HFSPLUS_MAX_STRLEN;
> +		len = 3 * HFSPLUS_MAX_STRLEN;
> 		err = hfsplus_uni2asc(sb, &fd.key->cat.name, strbuf, &len);
> 		if (err)
> 			goto out;
> -- 
> 1.9.0

-- 
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

[PATCH] hfsplus: fixes worst-case unicode to char conversion of file names

[Hin-Tak Leung]

From: Hin-Tak Leung <htl10@users.sourceforge.net>

The HFS Plus Volume Format specification (TN1150) states that
file names are stored internally as a maximum of 255 unicode
characters, as defined by The Unicode Standard, Version 2.0
[Unicode, Inc. ISBN 0-201-48345-9]. File names are converted by
the NLS system on Linux before presented to the user.

Though it is rare, the worst-case is 255 CJK characters converting
to UTF-8 with 1 unicode character to 3 bytes. Surrogate pairs are
no worse. The receiver buffer needs to be 255 x 3 bytes,
not 255 bytes as the code has always been.

Signed-off-by: Hin-Tak Leung <htl10@users.sourceforge.net>
CC: Vyacheslav Dubeyko <slava@dubeyko.com>
CC: Al Viro <viro@zeniv.linux.org.uk>
CC: Christoph Hellwig <hch@infradead.org>
---
 fs/hfsplus/dir.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index bdec665..381c668 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -127,7 +127,7 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
 	struct inode *inode = file_inode(file);
 	struct super_block *sb = inode->i_sb;
 	int len, err;
-	char strbuf[HFSPLUS_MAX_STRLEN + 1];
+	char strbuf[3 * HFSPLUS_MAX_STRLEN + 1];
 	hfsplus_cat_entry entry;
 	struct hfs_find_data fd;
 	struct hfsplus_readdir_data *rd;
@@ -193,7 +193,7 @@ 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 = HFSPLUS_MAX_STRLEN;
+		len = 3 * HFSPLUS_MAX_STRLEN;
 		err = hfsplus_uni2asc(sb, &fd.key->cat.name, strbuf, &len);
 		if (err)
 			goto out;
-- 
1.9.0