Messages in this thread Patch in this message | | | Date | Thu, 31 Jan 2008 20:57:47 +0100 | From | Marcin Slusarz <> | Subject | Re: [PATCH 02/10] udf: fix udf_build_ustr |
| |
On Thu, Jan 31, 2008 at 11:45:32AM +0100, Jan Kara wrote: > On Wed 30-01-08 22:03:52, marcin.slusarz@gmail.com wrote: > > udf_build_ustr was completely broken when > > size >= UDF_NAME_LEN - 1 or size < 2 > > > > nobody noticed because all callers set size > > to acceptable values (constants) > > > > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com> > > Cc: Jan Kara <jack@suse.cz> > > --- > > fs/udf/unicode.c | 12 ++++++------ > > 1 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c > > index f969617..f4e54e5 100644 > > --- a/fs/udf/unicode.c > > +++ b/fs/udf/unicode.c > > @@ -47,16 +47,16 @@ static int udf_char_to_ustr(struct ustr *dest, const uint8_t *src, int strlen) > > */ > > int udf_build_ustr(struct ustr *dest, dstring *ptr, int size) > > { > > - int usesize; > > + u8 usesize; > What is the purpose for this? Why not just leave int there? Arithmetics > is usually best done in ints if that's possible... I made it to stress that length of string fits in one byte. (struct ustr->u_len is uint8_t)
> > - if ((!dest) || (!ptr) || (!size)) > > + if (!dest || !ptr || size < 2) > > return -1; > > > > - memset(dest, 0, sizeof(struct ustr)); > > - usesize = (size > UDF_NAME_LEN) ? UDF_NAME_LEN : size; > > + usesize = min_t(size_t, size - 2, sizeof(dest->u_name)); > > dest->u_cmpID = ptr[0]; > > - dest->u_len = ptr[size - 1]; > > - memcpy(dest->u_name, ptr + 1, usesize - 1); > > + dest->u_len = usesize; > > + memcpy(dest->u_name, ptr + 1, usesize); > > + memset(dest->u_name + usesize, 0, sizeof(dest->u_name) - usesize); > Hmm, after parsing what the standard says (ugh), I don't think the code is > wrong (at least I think you made it incorrect). The caller of > udf_char_to_ustr() specifies length of the field (not length of the > string). Now, in the last character of the field is stored the number of > characters in the string and in the first character of the field is stored > encoding of the string. So the original code seems correct. You are right. I broke length calculation.
But observe that: - when size == 1: dest->u_len = ptr[1 - 1], but at ptr[0] there's cmpID, so we create string with wrong length - when size > 1 and size < UDF_NAME_LEN: we set u_len correctly, but memcpy copies one needless byte - when size == UDF_NAME_LEN - 1: memcpy overwrites u_len - with correct value, but... - when size >= UDF_NAME_LEN: we copy UDF_NAME_LEN - 1 bytes, but dest->u_name is array of UDF_NAME_LEN - 2 bytes, so we are overwriting u_len with character from input string
So if I didn't mess up someting, correct change would look like this: --- udf: fix udf_build_ustr
udf_build_ustr was broken when size >= UDF_NAME_LEN or size < 2
nobody noticed because all callers set size to acceptable values (constants whitin range)
Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com> Cc: Jan Kara <jack@suse.cz> --- fs/udf/unicode.c | 13 +++++++------ 1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c index 335fc56..83ae9fc 100644 --- a/fs/udf/unicode.c +++ b/fs/udf/unicode.c @@ -47,16 +47,17 @@ static int udf_char_to_ustr(struct ustr *dest, const uint8_t *src, int strlen) */ int udf_build_ustr(struct ustr *dest, dstring *ptr, int size) { - int usesize; + u8 usesize; - if ((!dest) || (!ptr) || (!size)) + if (!dest || !ptr || size < 2) return -1; - memset(dest, 0, sizeof(struct ustr)); - usesize = (size > UDF_NAME_LEN) ? UDF_NAME_LEN : size; + usesize = min_t(size_t, ptr[size - 1], sizeof(dest->u_name)); + usesize = min_t(size_t, usesize, size - 2); dest->u_cmpID = ptr[0]; - dest->u_len = ptr[size - 1]; - memcpy(dest->u_name, ptr + 1, usesize - 1); + dest->u_len = usesize; + memcpy(dest->u_name, ptr + 1, usesize); + memset(dest->u_name + usesize, 0, sizeof(dest->u_name) - usesize); return 0; } -- 1.5.3.7
| |