Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fd gets closed twice when using libssh for sftp #8708

Closed
JayDommaschk opened this issue Apr 14, 2022 · 5 comments
Closed

fd gets closed twice when using libssh for sftp #8708

JayDommaschk opened this issue Apr 14, 2022 · 5 comments
Labels
SCP/SFTP

Comments

@JayDommaschk
Copy link

I did this

Hello,
I made the following simple test program to demonstrate the issue:

#include <curl/curl.h>
#include <sys/stat.h>
#include <unistd.h>

int main()
{
    FILE *file = fopen("/tmp/test.c", "rb");
    struct stat file_info;
    stat("/tmp/test.c", &file_info);

    char SERVER_URL[512];
    CURLcode res = CURLE_OK;
    CURL *curl = curl_easy_init();

    curl_version_info_data *ver = curl_version_info(CURLVERSION_NOW);
    printf("curl version: %u.%u.%u\n", (ver->version_num >> 16) & 0xff,
                                       (ver->version_num >> 8) & 0xff,
                                       ver->version_num & 0xff);

    curl_easy_setopt(curl, CURLOPT_USERNAME, "user");
    curl_easy_setopt(curl, CURLOPT_PASSWORD, "password");

    snprintf(SERVER_URL, sizeof(SERVER_URL), "sftp://%s:%s/~/%s", "192.168.2.11", "22", "curltest.txt");
    curl_easy_setopt(curl, CURLOPT_URL, SERVER_URL);

    curl_easy_setopt(curl, CURLOPT_INFILESIZE_LARGE, (curl_off_t)(file_info.st_size));
    curl_easy_setopt(curl, CURLOPT_FTP_CREATE_MISSING_DIRS, CURLFTP_CREATE_DIR_RETRY);

    curl_easy_setopt(curl, CURLOPT_READDATA, file);
    curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);

    curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);

    res = curl_easy_perform(curl);

    printf("curl result: %s (%d)\n", curl_easy_strerror(res), res);
}

Now if I compile it and run "strace -e trace=open,close" on it, I get the following:

