tcp: connections are never abandoned when the sender retries a blocked send

[tinic]

Summary

NX_TCP_MAXIMUM_RETRIES cannot be reached on a socket whose sender retries a blocked
send. The connection is never abandoned: it retransmits until the interval grows without
bound, and the application waiting on it blocks with no error for as long as it is left
running.

Two changes in two files, 31 insertions and 5 deletions. No API or configuration change.

Symptom

With a router silently dropping oversized datagrams, and NX_TCP_MAXIMUM_RETRIES 6 /
NX_TCP_RETRY_SHIFT 1 — which should abandon the connection at 127 s — retransmissions of
the same sequence number were observed at:

+1s  +2s  +4s  +8s  +16s  +32s  +64s  +128s   ... and no reset

The doubling shows NX_TCP_RETRY_SHIFT taking effect. The +128 shows the limit did not.

Root cause

_nx_tcp_fast_periodic_processing() (nx_tcp_fast_periodic_processing.c:129) tests the
retry limit against one of two counters, selected by
nx_tcp_socket_zero_window_probe_has_data:

else if (((socket_ptr -> nx_tcp_socket_timeout_retries >= socket_ptr -> nx_tcp_socket_timeout_max_retries) &&
          (socket_ptr -> nx_tcp_socket_zero_window_probe_has_data == NX_FALSE)) ||
         ((socket_ptr -> nx_tcp_socket_zero_window_probe_failure >= socket_ptr -> nx_tcp_socket_timeout_max_retries) &&
          (socket_ptr -> nx_tcp_socket_zero_window_probe_has_data == NX_TRUE)))

Two defects mean neither branch can fire.

1. nx_tcp_socket_send_internal.c arms the probe for any send it cannot queue.

There are three reasons data does not go out — the receiver's advertised window, the
congestion window, and the transmit queue depth — and only the first is a zero window.
Both sites test only zero_window_probe_has_data == NX_FALSE before declaring a probe, so
a send blocked by congestion or by queue depth marks the socket as being in the persist
state when it is not. That moves the limit onto zero_window_probe_failure, which the
ordinary data path never advances.

2. nx_tcp_socket_retransmit.c clears zero_window_probe_failure on every probe
rather than when a probe begins, pinning it at 1. So even a genuine persist against a peer
that has stopped answering never reaches the limit either.

Why neither defect alone is sufficient

This is the part that is not visible in either hunk on its own, and it is why the bug
survives casual reading.

A single blocked send does not hide the limit: the next retransmission clears the flag and
the ordinary counter resumes. What hides it is the caller. A blocking sender that
re-enters nx_tcp_socket_send() after NX_WINDOW_OVERFLOW or NX_TX_QUEUE_DEPTH — and a
non-blocking sender returning on its own select loop — re-arms the flag faster than the
retransmission interval doubles. From the second interval onwards, the flag is set every
time the timer looks, so the limit is tested against the wrong counter every time.

That is the ordinary shape of a blocking socket API, which is why this is reachable in
normal use rather than only under a contrived sequence.

The fix

nx_tcp_socket_send_internal.c, both sites: require
nx_tcp_socket_tx_window_advertised == 0 in addition to the existing condition, so the
flag means what its name says.

nx_tcp_socket_retransmit.c: clear zero_window_probe_failure only when a new probe
starts, matching the two sites above.

nx_tcp_fast_periodic_processing.c is deliberately untouched — with the flag correct, its
condition is correct.

Why this does not drop peers that are legitimately holding a zero window

The obvious alternative fix — ignoring the probe counter — would abandon a live peer
advertising a zero window, which is exactly what the persist state exists to avoid.

That path is preserved. A peer that answers its probes still clears
zero_window_probe_failure in nx_tcp_socket_state_ack_check.c:555, so a socket in a
genuine persist against a responsive peer is never reset however long the window stays
shut. Only a peer that has stopped answering is given up on, after the configured number
of retries.

Reproduction

A regression test drives six of these translation units against a host shim and steps the
timer directly, so 600 simulated seconds run in about 0.3 s with no target hardware:

