From an email conversation with Ralph. >>I don't think this will work as in smbd_dirptr_lanman2_entry() we're >>doing a copy of the smb_fname so the caller >>smb2_query_directory_next_entry() passes an smb_fname without >>pathref to fetch_dos_mode_send(). > >If that is the case then "smbd async dosmode = yes" is broken >isn't it ? As in fetch_dos_mode_send() we just call dos_mode_at_send() >which has: > > if (!VALID_STAT(smb_fname->st)) { > tevent_req_done(req); > return tevent_req_post(req, ev); > } > > if (smb_fname->fsp == NULL) { > /* > * The pathological case where a caller does > * dos_mode_at_send() and smb_fname points at a > * symlink in POSIX context. smb_fname->fsp is NULL. > * > * FIXME ? Should we move to returning > * FILE_ATTRIBUTE_REPARSE_POINT here ? > */ > state->dosmode = FILE_ATTRIBUTE_NORMAL; > tevent_req_done(req); > return tevent_req_post(req, ev); > } > > subreq = SMB_VFS_GET_DOS_ATTRIBUTES_SEND(state, > ev, > dir_fsp, > smb_fname); > >so isn't this always going to return FILE_ATTRIBUTE_NORMAL >for everything ? I need to check if this is broken in 4.14.x. We need a test for "smbd async dosmode = yes"
Broke here: commit 72ace149f96cf98c2ea68f93306b5b50228f6e65 Author: Jeremy Allison <jra@samba.org> Date: Thu Jun 10 10:30:17 2021 -0700 s3: smbd: Protect dos_mode_at_send() from running into a symlink. Signed-off-by: Jeremy Allison <jra@samba.org> Reviewed-by: Noel Power<npower@samba.org> diff --git a/source3/smbd/dosmode.c b/source3/smbd/dosmode.c index c4c8be0b722..84ee8994a98 100644 --- a/source3/smbd/dosmode.c +++ b/source3/smbd/dosmode.c @@ -817,6 +817,20 @@ struct tevent_req *dos_mode_at_send(TALLOC_CTX *mem_ctx, return tevent_req_post(req, ev); } + if (smb_fname->fsp == NULL) { + /* + * The pathological case where a caller does + * dos_mode_at_send() and smb_fname points at a + * symlink in POSIX context. smb_fname->fsp is NULL. + * + * FIXME ? Should we move to returning + * FILE_ATTRIBUTE_REPARSE_POINT here ? + */ + state->dosmode = FILE_ATTRIBUTE_NORMAL; + tevent_req_done(req); + return tevent_req_post(req, ev); + } + subreq = SMB_VFS_GET_DOS_ATTRIBUTES_SEND(state, ev, dir_fsp,
> > > > fwiw, there should be tests... look for "smbd:async dosmode" in selftest/ > > Probably > > make test TESTS=smb2.compound_find Hahaha. The tests don't work, because: lp_smbd_async_dosmode(SNUM(conn)); expects "smbd async dosmode = yes", not "smbd:async dosmode = yes".
Only seems to be in master, but this is probably a blocker for 4.15.rcX
Oh, the fix for this is easy, we just don't make the secondary copy inside smbd_dirptr_lanman2_entry() and just copy out the smb_fname for which we *know* we have a valid smb_fname->fsp. We still need to fix the error in the test config though.
Here's the (raw) untested patch. if (_smb_fname != NULL) { /* * smb_fname is already talloc'ed off ctx. * We just need to make sure we don't return * any stream_name, and replace base_name * with fname in case base_name got mangled. * This allows us to preserve any smb_fname->fsp * for asynchronous handle lookups. */ TALLOC_FREE(smb_fname->stream_name); TALLOC_FREE(smb_fname->base_name); smb_fname->base_name = talloc_strdup(smb_fname, fname); if (smb_fname->base_name == NULL) { TALLOC_FREE(smb_fname); TALLOC_FREE(fname); return NT_STATUS_NO_MEMORY; } *_smb_fname = smb_name; } else { TALLOC_FREE(smb_fname); }
Here's an interesting wrinkle. The current async code using getxattr() on a returned directory entry will fail, if the name was a mangled one (e.g. returning to a Windows client with an underlying file named foo:::::bar). The previously returned smb_fname has smb_fname->base_name == mangled_name. The new code also does that, but it preserves smb_fname->fsp, which means we can still correctly do a fgetxattr() on the name foo:::::bar.
Created attachment 16679 [details] git-am fix for master. Works in hand testing, but as I mentioned our tests need fixing too.
None of the smb2.compound_find tests were actually going async :-).
Created attachment 16680 [details] git-am fix for master. Now includes regression test and fixes our testing so that "smb2.compound_find" actually goes async.
Fix (including tests) passes ci: https://gitlab.com/samba-team/devel/samba/-/pipelines/337272689 MR is here: https://gitlab.com/samba-team/samba/-/merge_requests/2081 Let's get this done for 4.15 !
This bug was referenced in samba master: 6e7ffa8da34b85ac27396ee3fe29afb5db534e9e 8f8d0eaad68620561eb5bdc95fbb855b90f31fc5 d1ffcc8064294060d05d2b657385f69a75df49cf e0b327f2eb5781a119efde1a2450de4e6d2570e8 24dc3ca67a5593954fa13fad56ca3aaf8c1a15c8 2b4062b4a1fffd3329c27ea7a840b04cf2069669
Now this is in master it doesn't need back-porting. Closing this one.