[PATCH] two corner case fixes to shadow_copy2

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] two corner case fixes to shadow_copy2

Samba - samba-technical mailing list
Hi all,

we are hitting a regression with Samba 4.6 and the
combination of the shadow_copy2 and glusterfs vfs
modules.

It seems to have been introduced by the patches of
https://bugzilla.samba.org/show_bug.cgi?id=12531

It's my fault that I did not notice the patches
and the request for review at that time... :-)

I still need to do more analysis and testing,
but it seems that there are two corner cases
that are at least hit when using the glusterfs
vfs module that mimicks the share root to "/":

1) make_relative_path does not treat the
   case of cwd == "/" correctly.

2) shadow_copy2_strip_snapshot does not treat the
   corner case where the name only consists of
   "/" followed by the @GMT-token correctly.

These corner cases are not coverered by our
test cases, and as I said, I need to do more
testing (e.g. if this is all), but attached find
two patches that fix the above corner cases
in a rahter obvious way.

Thanks for review...

Michael

patch3 (2K) Download Attachment
signature.asc (169 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] two corner case fixes to shadow_copy2

Samba - samba-technical mailing list
On Tue, Apr 11, 2017 at 12:53:25PM +0200, Michael Adam via samba-technical wrote:

> Hi all,
>
> we are hitting a regression with Samba 4.6 and the
> combination of the shadow_copy2 and glusterfs vfs
> modules.
>
> It seems to have been introduced by the patches of
> https://bugzilla.samba.org/show_bug.cgi?id=12531
>
> It's my fault that I did not notice the patches
> and the request for review at that time... :-)
>
> I still need to do more analysis and testing,
> but it seems that there are two corner cases
> that are at least hit when using the glusterfs
> vfs module that mimicks the share root to "/":
>
> 1) make_relative_path does not treat the
>    case of cwd == "/" correctly.
>
> 2) shadow_copy2_strip_snapshot does not treat the
>    corner case where the name only consists of
>    "/" followed by the @GMT-token correctly.
>
> These corner cases are not coverered by our
> test cases, and as I said, I need to do more
> testing (e.g. if this is all), but attached find
> two patches that fix the above corner cases
> in a rahter obvious way.
>
> Thanks for review...

Yep, good catch. Wish you'd found these
earlier (I did ask :-). Never mind, better
late than never.

RB+. Pushed, with an update to the comments
inline.

Jeremy.

