apps/ftpd: bound the pasv port search #710

2026-09-20T19:02:38Z

igorsh commented

2026-09-20 19:02:38 +00:00

On a failed bind cmdPASV jumped straight back to .next_port, and
nextpasvport wraps within [start, end], so a range with no usable port
spun forever without reading the control connection again: the client
hangs.

Count the ports in the range (end - start + 1), decrement after a
failed bind and give up at zero, then close the listener, reset the
state and answer 425 Can't open data connection. The counter lives in a
new thread_data field rather than a register, because mcall bind
clobbers eax/ebx/ecx/edx/esi/edi and esi already carries the sockaddr
length for bind. Decrementing after the failed bind and not before the
next attempt is what keeps a single-port range (start = end) working:
it still gets its one attempt.

Also validate the pair read from the ini. With start > end the range
degenerates and no port is bindable, so fall back to 2000/5000.

On a failed bind cmdPASV jumped straight back to .next_port, and nextpasvport wraps within [start, end], so a range with no usable port spun forever without reading the control connection again: the client hangs. Count the ports in the range (end - start + 1), decrement after a failed bind and give up at zero, then close the listener, reset the state and answer 425 Can't open data connection. The counter lives in a new thread_data field rather than a register, because mcall bind clobbers eax/ebx/ecx/edx/esi/edi and esi already carries the sockaddr length for bind. Decrementing after the failed bind and not before the next attempt is what keeps a single-port range (start = end) working: it still gets its one attempt. Also validate the pair read from the ini. With start > end the range degenerates and no port is bindable, so fall back to 2000/5000.

CODEOWNERS rules requested review from network 2026-09-20 19:02:39 +00:00

Leency approved these changes

2026-09-20 21:02:17 +00:00

Burer requested changes

2026-09-23 08:17:42 +00:00

Dismissed

Burer left a comment

Two minor notes:

1. Comment contradicts the field

pasv_tries      dd ?    ; ports tried for the current PASV

It counts down from the range size, so it is "ports left to try", not "tried". Fix the comment or the name.

2. Adjacent inconsistency (pre-existing, not introduced here): two lines below, a failed listen jumps to socketerror and leaves the just-bound passive socket open, while the new exhausted-range path closes it. Since this block is being touched anyway, the same call close_pasv_sock fits there.

Two minor notes: **1. Comment contradicts the field** ```asm pasv_tries dd ? ; ports tried for the current PASV ``` It counts *down* from the range size, so it is "ports left to try", not "tried". Fix the comment or the name. **2. Adjacent inconsistency** (pre-existing, not introduced here): two lines below, a failed `listen` jumps to `socketerror` and leaves the just-bound passive socket open, while the new exhausted-range path closes it. Since this block is being touched anyway, the same `call close_pasv_sock` fits there.

CODEOWNERS rules requested review from network 2026-09-24 16:51:17 +00:00

Burer approved these changes

2026-09-25 09:14:13 +00:00

Burer added 3 commits 2026-09-25 09:14:27 +00:00

ftpd: bound the PASV port search instead of looping forever eb28ff4c90

On a failed bind cmdPASV jumped straight back to .next_port, and
nextpasvport wraps within [start, end], so a range with no usable port
spun forever without reading the control connection again: the client
hangs. The source itself said "TODO: break the endless loop".

Count the ports in the range (end - start + 1), decrement after a
failed bind and give up at zero, then close the listener, reset the
state and answer 425 Can't open data connection. The counter lives in a
new thread_data field rather than a register, because mcall bind
clobbers eax/ebx/ecx/edx/esi/edi and esi already carries the sockaddr
length for bind. Decrementing after the failed bind and not before the
next attempt is what keeps a single-port range (start = end) working:
it still gets its one attempt.

Also validate the pair read from the ini. With start > end the range
degenerates and no port is bindable, so fall back to 2000/5000.

Assisted-by: ZCode:deepseek-flash

Use close_pasv_sock fc18d69327

ftpd: close the listener on a failed listen, rename the PASV counter

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

Details

Test PR / Build (ru_RU) (pull_request) Successful in 2m13s

Details

Test PR / Build (en_US) (pull_request) Successful in 2m16s

Details

95cd348fff

The per-thread counter counts down from the range size, so it holds the
number of ports left to try, not the number already tried: rename
pasv_tries to pasv_ports_left, which says what it counts.

Both ways out of the port search now share one exit. A failed listen
used to jump straight to socketerror and leave the socket it had just
bound open, while the exhausted-range path already closed it; the
cleanup moves to a .fail label that both paths reach. close_data_sock
is a no-op on this path (the mode is MODE_NOTREADY there, as cmdPASV
closes both sockets on entry and MODE_PASSIVE_WAIT is set only after a
successful listen) and stays only so the single exit drops whatever
socket this thread owns.

Found during the review of #710.

Assisted-by: ZCode:deepseek-flash

Burer force-pushed ftpd-bound-the-PASV-port-search from 0620223f5a to 95cd348fff

2026-09-25 09:14:27 +00:00

Compare

Burer merged commit 8ca3758efb into main

2026-09-25 09:17:18 +00:00

Burer deleted branch ftpd-bound-the-PASV-port-search

2026-09-25 09:17:19 +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#710

Allow edits from maintainers