case                              behaviour
--------------------------------  --------------------------------------
caller retrying its write         +1 +3 +7 +15 +31 +63   reset at 127 s
zero window, probes unanswered    +1 +3 +7 +15 +31 +63   reset at 127 s
zero window, probes answered      +1 ... +511            no reset

Before this change the first two cases print +1 +3 +7 +15 +31 +63 +127 +255 +511 NO RESET. The third case is the guard against over-fixing described above, and is unchanged
by the patch.

I am happy to contribute the test in whatever form suits the project's test layout; it is
currently written against a downstream harness rather than this repository's.

[fdesbiens]

Thank you — this is the most intricate of your PRs and the write-up earns its length. I checked each link rather than the conclusion, because a two-defect interaction is exactly where a plausible story can be wrong.

The condition is as you quote it. nx_tcp_fast_periodic_processing.c:129-133 selects between nx_tcp_socket_timeout_retries and nx_tcp_socket_zero_window_probe_failure on nx_tcp_socket_zero_window_probe_has_data, so a wrong flag really does move the limit onto the wrong counter.

Defect 1 confirmed, with the mechanism. The eligibility test is at nx_tcp_socket_send_internal.c:465:

if ((tx_window_current != 0) && (socket_ptr -> nx_tcp_socket_transmit_sent_count < socket_ptr -> nx_tcp_socket_transmit_queue_maximum))

and tx_window_current is min(tx_window_advertised, tx_window_congestion) from :420-441. So the branch that arms the probe is reached for three distinct reasons, exactly as you say, and only one of them is a zero window. The two error returns at :1086 (NX_WINDOW_OVERFLOW) and :1098 (NX_TX_QUEUE_DEPTH) confirm the other two are ordinary, recoverable conditions the caller is expected to retry.

Defect 2 confirmed. In nx_tcp_socket_retransmit.c, zero_window_probe_failure = 0 at :115 is followed by zero_window_probe_failure++ at :130 on the same pass, so it is pinned at 1. And that increment at :130 is the only increment anywhere in the tree — I searched for every write to the field. Clears are at :115, send_internal:1027 and :1072, and state_ack_check:555. With :115 firing on every pass the second branch of the periodic test cannot fire, and with the flag wrongly set the first branch cannot either. Neither, as you say.

One nuance worth having on record, because it bounds the claim rather than weakening it: the pinning depends on nx_tcp_socket_transmit_sent_head being non-NULL. When the sent queue is empty and a probe is already armed, the else if at :120 skips the clear and the counter does accumulate. In a persist there is normally unacked data queued, so the common case is the pinned one — but the bug is not quite unconditional, and if anyone tries to reproduce it with an empty send queue they will be confused.

Your guard against over-fixing holds. This was the thing I most wanted to check, since the naive repair does drop live peers. state_ack_check.c:550-556 clears zero_window_probe_failure when the ACK covers the probe sequence, so a responsive peer never accumulates failures; and because has_data is legitimately TRUE during a genuine persist, the first branch is disabled by its own has_data == NX_FALSE requirement. So neither branch can reset a socket that is in a real persist against a peer that is answering. Your third test case is testing the right thing.

One improvement the description does not claim. has_data has a second reader: state_ack_check.c:101-108 uses it to extend the acceptable ACK range to tx_sequence + 1, on the grounds that a probe puts one byte beyond the window. In the congestion and queue-depth cases no probe is ever sent — _nx_tcp_packet_send_probe() is reached only from the zero-window branch of retransmit.c, which requires tx_window_advertised == 0 — so accepting tx_sequence + 1 there was over-permissive. Your fix tightens ACK validation as a side effect, because the flag now describes reality for both readers rather than one. Worth adding to the commit message.

And two things that could have made the fix wrong, which it does not. Not setting zero_window_probe_data and zero_window_probe_sequence in the congestion case is safe, because the only consumer is _nx_tcp_packet_send_probe() on the zero-window path, and retransmit.c sets both itself from the queue head at :110 and :114. And your change deliberately leaves those two assignments outside the new guard so they keep tracking the current head on every pass, which is right — a probe should carry the current sequence, not the one from when the persist started.

