Re: [PATCH 02/10] udf: fix udf_build_ustr

[Jan Kara]

On Wed 30-01-08 22:03:52, marcin.slusarz@xxxxxxxxx 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@xxxxxxxxx>
> Cc: Jan Kara <jack@xxxxxxx>
> ---
>  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...
>  
> -	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.

								Honza
-- 
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/