Conversation
We always send a header with the expected lengths. So use malloced buffers to send and receive using this knowledge.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough
ChangesBuffer API Redesign and Context-Managed Allocation
Estimated code review effort🎯 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/dhcp.c (1)
1266-1268:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMemory leak on early return paths.
When
dhcp_readfile()succeeds but the lease is truncated (line 1266-1268), authentication fails (line 1286-1288), or authentication is now required (line 1296-1298), the function returns0without freeing the buffer allocated bydhcp_readfile()and stored in*bootp.🐛 Proposed fix
/* Ensure the packet is at lease BOOTP sized * with a vendor area of 4 octets * (it should be more, and our read packet enforces this so this * code should not be needed, but of course people could * scribble whatever in the stored lease file. */ if (bytes < DHCP_MIN_LEN) { logerrx("%s: %s: truncated lease", ifp->name, __func__); + free(*bootp); + *bootp = NULL; return 0; }Similar fixes needed at lines 1286-1288 and 1296-1298.
🤖 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/dhcp.c` around lines 1266 - 1268, The function returns 0 on three early return paths (truncated lease, authentication failure, and when authentication is now required) without freeing the buffer allocated by dhcp_readfile() that is stored in *bootp, causing memory leaks. Before each return 0 statement in the truncated lease check, the authentication failure check, and the authentication requirement check, free the buffer stored in *bootp using the appropriate memory deallocation function to prevent memory leaks on these early exit paths.src/dhcp6.c (1)
2757-2764:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMemory leak:
dhcp6not freed in error path.When
dhcp_readfilesucceeds (allocatesdhcp6) but a subsequent check fails (e.g.,dhcp6_validateleaseat line 2714 or auth validation at line 2733), the code jumps toex:wherestate->newis freed but the locally allocateddhcp6is leaked.Proposed fix
ex: dhcp6_freedrop_addrs(ifp, 0, IPV6_AF_DELEGATED, NULL); dhcp_unlink(ifp->ctx, state->leasefile); + free(dhcp6); free(state->new); state->new = NULL; state->new_len = 0;🤖 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/dhcp6.c` around lines 2757 - 2764, The error path at the `ex:` label frees `state->new` but fails to free the `dhcp6` variable that was allocated earlier when `dhcp_readfile` succeeded. When subsequent validation checks like `dhcp6_validatelease` or authentication validation fail, the code jumps to `ex:` causing a memory leak of `dhcp6`. Add a `free(dhcp6)` call in the `ex:` error handling block, ensuring the locally allocated `dhcp6` variable is properly deallocated alongside the existing `state->new` cleanup.
🧹 Nitpick comments (1)
src/dhcp.c (1)
1247-1251: 💤 Low valueDuplicate
*bootp = NULLassignment.Line 1251 duplicates the assignment already done at line 1245.
♻️ Remove duplicate assignment
if (state->leasefile[0] == '\0') logdebugx("reading standard input"); else logdebugx("%s: reading lease: %s", ifp->name, state->leasefile); - *bootp = NULL; sbytes = dhcp_readfile(ifp->ctx, state->leasefile, (void **)bootp, 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/dhcp.c` around lines 1247 - 1251, Remove the duplicate assignment of `*bootp = NULL;` at the end of the conditional block in the dhcp.c file. The `*bootp = NULL` assignment already exists earlier in the function (at line 1245) and should not be repeated after the logdebugx calls. Simply delete the duplicate `*bootp = NULL;` line that appears after the else branch in the diff.
🤖 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/common.c`:
- Around line 122-139: The readfile function has three resource management and
bounds-checking issues that need to be fixed. First, add a close(fd) call before
the return statement when fstat() fails to prevent fd leakage. Second, similarly
close fd before returning when realloc() fails to prevent fd leakage. Third, add
an error check after the read() call to ensure it did not return -1 (error
condition) before attempting to write the null terminator to buf[bytes], as
writing to a negative index would cause undefined behavior. These fixes should
be applied around the fstat, realloc, and read operations in sequence within the
readfile function.
In `@src/dhcp-common.c`:
- Around line 1020-1046: The issue is that after realloc expands the buffer, the
variable blen is not updated to reflect the newly available space. When len is
initially 0, blen becomes 0, and read() is called with 0 bytes to read, causing
immediate return. After reallocating the buffer and updating p to the new buffer
position (in the realloc block where nbuf is assigned to *data and p), calculate
blen as the remaining available space in the newly allocated buffer by setting
it to needed minus bytes, so that read() reads the correct amount of data on the
next iteration.
In `@src/privsep-bsd.c`:
- Around line 392-399: The pointer variable `p` is assigned from `ctx->ps_buf`
before the `ps_bufalloc()` call, but since this function may reallocate the
buffer, the stored pointer `p` can become invalid if the buffer is moved to a
new memory location. Move the assignment of `p` from `ctx->ps_buf` to occur
after the `ps_bufalloc(ctx, buflen)` check succeeds and returns, ensuring that
`p` points to the current valid buffer address before it is used in subsequent
memcpy operations.
- Around line 422-423: In the ps_root_sysctl function, the variable buf
referenced in the ps_root_readerror call on line 422 is undefined and should be
replaced with ctx->ps_buf. Change the ps_root_readerror invocation to use
ctx->ps_buf as the buffer argument instead of the undefined buf variable.
Additionally, verify that sizeof(ctx->ps_buf) is the correct size to pass, as
other similar calls in the function use more explicit sizes like sizeof(*time)
or sizeof(*rdm) rather than sizeof of the entire buffer structure.
In `@src/privsep-root.c`:
- Around line 569-575: In the PS_GETHOSTNAME case block, there is a typo on the
line calculating rlen where cts->ps_buf is referenced instead of ctx->ps_buf.
The variable name is ctx, not cts, so the strlen call needs to be corrected to
use ctx->ps_buf to match the variable used elsewhere in the same block (where
rdata is assigned and gethostname is called). Fix this typo by replacing cts
with ctx in the strlen function call.
In `@src/privsep.c`:
- Around line 145-148: The unconditional return statement immediately after the
chdir("/") and logerr call is causing the function to exit prematurely, skipping
all subsequent privilege-dropping operations including setgroups, setgid,
setuid, and setrlimit calls. Remove this errant return statement so that
execution continues through the privilege-dropping code. The return statement
should only be present conditionally for the __sun platform where privilege
dropping is intentionally skipped, not unconditionally after the chdir
operation.
---
Outside diff comments:
In `@src/dhcp.c`:
- Around line 1266-1268: The function returns 0 on three early return paths
(truncated lease, authentication failure, and when authentication is now
required) without freeing the buffer allocated by dhcp_readfile() that is stored
in *bootp, causing memory leaks. Before each return 0 statement in the truncated
lease check, the authentication failure check, and the authentication
requirement check, free the buffer stored in *bootp using the appropriate memory
deallocation function to prevent memory leaks on these early exit paths.
In `@src/dhcp6.c`:
- Around line 2757-2764: The error path at the `ex:` label frees `state->new`
but fails to free the `dhcp6` variable that was allocated earlier when
`dhcp_readfile` succeeded. When subsequent validation checks like
`dhcp6_validatelease` or authentication validation fail, the code jumps to `ex:`
causing a memory leak of `dhcp6`. Add a `free(dhcp6)` call in the `ex:` error
handling block, ensuring the locally allocated `dhcp6` variable is properly
deallocated alongside the existing `state->new` cleanup.
---
Nitpick comments:
In `@src/dhcp.c`:
- Around line 1247-1251: Remove the duplicate assignment of `*bootp = NULL;` at
the end of the conditional block in the dhcp.c file. The `*bootp = NULL`
assignment already exists earlier in the function (at line 1245) and should not
be repeated after the logdebugx calls. Simply delete the duplicate `*bootp =
NULL;` line that appears after the else branch in the diff.
🪄 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: bdda112a-985d-4b50-9d0b-c72695992444
📒 Files selected for processing (13)
src/common.csrc/common.hsrc/dhcp-common.csrc/dhcp-common.hsrc/dhcp.csrc/dhcp6.csrc/dhcpcd.hsrc/if-options.csrc/privsep-bsd.csrc/privsep-root.csrc/privsep-root.hsrc/privsep.csrc/privsep.h
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/privsep-root.c (1)
569-574:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound hostname length before computing reply size.
Line 573 uses
strlenonctx->ps_bufright aftergethostname. If the hostname is truncated and not NUL-terminated, this can read past the allocated buffer and leak memory throughrlen.🐛 Proposed fix
case PS_GETHOSTNAME: err = gethostname((char *)ctx->ps_buf, ctx->ps_buflen); if (err != -1) { + ((char *)ctx->ps_buf)[ctx->ps_buflen - 1] = '\0'; rdata = ctx->ps_buf; - rlen = strlen((char *)ctx->ps_buf) + 1; + rlen = strnlen((char *)ctx->ps_buf, ctx->ps_buflen) + 1; } break;🤖 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/privsep-root.c` around lines 569 - 574, In the PS_GETHOSTNAME case block, the strlen call on ctx->ps_buf after gethostname can read past the allocated buffer if the hostname fills the entire buffer without null-termination. Bound the hostname length calculation to not exceed ctx->ps_buflen by either manually null-terminating the buffer after the gethostname call and then limiting the strlen result, or by using a bounded string length function like strnlen that respects the ctx->ps_buflen limit to safely compute rlen.
🤖 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.
Outside diff comments:
In `@src/privsep-root.c`:
- Around line 569-574: In the PS_GETHOSTNAME case block, the strlen call on
ctx->ps_buf after gethostname can read past the allocated buffer if the hostname
fills the entire buffer without null-termination. Bound the hostname length
calculation to not exceed ctx->ps_buflen by either manually null-terminating the
buffer after the gethostname call and then limiting the strlen result, or by
using a bounded string length function like strnlen that respects the
ctx->ps_buflen limit to safely compute rlen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1510deaf-1ad3-4058-9818-a13349bfe333
📒 Files selected for processing (7)
src/common.csrc/dhcp-common.csrc/dhcpcd.csrc/if-linux.csrc/privsep-bsd.csrc/privsep-root.csrc/privsep.c
💤 Files with no reviewable changes (1)
- src/privsep.c
🚧 Files skipped from review as they are similar to previous changes (3)
- src/privsep-bsd.c
- src/dhcp-common.c
- src/common.c
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.
We always send a header with the expected lengths. So use malloced buffers to send and receive using this knowledge.