> From 76e15576e3144ca87bca3dee0ddba9f60ba04fed Mon Sep 17 00:00:00 2001
> From: Michael Adam <[hidden email]>
> Date: Tue, 11 Apr 2017 12:03:20 +0200
> Subject: [PATCH 1/2] s3:vfs:shadow_copy2: fix the corner case if cwd=/ in
>  make_relative_path
>
> Signed-off-by: Michael Adam <[hidden email]>
> ---
>  source3/modules/vfs_shadow_copy2.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
> index f3ec8b6..550c953 100644
> --- a/source3/modules/vfs_shadow_copy2.c
> +++ b/source3/modules/vfs_shadow_copy2.c
> @@ -444,7 +444,10 @@ static bool make_relative_path(const char *cwd, char *abs_path)
>   if (memcmp(abs_path, cwd, cwd_len) != 0) {
>   return false;
>   }
> - if (abs_path[cwd_len] != '/' && abs_path[cwd_len] != '\0') {
> + if (cwd_len != 1 &&
> +    abs_path[cwd_len] != '/' &&
> +    abs_path[cwd_len] != '\0')
> + {
>   return false;
>   }
>   if (abs_path[cwd_len] == '/') {
> --
> 2.9.3
>
>
> From 6b095855ecc3038a03278e81f53398bc7c35a8bf Mon Sep 17 00:00:00 2001
> From: Michael Adam <[hidden email]>
> Date: Tue, 11 Apr 2017 12:03:52 +0200
> Subject: [PATCH 2/2] s3:vfs:shadow_copy2: fix corner case of "/@GMT-token" in
>  shadow_copy2_strip_snapshot
>
> Signed-off-by: Michael Adam <[hidden email]>
> ---
>  source3/modules/vfs_shadow_copy2.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
> index 550c953..d06e321 100644
> --- a/source3/modules/vfs_shadow_copy2.c
> +++ b/source3/modules/vfs_shadow_copy2.c
> @@ -670,10 +670,11 @@ static bool shadow_copy2_strip_snapshot_internal(TALLOC_CTX *mem_ctx,
>   * with a path prefix.
>   */
>   if (pstripped != NULL) {
> - if (len_before_gmt > 0) {
> + if (len_before_gmt > 1) {
>   /*
> - * There is a slash before
> - * the @GMT-. Remove it.
> + * There is a path (and not only a slash)
> + * before the @GMT-. Remove the trailing
> + * slash character.
>   */
>   len_before_gmt -= 1;
>   }
> --
> 2.9.3
>




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] two corner case fixes to shadow_copy2

Samba - samba-technical mailing list
On 2017-04-11 at 11:27 -0700, Jeremy Allison wrote:

> On Tue, Apr 11, 2017 at 12:53:25PM +0200, Michael Adam via samba-technical wrote:
> > Hi all,
> >
> > we are hitting a regression with Samba 4.6 and the
> > combination of the shadow_copy2 and glusterfs vfs
> > modules.
> >
> > It seems to have been introduced by the patches of
> > https://bugzilla.samba.org/show_bug.cgi?id=12531
> >
> > It's my fault that I did not notice the patches
> > and the request for review at that time... :-)
> >
> > I still need to do more analysis and testing,
> > but it seems that there are two corner cases
> > that are at least hit when using the glusterfs
> > vfs module that mimicks the share root to "/":
> >
> > 1) make_relative_path does not treat the
> >    case of cwd == "/" correctly.
> >
> > 2) shadow_copy2_strip_snapshot does not treat the
> >    corner case where the name only consists of
> >    "/" followed by the @GMT-token correctly.
> >
> > These corner cases are not coverered by our
> > test cases, and as I said, I need to do more
> > testing (e.g. if this is all), but attached find
> > two patches that fix the above corner cases
> > in a rahter obvious way.
> >
> > Thanks for review...
>
> Yep, good catch. Wish you'd found these
> earlier (I did ask :-).
As I said: My bad for not having managed to
be attentive ... :-/

> Never mind, better late than never.
>
> RB+. Pushed, with an update to the comments
> inline.

Thanks!

Will continue the testing and reviewing.

Thinking about how to cover these scenarios
in selftest...  I could imagine writing
a (test-only) vfs module to put at the bottom
of the vfs stack instead of vfs_glusterfs or
other similar modules, and just shields away
the real share root dir into a module
specific variable, having the path of
the share definition at "/"...

Michael


> Jeremy.
>
> > From 76e15576e3144ca87bca3dee0ddba9f60ba04fed Mon Sep 17 00:00:00 2001
> > From: Michael Adam <[hidden email]>
> > Date: Tue, 11 Apr 2017 12:03:20 +0200
> > Subject: [PATCH 1/2] s3:vfs:shadow_copy2: fix the corner case if cwd=/ in
> >  make_relative_path
> >
> > Signed-off-by: Michael Adam <[hidden email]>
> > ---
> >  source3/modules/vfs_shadow_copy2.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
> > index f3ec8b6..550c953 100644
> > --- a/source3/modules/vfs_shadow_copy2.c
> > +++ b/source3/modules/vfs_shadow_copy2.c
> > @@ -444,7 +444,10 @@ static bool make_relative_path(const char *cwd, char *abs_path)
> >   if (memcmp(abs_path, cwd, cwd_len) != 0) {
> >   return false;
> >   }
> > - if (abs_path[cwd_len] != '/' && abs_path[cwd_len] != '\0') {
> > + if (cwd_len != 1 &&
> > +    abs_path[cwd_len] != '/' &&
> > +    abs_path[cwd_len] != '\0')
> > + {
> >   return false;
> >   }
> >   if (abs_path[cwd_len] == '/') {
> > --
> > 2.9.3
> >
> >
> > From 6b095855ecc3038a03278e81f53398bc7c35a8bf Mon Sep 17 00:00:00 2001
> > From: Michael Adam <[hidden email]>
> > Date: Tue, 11 Apr 2017 12:03:52 +0200
> > Subject: [PATCH 2/2] s3:vfs:shadow_copy2: fix corner case of "/@GMT-token" in
> >  shadow_copy2_strip_snapshot
> >
> > Signed-off-by: Michael Adam <[hidden email]>
> > ---
> >  source3/modules/vfs_shadow_copy2.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
> > index 550c953..d06e321 100644
> > --- a/source3/modules/vfs_shadow_copy2.c
> > +++ b/source3/modules/vfs_shadow_copy2.c
> > @@ -670,10 +670,11 @@ static bool shadow_copy2_strip_snapshot_internal(TALLOC_CTX *mem_ctx,
> >   * with a path prefix.
> >   */
> >   if (pstripped != NULL) {
> > - if (len_before_gmt > 0) {
> > + if (len_before_gmt > 1) {
> >   /*
> > - * There is a slash before
> > - * the @GMT-. Remove it.
> > + * There is a path (and not only a slash)
> > + * before the @GMT-. Remove the trailing
> > + * slash character.
> >   */
> >   len_before_gmt -= 1;
> >   }
> > --
> > 2.9.3
> >
>
>
>

signature.asc (169 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] two corner case fixes to shadow_copy2

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, 2017-04-11 at 11:27 -0700, Jeremy Allison via samba-technical wrote:

> On Tue, Apr 11, 2017 at 12:53:25PM +0200, Michael Adam via samba-technical wrote:
> > Hi all,
> >
> > we are hitting a regression with Samba 4.6 and the
> > combination of the shadow_copy2 and glusterfs vfs
> > modules.
> >
> > It seems to have been introduced by the patches of
> > https://bugzilla.samba.org/show_bug.cgi?id=12531
> >
> > It's my fault that I did not notice the patches
> > and the request for review at that time... :-)
> >
> > I still need to do more analysis and testing,
> > but it seems that there are two corner cases
> > that are at least hit when using the glusterfs
> > vfs module that mimicks the share root to "/":
> >
> > 1) make_relative_path does not treat the
> >    case of cwd == "/" correctly.
> >
> > 2) shadow_copy2_strip_snapshot does not treat the
> >    corner case where the name only consists of
> >    "/" followed by the @GMT-token correctly.
> >
> > These corner cases are not coverered by our
> > test cases, and as I said, I need to do more
> > testing (e.g. if this is all), but attached find
> > two patches that fix the above corner cases
> > in a rahter obvious way.
> >
> > Thanks for review...
>
> Yep, good catch. Wish you'd found these
> earlier (I did ask :-). Never mind, better
> late than never.
>
> RB+. Pushed, with an update to the comments
> inline.
>

I have filed https://bugzilla.samba.org/show_bug.cgi?id=12743 for backporting the fixes. Can we have
those cherry-picked backports attached to the bug?

> Jeremy.
>
> > From 76e15576e3144ca87bca3dee0ddba9f60ba04fed Mon Sep 17 00:00:00 2001
> > From: Michael Adam <[hidden email]>
> > Date: Tue, 11 Apr 2017 12:03:20 +0200
> > Subject: [PATCH 1/2] s3:vfs:shadow_copy2: fix the corner case if cwd=/ in
> >  make_relative_path
> >
> > Signed-off-by: Michael Adam <[hidden email]>
> > ---
> >  source3/modules/vfs_shadow_copy2.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
> > index f3ec8b6..550c953 100644
> > --- a/source3/modules/vfs_shadow_copy2.c
> > +++ b/source3/modules/vfs_shadow_copy2.c
> > @@ -444,7 +444,10 @@ static bool make_relative_path(const char *cwd, char *abs_path)
> >   if (memcmp(abs_path, cwd, cwd_len) != 0) {
> >   return false;
> >   }
> > - if (abs_path[cwd_len] != '/' && abs_path[cwd_len] != '\0') {
> > + if (cwd_len != 1 &&
> > +     abs_path[cwd_len] != '/' &&
> > +     abs_path[cwd_len] != '\0')
> > + {
> >   return false;
> >   }
> >   if (abs_path[cwd_len] == '/') {
> > -- 
> > 2.9.3
> >
> >
> > From 6b095855ecc3038a03278e81f53398bc7c35a8bf Mon Sep 17 00:00:00 2001
> > From: Michael Adam <[hidden email]>
> > Date: Tue, 11 Apr 2017 12:03:52 +0200
> > Subject: [PATCH 2/2] s3:vfs:shadow_copy2: fix corner case of "/@GMT-token" in
> >  shadow_copy2_strip_snapshot
> >
> > Signed-off-by: Michael Adam <[hidden email]>
> > ---
> >  source3/modules/vfs_shadow_copy2.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
> > index 550c953..d06e321 100644
> > --- a/source3/modules/vfs_shadow_copy2.c
> > +++ b/source3/modules/vfs_shadow_copy2.c
> > @@ -670,10 +670,11 @@ static bool shadow_copy2_strip_snapshot_internal(TALLOC_CTX *mem_ctx,
> >    * with a path prefix.
> >    */
> >   if (pstripped != NULL) {
> > - if (len_before_gmt > 0) {
> > + if (len_before_gmt > 1) {
> >   /*
> > -  * There is a slash before
> > -  * the @GMT-. Remove it.
> > +  * There is a path (and not only a slash)
> > +  * before the @GMT-. Remove the trailing
> > +  * slash character.
> >    */
> >   len_before_gmt -= 1;
> >   }
> > -- 
> > 2.9.3
> >
>
>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] two corner case fixes to shadow_copy2

Samba - samba-technical mailing list
On Thu, Apr 20, 2017 at 03:25:13PM +0530, Anoop C S via samba-technical wrote:

> On Tue, 2017-04-11 at 11:27 -0700, Jeremy Allison via samba-technical wrote:
> > On Tue, Apr 11, 2017 at 12:53:25PM +0200, Michael Adam via samba-technical wrote:
> > > Hi all,
> > >
> > > we are hitting a regression with Samba 4.6 and the
> > > combination of the shadow_copy2 and glusterfs vfs
> > > modules.
> > >
> > > It seems to have been introduced by the patches of
> > > https://bugzilla.samba.org/show_bug.cgi?id=12531
> > >
> > > It's my fault that I did not notice the patches
> > > and the request for review at that time... :-)
> > >
> > > I still need to do more analysis and testing,
> > > but it seems that there are two corner cases
> > > that are at least hit when using the glusterfs
> > > vfs module that mimicks the share root to "/":
> > >
> > > 1) make_relative_path does not treat the
> > >    case of cwd == "/" correctly.
> > >
> > > 2) shadow_copy2_strip_snapshot does not treat the
> > >    corner case where the name only consists of
> > >    "/" followed by the @GMT-token correctly.
> > >
> > > These corner cases are not coverered by our
> > > test cases, and as I said, I need to do more
> > > testing (e.g. if this is all), but attached find
> > > two patches that fix the above corner cases
> > > in a rahter obvious way.
> > >
> > > Thanks for review...
> >
> > Yep, good catch. Wish you'd found these
> > earlier (I did ask :-). Never mind, better
> > late than never.
> >
> > RB+. Pushed, with an update to the comments
> > inline.
> >
>
> I have filed https://bugzilla.samba.org/show_bug.cgi?id=12743 for backporting the fixes. Can we have
> those cherry-picked backports attached to the bug?

Yes, but that is something you can do yourself you know :-).

Loading...