Bug 14877 - Deleting file with alternate data stream closes connection
Summary: Deleting file with alternate data stream closes connection
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.15.0
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-10-20 00:50 UTC by Jordan
Modified: 2021-10-20 21:12 UTC (History)
0 users

See Also:


Attachments
Samba logs (618.19 KB, text/x-log)
2021-10-20 00:50 UTC, Jordan
no flags Details
smb.conf (640 bytes, text/plain)
2021-10-20 03:43 UTC, Jordan
no flags Details
GDB Backtrace (6.08 KB, text/plain)
2021-10-20 06:21 UTC, Jordan
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jordan 2021-10-20 00:50:34 UTC
Created attachment 16857 [details]
Samba logs

I've upgraded my test suite from Samba 4.10 (CentOS 7) to the 4.15.0 and the tests are now failing to delete a file that contains an alternate data stream.

When deleting the file with an ADS Samba will close it's end of the socket causing failures in subsequent commands and the file will not be deleted.

To replicate this problem outside of my SMB client you can use the following PowerShell code to reproduce the problem.

> $file = "\\samba-server\share\file.txt"
> Set-Content -Path $file -Value data
> Set-Content -Path $file -Stream ALT -Value data
> Get-Item $file -Stream *
> Remove-Item -Path $file -Force

Trying to delete the file through Windows Explorer will fail with a message saying there's no access to the network path due to Samba is closing it's end of the socket.

I've attached the Samba debug logs (at level 10) and when parsing through it this seems suspect.

> PANIC: assert failed at ../../source3/modules/vfs_streams_xattr.c(499): smb_fname->fsp != NULL
> ===============================================================
> INTERNAL ERROR: assert failed: smb_fname->fsp != NULL in pid 1342 (4.15.0)
> If you are running a recent Samba version, and if you think this problem is not yet fixed in the latest versions, please consider reporting this bug, see https://wiki.samba.org/index.php/Bug_Reporting
> ===============================================================
> PANIC (pid 1342): assert failed: smb_fname->fsp != NULL in 4.15.0
> BACKTRACE: 28 stack frames:
>  #0 /usr/lib/libsamba-util.so.0(log_stack_trace+0x31) [0x7fe0a93068c1]
>  #1 /usr/lib/libsamba-util.so.0(smb_panic+0xa) [0x7fe0a9306b2a]
>  #2 /usr/lib/samba/vfs/streams_xattr.so(+0x5ddc) [0x7fe0a4001ddc]
>  #3 /usr/lib/samba/vfs/xattr_tdb.so(+0x29ff) [0x7fe0a3ff69ff]
>  #4 /usr/lib/samba/libsmbd-base-samba4.so(delete_all_streams+0xcf) [0x7fe0a948b42f]
>  #5 /usr/lib/samba/libsmbd-base-samba4.so(close_file+0x1a51) [0x7fe0a948d721]
>  #6 /usr/lib/samba/libsmbd-base-samba4.so(+0x15af92) [0x7fe0a94c7f92]
>  #7 /usr/lib/samba/libsmbd-base-samba4.so(smbd_smb2_request_process_close+0x222) [0x7fe0a94c8782]
>  #8 /usr/lib/samba/libsmbd-base-samba4.so(smbd_smb2_request_dispatch+0x17db) [0x7fe0a94bc5fb]
>  #9 /usr/lib/samba/libsmbd-base-samba4.so(smbd_smb2_request_dispatch_immediate+0x91) [0x7fe0a94bd811]
>  #10 /usr/lib/libtevent.so.0(tevent_common_invoke_immediate_handler+0x193) [0x7fe0a8f668b3]
>  #11 /usr/lib/libtevent.so.0(tevent_common_loop_immediate+0x1b) [0x7fe0a8f668db]
>  #12 /usr/lib/libtevent.so.0(+0xd7b0) [0x7fe0a8f6c7b0]
>  #13 /usr/lib/libtevent.so.0(+0xbaa9) [0x7fe0a8f6aaa9]
>  #14 /usr/lib/libtevent.so.0(_tevent_loop_once+0x95) [0x7fe0a8f65735]
>  #15 /usr/lib/libtevent.so.0(tevent_common_loop_wait+0x1c) [0x7fe0a8f65a3c]
>  #16 /usr/lib/libtevent.so.0(+0xba49) [0x7fe0a8f6aa49]
>  #17 /usr/lib/samba/libsmbd-base-samba4.so(smbd_process+0x89b) [0x7fe0a94a9d4b]
>  #18 /usr/sbin/smbd(+0xdde1) [0x5591aa3a8de1]
>  #19 /usr/lib/libtevent.so.0(tevent_common_invoke_fd_handler+0x93) [0x7fe0a8f66323]
>  #20 /usr/lib/libtevent.so.0(+0xd9e8) [0x7fe0a8f6c9e8]
>  #21 /usr/lib/libtevent.so.0(+0xbaa9) [0x7fe0a8f6aaa9]
>  #22 /usr/lib/libtevent.so.0(_tevent_loop_once+0x95) [0x7fe0a8f65735]
>  #23 /usr/lib/libtevent.so.0(tevent_common_loop_wait+0x1c) [0x7fe0a8f65a3c]
>  #24 /usr/lib/libtevent.so.0(+0xba49) [0x7fe0a8f6aa49]
>  #25 /usr/sbin/smbd(main+0x1b75) [0x5591aa3a2bb5]
>  #26 /usr/lib/libc.so.6(__libc_start_main+0xd5) [0x7fe0a8c63b25]
>  #27 /usr/sbin/smbd(_start+0x2e) [0x5591aa3a314e]

