A read of zero bytes that looked like a closed socket

This is my favourite of the FRR bugs I found, because the failure alternates. Run show bfd peers counters and it works. Run it again and it tears down the dataplane connection partway through. Run it again and it works. Every time, period two.

Anything with period two is a state machine with somewhere to hide, and in this case the hiding place was the input buffer.

Two defects, and neither is enough on its own

First: consumed space is never reclaimed on the synchronous path.

bfd_dplane_expect() calls stream_pulldown() only when reads >= 3 within a single call. A counters query consumes exactly one message per call, the reply whose id matches, and then breaks. So the pulldown never fires.

The event-driven reader, bfd_dplane_read(), does pull down, but only when a read event fires. Between two CLI invocations the dataplane sends nothing unprompted, so no read event happens and nothing reclaims the space.

After a successful sweep of N sessions, endp is parked at N times the reply size with the buffer logically empty. The bytes are consumed. The cursor does not know that.

Second: a full buffer is misdiagnosed as the peer closing.

When the buffer fills mid-message, the parse loop’s goto read_again jumps back past the buffer-full guard at the top of the function. stream_read_try() is then called with size 0. Its guard is STREAM_WRITEABLE < size, which evaluates to 0 < 0, which is false, so it proceeds and issues the equivalent of read(fd, buf, 0).

A zero-length read returns 0 by definition. And the rv == 0 path means “the socket closed”, so bfdd calls bfd_dplane_ctx_free() and tears down a perfectly healthy connection, with the tail of a valid reply still unread in the kernel buffer. That unread data is why the wire shows an RST rather than a FIN.

The comment above the entry guard shows the bogus-close hazard was already known to whoever wrote it. The mid-loop re-read path just was not covered by it.

The arithmetic is exact, which is how I knew I had it

64 sessions, 80-byte counters replies, an 8192-byte buffer:

• A successful sweep leaves 64 x 80 = 5120 bytes of dead space.
• The next sweep has 8192 - 5120 = 3072 bytes of headroom, which is 38 full replies.
• Reply 39 can only partially append. Buffer exactly full. Zero-length read. Forged EOF. Teardown.
• The reconnect calls stream_reset(), which clears the residue, so the cycle starts over. Period two.

64 + 39 = 103 replies per connection, deterministically, every time. When a prediction like that lands on the nose, you are done guessing.

Why this is worse than a crash

The failing invocation still prints a full peer list. Every session past the failure point shows stale or zero counters. The operator gets wrong data presented as good data, with no error, unless debug bfd distributed happens to be on.

Each occurrence also drops the connection and re-triggers the entire session registration burst on reconnect. Monitoring that polls this command hits it persistently.

The fix

Pull the input buffer down at function entry, so consumed space is reclaimed regardless of which caller you are. And never issue a zero-length read: reclaim first, and if the buffer is still full of an incomplete message, treat that as the protocol error it actually is, namely a single message larger than the entire buffer.

stream_pulldown(bdc->inbuf);

/*
 * Never issue a zero-length read: `read()` returns 0 and would be
 * misdiagnosed below as the peer closing the connection. If there
 * is no headroom, reclaim consumed space first; a buffer that is
 * still full after that holds a message larger than the buffer,
 * which is a protocol violation.
 */
if (STREAM_WRITEABLE(bdc->inbuf) == 0) {
        stream_pulldown(bdc->inbuf);
        if (STREAM_WRITEABLE(bdc->inbuf) == 0) {
                zlog_err("%s: input buffer full with incomplete message", __func__);
                bfd_dplane_ctx_free(bdc);
                return -1;
        }
}

The unconditional stream_pulldown() at the top is the actual fix: it runs for every caller, on every call, so no path can accumulate dead space any more. The block below it is the belt to that braces, and it turns the impossible case into a loud one instead of a zero-length read.

A review comment pushed that second half further. My first version returned -1 on the buffer-full branch without freeing the context, which leaves a wedged connection behind. It now calls bfd_dplane_ctx_free() first, matching the socket-closed and socket-error paths: a peer claiming a message larger than the whole 8KB buffer is a protocol violation the parser cannot recover from, so tearing the connection down and reconnecting is the right answer.

While there I noticed the pre-existing bad-version check a few lines up has exactly the same gap, return -1 with no free. I left it out rather than widen a bug fix into unrelated cleanup, and said so on the PR with an offer to send a follow-up. Scope discipline is easier to defend than to practise.

Validation:

• Responder harness, 64 sessions: unpatched delivers exactly 103 replies per connection before the RST, repeating every two invocations. Patched delivers 384 out of 384 across six consecutive invocations on one uninterrupted connection, zero resets.
• Against a real dataplane with 64 established sessions: unpatched alternates strictly, 39 of 64 across ten trials, each failure logging bfd_dplane_expect: socket closed. Patched completes ten consecutive sweeps with zero disconnects.
• Built together with the shutdown drain from #22692 to confirm they coexist.

The review asked for a test, and the test needed a dataplane

Donald Sharp’s response to the fix was, reasonably, “please write a topotest that shows this problem is fixed.”

That is harder than it sounds. The bug lives in bfd_dplane_expect(), which only runs when bfdd is in distributed mode connected to an external dataplane over the bffdp protocol. A standalone bfdd never reaches the code at all, so there was nothing in the FRR test tree that could trigger it. I had a Python responder that worked, but wiring a Python helper into a topotest is a one-off.

He pointed me at fpm_listener in fpm_testing_topo1, which solves the identical shape of problem for zebra’s FPM: a mock receiver on the far end of an offload socket that dumps its state on SIGUSR1 so a test can inspect it.

So the PR grew a second and third commit:

bfdd/bfd_dplane_listener, a minimal in-tree stand-in for a dataplane. It accepts the bffdp connection, tracks the sessions bfdd registers and reports them up, answers counter requests, and dumps what it received on SIGUSR1. It runs no BFD state machine and sends no BFD packets. A session is simply declared up once registered, which is enough for bfdd to treat it as established.

bfd_dplane_counters_topo1, the test itself: eight peers, twenty counter sweeps, then assertions that every request was answered, the connection is still up, and bfdd never reconnected. Against the unpatched daemon it fails with 103 of 160 counter requests were answered, which is exactly where the buffer boundary sits.

Getting that merged-shaped took a round of unglamorous work that I would not have predicted:

• The listener needs XREF_SETUP(), because anything in sbin_PROGRAMS gets an xrelfo pass. Missing it broke the build.
• RPM needs an %exclude in redhat/frr.spec.in; Debian needs an entry in debian/frr-test-tools.install. The two distros handle fpm_listener differently, so both were needed and neither was optional.
• OpenBSD builds with clang and -Werror, which flagged sigterm_handler under -Wmissing-noreturn. Fixed with FRR_NORETURN, the same way fpm_listener declares its own.

Open as #22694.

What I took from it

read(fd, buf, 0) returning 0 is correct. Treating a 0 return as EOF is correct. Both of those are true and the combination is a bug, which is the shape a lot of the interesting ones have.

The guard that would have caught it existed. It was at the top of the function, and the failing path jumped over it with a goto. A precondition you can skip is a precondition you do not have.

The larger lesson was about what a fix costs. The actual repair is a handful of lines. Making it provable meant contributing test infrastructure that did not exist, and then chasing it across three packaging systems and a compiler that nobody in the conversation was using. The reviewer was right to ask. Untestable code paths stay broken, and the dataplane paths in bfdd had been untestable since they were written.