Many issues running libsmb2 in the context of a fuse filesystem

[memecode]

Many issues running libsmb2 in the context of a fuse filesystem

[sahlberg]

That is an odd error. Please make sure that the application is single-threaded. The library is meant for event-driven applications and is not threads-safe.

…

On Mon, 25 Mar 2024 at 16:06, memecode ***@***.***> wrote: Today I'm seeing smb2_decode_header (case SMB2_RECV_HEADER) fail because the last buffer in smb2->in.iov is 4 bytes long. That buffer was added by smb2_read_from_socket (SMB2_SPL_SIZE = 4). Ie the error iov->len < SMB2_HEADER_SIZE is triggered. — Reply to this email directly, view it on GitHub <#325 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADY3ECK6IOK7FR77GKAHB3YZ65EZAVCNFSM6AAAAABFEA3SEKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJXGI4TCMRXGE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[memecode]

The wrapper code is available here: https://github.com/memecode/fuse_libsmb2

I've added this to lib/CMakeLists.txt:
target_include_directories(smb2 PUBLIC ../include)

So that clients of the library can find the headers. It's the done thing with cmake targets.

[memecode]

I've captured the data for the hang using wireshark.

[memecode]

Ok I'm investigating the hang issue and found that in wait_for_reply(...) smb2_service is returning a t_socket. Which on windows is unsigned... so "-1" is actually 18446744073709551615. Ha. And the error handler doesn't work obviously. Normally on windows you'd use 'INVALID_SOCKET' instead of -1 and just compare against that.

In fixing that the double free issue starts cropping up a lot more. And the socket does now get closed. What leads up to niov == 0 in smb2_read_data before the call to 'func' is unknown at this stage. Well aside from the 'while (num_done >= tmpiov->iov_len)' loop decrementing it to zero. Why are all the vectors read already? IDK

Actually in reading the code in smb2_service_fd, I think it shouldn't return a t_socket at all, just a standard int. And obviously the same for smb2_service,

[memecode]

For the double free I captured some stacks:

    stack of alloc:
        00007FFE822E6C56: smb2.dll, smb\libmem\libmem.cpp:61
        00007FFE822E1EDC: smb2.dll, smb\sahlberg-libsmb2\lib\socket.c:455 smb2_read_data -> smb2_add_iovector(...malloc...)
        00007FFE822E2809: smb2.dll, smb\sahlberg-libsmb2\lib\socket.c:679 smb2_read_from_socket
        00007FFE822E0736: smb2.dll, smb\sahlberg-libsmb2\lib\socket.c:839 smb2_service_fd
        00007FFE822E023A: smb2.dll, smb\sahlberg-libsmb2\lib\socket.c:867 smb2_service
        00007FFE822E50E5: smb2.dll, smb\sahlberg-libsmb2\lib\sync.c:104
        00007FFE822E3F2D: smb2.dll, smb\sahlberg-libsmb2\lib\sync.c:232
        00007FF7B8ED39F8: fuse_libsmb2.exe, smb\fuse_libsmb2\main.cpp:234

    stack of first free:
        00007FFE822E6F88: smb2.dll, C:\Users\Matthew\work\smb\libmem\libmem.cpp:122
        00007FFE822C2D96: smb2.dll, C:\Users\Matthew\work\smb\sahlberg-libsmb2\lib\init.c:377 smb2_free_iovector
        00007FFE822E27BF: smb2.dll, C:\Users\Matthew\work\smb\sahlberg-libsmb2\lib\socket.c:674 smb2_read_from_socket
        00007FFE822E0736: smb2.dll, C:\Users\Matthew\work\smb\sahlberg-libsmb2\lib\socket.c:839 smb2_service_fd
        00007FFE822E023A: smb2.dll, C:\Users\Matthew\work\smb\sahlberg-libsmb2\lib\socket.c:867
        00007FFE822E50E5: smb2.dll, C:\Users\Matthew\work\smb\sahlberg-libsmb2\lib\sync.c:104
        00007FFE822E3F2D: smb2.dll, C:\Users\Matthew\work\smb\sahlberg-libsmb2\lib\sync.c:232
        00007FF7B8ED39F8: fuse_libsmb2.exe, C:\Users\Matthew\work\smb\fuse_libsmb2\main.cpp:234

    stack of 2nd free:
        smb2.dll!smb2_free_iovector(smb2_context * smb2, smb2_io_vectors * v) Line 377	C
        smb2.dll!smb2_read_from_socket(smb2_context * smb2) Line 674	C
        smb2.dll!smb2_service_fd(smb2_context * smb2, unsigned __int64 fd, int revents) Line 839	C
        smb2.dll!smb2_service(smb2_context * smb2, int revents) Line 867	C
        smb2.dll!wait_for_reply(smb2_context * smb2, sync_cb_data * cb_data) Line 104	C
        smb2.dll!smb2_stat(smb2_context * smb2, const char * path, smb2_stat_64 * st) Line 659	C
        fuse_libsmb2.exe!wrapper_getattr(const char * path, fuse_stat * stbuf, fuse_file_info * fi) Line 191	C++

[memecode]

Potentially the fix for the socket validity checking issue on windows would be something like this.

[memecode]

Today I'm seeing smb2_decode_header (case SMB2_RECV_HEADER) fail because the last buffer in smb2->in.iov is 4 bytes long. That buffer was added by smb2_read_from_socket (SMB2_SPL_SIZE = 4).

Ie the error iov->len < SMB2_HEADER_SIZE is triggered.

And even if I just add some more buffer using a new call to smb2_add_iovector the memcmp(iov->buf, smb2sign, 4) check fails as the data is 0x1, 0x0, 0x0, 0x0. So clearly the protocol parsing needs work. In fact the whole library is just not production ready. I've exhausted my time box for looking at these bugs.

The wireshark capture for this error is here.

[memecode]

Please make sure that the application is single-threaded.

Ah... that makes so much more sense now; thanks. I'm new to fuse and I assumed that all the fuse call back functions would be executed in the same thread. Clearly they are not.

In any case, I wrapped all the callbacks in std::scoped_lock<std::mutex> and things have settled down a lot. I get a lot further into the share via Explorer. I'll clean up my debugging stuff and see how performant it is.

[memecode]

So here's a curly question for you @sahlberg - if I lock the global mutex over the call to smb2_stat_async and smb2_service I should be able to have multiple outstanding stat's going at the same time right? I haven't had the time yet to try and my code is currently using all sync versions of the functions. And performance is "ok" but could it be better? Maybe...

Ok so I think I'll close this bug, it's my fault for getting the threading model wrong. But there are still some worthwhile changes for building under windows here. Is it worth create a merge request for those? Do you think they're useful to other users?