Bug 14350 - vfs_shadow_copy2 doesn't fails case lookin in snapdirseverywhere mode
Summary: vfs_shadow_copy2 doesn't fails case lookin in snapdirseverywhere mode
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-04-20 10:33 UTC by Ralph Böhme
Modified: 2020-05-17 05:13 UTC (History)
1 user (show)

See Also:


Attachments
Patch for 4.11 and 4.12 backported from master (11.64 KB, patch)
2020-05-08 12:57 UTC, Ralph Böhme
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ralph Böhme 2020-04-20 10:33:17 UTC
If shadow_copy2 is configured with "shadow:snapdirseverywhere = yes" fetching a previous version of a file fails if a intermediate path component has the wrong case. Eg

$ tree -a dir
dir
├── file
└── .snapshots
    └── @GMT-2001.10.30-12.34.56
        └── file

smb: \> get @GMT-2001.10.30-12.34.56/dir/file file
getting file \@GMT-2001.10.30-12.34.56\dir\file of size 4 as file (0.1 KiloBytes/sec) (average 0.1 KiloBytes/sec)

smb: \> get @GMT-2001.10.30-12.34.56/DIR/file file
NT_STATUS_OBJECT_PATH_NOT_FOUND opening remote file \@GMT-2001.10.30-12.34.56\DIR\file

Note that it works if just the last component has the wrong case:

