Conversation
WalkthroughRefactors BPF into a unified backend API; adds BSD, Linux (PF_PACKET), and libpcap backends; renames ARP/BOOTP entry points; adds bpf_setfilter/bpf_setwfilter/bpf_lock/bpf_writev; and adds configure-time libpcap detection and BUILDING.md notes. BPF abstraction and multi-platform implementations
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@configure`:
- Around line 123-124: The new configure flags --with-libpcap and
--without-libpcap are missing from the script's help/usage text; update the help
block printed by the configure script (the usage/help string or print_help
function) to include brief entries for "--with-libpcap enable building with
libpcap support" and "--without-libpcap disable libpcap support" so they
appear in ./configure --help; locate the help text area near other option
descriptions (the block that lists flags like --with-* and --without-*) and add
these two lines consistent with existing formatting.
- Around line 1537-1545: The test compilation using $XCC with _libpcap.c and
-lpcap succeeds but the fallback branch doesn't persist the linker flag to
config.mk, causing unresolved pcap_* symbols after you `#define` USE_LIBPCAP;
update the success branch (the block that runs after $XCC _libpcap.c -o libpcap
-lpcap 2>&3 and sets abort=false) to append the required linker flag into
config.mk (e.g., add the -lpcap token to whatever variable holds linker flags in
config.mk) so that builds without pkg-config still link libpcap; keep the
existing cleanup and error-exit behavior unchanged.
In `@src/bpf-bsd.c`:
- Around line 162-166: The code copies a full struct bpf_hdr into 'packet' from
'payload' before ensuring the buffer contains sizeof(packet) bytes and then
computes packet.bh_caplen+packet.bh_hdrlen which can overflow; update the logic
to first compute 'remaining = bpf->bpf_len - bpf->bpf_pos' (use size_t) and
ensure remaining >= sizeof(struct bpf_hdr) before calling memcpy into 'packet',
then verify that adding bh_hdrlen and bh_caplen cannot overflow (e.g., check
bh_hdrlen <= SIZE_MAX - bh_caplen) and that bh_hdrlen + bh_caplen <= remaining
before using them; apply the same pre-check pattern to the other memcpy site
that reads a header (the block referencing payload/memcpy around lines
~178-179).
In `@src/bpf-linux.c`:
- Around line 62-73: The bpf structure's file descriptor is left at 0 on early
failure paths, which causes eexit to mistakenly close stdin; after allocating
bpf (the calloc result used via variable bpf) set bpf->bpf_fd = -1 immediately
(right after bpf = calloc(...) and before any potential goto eexit) so any
cleanup that closes the fd (in the eexit/cleanup path) will skip closing an
invalid/standard fd; also ensure the same initialization applies before any
later failure paths around socket creation/assignment that rely on bpf->bpf_fd
(the code handling bpf->bpf_fd in the eexit block).
In `@src/bpf-pcap.c`:
- Around line 125-134: pcap_next_ex(bpf->bpf_handle, &pkt_header, &pkt_data) can
return 0 for a timeout and leaves pkt_header/pkt_data uninitialized; update the
code after calling pcap_next_ex to explicitly check for err == 0 and return 0
(no packet) before any use of pkt_header or pkt_data, keeping the existing err <
0 check unchanged and then proceed to read pkt_header->caplen and memcpy only
when err > 0.
In `@src/bpf.c`:
- Around line 365-368: The trailing unreachable "return -1;" should be removed
from the end of the function since both branches already return (calls to
bpf_setfilter and bpf_setwfilter return unconditionally); edit the block where
len is computed and the conditional on recv (the lines using len, recv,
bpf_setfilter and bpf_setwfilter) to delete the stray "return -1;" so there is
no dead code left behind.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a35dad7-9695-47fe-96f9-32a2e8115435
📒 Files selected for processing (11)
BUILDING.mdconfiguresrc/arp.csrc/bpf-bsd.csrc/bpf-linux.csrc/bpf-pcap.csrc/bpf.csrc/bpf.hsrc/dhcp.csrc/if-linux.csrc/privsep-bpf.c
💤 Files with no reviewable changes (1)
- src/if-linux.c
| --with-libpcap) LIBPCAP=yes;; | ||
| --without-libpcap) LIBPCAP=no;; |
There was a problem hiding this comment.
Add libpcap options to ./configure --help.
Line 123 and Line 124 add new flags, but the help text block does not list them. This makes feature discovery harder for users relying on --help.
Suggested help text addition
Optional Features:
--disable-FEATURE do not include FEATURE (same as --enable-FEATURE=no)
--enable-FEATURE[=ARG] include FEATURE [ARG=yes]
+ --with-libpcap use libpcap backend for BPF support
+ --without-libpcap disable libpcap backend🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@configure` around lines 123 - 124, The new configure flags --with-libpcap and
--without-libpcap are missing from the script's help/usage text; update the help
block printed by the configure script (the usage/help string or print_help
function) to include brief entries for "--with-libpcap enable building with
libpcap support" and "--without-libpcap disable libpcap support" so they
appear in ./configure --help; locate the help text area near other option
descriptions (the block that lists flags like --with-* and --without-*) and add
these two lines consistent with existing formatting.
This makes it easier to support OS's where we don't have kernel support for any BPF ourselves, such as GNU/Hurd.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/bpf.c (1)
501-511: 💤 Low valueIndentation inconsistency on line 505.
Line 505 (
return -1;) appears to be indented with spaces while the surrounding code (e.g., line 507) uses a tab. Cosmetic, but it sticks out in a file that otherwise uses tabs consistently.✏️ Proposed fix
if (bpf_bootp_rw(bpf, true) == -1) - return -1; + return -1; if (bpf_bootp_rw(bpf, false) == -1 && errno != ENOSYS) return -1;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bpf.c` around lines 501 - 511, In bpf_filter_bootp, the indentation of the early return statement in the bpf_bootp_rw(true) error branch uses spaces instead of the file's tab-based indentation; update the indentation of the `return -1;` inside the bpf_filter_bootp function so it uses a single leading tab (matching surrounding lines and the style used in bpf_bootp_rw and bpf_lock branches) to keep formatting consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/bpf.c`:
- Around line 501-511: In bpf_filter_bootp, the indentation of the early return
statement in the bpf_bootp_rw(true) error branch uses spaces instead of the
file's tab-based indentation; update the indentation of the `return -1;` inside
the bpf_filter_bootp function so it uses a single leading tab (matching
surrounding lines and the style used in bpf_bootp_rw and bpf_lock branches) to
keep formatting consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0041c6fe-df52-47ef-86e5-8f753138e5e9
📒 Files selected for processing (7)
BUILDING.mdconfiguresrc/bpf-bsd.csrc/bpf-linux.csrc/bpf-pcap.csrc/bpf.csrc/bpf.h
✅ Files skipped from review due to trivial changes (1)
- BUILDING.md
🚧 Files skipped from review as they are similar to previous changes (5)
- src/bpf.h
- configure
- src/bpf-linux.c
- src/bpf-pcap.c
- src/bpf-bsd.c
rsmarples
force-pushed
the
bpf
branch
2 times, most recently
from
8359aeb to
b7e44d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/bpf-bsd.c (2)
162-196: 💤 Low valueRedundant
bytes != -1check at end of loop.
bytesis set to-1at line 162 only as a temporary marker; on every error path it jumps toerr:viagoto, and on the success path lines 181/183 unconditionally assign a non-negative value tobytes. The check at line 195 is therefore always true and the loop never actually iterates a second time through the parsing block. This obscures intent. Consider returning directly after the parse succeeds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bpf-bsd.c` around lines 162 - 196, The final conditional "if (bytes != -1) return bytes;" is redundant because bytes is set to a non-negative value on every successful parse path; remove that check and return bytes directly after memcpy/data update and after advancing bpf->bpf_pos (i.e., replace the conditional with an unconditional return of bytes), leaving the existing error paths that goto err: intact; locate references to the local variable bytes, the payload/copy logic, and the err: label in the bpf packet parsing block to make the change.
127-131: 💤 Low valueInconsistent cleanup in
eexit:bpf->bpf_bufferis not freed.If a future failure path is added after line 121 (after
bpf_bufferis successfully allocated), this would leak the buffer. The siblingsrc/bpf-linux.c(lines 110-114) defensively frees bothbpf_bufferandbpf. For consistency and resilience to future edits, mirror that pattern here.♻️ Proposed fix
eexit: if (bpf->bpf_fd != -1) close(bpf->bpf_fd); + free(bpf->bpf_buffer); free(bpf); return NULL;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bpf-bsd.c` around lines 127 - 131, The eexit cleanup currently closes bpf->bpf_fd and frees bpf but forgets to free bpf->bpf_buffer; update the eexit block to check and free bpf->bpf_buffer (e.g., if (bpf->bpf_buffer) free(bpf->bpf_buffer);) before freeing bpf so the function mirrors the defensive cleanup used in src/bpf-linux.c and avoids leaking the buffer if allocation succeeded earlier.src/bpf-linux.c (1)
142-167: 💤 Low valueTruncation of oversized frames is not signaled to the caller.
recvmsgis called withoutMSG_TRUNC, andmsg.msg_flagsis not inspected. If a frame larger thanETH_FRAME_LEN(e.g., VLAN-tagged or jumbo) arrives, the kernel silently truncates the returned length and the caller has no way to detect it. Since DHCP frames typically fit, this is unlikely to be hit in practice, but consider checkingmsg.msg_flags & MSG_TRUNC(or sizing the buffer to also cover the AUXDATA scenario) so that a truncated frame is treated as an error rather than parsed as if complete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bpf-linux.c` around lines 142 - 167, Call recvmsg with MSG_TRUNC (e.g., recvmsg(bpf->bpf_fd, &msg, MSG_TRUNC)) and after the call inspect msg.msg_flags for MSG_TRUNC; if set treat the read as an error (return -1 or propagate an error) instead of silently truncating the frame. Update the recv path that touches bpf->bpf_fd, msg, msg.msg_flags, bpf->bpf_buffer and the existing truncation check (the (size_t)bytes > len branch) so oversized frames (larger than ETH_FRAME_LEN or jumbo/VLAN frames) are detected via msg.msg_flags & MSG_TRUNC and not incorrectly processed.src/bpf-pcap.c (1)
141-160: 💤 Low value
bpf_writevreturnspcap_inject'sintand may silently drop the error value range.
pcap_injectreturnsint(number of bytes injected, orPCAP_ERROR/-1on error). The implicit conversion tossize_tis fine for current frame sizes, but ifpcap_injectever returns a value below-1(e.g., extendedPCAP_ERROR_*codes), callers comparing only== -1may misinterpret the result. Consider normalizing to-1on any negative return and propagatingerrno(libpcap doesn't seterrno; you may want to callpcap_geterrand log it).What error values can pcap_inject() return according to the libpcap documentation?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bpf-pcap.c` around lines 141 - 160, bpf_writev currently returns pcap_inject()'s raw int which can be <-1 and confuse callers; change bpf_writev to treat any negative pcap_inject return as a generic error: call pcap_geterr(bpf->bpf_handle) to obtain the libpcap error string (and log it or include it in diagnostics), set errno to a suitable POSIX error (e.g., EIO) and return -1, otherwise return the positive byte count as ssize_t; update references to bpf_writev and document that callers should still check for -1.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bpf-bsd.c`:
- Line 43: The exported string constant bpf_name contains a misspelling
("Berkley Packet Filter"); update the value to the correct spelling "Berkeley
Packet Filter" and make the identical fix for the same constant in the other
translation unit that defines it (the occurrence in bpf-pcap.c), ensuring both
definitions use the canonical "Berkeley" spelling to keep logs/user-facing text
consistent.
In `@src/bpf-pcap.c`:
- Line 50: The user-visible typo is in the const char *bpf_name definition
("Berkley Packet Filter (libpcap)") — update the string to "Berkeley Packet
Filter (libpcap)". Also check and fix the identical typo in the corresponding
bpf_name in src/bpf-bsd.c (the same variable/string) so both user-facing strings
read "Berkeley".
- Around line 115-139: The libpcap backend in bpf_read does not update the
BPF_BCAST flag, causing stale flag state; after a successful pcap_next_ex read
(in function bpf_read) compute cap_len as you already do, then call the existing
helper bpf_frame_bcast(pkt_data, cap_len) and, if it returns true, set
bpf->bpf_flags |= BPF_BCAST (clearing it otherwise is optional if other code
expects per-packet setting); ensure you reference pkt_data and cap_len (same
locals) and update bpf->bpf_flags before returning the captured length so
ARP/other callers see the correct BPF_BCAST state.
---
Nitpick comments:
In `@src/bpf-bsd.c`:
- Around line 162-196: The final conditional "if (bytes != -1) return bytes;" is
redundant because bytes is set to a non-negative value on every successful parse
path; remove that check and return bytes directly after memcpy/data update and
after advancing bpf->bpf_pos (i.e., replace the conditional with an
unconditional return of bytes), leaving the existing error paths that goto err:
intact; locate references to the local variable bytes, the payload/copy logic,
and the err: label in the bpf packet parsing block to make the change.
- Around line 127-131: The eexit cleanup currently closes bpf->bpf_fd and frees
bpf but forgets to free bpf->bpf_buffer; update the eexit block to check and
free bpf->bpf_buffer (e.g., if (bpf->bpf_buffer) free(bpf->bpf_buffer);) before
freeing bpf so the function mirrors the defensive cleanup used in
src/bpf-linux.c and avoids leaking the buffer if allocation succeeded earlier.
In `@src/bpf-linux.c`:
- Around line 142-167: Call recvmsg with MSG_TRUNC (e.g., recvmsg(bpf->bpf_fd,
&msg, MSG_TRUNC)) and after the call inspect msg.msg_flags for MSG_TRUNC; if set
treat the read as an error (return -1 or propagate an error) instead of silently
truncating the frame. Update the recv path that touches bpf->bpf_fd, msg,
msg.msg_flags, bpf->bpf_buffer and the existing truncation check (the
(size_t)bytes > len branch) so oversized frames (larger than ETH_FRAME_LEN or
jumbo/VLAN frames) are detected via msg.msg_flags & MSG_TRUNC and not
incorrectly processed.
In `@src/bpf-pcap.c`:
- Around line 141-160: bpf_writev currently returns pcap_inject()'s raw int
which can be <-1 and confuse callers; change bpf_writev to treat any negative
pcap_inject return as a generic error: call pcap_geterr(bpf->bpf_handle) to
obtain the libpcap error string (and log it or include it in diagnostics), set
errno to a suitable POSIX error (e.g., EIO) and return -1, otherwise return the
positive byte count as ssize_t; update references to bpf_writev and document
that callers should still check for -1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95d7ecba-a659-4638-bed4-3f52c7e1b072
📒 Files selected for processing (4)
src/bpf-bsd.csrc/bpf-linux.csrc/bpf-pcap.csrc/bpf.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bpf.c
|
@coderabbitai full revie |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bpf-bsd.c`:
- Around line 66-70: After allocating the bpf struct with calloc, initialize
bpf->bpf_fd = -1 immediately (before any early returns/goto eexit) to avoid
accidentally closing stdin on cleanup; locate the allocation block where bpf is
set (the lines setting bpf = calloc(...), bpf->bpf_ifp, bpf->bpf_flags =
BPF_EOF) and add bpf->bpf_fd = -1 right after the NULL check so that cleanup
code that calls close(bpf->bpf_fd) is safe if no valid fd was opened.
- Around line 145-152: The code in src/bpf-bsd.c uses `continue` inside an `#if
defined(__sun)` block without a loop; replace this with a retry mechanism around
the `read()` that sets `bytes` (e.g., a small `for`/`while` retry loop or a
`goto retry_read` label) so that when `(bytes == -1 && errno == EINVAL)` you
call `lseek(bpf->bpf_fd, 0, SEEK_SET)` and then retry the `read()` instead of
using `continue`; update the logic around the `read()` call and the
`bytes`/`errno` checks (referencing `bytes`, `errno`, `EINVAL`, `lseek`, and
`bpf->bpf_fd`) to ensure a single clear retry path and no dangling `continue`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c0c17d0-e9c7-4b9c-bfc9-444facd00da6
📒 Files selected for processing (4)
src/bpf-bsd.csrc/bpf-linux.csrc/bpf-pcap.csrc/bpf.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bpf.c
| bpf = calloc(1, sizeof(*bpf)); | ||
| if (bpf == NULL) | ||
| return NULL; | ||
| bpf->bpf_ifp = ifp; | ||
| bpf->bpf_flags = BPF_EOF; |
There was a problem hiding this comment.
Initialize bpf_fd to -1 immediately after allocation.
After calloc, bpf->bpf_fd is zero. If any failure occurs before Line 73 assigns a valid fd (e.g., an early goto eexit), the cleanup at Line 253-254 will call close(0), inadvertently closing stdin. The Linux backend correctly initializes bpf_fd = -1 at Line 67 of bpf-linux.c.
Proposed fix
bpf = calloc(1, sizeof(*bpf));
if (bpf == NULL)
return NULL;
+ bpf->bpf_fd = -1;
bpf->bpf_ifp = ifp;
bpf->bpf_flags = BPF_EOF;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bpf = calloc(1, sizeof(*bpf)); | |
| if (bpf == NULL) | |
| return NULL; | |
| bpf->bpf_ifp = ifp; | |
| bpf->bpf_flags = BPF_EOF; | |
| bpf = calloc(1, sizeof(*bpf)); | |
| if (bpf == NULL) | |
| return NULL; | |
| bpf->bpf_fd = -1; | |
| bpf->bpf_ifp = ifp; | |
| bpf->bpf_flags = BPF_EOF; |
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 68-68: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bpf-bsd.c` around lines 66 - 70, After allocating the bpf struct with
calloc, initialize bpf->bpf_fd = -1 immediately (before any early returns/goto
eexit) to avoid accidentally closing stdin on cleanup; locate the allocation
block where bpf is set (the lines setting bpf = calloc(...), bpf->bpf_ifp,
bpf->bpf_flags = BPF_EOF) and add bpf->bpf_fd = -1 right after the NULL check so
that cleanup code that calls close(bpf->bpf_fd) is safe if no valid fd was
opened.
There was a problem hiding this comment.
This is not an issue because bpf->bpd_fd will either be a valid or -1 before any error path is taken because the first action is to open the bpf device.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat src/bpf-bsd.c
Length of output: 6440
@rsmarples, you're right — I apologize for the noise. Looking at the full function, the open() calls are the very first operations performed after calloc, and the first goto eexit is only reached after if (bpf->bpf_fd == -1) — so bpf_fd is always either a valid descriptor or -1 before any cleanup path. The guard in bpf_close (if (bpf->bpf_fd != -1)) is therefore always operating on a correct value. The concern was invalid.
✏️ Learnings added
Learnt from: rsmarples
Repo: NetworkConfiguration/dhcpcd PR: 607
File: src/bpf-bsd.c:0-0
Timestamp: 2026-05-17T17:27:48.361Z
Learning: In `src/bpf-bsd.c` (dhcpcd project), `bpf->bpf_fd` does NOT need to be initialized to `-1` after `calloc` in `bpf_open`. The first statements after `calloc` immediately call `open()` which sets `bpf_fd` to either a valid fd or -1, so all `goto eexit` error paths already have a correct fd value. `bpf_close` also guards with `if (bpf->bpf_fd != -1)` before calling `close()`. Do not flag this as a missing initialization.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bpf-linux.c`:
- Around line 69-73: The PF_PACKET receive buffer is currently fixed to
ETH_FRAME_LEN which drops frames on interfaces with MTU > 1500; change the
allocation to size bpf->bpf_size = bpf_frame_header_len(ifp) + ifp->mtu (or at
least that value) before calling malloc for bpf->bpf_buffer (use existing
symbols bpf->bpf_size, bpf->bpf_buffer and the interface struct ifp), and keep
the NULL check/goto eexit as-is; optionally clamp to a safe minimum like
ETH_FRAME_LEN if mtu isn't available.
In `@src/bpf-pcap.c`:
- Around line 153-160: The loop that accumulates len before copying (variables
iov, iovcnt, len, bpf->bpf_size, bp, memcpy) can overflow when doing len +=
iov[i].iov_len and bypass the size check; fix it by computing a safe new_len
(e.g. new_len = len + iov[i].iov_len) and validate that iov[i].iov_len is not
larger than bpf->bpf_size and that new_len <= bpf->bpf_size (and detect overflow
by checking new_len < len) before doing the memcpy, returning -1 and setting
errno=ENOBUFS if the checks fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cceca5c6-8ef6-4a7b-82a5-4a751366074c
📒 Files selected for processing (4)
src/bpf-bsd.csrc/bpf-linux.csrc/bpf-pcap.csrc/bpf.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bpf.c
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/bpf-bsd.c (1)
66-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
bpf_fdnot initialized to-1before first failure path.A past review flagged this as addressed, but the initialization is still missing. After
calloc,bpf->bpf_fdis zero. While the current code flow assignsbpf_fdat Line 71 before anygoto eexit, defensive initialization prevents future regressions if code is inserted between allocation and the first open.Proposed fix
bpf = calloc(1, sizeof(*bpf)); if (bpf == NULL) return NULL; + bpf->bpf_fd = -1; /* /dev/bpf is a cloner on modern kernels */ bpf->bpf_fd = open("/dev/bpf", BPF_OPEN_FLAGS);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bpf-bsd.c` around lines 66 - 71, After allocating the bpf struct with calloc in the function that creates the BSD BPF handle, initialize the file descriptor field to a sentinel (bpf->bpf_fd = -1) immediately after the calloc(1, sizeof(*bpf)) call so any early failure paths or future inserted checks see a valid "closed" value; keep the subsequent open("/dev/bpf", BPF_OPEN_FLAGS) call unchanged and ensure cleanup paths (e.g., via goto eexit) treat -1 as "not opened".
🧹 Nitpick comments (1)
src/bpf-linux.c (1)
217-223: 💤 Low valueGuard
close()with fd check for consistency.Unlike the BSD backend and the inline cleanup in
eexit,bpf_closecallsclose(bpf->bpf_fd)unconditionally. While currentlybpf_closeis only called on successfully opened handles, adding a guard improves robustness and maintains consistency across backends.Proposed fix
void bpf_close(struct bpf *bpf) { - close(bpf->bpf_fd); + if (bpf->bpf_fd != -1) + close(bpf->bpf_fd); free(bpf->bpf_buffer); free(bpf); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bpf-linux.c` around lines 217 - 223, bpf_close currently calls close(bpf->bpf_fd) unconditionally; guard that call by checking the file descriptor (e.g., ensure bpf != NULL and bpf->bpf_fd >= 0) before calling close, and after closing set bpf->bpf_fd to -1 to avoid double-close; still free bpf->bpf_buffer and bpf to ensure cleanup even if the fd was invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/bpf-bsd.c`:
- Around line 66-71: After allocating the bpf struct with calloc in the function
that creates the BSD BPF handle, initialize the file descriptor field to a
sentinel (bpf->bpf_fd = -1) immediately after the calloc(1, sizeof(*bpf)) call
so any early failure paths or future inserted checks see a valid "closed" value;
keep the subsequent open("/dev/bpf", BPF_OPEN_FLAGS) call unchanged and ensure
cleanup paths (e.g., via goto eexit) treat -1 as "not opened".
---
Nitpick comments:
In `@src/bpf-linux.c`:
- Around line 217-223: bpf_close currently calls close(bpf->bpf_fd)
unconditionally; guard that call by checking the file descriptor (e.g., ensure
bpf != NULL and bpf->bpf_fd >= 0) before calling close, and after closing set
bpf->bpf_fd to -1 to avoid double-close; still free bpf->bpf_buffer and bpf to
ensure cleanup even if the fd was invalid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b104c234-3feb-49df-9f8a-4aa3e0377c6a
📒 Files selected for processing (4)
src/bpf-bsd.csrc/bpf-linux.csrc/bpf-pcap.csrc/bpf.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bpf.c
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bpf-pcap.c`:
- Around line 67-69: The snaplen used for libpcap must include the link-layer
header length, so compute a full_snaplen = bpf_frame_header_len(ifp) + snaplen
and use that for both the pcap_set_snaplen call and bpf->bpf_size; update the
assignment site (where snaplen, bpf->bpf_ifp and bpf->bpf_size are set) to set
bpf->bpf_size = full_snaplen (cast to size_t as needed) and pass full_snaplen to
pcap_set_snaplen instead of just mtu so the backend captures the entire
L2+payload frame like the BSD/Linux backends do.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09ca3146-e16a-4e28-9ba8-c3617d19bed4
📒 Files selected for processing (4)
src/bpf-bsd.csrc/bpf-linux.csrc/bpf-pcap.csrc/bpf.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bpf.c
| snaplen = ifp->mtu ? ifp->mtu : ETH_MTU; | ||
| bpf->bpf_ifp = ifp; | ||
| bpf->bpf_size = bpf_frame_header_len(ifp) + (size_t)snaplen; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to the official libpcap documentation, does pcap_set_snaplen() limit the total number of bytes captured starting at the link-layer header, so the snaplen should include link-layer header bytes as well as the interface MTU?
💡 Result:
Yes, according to the official libpcap documentation, the snapshot length (snaplen) set by pcap_set_snaplen limits the total number of bytes captured from each packet, and this limit applies to the entire packet as it is provided to the application, starting from the link-layer header [1][2][3]. When the snapshot length is set to a specific value, libpcap will capture and provide only the first snaplen bytes of each packet [1][2]. Because this limit is applied to the raw packet data as it is received from the capture mechanism, it necessarily includes the link-layer header bytes [1][3]. Consequently, if you intend to capture a certain amount of payload data, your snaplen must be large enough to account for both the link-layer header and the desired payload [4]. If the snaplen is smaller than the total size of the packet (including the link-layer header), the packet will be truncated to that length [1][2].
Citations:
- 1: https://man7.org/linux/man-pages/man3/pcap.3pcap.html
- 2: https://manpages.debian.org/unstable/libpcap0.8-dev/pcap.3pcap.en.html
- 3: https://www.tcpdump.org/manpages/pcap.3pcap.html
- 4: https://osqa-ask.wireshark.org/questions/3978/whats-an-appropriate-snaplen-for-https-traffic/
Use the full frame length for the libpcap snaplen.
Lines 67–69 allocate a buffer for bpf_frame_header_len(ifp) + mtu, but Line 83 configures libpcap to capture only mtu bytes. Since pcap_set_snaplen() measures the total capture length from the start of the link-layer frame (including L2 header bytes), this backend will truncate full-MTU packets by the L2 header length while the BSD and Linux backends preserve the entire frame.
Suggested fix
- PCAP_CHECK(pcap_set_snaplen(handle, snaplen), "pcap_set_snaplen");
+ PCAP_CHECK(pcap_set_snaplen(handle, (int)bpf->bpf_size),
+ "pcap_set_snaplen");🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 68-68: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bpf-pcap.c` around lines 67 - 69, The snaplen used for libpcap must
include the link-layer header length, so compute a full_snaplen =
bpf_frame_header_len(ifp) + snaplen and use that for both the pcap_set_snaplen
call and bpf->bpf_size; update the assignment site (where snaplen, bpf->bpf_ifp
and bpf->bpf_size are set) to set bpf->bpf_size = full_snaplen (cast to size_t
as needed) and pass full_snaplen to pcap_set_snaplen instead of just mtu so the
backend captures the entire L2+payload frame like the BSD/Linux backends do.
2 participants
This makes it easy to maintain the BPF sources.
libpcap makes it easier to support new OS such as GNU/Hurd where we have no BPF support right away.