ftpd: actually close the passive listening socket #707

2026-09-19T15:34:31Z

igorsh commented

2026-09-19 15:34:31 +00:00

mcall close takes the socket number in ecx, but the operand was
commented out in the two places that close the PASV listener, so close
ran with whatever ecx happened to hold. Both call sites had just put the
listener in ecx, so the socket was closed by accident: the code worked
only as long as that accident held. The cmdPASV cleanup was commented
out completely, so a second PASV before the client connected left the
previous listener open.

Restore the operand in threadloop (ftpd.asm) and in open_datasock, and
re-enable the cmdPASV prologue, which also resets passivesocknum to -1.

Closing the data socket needed the same care: datasocketnum only holds
a live socket in MODE_PASSIVE_OK and MODE_ACTIVE, so the new
close_data_sock helper closes it in those two states only. The
.cannot_open label is also reachable before open_datasock runs (the
length checks at the top of cmdSTOR), where datasocketnum is either
stale or never initialised, and closing it unconditionally there could
have closed an unrelated socket that had reused the number. The field
is now initialised to -1 at thread start and reset to -1 after every
close.

mcall close takes the socket number in ecx, but the operand was commented out in the two places that close the PASV listener, so close ran with whatever ecx happened to hold. Both call sites had just put the listener in ecx, so the socket was closed by accident: the code worked only as long as that accident held. The cmdPASV cleanup was commented out completely, so a second PASV before the client connected left the previous listener open. Restore the operand in threadloop (ftpd.asm) and in open_datasock, and re-enable the cmdPASV prologue, which also resets passivesocknum to -1. Closing the data socket needed the same care: datasocketnum only holds a live socket in MODE_PASSIVE_OK and MODE_ACTIVE, so the new close_data_sock helper closes it in those two states only. The .cannot_open label is also reachable before open_datasock runs (the length checks at the top of cmdSTOR), where datasocketnum is either stale or never initialised, and closing it unconditionally there could have closed an unrelated socket that had reused the number. The field is now initialised to -1 at thread start and reset to -1 after every close.

CODEOWNERS rules requested review from network 2026-09-19 15:34:32 +00:00

igorsh referenced this pull request

2026-09-19 15:35:27 +00:00

ftpd: truncate the file on STOR instead of overwriting in place #706

Leency approved these changes

2026-09-19 17:32:40 +00:00

Burer added 2 commits 2026-09-19 18:25:41 +00:00

ftpd: actually close the passive listening socket fc9aabc663

mcall close takes the socket number in ecx, but the operand was
commented out in the two places that close the PASV listener, so close
ran with whatever ecx happened to hold. Both call sites had just put the
listener in ecx, so the socket was closed by accident: the code worked
only as long as that accident held. The cmdPASV cleanup was commented
out completely, so a second PASV before the client connected left the
previous listener open.

Restore the operand in threadloop (ftpd.asm) and in open_datasock, and
re-enable the cmdPASV prologue, which also resets passivesocknum to -1.

Closing the data socket needed the same care: datasocketnum only holds
a live socket in MODE_PASSIVE_OK and MODE_ACTIVE, so the new
close_data_sock helper closes it in those two states only. The
.cannot_open label is also reachable before open_datasock runs (the
length checks at the top of cmdSTOR), where datasocketnum is either
stale or never initialised, and closing it unconditionally there could
have closed an unrelated socket that had reused the number. The field
is now initialised to -1 at thread start and reset to -1 after every
close.

Measured on the VM with a single-port PASV range (start = end = 6090):
17+ consecutive transfers all completed, so the listener is released
and the port is genuinely reusable. Rows left in net action=sockets by
finished threads are not evidence of a leak - the kernel keeps the row
but frees the port, and binding a port that still has such a row
succeeds.

Build: 8969 bytes.

Assisted-by: ZCode:deepseek-flash

ftpd: close the data socket and the file on every exit path

Test PR / Build (es_ES) (pull_request) Successful in 2m57s

Details

Test PR / Build (ru_RU) (pull_request) Successful in 3m2s

Details

Test PR / Build (en_US) (pull_request) Successful in 3m2s

Details

4982652b1d

Route every close of the data socket through close_data_sock so that the
number is reset to -1 afterwards, and add close_pasv_sock for the passive
listener. PASV and PORT now drop the data connection left behind by the
previous PASV/PORT, QUIT closes both the data socket and a pending
listener, and the 550 paths of LIST/RETR close the data socket the client
already connected. A failed file.read/file.write, and a failed send/recv
mid-transfer (transfer_error), now close the file and the data socket
instead of leaking both.

Verified in QEMU with a scripted FTP client: LIST/RETR/STOR roundtrip,
PASV after PASV, PORT after PASV, RETR of a missing file, STOR with an
over-long name, 24 LIST cycles with stray data connections, QUIT.

Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>

Burer force-pushed ftpd-actually-close-the-passive-listening-socket from a46f527b75 to 4982652b1d

2026-09-19 18:25:41 +00:00

Compare

CODEOWNERS rules requested review from network 2026-09-19 18:25:41 +00:00

Burer approved these changes

2026-09-19 18:28:19 +00:00

Leency merged commit 0150178c90 into main

2026-09-19 23:45:36 +00:00

Sign in to join this conversation.

No Reviewers

Leency

Burer

KolibriOS/network

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KolibriOS/kolibrios#707

Allow edits from maintainers