ftpd: accept the passive connection on the transfer command, not blindly
threadloop accepted on the PASV listener before reading the control socket. That listener is blocking, so a command that arrives before the client connects - PASV followed by anything - parks the thread inside accept and it never reads the control connection again: the client hangs. A second PASV did the same, while a client that opens the data connection first worked, which is why this went unnoticed. Accept in open_datasock instead, which runs when the transfer command arrives and the data connection is actually needed. The client's connection waits in the listen backlog, so connecting first still works, and a command sent first now gets an answer instead of hanging the server. Accept retries are bounded (10 x 200 ms) with a counter of their own, so a persistent accept failure ends in 425 rather than the single retry the code had; the MODE_PASSIVE_FAILED state, which only ever marked "assume that we will fail", goes away with it, and so does its constant. The listening socket was also created with SO_NONBLOCK in the third argument, under the comment "we don't want to block on accept". mcall socket maps its arguments to ecx/edx/esi (domain/type/protocol) and socket_open looks for SO_NONBLOCK in the type, so the flag sat in the protocol slot and did nothing; protocol is ignored anyway, the AF_INET4/SOCK_STREAM branch overwrites it with IP_PROTO_TCP. Pass 0 instead, so that the code and its comment agree that the listener blocks - making it genuinely non-blocking is a separate change, since the non-blocking accept path misbehaves on this kernel. Assisted-by: ZCode:deepseek-flash
This commit is contained in:
1 parent
8ca3758efb
commit
64c9b63c5a
2 files changed
+16
-23
No files matched your search
@@ -15,6 +15,7 @@ struct thread_data
|
||||
state dd ? ; disconnected/logging in/logged in/..
|
||||
passivesocknum dd ? ; when in passive mode, this is the listening socket
|
||||
pasv_ports_left dd ? ; ports left to try for the current PASV
|
||||
pasv_accepts_left dd ? ; accept attempts left for the passive connection
|
||||
datasocketnum dd ? ; socket used for data transfers
|
||||
permissions dd ? ; read/write/execute/....
|
||||
buffer_ptr dd ?
|
||||
@@ -155,8 +156,8 @@ close_data_sock:
|
||||
|
||||
align 4
|
||||
; Close the passive listening socket if one is still open. It is open only
|
||||
; while waiting for the client (MODE_PASSIVE_WAIT, MODE_PASSIVE_FAILED) or after
|
||||
; a failed listen, so no data connection exists and the mode drops to NOTREADY.
|
||||
; while waiting for the client (MODE_PASSIVE_WAIT) or after a failed listen,
|
||||
; so no data connection exists and the mode drops to NOTREADY.
|
||||
close_pasv_sock:
|
||||
cmp [ebp + thread_data.passivesocknum], -1
|
||||
je .done
|
||||
@@ -350,17 +351,18 @@ open_datasock:
|
||||
jne .socketerror
|
||||
|
||||
.try_now:
|
||||
mov dword [ebp + thread_data.pasv_accepts_left], 10 ; do not retry accept forever
|
||||
.try_again:
|
||||
mov ecx, [ebp + thread_data.passivesocknum]
|
||||
lea edx, [ebp + thread_data.datasock]
|
||||
mov esi, sizeof.thread_data.datasock
|
||||
mcall accept
|
||||
cmp eax, -1
|
||||
jne .pasv_ok
|
||||
mov [ebp + thread_data.mode], MODE_PASSIVE_FAILED ; assume that we will fail
|
||||
mcall 23, 200
|
||||
mcall accept
|
||||
cmp eax, -1
|
||||
je .socketerror
|
||||
dec dword [ebp + thread_data.pasv_accepts_left]
|
||||
jnz .try_again
|
||||
jmp .socketerror
|
||||
.pasv_ok:
|
||||
mov [ebp + thread_data.datasocketnum], eax
|
||||
mov [ebp + thread_data.mode], MODE_PASSIVE_OK
|
||||
|
||||
@@ -38,7 +38,6 @@ MODE_NOTREADY = 0
|
||||
MODE_ACTIVE = 1
|
||||
MODE_PASSIVE_WAIT = 2
|
||||
MODE_PASSIVE_OK = 3
|
||||
MODE_PASSIVE_FAILED = 4
|
||||
|
||||
PERMISSION_EXEC = 1b ; LIST
|
||||
PERMISSION_READ = 10b
|
||||
@@ -139,7 +138,11 @@ start:
|
||||
add esp, 8
|
||||
|
||||
; open listening socket
|
||||
mcall socket, AF_INET4, SOCK_STREAM, SO_NONBLOCK ; we don't want to block on accept
|
||||
; Blocking on purpose: the accept happens in open_datasock, on the transfer
|
||||
; command. SO_NONBLOCK used to sit here in the protocol slot and did nothing,
|
||||
; because socket_open reads it from the type, and protocol is overwritten
|
||||
; with IP_PROTO_TCP anyway.
|
||||
mcall socket, AF_INET4, SOCK_STREAM, 0
|
||||
cmp eax, -1
|
||||
je sock_err
|
||||
mov [socketnum], eax
|
||||
@@ -263,21 +266,9 @@ threadloop:
|
||||
test eax, eax
|
||||
jz threadloop
|
||||
|
||||
cmp [ebp + thread_data.mode], MODE_PASSIVE_WAIT
|
||||
jne .not_passive
|
||||
mov ecx, [ebp + thread_data.passivesocknum]
|
||||
lea edx, [ebp + thread_data.datasock]
|
||||
mov esi, sizeof.thread_data.datasock
|
||||
mcall accept
|
||||
cmp eax, -1
|
||||
je .not_passive
|
||||
mov [ebp + thread_data.datasocketnum], eax
|
||||
mov [ebp + thread_data.mode], MODE_PASSIVE_OK
|
||||
mcall close, [ebp + thread_data.passivesocknum]
|
||||
mov [ebp + thread_data.passivesocknum], -1
|
||||
|
||||
invoke con_write_asciiz, str_datasock
|
||||
.not_passive:
|
||||
; The passive data connection is accepted in open_datasock, on the transfer
|
||||
; command. Accepting it here, before the control socket is read, would park
|
||||
; the thread in a blocking accept whenever a command arrives first.
|
||||
|
||||
mov ecx, [ebp + thread_data.socketnum]
|
||||
mov edx, [ebp + thread_data.buffer_ptr]
|
||||
|
||||
Reference in new issue
Block a user