diff --git a/CHANGELOG-INTERNAL.md b/CHANGELOG-INTERNAL.md index e2a93a9..7d3df9a 100644 --- a/CHANGELOG-INTERNAL.md +++ b/CHANGELOG-INTERNAL.md @@ -42,6 +42,35 @@ a `### BREAKING` section placed FIRST in that version, and each such line is pre - Help text and the flag tables in both READMEs now state that the flag is valid on its own. - Version bump deliberately NOT taken (convention 34): the owner closes it in `VERSION.txt`. +### Fixed: a dropped packet no longer revives the NAT mapping it was dropped for (audit F1) + +`BeanCore.decide()` step 3 read and wrote the activity stamp in one `_FlowTable.touch()`, so the +write happened BEFORE the expiry verdict and applied to the drop path too. The packet rejected +with reason `nat` therefore stamped the flow as active, and the mapping was back: the direction +lost exactly one packet per `nat_timeout_s` and carried traffic in between, with nothing outbound +involved. Measured before the fix (timeout 5 s, one outbound at t=0, inbound only afterwards): +`t=10 DROP, t=11 pass, t=12 pass, t=13 pass, t=20 DROP`. The impairment exists to test whether an +application sends keep-alives, and in that shape the test could not fail. + +Split into `get()` + a `set()` placed after the verdict, so the drop path skips the write. Same +cost - `touch()` was a `get` plus this same write - measured 160 ns/op both ways at 200k +iterations, difference below the noise floor. `_FlowTable.touch()` had exactly one caller and is +removed with it. + +How long the blackhole holds is bounded by the flow table, and the comment in `core.py` now says +so with numbers instead of a general claim: the drop path returns above the `_prune()` call, so it +never rotates anything itself. Measured with the same setup - this flow alone: still blackholed at +t=200; one other flow driving `_prune`: reopened at t=30, the first rotation. That is the table's +documented safe direction (it can lose an impairment, never invent one) and is a different thing +from the resurrection above, where the dropped packet did the reopening itself. + +New test: `tests/test_core.py::test_a_dropped_packet_does_not_revive_an_expired_nat_mapping` - +asserts every inbound packet after expiry is dropped (not just the first) and that an OUTBOUND +packet is what brings the mapping back. Verified by mutation, not assumed: restoring the +write-before-verdict order turns it red with `drops=[True, False, False, False]`, the exact +symptom above. The two existing NAT tests (`test_nat_expiry`, `test_nat_outbound_refreshes`) pass +unchanged - neither of them pinned the buggy behaviour, which is why it survived. + ### Performance: PortTable.refresh collects the socket table outside its lock The capture thread takes `PortTable._lock` too - `name_of(cheap=True)` -> `info()` in diff --git a/CHANGELOG.md b/CHANGELOG.md index 46a0d59..4ca13d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,17 @@ The format follows [Keep a Changelog](https://keepachangelog.com/); versions fol ### Fixed +- **"NAT mapping expiry" now really cuts the incoming direction, instead of losing one packet + every few seconds.** This impairment is there to answer one question: does the application + notice its mapping is gone and send something to re-open it? It could not answer that. The + packet the tool rejected for "the mapping has expired" was itself counted as activity on that + connection, so the mapping came back on the spot and traffic flowed again for another whole + timeout - then one more packet was dropped, and so on. With `--nat-timeout 5` an app that never + sent a keep-alive lost about one packet every five seconds and otherwise carried on working, so + it passed a test it should have failed. Now incoming traffic stays cut until the application + actually sends something outbound, which is what re-opens the mapping on a real NAT. Nothing + changes when you leave the setting at 0 (off, the default). + - **Short-lived connections now show which program they belong to.** The Connections table works out the owning program by asking Windows which application holds each socket, and that answer used to come from a list refreshed a few times a second. A connection that opened and finished diff --git a/beantester/core.py b/beantester/core.py index 8a81614..9844040 100644 --- a/beantester/core.py +++ b/beantester/core.py @@ -105,13 +105,6 @@ def set(self, key, value): self._new[key] = value self._enforce_ceiling() - def touch(self, key, value): - """Read the previous value and write the new one in a single lookup pass.""" - previous = self.get(key) - self._new[key] = value - self._enforce_ceiling() - return previous - def _rotate(self): """Retire a generation. Truly O(1): nothing is freed on this thread.""" if self._old: @@ -479,11 +472,42 @@ def decide(self, size, is_outbound, local_port, now, rng, # NAT check is its only reader, and NAT is off by default. So the common # configuration paid for a table it never looked at: 50 000 packets left # 50 000 entries behind, for nothing. + # + # A DROPPED packet must not refresh the mapping. This used to be a + # single `touch()` (read the old stamp, write the new one), so the very + # packet being dropped for "your mapping is gone" wrote the stamp that + # brought it back: after the first drop, inbound traffic flowed again for + # a whole further timeout window with nothing outbound in between. + # Measured before the split (timeout 5 s, one outbound at t=0, inbound + # only): drop at t=10, then PASS at t=11, t=12, t=13, drop at t=20 - i.e. + # one lost packet per timeout instead of a direction that stays shut + # until the application sends something. That "does the app send its + # keep-alives" test is the entire point of this impairment, and it could + # not fail. + # + # Split into get + set so the write is skipped on the drop path. Same + # cost: `touch` was a get plus this same write (measured 160 ns/op both + # ways, difference below the noise floor at 200k iterations). + # + # Bounded honesty: the stamp lives in a _FlowTable, which retires a + # generation on age (FLOW_ROTATE_S) and on size, and a retired record + # reads back as "never seen" - so the direction CAN reopen with no + # outbound packet at all. How long the blackhole really holds turns on + # whether other flows are moving, because the drop path returns above the + # _prune() call and so never rotates anything itself. Measured (timeout + # 5 s, drops from t=10): with this flow alone the blackhole was still + # holding at t=200; with one other flow driving _prune it ended at t=30, + # the first rotation. So it lasts at least one rotation window, and holds + # indefinitely only when nothing else is on the wire. That is the table's + # documented safe direction (it can lose an impairment, never invent one) + # and it is NOT the resurrection above - there the dropped packet did the + # reopening itself, once per timeout, for as long as traffic kept coming. if self.nat_timeout_s > 0 and key is not None: - last = self._flow_last.touch(key, now) + last = self._flow_last.get(key) expired = last is not None and (now - last) > self.nat_timeout_s if expired and not is_outbound: return Decision(True, False, [], "nat") + self._flow_last.set(key, now) # Rotate the bounded tables. O(1) (see _FlowTable) and throttled, so it # is safe to call from the hot path - which is the point: the tables must diff --git a/tests/test_core.py b/tests/test_core.py index aa7451a..8466fc8 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -409,6 +409,32 @@ def test_nat_outbound_refreshes(): check("NAT: outbound refreshes the mapping (inbound passes)", inn.drop is False) +def test_a_dropped_packet_does_not_revive_an_expired_nat_mapping(): + """An expired mapping stays expired until the application sends something. + + The drop used to write the activity stamp itself (one `touch()` did the read + and the write), so the packet rejected for "the mapping is gone" reopened it: + the direction lost exactly ONE packet per timeout and then carried traffic + again, with nothing outbound in between. A keep-alive test that cannot fail is + worse than no test, because it reads as a pass. + """ + core = BeanCore() + core.set_nat(5.0) + rng = random.Random(1) + kw = dict(remote_ip="1.2.3.4", remote_port=443, is_tcp=True) + core.decide(100, True, 5000, 0.0, rng, **kw) # mapping created + # inbound only from here on - nothing may reopen the mapping + verdicts = [core.decide(100, False, 5000, t, rng, **kw).drop + for t in (10.0, 11.0, 12.0, 13.0)] + check("NAT: every inbound packet after expiry is dropped, not just the first", + all(verdicts), f"(drops={verdicts})") + # ...and an OUTBOUND packet is what brings it back + out = core.decide(100, True, 5000, 14.0, rng, **kw) + back = core.decide(100, False, 5000, 14.5, rng, **kw) + check("NAT: outbound reopens the mapping", out.drop is False) + check("NAT: inbound flows again once the app has sent", back.drop is False) + + def test_reset_buckets_clears_flow_state(): core = BeanCore() core.set_rst(100, 30.0) # long cooldown to make state persistent