strace -e trace=open,close ./curltest 
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(3)                                = 0
close(4)                                = 0
curl version: 7.82.0
close(4)                                = 0
close(4)                                = 0
*   Trying 192.168.2.11:22...
* Connected to 192.168.2.11 (192.168.2.11) port 22 (#0)
close(7)                                = 0
close(7)                                = 0
close(7)                                = 0
close(7)                                = 0
close(7)                                = 0
close(7)                                = 0
close(7)                                = 0
close(7)                                = 0
close(7)                                = 0
close(7)                                = 0
close(7)                                = 0
close(7)                                = 0
* User: user
close(9)                                = 0
close(9)                                = 0
close(4)                                = 0
* Closing connection 0
close(4)                                = -1 EBADF (Bad file descriptor)
curl result: SSL peer certificate or SSH remote key was not OK (60)
close(7)                                = 0
close(8)                                = 0
+++ exited with 0 +++

so at the end, some file descriptor seems to get closed twice.

That is probably due to the call to ssh_disconnect in lib/vssh/libssh.c, because in that function, the socket gets already closed, but then curl closes it again later, at least as far as I understand the source code.

I expected the following

The socket should not get closed twice. If another thread opens a new socket after libssh closed it and if that socket gets the same "number" as the one already closed, then curl would kill that new socket and thus mess up some things.

curl/libcurl version

curl 7.82.0-DEV (Linux) libcurl/7.82.0-DEV OpenSSL/1.1.1 zlib/1.2.11 libssh/0.9.6/openssl/zlib
Release-Date: [unreleased]
Protocols: dict ftp ftps http https imap imaps ldap mqtt pop3 pop3s scp sftp smtp smtps tftp
Features: alt-svc AsynchDNS HSTS HTTPS-proxy IPv6 Largefile libz NTLM SSL UnixSockets

operating system

Linux BEAST 4.15.0-147-generic #151-Ubuntu SMP Fri Jun 18 19:21:19 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
@danielgustafsson
Copy link
Member

That is probably due to the call to ssh_disconnect in lib/vssh/libssh.c, because in that function, the socket gets already closed, but then curl closes it again later, at least as far as I understand the source code.

That seems like a reasonable assumption from a brief skim of the code. Would you like to propose a PR with a fix?

@JayDommaschk
Copy link
Author

JayDommaschk commented Apr 15, 2022 •
edited

That is probably due to the call to ssh_disconnect in lib/vssh/libssh.c, because in that function, the socket gets already closed, but then curl closes it again later, at least as far as I understand the source code.

That seems like a reasonable assumption from a brief skim of the code. Would you like to propose a PR with a fix?

Well, it's not like I don't want, I just wasn't completely sure about how to handle this best. My naive idea would be to define some bool socketAlreadyClosed = false; at the beginning of the myssh_statematch_act function and set it to true when ssh_disconnect is called and at the end only call connclose if the new variable is false. But I am not entirely sure whether this is actually correct, that is, whether not calling connclose would have other effects, since this function doesn't seem to actually close the socket but set some bit. (of course, I could go through to whole code and figure everything out, but, well, I guess hearing the opinion of an expert is more efficient and less error prone)

If you think it is ok, I can just make a PR, though. To be honest, this is my first time doing a bug report, so I might not be completely familiar with the usual protocol.

@bagder
Copy link
Member

bagder commented Apr 15, 2022

My naive idea would be to define some bool socketAlreadyClosed = false;

We typically set it to CURL_SOCKET_BAD on close for similar logic elsewhere, since that's a value that cannot be used for an actual file descriptor. (-1 on *nix)

@JayDommaschk
Copy link
Author

My naive idea would be to define some bool socketAlreadyClosed = false;

We typically set it to CURL_SOCKET_BAD on close for similar logic elsewhere, since that's a value that cannot be used for an actual file descriptor. (-1 on *nix)

I see. I like that much more than my idea, since that doesn't require introducing some additional variable. More importantly, after analysing the code more, I doubt that my idea would have even worked.

Here is what happens according to my findings:
In lib/vssh/libssh.c in the function myssh_connect ssh_options_set(ssh->ssh_session, SSH_OPTIONS_FD, &sock) is called with curl_socket_t sock = conn->sock[FIRSTSOCKET]; That sets ssh->ssh_session->opts.fd in libssh in src/options.c

Later, in myssh_statemach_act, when ssh_connect(sshc->ssh_session); is called, it checks whether opts.fd is valid and if yes, it uses that for its socket->fd rather than opening a new one.

On ssh_disconnect(sshc->ssh_session); this gets then closed, i.e. conn->sock[FIRSTSOCKET] basically gets closed and later it gets closed again on normal shutdown.

I see 3 ways on how to tackle this:

  • set conn->sock[FIRSTSOCKET] = CURL_SOCKET_BAD after ssh_disconnect
  • set conn->sock[FIRSTSOCKET] = CURL_SOCKET_BAD already after ssh_options_set(ssh->ssh_session, SSH_OPTIONS_FD, &sock)
  • don't do ssh_options_set(ssh->ssh_session, SSH_OPTIONS_FD, &sock) at all and let libssh handle the socket on its own. (probably not good)

I think option two is cleaner, because then conn->sock[FIRSTSOCKET] is disabled immediately after giving up its ownership of the fd. However, I just tested it and it seems to be slower than option 1 for whatever reason. There is a noticeable delay after it displays the user name. So option one seems to be better after all.

If there are no objections, I will make a PR accordingly.

@bagder
Copy link
Member

bagder commented Apr 16, 2022

sounds reasonable to me!

@JayDommaschk JayDommaschk mentioned this issue Apr 18, 2022
Closed
bagder pushed a commit to JayDommaschk/curl that referenced this issue Apr 19, 2022
@bagder
libssh: fix double close
4a2951e 
libssh closes the socket in ssh_diconnect() so make sure that libcurl
does not also close it.

Fixes curl#8708
@bagder bagder closed this as completed in c4d032a Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
SCP/SFTP
Milestone
No milestone
Development

No branches or pull requests
3 participants
@bagder @danielgustafsson @JayDommaschk