sahlberg / libsmb2

SMB2/3 userspace client
Other
322 stars 135 forks source link

"Wrong signature in received PDU" with multithreading on Linux #230

Closed ahabersaat closed 1 year ago

ahabersaat commented 2 years ago

Hello, I'm having issues when connecting to a Samba server on Ubuntu, with 2 clients started in parallel in 2 separate threads. The connection fails with: Wrong signature in received PDU. It mainly fails in a unit test, using a local Samba server (127.0.0.1), but I'm worried that it could fail in a similar way on a real app with a distant server.

The code executed in the threads looks like that:

struct smb2_context* m_smb2 = smb2_init_context();
if (m_smb2)
{
    smb2_set_user(m_smb2, user.toUtf8().constData());
    smb2_set_password(m_smb2, password.toUtf8().constData());
    smb2_set_security_mode(m_smb2, SMB2_NEGOTIATE_SIGNING_ENABLED);
    struct smb2_url* url = smb2_parse_url(m_smb2, host.toUtf8().constData());
    if (url)
    {
        if (smb2_connect_share(m_smb2, url->server, url->share, url->user) >= 0)
        {
            m_connected = true;
        }
        smb2_destroy_url(url);
    }
}

Then, I'm using the synchronous API to list and read/download filesin the different threads, however it fails before reaching that point.

I have seen that similar issue were raised before, but they seemed to be fixed? However I'm still getting it occasionaly on Linux (~30% of the runs). On Windows, I have not seen it yet, even when running the test 100x with 4 threads in parallel. But maybe the threads are not started as precisely on Windows as on Linux and thus avoid concurrent access.

Are there any known concurrency issue in the library that could make this connection fail?

delins commented 2 years ago

183 fixed encryption. I wouldn't be surprised if the library has similar problems with signing. I gave it a quick look and it still uses some static variables, for instance in lib/sha384-512.c and lib/sha224-256.c. The signing code probably uses these files, so it would be a good place to start debugging, if you're willing to. Declaring the static, non-const variables as local variables inside the functions that use them should at least be a major improvement.

sahlberg commented 1 year ago

This should be fixed now. All those global variables are now removed and turned into stack variables are needed.