fix(socketcan): harden BCM periodic TX and tolerate missing ctrlmode - #20
Open
dborgards wants to merge 6 commits into
Open
fix(socketcan): harden BCM periodic TX and tolerate missing ctrlmode#20dborgards wants to merge 6 commits into
dborgards wants to merge 6 commits into
Conversation
…ount
BCMPeriodicTx had two related bugs:
1. RemainingCount issued write(TX_READ) and immediately read() on a
non-blocking query FD. Because the kernel posts TX_STATUS
asynchronously, the read frequently raced the reply and returned
EAGAIN, which the code surfaced as an unconditional
ThrowErrno("read(BCM TX_STATUS)"). RemainingCount now reads first,
falls back to poll(POLLIN, 100 ms) and retries up to 5 attempts on
EAGAIN, retries on EINTR, skips unexpected opcodes, and otherwise
returns a cached last-known count instead of throwing.
2. The constructor stored the caller's CanFrame verbatim
(_frame = frame), so the BCM handle kept a reference to the caller's
IMemoryOwner for the lifetime of the periodic timer. If the caller
disposed their owner after TransmitPeriodic returned, subsequent
Update()/Stop() calls dereferenced freed memory via
_frame.Data.Span (used inside ToCanFrame/ToCanFdFrame). _frame is
now an independent copy rented from the bus's IBufferAllocator; the
previous copy is disposed on Update(frame: ...), and the current
copy on Stop()/Dispose.
Adds SocketCanBcmOwnershipTests covering the TX-lease copy, the
RemainingCount no-throw contract, and the Update/Stop lifecycle. The
tests self-skip unless CANKIT_TEST_ADAPTERS=CanKit.Adapter.SocketCAN
and run against the in-memory Fake backend under -c Fake; they bind to
the dedicated vcan2 iface so BCM emissions cannot race the vcan0/vcan1
matrix tests.
If setup fails between the frame copy and the write(TX_SETUP) — e.g. an errno throw from connect() or write() — the duplicated frame and the native fds were never released, because a constructor that throws never runs the caller's Dispose. Pre-initialize the handle fields to invalid FileDescriptorHandles so they are always safe to Dispose, and wrap the ctor body in a try/finally that releases _fd/_queryFd/_cancelFd/_epfd and the copied frame when construction does not complete. default(CanFrame) has a null memory owner, so Dispose is a no-op on the pre-copy path. Also correct the RemainingCount encoding for infinite jobs. The BCM kernel encodes an infinite job as count=0, but IPeriodicTx reserves -1 for infinite (0 means "finite job finished"). Seed _lastKnownCount to -1 when RepeatCount<0 in the ctor, remap head.count==0 to -1 in the TX_STATUS read path whenever the handle is infinite, and refresh _lastKnownCount inline in Update() whenever repeatCount is provided so a later RemainingCount fallback cannot surface the stale pre-update value. Tests: cover infinite RemainingCount at ctor time, across TX_STATUS refreshes, and after Update(repeatCount: -1).
The Fake RunBcmJobAsync loop ignored TX_DELETE, so Stop() left background EmitFrame loops running and polluted parallel matrix tests that share the default Fake nominal bitrate. Track a per-socket CancellationTokenSource and cancel the in-flight job on replace (TX_SETUP is an upsert) and on TX_DELETE. TX_EXPIRED is now only enqueued when a finite job completes naturally, matching the kernel.
Rework BCMPeriodicTx to match how the kernel BCM actually behaves: * Drop the separate query socket. TX_READ/TX_STATUS operate on the same BCM socket that owns the TX operation; a second connected socket had no associated op and a real kernel answers TX_READ for an unknown can_id with EINVAL. All socket access (TX_SETUP update, TX_READ round-trip, TX_DELETE, monitor read loop) is serialized through a _socketGate lock. * TX_READ and TX_DELETE now carry CAN_FD_FRAME for FD frames, matching the op's frame format; TX_READ failure falls back to the last known count instead of throwing, and a stray TX_EXPIRED observed while reading raises Completed. * Update() always reprograms with the current frame (nframes=1) and sets CAN_FD_FRAME based on the stored frame kind rather than on whether a new frame was passed. This also fixes a copy-paste bug where the FD branch was unreachable (a duplicated Can20 check), which made Update(frame:) throw NotSupportedException for CAN FD, and sizes the setup buffer to the actual frame kind instead of always reserving a max-FD frame. * SETTIMER is set when either period or repeatCount changes. The Fake libc backend is aligned accordingly: TX_SETUP with nframes < 1 or a buffer size that does not match head + nframes * frame-size fails with EINVAL; TX_DELETE/TX_READ for an unknown (can_id, is_fd) op fail with EINVAL; TX_STATUS echoes the request flags; and each socket tracks its programmed TX ops. Tests: add an FD case proving RemainingCount and Update operate on the owning BCM op without throwing.
Align SocketCanBus netlink setup with QueryCapabilities so existing vcan-style links that leave errno 0 on can_get_ctrlmode skip config instead of throwing when UseNetLink is enabled. libsocketcan returns -1 without setting errno when the kernel link omits IFLA_CAN_CTRLMODE, which vcan interfaces do.
Real vcan interfaces can omit IFLA_CAN_CTRLMODE, in which case libsocketcan's can_get_ctrlmode returns -1 without setting errno. QueryCapabilities only tolerated EOPNOTSUPP, so opening a bus against such a link threw instead of falling back to the static feature set. QueryCapabilities now returns the static capabilities when the interface exists (if_nametoindex != 0) and the failure is either errno 0 (attribute missing) or EOPNOTSUPP; genuine native errors on existing interfaces and queries against non-existent interfaces still throw. The Fake backends model all three cases: vcan3 omits ctrlmode (errno 0), vcan4 fails with EACCES, and unknown names fail without an interface index. Adds SocketCanCapabilityTests covering the fallback and both throwing paths.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Six focused bug fixes for the SocketCAN adapter's BCM periodic transmit (
BCMPeriodicTx) and capability probing, ported from fixes we have been running in our fork. All changes are scoped toCanKit.Adapter.SocketCANplus its test cases; no public API changes.Fixes
RemainingCount— the ctor stored the caller'sCanFrameverbatim, so the BCM handle kept a reference to the caller'sIMemoryOwnerfor the timer's lifetime (use-after-free via_frame.Data.Spanonce the caller disposed their owner); the handle now rents a private copy from the busIBufferAllocator.RemainingCountno longer throws on EAGAIN: read-first,poll(100 ms)+ retry, EINTR retry, cached last-known-count fallback.Dispose, leaking the duplicated frame and native fds; handle fields are pre-initialized and the ctor body is wrapped in try/finally. Also fixes the infinite-jobRemainingCountencoding (kernelcount=0⇄IPeriodicTx-1) and a stale cached count afterUpdate(repeatCount:).Stop()left background emit loops running and polluted parallel matrix tests.CAN_FD_FRAMEon TX_READ/TX_DELETE, andUpdate()always reprograms with nframes=1. This also fixes a copy-paste bug that madeUpdate(frame:)throwNotSupportedExceptionfor CAN FD (unreachable FD branch). The Fake libc backend validates ops accordingly (EINVAL on malformed setup/unknown ops, TX_STATUS flags echo).can_get_ctrlmodereturns -1 without setting errno when a (v)can link omitsIFLA_CAN_CTRLMODE; netlink setup now skips config instead of throwing, matchingQueryCapabilities.QueryCapabilitiesfalls back to the static feature set when the interface exists and the failure is errno 0 (attribute missing) or EOPNOTSUPP; genuine native errors and non-existent interfaces still throw. The Fake backends model all three cases (vcan3: missing ctrlmode, vcan4: EACCES, unknown names).Notes for review
CanFramehas noDuplicate(IBufferAllocator)API, so the owned copy is built by a private static helper insideBCMPeriodicTxusing the existing public memory-owner factories — zero core API surface added. If you would rather have a publicCanFrame.Duplicatecore API instead, that is an easy follow-up.poll/EAGAIN retry path ofRemainingCountis only fully exercised on real Linux vcan; under-c Fakethe in-memory backend answers synchronously (noted in the test file's doc comment).Tests
SocketCanBcmOwnershipTests(TX-lease copy, RemainingCount no-throw contract incl. infinite encoding, Update/Stop lifecycle, FD case) andSocketCanCapabilityTests(ctrlmode fallback + both throwing paths).dotnet build CanKitAdapters.slnf -c Releaseand-c Fakeclean;CANKIT_TEST_ADAPTERS=CanKit.Adapter.Virtual dotnet test CanKitAdapters.slnf -c Release -f net8.0→ 79 passed;CANKIT_TEST_ADAPTERS=CanKit.Adapter.SocketCAN dotnet test CanKitAdapters.slnf -c Fake -f net8.0→ 82 passed (Fake backend is fully in-memory, so the SocketCAN matrix genuinely runs without Linux).