Using the PASV command can leave the opened socket hanging open permantently

[dani-garcia]

When a client uses the PASV command, but doesn't connect to the returned port (because of flaky internet or a faulty FTP implementation), the opened TCP listener gets stuck permanently open without a timeout. This can exhaust all the available passive ports, given enough time.

This is the current implementation, the socket will remain open unless someone tries to connect to it.

https://github.com/bolcom/libunftp/blob/ca88d0f0d215014b3ef7aff48aa31be7a8c2e91d/src/server/controlchan/commands/pasv.rs#L126-L132

I've tested the code changing it to the following and it works, but I'm not sure if it should be configurable with idle_session_timeout, a new option, or simply hardcoded:

    // Open the data connection in a new task and process it.
    // We cannot await this since we first need to let the client know where to connect :-)
    tokio::spawn(async move {
        if let Ok(Ok((socket, _socket_addr))) = tokio::time::timeout(std::time::Duration::from_secs(30), listener.accept()).await {
            datachan::spawn_processing(logger, session, socket).await
        }
    });

[robklg]

Hi, @dani-garcia sorry for the late response. Thanks for this find and the suggested fix! We will discuss this in our standup.

[hannesdejager]

@dani-garcia We can just hard-code it for now. We can add an option for it at a later stage. This setting would be different from idle_session_timeout which would normally be quite larger and pertains to the control channel instead.

Would you like to make an MR or should we take it from here?

[dani-garcia]

I can open a PR with the changes yeah, do you think the 30 seconds value is a good default for it?

I assume clients would normally connect immediately so the value could be reduced to 10 seconds or less, but maybe we prefer to be a bit more conservative at first.

[hannesdejager]

@dani-garcia Thanks!! lets make it 15.

[dani-garcia]

Done https://github.com/bolcom/libunftp/pull/436

[hannesdejager]

@dani-garcia Thank you for your contribution. I just made a release that also includes this change.