smb: \> get @GMT-2001.10.30-12.34.56/dir/FILE file
getting file \@GMT-2001.10.30-12.34.56\dir\FILE of size 4 as file (0.8 KiloBytes/sec) (average 0.1 KiloBytes/sec)
Comment 1 Jeremy Allison 2020-04-20 16:54:56 UTC
Impressive. How did you find that one ?
Comment 2 Ralph Böhme 2020-04-20 17:03:26 UTC
(In reply to Jeremy Allison from comment #1)
I didn't, a customer did. :)
Comment 3 Jeremy Allison 2020-04-20 17:07:02 UTC
Damn, I really hate that module. Why do people use it :-(.
Comment 4 Jeremy Allison 2020-04-20 21:53:39 UTC
Looking at this, when the path isn't found by the initial stat("@GMT-TIME/complete/pathname") then it goes into the walk-the path lookup functions one component at a time.

So it starts with a lookup on "@GMT-TIME" without the rest of the path as context. As snap dirs can be anywhere, we don't know that the actual @GMT-TIME directory exists somewhere below this, so we just fail with NT_STATUS_OBJECT_PATH_NOT_FOUND.

We'll have to add case-seeking behavior into vfs_shadow_copy2 in order to fix this, so the initial stat of "@GMT-TIME/complete/pathname" always works even if the path on disk is "COMPLETE/.snapshots/"@GMT-TIME/pathname".

Only way to fix this I think.
Comment 5 Jeremy Allison 2020-04-20 23:31:20 UTC
Long term I think the snapdirseverywhere functionality needs to be split out into a separate vfs_snapshotseverywhere module and this insanity removed from shadow_copy2.

We can then clean up shadow_copy2 and possibly split it up further into modules that provide the different functionalities currently munged (badly) into the one module.
Comment 6 Jeremy Allison 2020-04-21 00:05:54 UTC
The reason:

get @GMT-2001.10.30-12.34.56/dir/FILE file

works is that the .shapshots directory is just above the last component, so we run into
the optimization for creating a new file (even though we're not).




 703                 if (errno == ENOENT) {
 704                         /* Optimization when creating a new file - only
 705                            the last component doesn't exist.
 706                            NOTE : check_parent_exists() doesn't preserve errno.
 707                         */
 708                         int saved_errno = errno;
 709                         status = check_parent_exists(ctx,
 710                                                 conn,
 711                                                 posix_pathnames,
 712                                                 smb_fname,
 713                                                 &dirpath,
 714                                                 &start);
 715                         errno = saved_errno;
 716                         if (!NT_STATUS_IS_OK(status)) {
 717                                 goto fail;
 718                         }
 719                 }

Note this will set start to the character after @GMT-2001.10.30-12.34.56/dir/, meaning start=FILE. We then go directly into get_real_filename() which will do the case-insensitive lookup and find "file".

The problem occurs when the .snapshots directory is elsewhere in the path.
Comment 7 Ralph Böhme 2020-04-21 09:55:13 UTC
(In reply to Jeremy Allison from comment #4)
Yup, I came to the same conclusion. I'll add a test, stay tuned...
Comment 8 Jeremy Allison 2020-04-23 01:01:54 UTC
I *think* I have a way to make the

@@ -908,8 +908,35 @@ static char *shadow_copy2_do_convert(TALLOC_CTX *mem_ctx,
               /*
+                * name here is the passed in path from the
+                * client. It may not be case canonicalized.
+                */
+               NTSTATUS status;
+               struct smb_filename *case_canon_fname = NULL;
+               uint32_t ucf_flags = UCF_ALWAYS_ALLOW_WCARD_LCOMP |
+                               UCF_PREP_CREATEFILE;
+
+               status = filename_convert(mem_ctx,
+                               handle->conn,
+                               name,
+                               ucf_flags,
+                               NULL,
+                               NULL,
+                               &case_canon_fname);
+               if (!NT_STATUS_IS_OK(status)) {
+                       errno = map_errno_from_nt_status(status);
+                       goto fail;
+               }
+               path = talloc_asprintf(mem_ctx,
+                               "%s/%s",
+                               handle->conn->connectpath,
+                               case_canon_fname->base_name);

fix work for the @GMT....\BASE\FILE case. But it's
ugly. Let me see if I can get it working tomorrow.

Thinking some more, I really don't want to change
anything outside of vfs_shadow_copy2.c for this if
I can help it :-).
Comment 9 Ralph Böhme 2020-04-23 14:17:51 UTC
As explained offline, anything that uses filename_convert() on the @GMT stripped path can't be used, as it will fail if a parent directory containing the file has been removed (but is of course present in the snapshot), eg

a
└── b
    └── .snapshots
        └── @GMT-2001.10.30-12.34.56
            └── c
                └── file

What seems to work is pimping shadow_copy2_get_real_filename() to do the work for us. Have working WIP patch, I'll followup per mail once I have something.
Comment 10 Jeremy Allison 2020-04-23 16:28:46 UTC
(In reply to Ralph Böhme from comment #9)

Oh, the missing parent problem is subtle. I hadn't thought of that. I'd love to see the WIP patch also if you have time !
Comment 11 Ralph Böhme 2020-04-23 16:31:44 UTC
Just passed CI: https://gitlab.com/samba-team/samba/-/merge_requests/1295
Comment 12 Jeremy Allison 2020-04-23 16:41:25 UTC
(In reply to Ralph Böhme from comment #11)

Oh that's clever :-). I hadn't thought of that kind of fix. Congratulations. I'll do a full review when you're ready.
Comment 13 Ralph Böhme 2020-04-23 16:58:54 UTC
(In reply to Jeremy Allison from comment #12)
Will do. It seems in the end this version looks quite neat:

https://gitlab.com/samba-team/devel/samba/-/commit/01bd0373f3c4e89c2b86a8f7766c1c6e5c6ca5a6

I'll let you know when I'm done.
Comment 14 Ralph Böhme 2020-05-08 12:57:20 UTC
Created attachment 15963 [details]
Patch for 4.11 and 4.12 backported from master

Fwiw, this is basically the original version based on @GMT before I had to rewrite the patchset on-top of the twrp stuff. :)
Comment 15 Jeremy Allison 2020-05-08 17:57:15 UTC
Comment on attachment 15963 [details]
Patch for 4.11 and 4.12 backported from master

LGTM.
Comment 16 Jeremy Allison 2020-05-08 17:57:41 UTC
Reassigning to Karolin for inclusion in 4.12.next, 4.11.next.
Comment 17 Karolin Seeger 2020-05-13 10:32:26 UTC
(In reply to Jeremy Allison from comment #16)
Pushed to autobuild-v4-{12,11}-test.
Comment 18 Karolin Seeger 2020-05-17 05:13:39 UTC
(In reply to Karolin Seeger from comment #17)
Pushed to both branches.
Closing out bug report.

Thanks!