On Fri, Jun 07, 2024 at 09:59:14PM +0300, Michael Tokarev via samba wrote:>07.06.2024 20:52, Jeremy Allison wrote: > >>Fair enough. Can you post a minimal smb.conf and directory >>setup that reproduces the problem and how to demo it with smbclient >>command lines please ? I know I'm asking to be spoon-fed but my time for >>Samba these days is quite limited and this would aid immensely >>in creating a properly tested fix. > >You're not asking to be spoon-feeding. Because the thing turned out to be >tricky. This is why it took so long for me to create a small reproducer, - >initially I found out that turning off wide links triggers this issue. >However, it turned out to be insufficient alone. And especially tricky to >me was another parameter which is also necessary. Actually two, but one >of them is something I haven't tried for a long time here, because it is >set by samba implicitly already.Here is the fix. Not sure if it breaks anything else (yet) :-). Can you create me a bug please and I'll add a regression test for this ? Thanks, Jeremy. diff --git a/source3/modules/vfs_widelinks.c b/source3/modules/vfs_widelinks.c index c5b5084e108..f3c9f0e359c 100644 --- a/source3/modules/vfs_widelinks.c +++ b/source3/modules/vfs_widelinks.c @@ -383,8 +383,14 @@ static int widelinks_openat(vfs_handle_struct *handle, } lstat_ret = SMB_VFS_NEXT_LSTAT(handle, full_fname); - if (lstat_ret != -1 && - VALID_STAT(full_fname->st) && + if (lstat_ret == -1) { + /* Can't be a DFS link. */ + int saved_errno = errno; + TALLOC_FREE(full_fname); + errno = saved_errno; + return -1; + } + if (VALID_STAT(full_fname->st) && S_ISLNK(full_fname->st.st_ex_mode)) { fsp->fsp_name->st = full_fname->st; }
On Mon, Jun 10, 2024 at 10:55:38AM -0700, Jeremy Allison via samba wrote:>On Fri, Jun 07, 2024 at 09:59:14PM +0300, Michael Tokarev via samba wrote: >>07.06.2024 20:52, Jeremy Allison wrote: >> >>>Fair enough. Can you post a minimal smb.conf and directory >>>setup that reproduces the problem and how to demo it with smbclient >>>command lines please ? I know I'm asking to be spoon-fed but my time for >>>Samba these days is quite limited and this would aid immensely >>>in creating a properly tested fix. >> >>You're not asking to be spoon-feeding. Because the thing turned out to be >>tricky. This is why it took so long for me to create a small reproducer, - >>initially I found out that turning off wide links triggers this issue. >>However, it turned out to be insufficient alone. And especially tricky to >>me was another parameter which is also necessary. Actually two, but one >>of them is something I haven't tried for a long time here, because it is >>set by samba implicitly already. > >Here is the fix. Not sure if it breaks anything else (yet) :-). > >Can you create me a bug please and I'll add a regression test >for this ?https://bugzilla.samba.org/show_bug.cgi?id=15662 I have the regression test now.
10.06.2024 20:55, Jeremy Allison via samba wrote:> Here is the fix. Not sure if it breaks anything else (yet) :-).Wow. With the additional info from my previous email in mind, it looks like there's something else going on here. Namely, why the issue happens only if unix extensions is *excplicitly* turned off, while doesn't happen when it is turned off implicitly due to wide links? Note that wide links is share-level parameter while unix extensions is global. Also, the fix you created does not look like well-placed. Maybe we should check for msdfs: symlinks before evaluating symlink target, ie, before vfs_widelinks is called?> Can you create me a bug please and I'll add a regression test > for this ?I went to bed early yesterday, - thought I'll do this properly today. But it looks like you were faster :)> diff --git a/source3/modules/vfs_widelinks.c b/source3/modules/vfs_widelinks.c > index c5b5084e108..f3c9f0e359c 100644 > --- a/source3/modules/vfs_widelinks.c > +++ b/source3/modules/vfs_widelinks.c > @@ -383,8 +383,14 @@ static int widelinks_openat(vfs_handle_struct *handle, > ???????? } > ???????? lstat_ret = SMB_VFS_NEXT_LSTAT(handle, > ???????????????? full_fname); > -??????? if (lstat_ret != -1 && > -??????????? VALID_STAT(full_fname->st) && > +??????? if (lstat_ret == -1) { > +??????????? /* Can't be a DFS link. */ > +??????????? int saved_errno = errno; > +??????????? TALLOC_FREE(full_fname); > +??????????? errno = saved_errno;Can TALLOC_FREE change errno?> +??????????? return -1; > +??????? } > +??????? if (VALID_STAT(full_fname->st) && > ???????????? S_ISLNK(full_fname->st.st_ex_mode)) { > ???????????? fsp->fsp_name->st = full_fname->st; > ???????? } >Thank you very much, Jeremy! /mjt -- GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24. New key: rsa4096/61AD3D98ECDF2C8E 9D8B E14E 3F2A 9DD7 9199 28F1 61AD 3D98 ECDF 2C8E Old key: rsa2048/457CE0A0804465C5 6EE1 95D1 886E 8FFB 810D 4324 457C E0A0 8044 65C5 Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt