Re: [PATCH] cifs: Fix insufficient memory allocation for nativeFileSystem field

On Thu, 09 Apr 2009 19:49:53 +0530
Suresh Jayaraman <[email protected]> wrote:

> Jeff Layton wrote:
> > On Tue, 07 Apr 2009 18:45:46 +0530
> > Suresh Jayaraman <[email protected]> wrote:
> > 
> >> Do we still need this conversion again?
> >>
> > 
> > I know this isn't a "real" patch submission yet, but some comments
> > below...
> > 
> >> diff --git a/fs/cifs/cifs_unicode.h b/fs/cifs/cifs_unicode.h
> >> index 14eb9a2..0396bdc 100644
> >> --- a/fs/cifs/cifs_unicode.h
> >> +++ b/fs/cifs/cifs_unicode.h
> >> @@ -159,6 +159,23 @@ UniStrnlen(const wchar_t *ucs1, int maxlen)
> >>  }
> >>
> >>  /*
> >> + * UniStrnlenBytes: Return the length in bytes of a UTF-8 string
> >> + */
> >> +static inline size_t
> >> +UniStrnlenBytes(const unsigned char *str, int maxlen)
> >> +{
> >> +       size_t nbytes = 0;
> >> +       wchar_t *uni;
> > 		^^^^^
> > I think you need to allocate actual storage for the character here.
> > 
> 
> Oh, wait no storage is required I think. We reuse the pointer to wchar_t
> and it seems char2uni() returns 1 or -EINVAL
> and I was thinking strlen() on a wide char returns length in bytes?
> 
> fs/nls/nls_base.c
> 
> static int char2uni(const unsigned char *rawstring, int boundlen,
> wchar_t *uni)
> {
>         *uni = charset2uni[*rawstring];

^^^ No, you do storage. You're passing a pointer to a wchar_t to
char2uni(). char2uni() then assigns the target of that pointer a value.
With your current code, that pointer is just junk from the stack when
you pass it to char2uni. It could panic or cause random memory
corruption in its current form.

What you really want to do is change that declaration to:

	wchar_t	uni;

...and then do:

	char2uni(str, maxlen, &uni)


-- 
Jeff Layton <[email protected]>