Always verdict before returning from getNfCallback - don't kill the queue's read loop #71

Merged

danny merged 1 commit from issue32_dont_kill_queue_on_bad_packet into main

2026-07-07 15:47:05 +01:00

danny commented

2026-07-07 15:33:38 +01:00

(Migrated from gitlab.keyop.co.uk)

Closes #32

Background

Found while investigating #4. getNfCallback's getPacketInfo error path (a malformed/unparseable packet) returned 1 from the callback with no verdict ever set on that packet. go-nfqueue's read loop treats any non-zero callback return as fatal and never restarts that queue's read loop for the rest of the process's life - runConnection has no logic to detect or recover from this either.

This meant a single malformed/truncated packet didn't just fail to get a verdict - it silently ended all packet processing for every destination on that connection, for the rest of the process's uptime, with nothing surfacing the failure short of noticing traffic had stopped. Much larger blast radius than a single dropped packet.

Fix

getPacketInfo failure now issues an explicit nfqueue.NfDrop verdict (the packet can't be safely forwarded since it couldn't even be parsed) and returns 0, keeping the read loop alive for every other destination on the connection.

Testing

TestGetNfCallbackBadPacketDropsWithoutKillingTheQueue (renamed from ...ReturnsOneWithoutVerdict) asserts the callback now returns 0 with an NfDrop verdict set, rather than 1 with none - a direct regression test against reintroducing the read-loop-killing bug. make lint/make test both clean.

Docs

Added a CLAUDE.md Design notes bullet explaining the go-nfqueue contract (any non-zero callback return is fatal to that queue) and why every path through getNfCallback must verdict-then-return-0.

Part of the same investigation as #4 (merged, v1.1.1). Next in the agreed order: #31 (unconditional drop + a 5s blanket cooldown affecting unrelated destinations), then #33.

Closes #32 ## Background Found while investigating #4. `getNfCallback`'s `getPacketInfo` error path (a malformed/unparseable packet) returned `1` from the callback with **no verdict ever set** on that packet. `go-nfqueue`'s read loop treats any non-zero callback return as fatal and never restarts that queue's read loop for the rest of the process's life - `runConnection` has no logic to detect or recover from this either. This meant a single malformed/truncated packet didn't just fail to get a verdict - it silently ended *all* packet processing for *every destination* on that connection, for the rest of the process's uptime, with nothing surfacing the failure short of noticing traffic had stopped. Much larger blast radius than a single dropped packet. ## Fix `getPacketInfo` failure now issues an explicit `nfqueue.NfDrop` verdict (the packet can't be safely forwarded since it couldn't even be parsed) and returns `0`, keeping the read loop alive for every other destination on the connection. ## Testing `TestGetNfCallbackBadPacketDropsWithoutKillingTheQueue` (renamed from `...ReturnsOneWithoutVerdict`) asserts the callback now returns `0` with an `NfDrop` verdict set, rather than `1` with none - a direct regression test against reintroducing the read-loop-killing bug. `make lint`/`make test` both clean. ## Docs Added a `CLAUDE.md` Design notes bullet explaining the `go-nfqueue` contract (any non-zero callback return is fatal to that queue) and why every path through `getNfCallback` must verdict-then-return-0. ## Related Part of the same investigation as #4 (merged, v1.1.1). Next in the agreed order: #31 (unconditional drop + a 5s blanket cooldown affecting unrelated destinations), then #33.

danny (Migrated from gitlab.keyop.co.uk) approved these changes

2026-07-07 15:33:38 +01:00

danny commented

2026-07-07 15:33:50 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in issue #32