diff mbox series

Convert trailing spaces and periods in path components

Message ID CAHhKpQ4UFhtfRhByRiAm6KPy=KAzttYzZADLfakbMwpsp5GjpA@mail.gmail.com
State New
Headers show
Series Convert trailing spaces and periods in path components | expand

Commit Message

Boris Protopopov Sept. 29, 2020, 5:08 p.m. UTC
Testing:

Prior to the patch:

% mount -v
…
//host/share/home on /tmp/diry type cifs (rw,relatime,vers=default,...
% ls -l /tmp/diry/tmp
total 0
% mkdir /tmp/diry/tmp/DirWithTrailingDot.
% ls -l  /tmp/diry/tmp/DirWithTrailingDot.
total 0
% touch  /tmp/diry/tmp/DirWithTrailingDot./file
touch: cannot touch ‘/tmp/diry/tmp/DirWithTrailingDot./file’: No such
file or directory
% mkdir  /tmp/diry/tmp/DirWithTrailingDot./dir
mkdir: cannot create directory
‘/tmp/diry/tmp/DirWithTrailingDot./dir’: No such file or directory
% find  /tmp/diry/tmp/DirWithTrailingDot.
/tmp/diry/tmp/DirWithTrailingDot.
% find  /tmp/diry/tmp/DirWithTrailingSpace\
find: `/tmp/diry/tmp/DirWithTrailingSpace ': No such file or directory
% mkdir  /tmp/diry/tmp/DirWithTrailingSpace\
% ls -l  /tmp/diry/tmp/DirWithTrailingSpace\
total 0
% touch /tmp/diry/tmp/DirWithTrailingSpace\ /file
touch: cannot touch ‘/tmp/diry/tmp/DirWithTrailingSpace /file’: No
such file or directory
% mkdir /tmp/diry/tmp/DirWithTrailingSpace\ /dir
mkdir: cannot create directory ‘/tmp/diry/tmp/DirWithTrailingSpace
/dir’: No such file or directory

After the patch:

% umount /tmp/diry
% modprobe -r cifs
# load the fix
% modprobe cifs
% mount -t cifs -o...  //host/share/home /tmp/diry
...
% mkdir /tmp/diry/tmp/DirWithTrailingSpace\ /dir
% touch /tmp/diry/tmp/DirWithTrailingSpace\ /file
% mkdir  /tmp/diry/tmp/DirWithTrailingDot./dir
% touch  /tmp/diry/tmp/DirWithTrailingDot./file
% find  /tmp/diry/tmp/
/tmp/diry/tmp/
/tmp/diry/tmp/DirWithTrailingDot.
/tmp/diry/tmp/DirWithTrailingDot./dir
/tmp/diry/tmp/DirWithTrailingDot./file
/tmp/diry/tmp/DirWithTrailingSpace
/tmp/diry/tmp/DirWithTrailingSpace /dir
/tmp/diry/tmp/DirWithTrailingSpace /file
% rm -rf /tmp/diry/tmp/*
% find  /tmp/diry/tmp/
/tmp/diry/tmp/

---------- Forwarded message ---------
From: Boris Protopopov <pboris@amazon.com>
Date: Wed, Sep 23, 2020 at 8:39 PM
Subject: [PATCH] Convert trailing spaces and periods in path components
To:
Cc: <linux-cifs@vger.kernel.org>, <samba-technical@lists.samba.org>,
Boris Protopopov <pboris@amazon.com>


When converting trailing spaces and periods in paths, do so
for every component of the path, not just the last component.
If the conversion is not done for every path component, then
subsequent operations in directories with trailing spaces or
periods (e.g. create(), mkdir()) will fail with ENOENT. This
is because on the server, the directory will have a special
symbol in its name, and the client needs to provide the same.

Signed-off-by: Boris Protopopov <pboris@amazon.com>
---
 fs/cifs/cifs_unicode.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

                                end_of_string = false;
--
2.18.4

Comments

Tom Talpey Sept. 29, 2020, 5:23 p.m. UTC | #1
What was the server for these tests?

On 9/29/2020 1:08 PM, Boris Protopopov wrote:
> Testing:
> 
> Prior to the patch:
> 
> % mount -v
> …
> //host/share/home on /tmp/diry type cifs (rw,relatime,vers=default,...
> % ls -l /tmp/diry/tmp
> total 0
> % mkdir /tmp/diry/tmp/DirWithTrailingDot.
> % ls -l  /tmp/diry/tmp/DirWithTrailingDot.
> total 0
> % touch  /tmp/diry/tmp/DirWithTrailingDot./file
> touch: cannot touch ‘/tmp/diry/tmp/DirWithTrailingDot./file’: No such
> file or directory
> % mkdir  /tmp/diry/tmp/DirWithTrailingDot./dir
> mkdir: cannot create directory
> ‘/tmp/diry/tmp/DirWithTrailingDot./dir’: No such file or directory
> % find  /tmp/diry/tmp/DirWithTrailingDot.
> /tmp/diry/tmp/DirWithTrailingDot.
> % find  /tmp/diry/tmp/DirWithTrailingSpace\
> find: `/tmp/diry/tmp/DirWithTrailingSpace ': No such file or directory
> % mkdir  /tmp/diry/tmp/DirWithTrailingSpace\
> % ls -l  /tmp/diry/tmp/DirWithTrailingSpace\
> total 0
> % touch /tmp/diry/tmp/DirWithTrailingSpace\ /file
> touch: cannot touch ‘/tmp/diry/tmp/DirWithTrailingSpace /file’: No
> such file or directory
> % mkdir /tmp/diry/tmp/DirWithTrailingSpace\ /dir
> mkdir: cannot create directory ‘/tmp/diry/tmp/DirWithTrailingSpace
> /dir’: No such file or directory
> 
> After the patch:
> 
> % umount /tmp/diry
> % modprobe -r cifs
> # load the fix
> % modprobe cifs
> % mount -t cifs -o...  //host/share/home /tmp/diry
> ...
> % mkdir /tmp/diry/tmp/DirWithTrailingSpace\ /dir
> % touch /tmp/diry/tmp/DirWithTrailingSpace\ /file
> % mkdir  /tmp/diry/tmp/DirWithTrailingDot./dir
> % touch  /tmp/diry/tmp/DirWithTrailingDot./file
> % find  /tmp/diry/tmp/
> /tmp/diry/tmp/
> /tmp/diry/tmp/DirWithTrailingDot.
> /tmp/diry/tmp/DirWithTrailingDot./dir
> /tmp/diry/tmp/DirWithTrailingDot./file
> /tmp/diry/tmp/DirWithTrailingSpace
> /tmp/diry/tmp/DirWithTrailingSpace /dir
> /tmp/diry/tmp/DirWithTrailingSpace /file
> % rm -rf /tmp/diry/tmp/*
> % find  /tmp/diry/tmp/
> /tmp/diry/tmp/
> 
> ---------- Forwarded message ---------
> From: Boris Protopopov <pboris@amazon.com>
> Date: Wed, Sep 23, 2020 at 8:39 PM
> Subject: [PATCH] Convert trailing spaces and periods in path components
> To:
> Cc: <linux-cifs@vger.kernel.org>, <samba-technical@lists.samba.org>,
> Boris Protopopov <pboris@amazon.com>
> 
> 
> When converting trailing spaces and periods in paths, do so
> for every component of the path, not just the last component.
> If the conversion is not done for every path component, then
> subsequent operations in directories with trailing spaces or
> periods (e.g. create(), mkdir()) will fail with ENOENT. This
> is because on the server, the directory will have a special
> symbol in its name, and the client needs to provide the same.
> 
> Signed-off-by: Boris Protopopov <pboris@amazon.com>
> ---
>   fs/cifs/cifs_unicode.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
> index 498777d859eb..9bd03a231032 100644
> --- a/fs/cifs/cifs_unicode.c
> +++ b/fs/cifs/cifs_unicode.c
> @@ -488,7 +488,13 @@ cifsConvertToUTF16(__le16 *target, const char
> *source, int srclen,
>                  else if (map_chars == SFM_MAP_UNI_RSVD) {
>                          bool end_of_string;
> 
> -                       if (i == srclen - 1)
> +                       /**
> +                        * Remap spaces and periods found at the end of every
> +                        * component of the path. The special cases of '.' and
> +                        * '..' do not need to be dealt with explicitly because
> +                        * they are addressed in namei.c:link_path_walk().
> +                        **/
> +                       if ((i == srclen - 1) || (source[i+1] == '\\'))
>                                  end_of_string = true;
>                          else
>                                  end_of_string = false;
> --
> 2.18.4
> 
>
Steve French Sept. 29, 2020, 5:45 p.m. UTC | #2
tentatively merged ... running the usual functional tests
http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/399

