* [RFC V0] Exminating the use of HFSPLUS_ATTR_MAX_STRLEN c.f. non-English attributes
@ 2014-04-10 1:04 Hin-Tak Leung
2014-04-10 7:34 ` Vyacheslav Dubeyko
0 siblings, 1 reply; 3+ messages in thread
From: Hin-Tak Leung @ 2014-04-10 1:04 UTC (permalink / raw)
To: linux-fsdevel; +Cc: Hin-Tak Leung
From: Hin-Tak Leung <htl10@users.sourceforge.net>
This isn't a real patch but just an RFC of examination of all usages of
HFSPLUS_ATTR_MAX_STRLEN .
Actually it turns out to be rather simple - except for
one use calling hfsplus_asc2uni (which should stay the same) and
one use calling hfsplus_uni2asc (which was changed in the just submitted
patch), all the other uses are of the forms:
1. char buffer[size]
2. bound check: "if (input_length > size) return failure;"
The buffer size change is simple enough - conversion between on-disk and char
(in whichever direction) always needs to be in the worst, so any char buffer
with that size almost all needs to have a NLS_MAX_CHARSET_SIZE x . They also
all need to switch to dynamic allocation (not done here yet, just for
simplicity ATM), and also possibly need to do kzalloc(?or null terminated
on first use?) since some of them are involved in strcpy.
(unlike the case of file names, which goes around with a length and
used as is).
The bound checks are all wrong. since they are all of comparing
nls_length to unicode_length_limit . changing them to comparing
nls_length to (unicode_length_limit * NLS_MAX) is rather stupid and useless.
So really, we want a cheap function to calculate the unicode length
of a given nls string, and do that instead of calling strlen() as many of these
functions do; and distinguish the two lengths where it matters.
Also, we have two other complications - sometimes the nls length is added to
XATTR_TRUSTED_PREFIX_LEN,XATTR_SECURITY_PREFIX_LEN,XATTR_MAC_OSX_PREFIX_LEN .
Can we assume that XATTR_TRUSTED_PREFIX, XATTR_SECURITY_PREFIX and
XATTR_MAC_OSX_PREFIX will forever stay Engish (i.e. their ascii length
is the same as their unicode length)?
Also with the switch to dynamic allocation, many of the routines
which formerly does
return hfsplus_{get,set}xattr(dentry, xattr_name ...)
needs to be turned into:
int res;
...
res = hfsplus_{get,set}xattr(dentry, xattr_name ...)
kfree(xattr_name);
return res;
As outlined above, all the changes are relatively mundane and tedious,
other than that of calculating the unicode length of an nls string.
Writing a wrapper around hfsplus_asc2uni is one possibility,
except it either returns a len or -ENAMETOOLONG, so we possibly
should convert all the length-based bound check to tests
for failure of conversion to unicode of length of
say HFSPLUS_ATTR_MAX_STRLEN - XATTR_MAC_OSX_PREFIX, and just
test for -ENAMETOOLONG or other failure instead.
Signed-off-by: Hin-Tak Leung <htl10@users.sourceforge.net>
---
fs/hfsplus/attributes.c | 2 +-
fs/hfsplus/xattr.c | 8 ++++----
fs/hfsplus/xattr_security.c | 12 ++++++------
fs/hfsplus/xattr_trusted.c | 8 ++++----
fs/hfsplus/xattr_user.c | 8 ++++----
5 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/fs/hfsplus/attributes.c b/fs/hfsplus/attributes.c
index caf89a7..86422bf 100644
--- a/fs/hfsplus/attributes.c
+++ b/fs/hfsplus/attributes.c
@@ -55,7 +55,7 @@ int hfsplus_attr_build_key(struct super_block *sb, hfsplus_btree_key *key,
key->attr.cnid = cpu_to_be32(cnid);
if (name) {
len = strlen(name);
- if (len > HFSPLUS_ATTR_MAX_STRLEN) {
+ if (len > NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN) {
pr_err("invalid xattr name's length\n");
return -EINVAL;
}
diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index 3034ce6..04b8f5b 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -805,14 +805,14 @@ 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 +
+ char xattr_name[NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN +
XATTR_MAC_OSX_PREFIX_LEN + 1] = {0};
size_t len = strlen(name);
if (!strcmp(name, ""))
return -EINVAL;
- if (len > HFSPLUS_ATTR_MAX_STRLEN)
+ if (len > NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN)
return -EOPNOTSUPP;
/*
@@ -828,14 +828,14 @@ static int hfsplus_osx_getxattr(struct dentry *dentry, const char *name,
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 +
+ char xattr_name[NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN +
XATTR_MAC_OSX_PREFIX_LEN + 1] = {0};
size_t len = strlen(name);
if (!strcmp(name, ""))
return -EINVAL;
- if (len > HFSPLUS_ATTR_MAX_STRLEN)
+ if (len > NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN)
return -EOPNOTSUPP;
/*
diff --git a/fs/hfsplus/xattr_security.c b/fs/hfsplus/xattr_security.c
index 0072276..4dfa15e 100644
--- a/fs/hfsplus/xattr_security.c
+++ b/fs/hfsplus/xattr_security.c
@@ -14,13 +14,13 @@
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};
+ char xattr_name[NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
size_t len = strlen(name);
if (!strcmp(name, ""))
return -EINVAL;
- if (len + XATTR_SECURITY_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
+ if (len + XATTR_SECURITY_PREFIX_LEN > NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN)
return -EOPNOTSUPP;
strcpy(xattr_name, XATTR_SECURITY_PREFIX);
@@ -32,13 +32,13 @@ static int hfsplus_security_getxattr(struct dentry *dentry, const char *name,
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};
+ char xattr_name[NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
size_t len = strlen(name);
if (!strcmp(name, ""))
return -EINVAL;
- if (len + XATTR_SECURITY_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
+ if (len + XATTR_SECURITY_PREFIX_LEN > NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN)
return -EOPNOTSUPP;
strcpy(xattr_name, XATTR_SECURITY_PREFIX);
@@ -62,7 +62,7 @@ static int hfsplus_initxattrs(struct inode *inode,
void *fs_info)
{
const struct xattr *xattr;
- char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
+ char xattr_name[NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
size_t xattr_name_len;
int err = 0;
@@ -73,7 +73,7 @@ static int hfsplus_initxattrs(struct inode *inode,
continue;
if (xattr_name_len + XATTR_SECURITY_PREFIX_LEN >
- HFSPLUS_ATTR_MAX_STRLEN)
+ NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN)
return -EOPNOTSUPP;
strcpy(xattr_name, XATTR_SECURITY_PREFIX);
diff --git a/fs/hfsplus/xattr_trusted.c b/fs/hfsplus/xattr_trusted.c
index 426cee2..965593b 100644
--- a/fs/hfsplus/xattr_trusted.c
+++ b/fs/hfsplus/xattr_trusted.c
@@ -12,13 +12,13 @@
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};
+ char xattr_name[NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
size_t len = strlen(name);
if (!strcmp(name, ""))
return -EINVAL;
- if (len + XATTR_TRUSTED_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
+ if (len + XATTR_TRUSTED_PREFIX_LEN > NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN)
return -EOPNOTSUPP;
strcpy(xattr_name, XATTR_TRUSTED_PREFIX);
@@ -30,13 +30,13 @@ static int hfsplus_trusted_getxattr(struct dentry *dentry, const char *name,
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};
+ char xattr_name[NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
size_t len = strlen(name);
if (!strcmp(name, ""))
return -EINVAL;
- if (len + XATTR_TRUSTED_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
+ if (len + XATTR_TRUSTED_PREFIX_LEN > NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN)
return -EOPNOTSUPP;
strcpy(xattr_name, XATTR_TRUSTED_PREFIX);
diff --git a/fs/hfsplus/xattr_user.c b/fs/hfsplus/xattr_user.c
index e340165..8ead0640 100644
--- a/fs/hfsplus/xattr_user.c
+++ b/fs/hfsplus/xattr_user.c
@@ -12,13 +12,13 @@
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};
+ char xattr_name[NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
size_t len = strlen(name);
if (!strcmp(name, ""))
return -EINVAL;
- if (len + XATTR_USER_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
+ if (len + XATTR_USER_PREFIX_LEN > NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN)
return -EOPNOTSUPP;
strcpy(xattr_name, XATTR_USER_PREFIX);
@@ -30,13 +30,13 @@ static int hfsplus_user_getxattr(struct dentry *dentry, const char *name,
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};
+ char xattr_name[NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
size_t len = strlen(name);
if (!strcmp(name, ""))
return -EINVAL;
- if (len + XATTR_USER_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
+ if (len + XATTR_USER_PREFIX_LEN > NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN)
return -EOPNOTSUPP;
strcpy(xattr_name, XATTR_USER_PREFIX);
--
1.9.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC V0] Exminating the use of HFSPLUS_ATTR_MAX_STRLEN c.f. non-English attributes
2014-04-10 1:04 [RFC V0] Exminating the use of HFSPLUS_ATTR_MAX_STRLEN c.f. non-English attributes Hin-Tak Leung
@ 2014-04-10 7:34 ` Vyacheslav Dubeyko
2014-04-10 9:35 ` Anton Altaparmakov
0 siblings, 1 reply; 3+ messages in thread
From: Vyacheslav Dubeyko @ 2014-04-10 7:34 UTC (permalink / raw)
To: Hin-Tak Leung; +Cc: linux-fsdevel, Hin-Tak Leung
On Thu, 2014-04-10 at 02:04 +0100, Hin-Tak Leung wrote:
> diff --git a/fs/hfsplus/attributes.c b/fs/hfsplus/attributes.c
> index caf89a7..86422bf 100644
> --- a/fs/hfsplus/attributes.c
> +++ b/fs/hfsplus/attributes.c
> @@ -55,7 +55,7 @@ int hfsplus_attr_build_key(struct super_block *sb, hfsplus_btree_key *key,
> key->attr.cnid = cpu_to_be32(cnid);
> if (name) {
> len = strlen(name);
> - if (len > HFSPLUS_ATTR_MAX_STRLEN) {
> + if (len > NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN) {
> pr_err("invalid xattr name's length\n");
> return -EINVAL;
> }
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 3034ce6..04b8f5b 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -805,14 +805,14 @@ 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 +
> + char xattr_name[NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN +
> XATTR_MAC_OSX_PREFIX_LEN + 1] = {0};
It needs to use dynamic memory allocation in all cases in this patch. It
is dangerous way to have such buffer on stack.
Thanks,
Vyacheslav Dubeyko.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC V0] Exminating the use of HFSPLUS_ATTR_MAX_STRLEN c.f. non-English attributes
2014-04-10 7:34 ` Vyacheslav Dubeyko
@ 2014-04-10 9:35 ` Anton Altaparmakov
0 siblings, 0 replies; 3+ messages in thread
From: Anton Altaparmakov @ 2014-04-10 9:35 UTC (permalink / raw)
To: Vyacheslav Dubeyko; +Cc: Hin-Tak Leung, linux-fsdevel, Hin-Tak Leung
On 10 Apr 2014, at 08:34, Vyacheslav Dubeyko <slava@dubeyko.com> wrote:
> On Thu, 2014-04-10 at 02:04 +0100, Hin-Tak Leung wrote:
>> diff --git a/fs/hfsplus/attributes.c b/fs/hfsplus/attributes.c
>> index caf89a7..86422bf 100644
>> --- a/fs/hfsplus/attributes.c
>> +++ b/fs/hfsplus/attributes.c
>> @@ -55,7 +55,7 @@ int hfsplus_attr_build_key(struct super_block *sb, hfsplus_btree_key *key,
>> key->attr.cnid = cpu_to_be32(cnid);
>> if (name) {
>> len = strlen(name);
>> - if (len > HFSPLUS_ATTR_MAX_STRLEN) {
>> + if (len > NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN) {
>> pr_err("invalid xattr name's length\n");
>> return -EINVAL;
>> }
>> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
>> index 3034ce6..04b8f5b 100644
>> --- a/fs/hfsplus/xattr.c
>> +++ b/fs/hfsplus/xattr.c
>> @@ -805,14 +805,14 @@ 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 +
>> + char xattr_name[NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN +
>> XATTR_MAC_OSX_PREFIX_LEN + 1] = {0};
>
> It needs to use dynamic memory allocation in all cases in this patch. It
> is dangerous way to have such buffer on stack.
Did you even _read_ the patch description?
<quote>
They also all need to switch to dynamic allocation (not done here yet, just for simplicity ATM)
</quote>
So your comment is superfluous...
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-04-10 9:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-10 1:04 [RFC V0] Exminating the use of HFSPLUS_ATTR_MAX_STRLEN c.f. non-English attributes Hin-Tak Leung
2014-04-10 7:34 ` Vyacheslav Dubeyko
2014-04-10 9:35 ` Anton Altaparmakov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).