Happy to share whatever other information may be necessary.
Comment 1 Jeremy Allison 2021-10-20 01:46:29 UTC
Can you add your smb.conf please so we can see what VFS object you've used on the share to add ADS support.

Also, if you could install the debug package (so we get full symbols) and then add the following to the [global] section of your smb.conf.

panic action = /bin/sleep 9999999

and then reproduce the problem. The crashed smbd will be the parent of the sleep process and you can attach to it with gdb and type a "bt" command to get a full stack crash backtrace with symbols. This will greatly aid us in tracking this one down.

Thanks !
Comment 2 Jordan 2021-10-20 03:42:32 UTC
Sorry I should have provided the smb.conf that was being used. This is what I'm using for testing https://github.com/jborean93/smbprotocol/blob/master/build_helpers/samba-setup.sh#L11-L47. The only difference is the 'log level' was upped to 10 to generate the detailed logs.

> [global]
> host msdfs = yes
> workgroup = WORKGROUP
> valid users = @smbgroup
> server signing = mandatory
> ea support = yes
> store dos attributes = yes
> vfs objects = xattr_tdb streams_xattr
> log level = 0
> 
> [dfs]
> comment = Test Samba DFS Root
> path = /srv/samba/dfsroot
> browsable = yes
> guest ok = no
> read only = no
> create mask = 0755
> msdfs root = yes
> 
> [$SMB_SHARE]
> comment = Test Samba Share
> path = /srv/samba/$SMB_SHARE
> browsable = yes
> guest ok = no
> read only = no
> create mask = 0755
> 
> [${SMB_SHARE}-encrypted]
> comment = Test Encrypted Samba Share
> path = /srv/samba/${SMB_SHARE}-encrypted
> browsable = yes
> guest ok = no
> read only = no
> create mask = 0755
> smb encrypt = required

For the symbols, I assume I will need to compile Samba myself from source with the relevant configure flags? I cannot find anything particular around a symbol package so I assume that is the case.
Comment 3 Jordan 2021-10-20 03:43:44 UTC
Created attachment 16858 [details]
smb.conf
Comment 4 Jordan 2021-10-20 06:21:00 UTC
Sorry for the delay, I've attached the gdb backtrace from a compiled source of 4.15.0. Just for full disclosure here are the steps I went through to replicate this https://gist.github.com/jborean93/a9d698c5a53277acded82e5a7393e08d. I'm only using Archlinux originally because that's one of the distros that ship with 4.15.0 as a package and I didn't want to compile it from source in CI.
Comment 5 Jordan 2021-10-20 06:21:16 UTC
Created attachment 16859 [details]
GDB Backtrace
Comment 6 Jeremy Allison 2021-10-20 16:10:32 UTC
Ah, I think you have a stacking violation.

Can you change the [global] line in smb.conf from:

vfs objects = xattr_tdb streams_xattr

to:
vfs objects = streams_xattr xattr_tdb

as all our regression tests use this stacking order to test streams on top of xattrs.
Comment 7 Jordan 2021-10-20 18:52:10 UTC
That looks like it fixes the problem. Is it just standard practice to sort it in a particular order (alphabetical maybe)?
Comment 8 Jeremy Allison 2021-10-20 19:30:01 UTC
No, the stacking is performed in parse order.

So when you do:

vfs objects = xattr_tdb streams_xattr

then xattr requests call into xattr_tdb first, which then call down into streams_xattr. As you can imagine this is the wrong order. The streams requests are dependent on streams being mapped into xattrs *first*, then mapped into xattrs stored in a tdb.

If you're running on a filesystem that already has EA (xattr) support, then you don't need the xattr_tdb module at all and can just use streams_xattr and rely on the underlying filesystem to store the streams mapped into EAs (xattrs).

Sorry for the misunderstanding, but this is more of a documentation bug than a coding one.
Comment 9 Jordan 2021-10-20 20:00:51 UTC
Thanks for the information, will definitely keep it in mind for the future. Just thought I should mention it that this was working beforehand back with 4.10.x so potentially other people may come across this when upgrading. I'm unsure if there is some sort of porting guide between the versions but if it is then this would be a great thing to document in there.

As for using xattr_tdb, I originally just relied on the FS extended attributes but found that testing on some platforms didn't have support for this, or at least not by default. For the simple stuff I was testing meant it was preferable for something to just work without further configurations required but I appreciate the advice.
Comment 10 Jeremy Allison 2021-10-20 20:21:39 UTC
It's possible the older versions of streams_xattr call back into the VFS stack and don't just call below. It's also possible you've been storing the streams in filesystem EA's without realizing it :-).

The problem is our regression test suite only checks the method I explained, so the other way around is essentially untested. And as you know... "untested code is broken code" :-).
Comment 11 Jordan 2021-10-20 21:12:10 UTC
Thanks for the explanation and help, really appreciate it.