Leaving nx_tcp_fast_periodic_processing.c untouched is the correct call. With the flag honest, its condition is already right.

The rest is process. Please do port the test.

[fdesbiens]

You offer to contribute the test in whatever form suits the project, and I would like to take that up rather than merge without it. The fix is two guards whose necessity is invisible from the surrounding code. Someone will eventually read if ((tx_window_advertised == 0) && (has_data == NX_FALSE)) and decide the first clause is redundant. Only a test stops that.

Your three cases are already the right set, and the third is the one I would insist on — a fix that abandons a peer legitimately holding a zero window would be worse than the bug, and that case is what proves it does not.

On where it goes: test/regression/netxduo_test/ is the home, driven by _nx_ram_network_driver over two NX_IP instances, which is how the TCP tests in that directory work. Two things should make the port easier than it sounds:

• There is precedent for reaching into socket state directly. netx_tcp_branch_test.c:1003 sets nx_tcp_socket_zero_window_probe_failure to nx_tcp_socket_timeout_max_retries by hand to reach a branch, so a test that inspects or nudges these fields is in keeping with the suite.
• Your approach of stepping the timer rather than waiting for it is also already used — several tests in that directory manipulate timing rather than sleeping through it — so 600 simulated seconds in a fraction of a second should be achievable without your downstream shim.

If the shim approach does not translate, an acceptable narrower version would be a white-box test that drives _nx_tcp_fast_periodic_processing() directly with the socket in each of the three states and asserts whether _nx_tcp_socket_connection_reset() was reached. Less faithful than yours, but it would pin the guards.

[fdesbiens]

I have raised this on two of your other PRs, so treat it as a consistent preference rather than a new objection — and here I think it is a closer call than usual, so I am not going to press it.

The three added comments run to eight lines each and most of the text explains what used to go wrong and which counter it moved the limit onto. That belongs in the commit message, where you have already written it better than the comment does.

What earns its place in the source is the invariant: a probe belongs to a zero window and to nothing else, and the flag must not be set for congestion or queue depth. Two lines. The reason I am softer on this one than on #401 is that the invariant genuinely is non-obvious and the comment is doing real work in protecting it — I would rather have a slightly overlong comment here than none. Trim the history, keep the rule.

One small thing while you are editing them: nx_tcp_fast_periodic_processing.c is referenced in prose in both files, which is helpful, but the reference in retransmit.c:123 points at nx_tcp_socket_state_ack_check.c without a line and the one in send_internal.c:1028 names no line either. Since you have the line numbers to hand, they would save the next reader a grep.

[fdesbiens]

Applies to common/src/nx_tcp_socket_retransmit.c:133-134.

I went looking for a way your fix could make something else worse, found this, and worked out that it cannot. Recording it so the analysis is not repeated.

During a genuine persist against a responsive peer, nx_tcp_socket_timeout_retries is incremented at retransmit.c:129 on every probe and is not reset by the ACK path — state_ack_check.c:588-601 only clears it when the ACK releases queued packets, which a pure probe ACK does not do. It then feeds the shift at :133:

socket_ptr -> nx_tcp_socket_timeout = socket_ptr -> nx_tcp_socket_timeout_rate <<
    (socket_ptr -> nx_tcp_socket_timeout_retries * socket_ptr -> nx_tcp_socket_timeout_shift);

With timeout_shift of 1 that is a shift of 32 once timeout_retries reaches 32, which is undefined behaviour on a 32-bit ULONG.

It is not reachable, though. Because each interval is twice the last, getting to the 32nd probe takes on the order of 2^32 ticks — over a century at a one-second rate. And with timeout_shift of 0 the shift count is always 0, so that configuration is safe by construction.

Importantly it is also unchanged by your patch: before it, a legitimate persist survived indefinitely too, because probe_failure was pinned at 1 and has_data was TRUE. So this is strictly pre-existing and your fix neither introduces nor extends it. No action needed; I mention it only because it lives in the lines you are editing and someone will eventually ask.