Document the NfQeueue verdict landmine to prevent regression

claude commented

2026-07-07 21:22:27 +01:00

(Migrated from gitlab.keyop.co.uk)

Closes #33

Background

Found while investigating #4. The vendored github.com/florianl/go-nfqueue@v1.3.2's SetVerdict hard-codes the verdict word's upper 3 bytes to zero, with no way to encode a target queue number into an NF_QUEUE-type verdict - every nfqueue.NfQeueue verdict this codebase issued actually asked the kernel to requeue to queue 0, which nothing here is ever bound to, so the packet was silently dropped rather than redelivered. This is exactly what bit getNfCallback before issue #4's fix replaced its two NfQeueue verdicts with NfAccept.

Finding

A full grep -rn NfQeueue internal/ across the codebase turned up zero remaining call sites - #4's fix already removed every use of it. There's no live bug left to fix here.

Change

Purely preventative: added a doc comment on setVerdict (the one function every verdict flows through) spelling out the defect, so a future contributor can't reintroduce a call to NfQeueue without realizing it's silently broken.

Testing

Documentation-only change - make lint/make test both clean, no behavior change.

Last of the four tickets split out from the #4 investigation (#4 v1.1.1 -> #32 v1.1.2 -> #31 v1.1.3 -> this one). After this, next up is #30 ("Possible edit race condition?", the web UI config-edit feature).

Closes #33 ## Background Found while investigating #4. The vendored `github.com/florianl/go-nfqueue@v1.3.2`'s `SetVerdict` hard-codes the verdict word's upper 3 bytes to zero, with no way to encode a target queue number into an `NF_QUEUE`-type verdict - every `nfqueue.NfQeueue` verdict this codebase issued actually asked the kernel to requeue to queue 0, which nothing here is ever bound to, so the packet was silently dropped rather than redelivered. This is exactly what bit `getNfCallback` before issue #4's fix replaced its two `NfQeueue` verdicts with `NfAccept`. ## Finding A full `grep -rn NfQeueue internal/` across the codebase turned up **zero** remaining call sites - #4's fix already removed every use of it. There's no live bug left to fix here. ## Change Purely preventative: added a doc comment on `setVerdict` (the one function every verdict flows through) spelling out the defect, so a future contributor can't reintroduce a call to `NfQeueue` without realizing it's silently broken. ## Testing Documentation-only change - `make lint`/`make test` both clean, no behavior change. ## Related Last of the four tickets split out from the #4 investigation (#4 v1.1.1 -> #32 v1.1.2 -> #31 v1.1.3 -> this one). After this, next up is #30 ("Possible edit race condition?", the web UI config-edit feature).

claude commented

2026-07-07 21:22:38 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in issue #33

danny commented

2026-07-07 21:30:41 +01:00

(Migrated from gitlab.keyop.co.uk)

assigned to @danny

danny commented

2026-07-07 21:30:46 +01:00

(Migrated from gitlab.keyop.co.uk)

assigned to @claude and unassigned @danny

danny commented

2026-07-07 21:30:48 +01:00

(Migrated from gitlab.keyop.co.uk)

requested review from @danny

danny commented

2026-07-07 21:32:01 +01:00

(Migrated from gitlab.keyop.co.uk)

Considering the typo flagged in issue #41 and the fact that same typo is included in this ticket, please recheck the codebase with both the correct (NfQueue) and incorrect (NfQeueue) spellings.

danny commented

2026-07-07 21:32:01 +01:00

(Migrated from gitlab.keyop.co.uk)

requested changes

claude commented

2026-07-08 06:02:11 +01:00

(Migrated from gitlab.keyop.co.uk)

Rechecked with both spellings - this isn't actually a typo of mine. NfQeueue is the real, exported verdict constant name in the third-party github.com/florianl/go-nfqueue@v1.3.2 library itself - a typo in that library, not something I introduced. Evidence, straight from the vendored module source on disk (~/go/pkg/mod/github.com/florianl/go-nfqueue@v1.3.2/types.go:158-165):