On Tue, Sep 29, 2020 at 12:08 PM Boris Protopopov
<boris.v.protopopov@gmail.com> wrote:
>
> Testing:
>
> Prior to the patch:
>
> % mount -v
> …
> //host/share/home on /tmp/diry type cifs (rw,relatime,vers=default,...
> % ls -l /tmp/diry/tmp
> total 0
> % mkdir /tmp/diry/tmp/DirWithTrailingDot.
> % ls -l  /tmp/diry/tmp/DirWithTrailingDot.
> total 0
> % touch  /tmp/diry/tmp/DirWithTrailingDot./file
> touch: cannot touch ‘/tmp/diry/tmp/DirWithTrailingDot./file’: No such
> file or directory
> % mkdir  /tmp/diry/tmp/DirWithTrailingDot./dir
> mkdir: cannot create directory
> ‘/tmp/diry/tmp/DirWithTrailingDot./dir’: No such file or directory
> % find  /tmp/diry/tmp/DirWithTrailingDot.
> /tmp/diry/tmp/DirWithTrailingDot.
> % find  /tmp/diry/tmp/DirWithTrailingSpace\
> find: `/tmp/diry/tmp/DirWithTrailingSpace ': No such file or directory
> % mkdir  /tmp/diry/tmp/DirWithTrailingSpace\
> % ls -l  /tmp/diry/tmp/DirWithTrailingSpace\
> total 0
> % touch /tmp/diry/tmp/DirWithTrailingSpace\ /file
> touch: cannot touch ‘/tmp/diry/tmp/DirWithTrailingSpace /file’: No
> such file or directory
> % mkdir /tmp/diry/tmp/DirWithTrailingSpace\ /dir
> mkdir: cannot create directory ‘/tmp/diry/tmp/DirWithTrailingSpace
> /dir’: No such file or directory
>
> After the patch:
>
> % umount /tmp/diry
> % modprobe -r cifs
> # load the fix
> % modprobe cifs
> % mount -t cifs -o...  //host/share/home /tmp/diry
> ...
> % mkdir /tmp/diry/tmp/DirWithTrailingSpace\ /dir
> % touch /tmp/diry/tmp/DirWithTrailingSpace\ /file
> % mkdir  /tmp/diry/tmp/DirWithTrailingDot./dir
> % touch  /tmp/diry/tmp/DirWithTrailingDot./file
> % find  /tmp/diry/tmp/
> /tmp/diry/tmp/
> /tmp/diry/tmp/DirWithTrailingDot.
> /tmp/diry/tmp/DirWithTrailingDot./dir
> /tmp/diry/tmp/DirWithTrailingDot./file
> /tmp/diry/tmp/DirWithTrailingSpace
> /tmp/diry/tmp/DirWithTrailingSpace /dir
> /tmp/diry/tmp/DirWithTrailingSpace /file
> % rm -rf /tmp/diry/tmp/*
> % find  /tmp/diry/tmp/
> /tmp/diry/tmp/
>
> ---------- Forwarded message ---------
> From: Boris Protopopov <pboris@amazon.com>
> Date: Wed, Sep 23, 2020 at 8:39 PM
> Subject: [PATCH] Convert trailing spaces and periods in path components
> To:
> Cc: <linux-cifs@vger.kernel.org>, <samba-technical@lists.samba.org>,
> Boris Protopopov <pboris@amazon.com>
>
>
> When converting trailing spaces and periods in paths, do so
> for every component of the path, not just the last component.
> If the conversion is not done for every path component, then
> subsequent operations in directories with trailing spaces or
> periods (e.g. create(), mkdir()) will fail with ENOENT. This
> is because on the server, the directory will have a special
> symbol in its name, and the client needs to provide the same.
>
> Signed-off-by: Boris Protopopov <pboris@amazon.com>
> ---
>  fs/cifs/cifs_unicode.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
> index 498777d859eb..9bd03a231032 100644
> --- a/fs/cifs/cifs_unicode.c
> +++ b/fs/cifs/cifs_unicode.c
> @@ -488,7 +488,13 @@ cifsConvertToUTF16(__le16 *target, const char
> *source, int srclen,
>                 else if (map_chars == SFM_MAP_UNI_RSVD) {
>                         bool end_of_string;
>
> -                       if (i == srclen - 1)
> +                       /**
> +                        * Remap spaces and periods found at the end of every
> +                        * component of the path. The special cases of '.' and
> +                        * '..' do not need to be dealt with explicitly because
> +                        * they are addressed in namei.c:link_path_walk().
> +                        **/
> +                       if ((i == srclen - 1) || (source[i+1] == '\\'))
>                                 end_of_string = true;
>                         else
>                                 end_of_string = false;
> --
> 2.18.4
Boris Protopopov Sept. 29, 2020, 5:52 p.m. UTC | #3
---------- Forwarded message ---------
From: Boris Protopopov <boris.v.protopopov@gmail.com>
Date: Tue, Sep 29, 2020 at 1:51 PM
Subject: Re: Fwd: [PATCH] Convert trailing spaces and periods in path components
To: Tom Talpey <tom@talpey.com>


Windows Server 2019, should have mentioned above.

On Tue, Sep 29, 2020 at 1:23 PM Tom Talpey <tom@talpey.com> wrote:
>
> What was the server for these tests?
>
> On 9/29/2020 1:08 PM, Boris Protopopov wrote:
> > Testing:
> >
> > Prior to the patch:
> >
> > % mount -v
> > …
> > //host/share/home on /tmp/diry type cifs (rw,relatime,vers=default,...
> > % ls -l /tmp/diry/tmp
> > total 0
> > % mkdir /tmp/diry/tmp/DirWithTrailingDot.
> > % ls -l  /tmp/diry/tmp/DirWithTrailingDot.
> > total 0
> > % touch  /tmp/diry/tmp/DirWithTrailingDot./file
> > touch: cannot touch ‘/tmp/diry/tmp/DirWithTrailingDot./file’: No such
> > file or directory
> > % mkdir  /tmp/diry/tmp/DirWithTrailingDot./dir
> > mkdir: cannot create directory
> > ‘/tmp/diry/tmp/DirWithTrailingDot./dir’: No such file or directory
> > % find  /tmp/diry/tmp/DirWithTrailingDot.
> > /tmp/diry/tmp/DirWithTrailingDot.
> > % find  /tmp/diry/tmp/DirWithTrailingSpace\
> > find: `/tmp/diry/tmp/DirWithTrailingSpace ': No such file or directory
> > % mkdir  /tmp/diry/tmp/DirWithTrailingSpace\
> > % ls -l  /tmp/diry/tmp/DirWithTrailingSpace\
> > total 0
> > % touch /tmp/diry/tmp/DirWithTrailingSpace\ /file
> > touch: cannot touch ‘/tmp/diry/tmp/DirWithTrailingSpace /file’: No
> > such file or directory
> > % mkdir /tmp/diry/tmp/DirWithTrailingSpace\ /dir
> > mkdir: cannot create directory ‘/tmp/diry/tmp/DirWithTrailingSpace
> > /dir’: No such file or directory
> >
> > After the patch:
> >
> > % umount /tmp/diry
> > % modprobe -r cifs
> > # load the fix
> > % modprobe cifs
> > % mount -t cifs -o...  //host/share/home /tmp/diry
> > ...
> > % mkdir /tmp/diry/tmp/DirWithTrailingSpace\ /dir
> > % touch /tmp/diry/tmp/DirWithTrailingSpace\ /file
> > % mkdir  /tmp/diry/tmp/DirWithTrailingDot./dir
> > % touch  /tmp/diry/tmp/DirWithTrailingDot./file
> > % find  /tmp/diry/tmp/
> > /tmp/diry/tmp/
> > /tmp/diry/tmp/DirWithTrailingDot.
> > /tmp/diry/tmp/DirWithTrailingDot./dir
> > /tmp/diry/tmp/DirWithTrailingDot./file
> > /tmp/diry/tmp/DirWithTrailingSpace
> > /tmp/diry/tmp/DirWithTrailingSpace /dir
> > /tmp/diry/tmp/DirWithTrailingSpace /file
> > % rm -rf /tmp/diry/tmp/*
> > % find  /tmp/diry/tmp/
> > /tmp/diry/tmp/
> >
> > ---------- Forwarded message ---------
> > From: Boris Protopopov <pboris@amazon.com>
> > Date: Wed, Sep 23, 2020 at 8:39 PM
> > Subject: [PATCH] Convert trailing spaces and periods in path components
> > To:
> > Cc: <linux-cifs@vger.kernel.org>, <samba-technical@lists.samba.org>,
> > Boris Protopopov <pboris@amazon.com>
> >
> >
> > When converting trailing spaces and periods in paths, do so
> > for every component of the path, not just the last component.
> > If the conversion is not done for every path component, then
> > subsequent operations in directories with trailing spaces or
> > periods (e.g. create(), mkdir()) will fail with ENOENT. This
> > is because on the server, the directory will have a special
> > symbol in its name, and the client needs to provide the same.
> >
> > Signed-off-by: Boris Protopopov <pboris@amazon.com>
> > ---
> >   fs/cifs/cifs_unicode.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
> > index 498777d859eb..9bd03a231032 100644
> > --- a/fs/cifs/cifs_unicode.c
> > +++ b/fs/cifs/cifs_unicode.c
> > @@ -488,7 +488,13 @@ cifsConvertToUTF16(__le16 *target, const char
> > *source, int srclen,
> >                  else if (map_chars == SFM_MAP_UNI_RSVD) {
> >                          bool end_of_string;
> >
> > -                       if (i == srclen - 1)
> > +                       /**
> > +                        * Remap spaces and periods found at the end of every
> > +                        * component of the path. The special cases of '.' and
> > +                        * '..' do not need to be dealt with explicitly because
> > +                        * they are addressed in namei.c:link_path_walk().
> > +                        **/
> > +                       if ((i == srclen - 1) || (source[i+1] == '\\'))
> >                                  end_of_string = true;
> >                          else
> >                                  end_of_string = false;
> > --
> > 2.18.4
> >
> >
Aurélien Aptel Sept. 30, 2020, 9:25 a.m. UTC | #4
Steve French <smfrench@gmail.com> writes:
> tentatively merged ... running the usual functional tests
> http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/399

