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.

libunftp/src/server/controlchan/commands/pasv.rs

Lines 126 to 132 in ca88d0f

	// 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((socket, _socket_addr)) = listener.accept().await {
	datachan::spawn_processing(logger, session, socket).await;
	}
	});

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 #436

[hannesdejager]

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