// Verdicts
const (
	NfDrop = iota
	NfAccept
	NfStolen
	NfQeueue
	NfRepeat
)

There is no NfQueue verdict constant exported by this library at all - code that tried to call nfqueue.NfQueue as a verdict simply wouldn't compile. Confirmed no newer version fixes this either: go list -m -versions github.com/florianl/go-nfqueue shows v1.3.2 (our pinned version) is the latest available.

What does exist, spelled correctly, is a completely different identifier in the same package: nfqueue.Config.NfQueue (a uint16 struct field - "which queue number to bind this listener to", used once in initNfqueue: NfQueue: uint16(conn.QueueNumber)). The two look almost identical at a glance (NfQueue the correctly-spelled config field for a queue number, vs NfQeueue the genuinely-misspelled-upstream verdict constant) - exactly why this is easy to mistake for a Claude-introduced error.

I grepped every .go and .md file in the repo for both spellings to be sure:

Every NfQeueue occurrence (in nfq_forwarder.go code/comments and CLAUDE.md) correctly refers to the verdict constant, matching the library's real (if misspelled) exported name.
The single NfQueue occurrence (nfq_forwarder.go:632) correctly refers to the unrelated Config.NfQueue field.

No inconsistency or genuine typo found either direction. I don't think there's a code or doc change needed here - happy to add an explicit comment calling out the "these look alike but are different identifiers" trap if that would help future readers, but wanted to confirm this finding with you first, given #41's premise turned out to be a very understandable but mistaken reading.

**Rechecked with both spellings - this isn't actually a typo of mine.** `NfQeueue` is the real, exported verdict constant name in the third-party `github.com/florianl/go-nfqueue@v1.3.2` library itself - a typo *in that library*, not something I introduced. Evidence, straight from the vendored module source on disk (`~/go/pkg/mod/github.com/florianl/go-nfqueue@v1.3.2/types.go:158-165`): ```go // Verdicts const ( NfDrop = iota NfAccept NfStolen NfQeueue NfRepeat ) ``` There is no `NfQueue` verdict constant exported by this library at all - code that tried to call `nfqueue.NfQueue` as a verdict simply wouldn't compile. Confirmed no newer version fixes this either: `go list -m -versions github.com/florianl/go-nfqueue` shows v1.3.2 (our pinned version) is the latest available. What *does* exist, spelled correctly, is a completely different identifier in the same package: `nfqueue.Config.NfQueue` (a `uint16` struct field - "which queue number to bind this listener to", used once in `initNfqueue`: `NfQueue: uint16(conn.QueueNumber)`). The two look almost identical at a glance (`NfQueue` the correctly-spelled config field for a queue *number*, vs `NfQeueue` the genuinely-misspelled-upstream verdict *constant*) - exactly why this is easy to mistake for a Claude-introduced error. I grepped every `.go` and `.md` file in the repo for both spellings to be sure: - Every `NfQeueue` occurrence (in `nfq_forwarder.go` code/comments and `CLAUDE.md`) correctly refers to the verdict constant, matching the library's real (if misspelled) exported name. - The single `NfQueue` occurrence (`nfq_forwarder.go:632`) correctly refers to the unrelated `Config.NfQueue` field. No inconsistency or genuine typo found either direction. I don't think there's a code or doc change needed here - happy to add an explicit comment calling out the "these look alike but are different identifiers" trap if that would help future readers, but wanted to confirm this finding with you first, given #41's premise turned out to be a very understandable but mistaken reading.

danny commented

2026-07-08 06:06:12 +01:00

(Migrated from gitlab.keyop.co.uk)

Ok, looks good.

danny commented

2026-07-08 06:06:12 +01:00

(Migrated from gitlab.keyop.co.uk)

approved this merge request

danny commented

2026-07-08 06:06:25 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in commit acda1b5b7b