Re: [PATCH 00/12] cifs: fix string conversions from wire format to local charset (try #3)

Jeff Layton wrote:
> This is the third set of patches for this problem. The main changes
> from the last set are some minor renames of some of the functions,
> some fleshing out of comments and a fix for an endianness problem
> that the earlier set introduced to CIFSTCon.
> 
> This patchset is intended to fix the string conversions in cifs that
> convert strings from "wire-format" (little-endian UTF-16) to whatever
> character set is in use on the client.

This patchset looks a lot cleaner, better and more comprehensive. I
spent some time and have looked at all the patches and I don't see any
major issues apart from a few minor comments (which I have sent as reply
to the patch itself)

Please add my Acked-by to other patches as well.
Acked-by: Suresh Jayaraman <[email protected]>

Steve: if you plan not to take some of these that are not directly
string conversion related (e.g. 08/12 11/12), I think they might result
in build warnings as there is a change in CIFSSMBUnixQuerySymLink()
prototype and in some cases build errors as some helpers used by NTLMSSP
code have been removed in previous patches.

Thanks,

> This patch is built upon work by Suresh Jayaraman. This shouldn't be
> taken as a comment on his work -- he did a fine job on the earlier
> patchsets. It became clear to me as I was reviewing them though that
> there were fundamental problems with the existing code that couldn't
> really be remedied without a more invasive set:
> 
> 1) When converting into a destination buffer, the size of that buffer
> isn't checked in a clear and consistent fashion. This patchset attempts
> to remedy that by providing functions that do this.
> 
> 2) In some functions there are clear limits on how many wide characters
> the functions should walk. The numbers passed to them don't always
> reflect the amount of data in the actual packet though, so it's possible
> for these functions to walk off of the end of the source buffer. This
> set attempts to fix that by making the callers of the new functions
> consistently pass in a max size the reflects the size of the data left
> in the response.
> 
> 3) The function interfaces are inconsistent and use wide characters for
> some lengths and bytes for others. This means that we often have to
> convert between different length units and leads to more confusing code.
> This patchset tries to settle on using lengths in bytes for most
> interfaces since that is more useful when allocating buffers and such.
> 
> 4) There are a lot of similar functions that could be consolidated. This
> set does that where it makes sense.
> 
> There are a number of other bugs that are fixed in this. It also removes
> some dead and broken code.
> 
> Comments and suggestions appreciated...
> 
> Jeff Layton (12):
>   nls: add a nls_nullsize inline
>   cifs: move #defines for mapchars into cifs_unicode.h
>   cifs: add replacement for cifs_strtoUCS_le called cifs_from_ucs2
>   cifs: add new function to get unicode string length in bytes
>   cifs: rename cifs_strlcpy_to_host and make it use new functions
>   cifs: convert CIFSTCon to use new unicode helper functions
>   cifs: fix session setup unicode string saving to use new unicode
>     helpers
>   cifs: remove cifs_readlink and CIFSSMBQueryReparseLinkInfo
>   cifs: change CIFSSMBUnixQuerySymLink to use new helpers
>   cifs: change cifs_get_name_from_search_buf to use new unicode helper
>   cifs: remove legacy NTLMSSP code
>   cifs: remove cifs_strfromUCS_le
> 
>  fs/cifs/cifs_unicode.c |  201 ++++++++-
>  fs/cifs/cifs_unicode.h |   20 +-
>  fs/cifs/cifsfs.h       |    2 -
>  fs/cifs/cifspdu.h      |   11 -
>  fs/cifs/cifsproto.h    |    5 +-
>  fs/cifs/cifssmb.c      |  216 +--------
>  fs/cifs/connect.c      | 1150 +-----------------------------------------------
>  fs/cifs/link.c         |  114 +-----
>  fs/cifs/misc.c         |   71 ---
>  fs/cifs/readdir.c      |   26 +-
>  fs/cifs/sess.c         |   80 +---
>  include/linux/nls.h    |   19 +
>  12 files changed, 299 insertions(+), 1616 deletions(-)
> 
> _______________________________________________
> linux-cifs-client mailing list
> [email protected]
> https://lists.samba.org/mailman/listinfo/linux-cifs-client


-- 
Suresh Jayaraman