We need to make sure it works with samba smb1&smb2 posix extensions.
In smb1 posix extensions the paths are sent with / and \ is a valid
component (and no restrictions on file name endings).

Cheers,
Boris Protopopov Sept. 30, 2020, 9:13 p.m. UTC | #5
I agree with testing, but also, looking at the comments:

/*
 * Convert 16 bit Unicode pathname to wire format from string in
current code
 * page. Conversion may involve remapping up the six characters that
are
 * only legal in POSIX-like OS (if they are present in the string).
Path
 * names are little endian 16 bit Unicode on the wire
 */
int
cifsConvertToUTF16(__le16 *target, const char *source, int srclen,
                 const struct nls_table *cp, int map_chars)
{
        int i, charlen;
        int j = 0;
        char src_char;
        __le16 dst_char;
        wchar_t tmp;
        wchar_t *wchar_to;      /* UTF-16 */
        int ret;
        unicode_t u;

        if (map_chars == NO_MAP_UNI_RSVD)
                return cifs_strtoUTF16(target, source, PATH_MAX, cp);

...

it would seem that as long as the correct map_chars is picked, it
should work correctly? The latter, if not automatically determined,
can be controlled with mount options, I believe.

Boris.

On Wed, Sep 30, 2020 at 5:25 AM Aurélien Aptel <aaptel@suse.com> wrote:
>
> Steve French <smfrench@gmail.com> writes:
> > tentatively merged ... running the usual functional tests
> > http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/399
>
> We need to make sure it works with samba smb1&smb2 posix extensions.
> In smb1 posix extensions the paths are sent with / and \ is a valid
> component (and no restrictions on file name endings).
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
diff mbox series

Patch

diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
index 498777d859eb..9bd03a231032 100644
--- a/fs/cifs/cifs_unicode.c
+++ b/fs/cifs/cifs_unicode.c
@@ -488,7 +488,13 @@  cifsConvertToUTF16(__le16 *target, const char
*source, int srclen,
                else if (map_chars == SFM_MAP_UNI_RSVD) {
                        bool end_of_string;

-                       if (i == srclen - 1)
+                       /**
+                        * Remap spaces and periods found at the end of every
+                        * component of the path. The special cases of '.' and
+                        * '..' do not need to be dealt with explicitly because
+                        * they are addressed in namei.c:link_path_walk().
+                        **/
+                       if ((i == srclen - 1) || (source[i+1] == '\\'))
                                end_of_string = true;
                        else