The Samba FreeBSD port patch https://svnweb.freebsd.org/ports/head/net/samba47/files/patch-source3__modules__vfs_streams_xattr.c?revision=464431&view=markup changes vfs_streams_xattr to not read and write an additional trailing byte (cf the comment lines containing "// ? -1" in the patch), but when creating a stream the trailing byte is still stored (cf streams_xattr_open() the code after the comment "Darn, xattrs need at least 1 byte").
Due to a vicious interaction with a bug that is present in the latest macOS 10.13.4 (not sure about earlier versions) what happens is this:
- the client send a request to create a stream "file:AFP_AfpInfo"
- the server creates the xattr for the stream and writes a 0 byte
- the client sends a request to read 60 bytes at offset 0 from the stream
- the server returns a one byte sized buffer containing a 0 instead of returning nread=0 and status=NT_STATUS_END_OF_FILE
- the final nail in the coffin is that the client, when writing the AFP_AfpInfo blob whos first four byte start with a magic string "AFP" takes the 0 byte the server returned and overwrites the first byte of the magic string
The fix for this twofold: first, we must fix vfs_streams_xattr to not store an initial zero byte when creating an xattr. Second, we must prepare vfs_fruit to allow such broken AFP_AfpInfo blobs, otherwise users who adding vfs_fruit run into the issue that vfs_fruit has a builtin check for the magic string...
Have patch, need bug number...
Fwiw, this is a bug only present in the FreeBSD Samba port.
Created attachment 193656 [details]
Possible patch for 4.7, may apply to other versions
Thanks a lot, Ralph, for investigating this and for the patch.
Just to make it clear, the provided patch is still WIP...
Btw, in case this wasn't clear enough: the patch https://svnweb.freebsd.org/ports/head/net/samba47/files/patch-source3__modules__vfs_streams_xattr.c?revision=464431&view=markup basically corrups the streams as it returns the internal 0 byte to the client.
A client which stored an 2 xattr value of 0xbe 0xef, so two bytes, will read an xattr of 3 bytes 0xbe 0xef 0x00 with the patch.
Any client who stored xattrs with and without the patch will have a mixup of the old storage format and the new one. Not sure what the best way out of this is.
(In reply to slow from comment #4)
Unfortunately, it's clear, Ralph. Next question is what to do with it, you are right.
(In reply to Timur I. Bakeyev from comment #5)
Meanwhile I've slightly modify your proposed patch, see the attachment. Basically, there are two small differences:
1. There is no need to pass address of the `null = \0'` variable to the VFS call, while we set buffer size to 0. It's better/easier to pass NULL and that's also documented behavior for the API, so is more consistent.
2. I've rearranged checks for the AFP_Signature and AFP_Version, as logically version makes sense only if we recognized the format by the signature. Result at the end should be the same, but it's a bit cleaner.
Otherwise I think it's a nice catch and good fix for the bug in the current FreeBSD's version of vfs_streams_xattr.
Created attachment 194034 [details]
Modified version of the proposed fix
The modified patch is applied to samba48 port, as of 4.8.2.
Created attachment 194644 [details]
Looks like this one somehow got lost. It takes care of fixing up broken AFP_AfpInfo streams on the fly when the client reads them.
Reopening to pick up additional patch.
(In reply to slow from comment #10)
Thanks for the update, Ralph! I've added your patch to the newer ports of 4.7.8 and 4.8.3.