cli: fix bash completion and vxlan display bugs#615
Merged
Conversation
📝 WalkthroughWalkthroughThis PR updates two independent areas. The CLI completion path now clones the command-list node before lex/expansion and changes completion buffer population to memcpy plus explicit null-termination, with some error paths returning early. The VXLAN CLI output now prints the "local" address using address-family-aware macros (ADDR_F/ADDR_W(...)) and passes the specific address field instead of the previous IPv4-only formatting. 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: 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 `@cli/complete.c`:
- Around line 95-96: Replace the unbounded strcpy call that copies COMP_LINE
into buf (buf, comp_line) with a bounded copy that respects BUFSIZ: compute or
check the length of comp_line and use a size-limited API (e.g., strncpy/strlcpy
or snprintf) to copy at most BUFSIZ-1 bytes and then explicitly null-terminate
buf; ensure the existing truncation logic that sets buf[i] = '\0' still cannot
write out of bounds by using the actual buffer capacity (BUFSIZ) instead of an
unchecked index i. This change should touch the code around the strcpy(buf,
comp_line) site and preserve intended truncation while eliminating the stack
overflow risk from unbounded input.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 74ee11d5-14bb-4bc9-a0fe-147459a93c5f
📒 Files selected for processing (2)
cli/complete.cmodules/l2/cli/vxlan.c
Comment on lines
+95
to
+96
| strcpy(buf, comp_line); | ||
| buf[i] = '\0'; |
There was a problem hiding this comment.
Unbounded strcpy introduces stack overflow risk
Line 95 copies untrusted COMP_LINE into buf[BUFSIZ] without a size check. A long completion line can overflow the stack before Line 96 truncates it.
Suggested fix
- strcpy(buf, comp_line);
- buf[i] = '\0';
+ if (i >= sizeof(buf)) {
+ errorf("COMP_POINT exceeds completion buffer");
+ goto end;
+ }
+ memcpy(buf, comp_line, (size_t)i);
+ buf[i] = '\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 `@cli/complete.c` around lines 95 - 96, Replace the unbounded strcpy call that
copies COMP_LINE into buf (buf, comp_line) with a bounded copy that respects
BUFSIZ: compute or check the length of comp_line and use a size-limited API
(e.g., strncpy/strlcpy or snprintf) to copy at most BUFSIZ-1 bytes and then
explicitly null-terminate buf; ensure the existing truncation logic that sets
buf[i] = '\0' still cannot write out of bounds by using the actual buffer
capacity (BUFSIZ) instead of an unchecked index i. This change should touch the
code around the strcpy(buf, comp_line) site and preserve intended truncation
while eliminating the stack overflow risk from unbounded input.
| goto end; | ||
| } | ||
|
|
||
| gr_strcpy(buf, i, comp_line); |
Collaborator
There was a problem hiding this comment.
gr_strcpy(buf, i+1, comp_line) ?
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cli/complete.c (1)
95-96:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical buffer overflow remains in completion-line copy path
Line 95 copies
ibytes intobuf[BUFSIZ]and Line 96 writesbuf[i]without checkingi < BUFSIZ. IfCOMP_LINEis longer thanBUFSIZ, this is out-of-bounds write on stack memory.Suggested minimal fix
- memcpy(buf, comp_line, i); - buf[i] = '\0'; + if (i >= sizeof(buf)) { + errorf("COMP_POINT exceeds completion buffer"); + return ret; + } + memcpy(buf, comp_line, (size_t)i); + buf[i] = '\0';#!/bin/bash # Read-only verification: # 1) confirm fixed-size stack buffer # 2) confirm copy/terminator use site # 3) confirm whether any bound check against sizeof(buf)/BUFSIZ exists before copy rg -n -C3 'char buf\[BUFSIZ\]|memcpy\(buf,\s*comp_line,\s*i\)|buf\[i\]\s*=\s*'\'\''\\0'\'\''' cli/complete.c rg -n -C3 'i\s*>=\s*sizeof\(buf\)|i\s*>=\s*BUFSIZ|BUFSIZ' cli/complete.c🤖 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 `@cli/complete.c` around lines 95 - 96, The code copies i bytes from comp_line into a fixed buffer buf[BUFSIZ] with memcpy(buf, comp_line, i) and then writes buf[i] = '\0' without bounding i; fix by clamping the number of bytes copied to at most BUFSIZ-1 and writing the terminator within bounds: compute a safe copy length (e.g., n = (i < BUFSIZ) ? i : BUFSIZ-1), memcpy(buf, comp_line, n) and set buf[n] = '\0' (or ensure buf[BUFSIZ-1] = '\0' after copying); update the code paths that use i, memcpy, comp_line, and buf to use this safe length so no out-of-bounds write occurs.
🤖 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 `@cli/complete.c`:
- Around line 95-96: The code copies i bytes from comp_line into a fixed buffer
buf[BUFSIZ] with memcpy(buf, comp_line, i) and then writes buf[i] = '\0' without
bounding i; fix by clamping the number of bytes copied to at most BUFSIZ-1 and
writing the terminator within bounds: compute a safe copy length (e.g., n = (i <
BUFSIZ) ? i : BUFSIZ-1), memcpy(buf, comp_line, n) and set buf[n] = '\0' (or
ensure buf[BUFSIZ-1] = '\0' after copying); update the code paths that use i,
memcpy, comp_line, and buf to use this safe length so no out-of-bounds write
occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05ecdc8b-bf9a-4c5c-a0c9-48c73ec68b83
📒 Files selected for processing (2)
cli/complete.cmodules/l2/cli/vxlan.c
maxime-leroy
approved these changes
May 20, 2026
The local VTEP address can now be IPv6. Fixes: 8999062 ("vxlan: add support for IPv6 underlay") Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Maxime Leroy <maxime@leroys.fr>
bash_complete() frees the cmdlist node tree, but main() also frees it in its cleanup path. This became a problem when the early return was replaced with goto end. Clone the provided node before using it to construct the bash_complete_node. Also use early return instead of goto if there is an error before the cmdlist node has been cloned. Fixes: 1e85ea8 ("cli: refactor bash_complete cleanup pattern") Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Maxime Leroy <maxime@leroys.fr>
gr_strcpy() does not support partial copy. It truncates one character past the requested length which breaks next word completion. Use memcpy() to copy exactly the bytes before the completion point and manually terminate the buffer. Fixes: 88c63f8 ("api: replace direct memccpy with gr_strcpy for safe string copy") Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Maxime Leroy <maxime@leroys.fr>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cli/complete.c (1)
95-96:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winStack buffer overflow:
iis unbounded relative tobuf[BUFSIZ]
iis validated againststrlen(comp_line)(line 90) but not againstsizeof(buf). IfCOMP_LINEexceedsBUFSIZ, both thememcpyand the null-termination write past the end ofbuf.Suggested fix
+ if (i >= sizeof(buf)) { + errorf("COMP_POINT exceeds completion buffer"); + return ret; + } memcpy(buf, comp_line, i); buf[i] = '\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 `@cli/complete.c` around lines 95 - 96, The code copies comp_line into buf using memcpy(buf, comp_line, i) and then sets buf[i] = '\0' but never bounds i against sizeof(buf) (buf is BUFSIZ), so COMP_LINE can overflow buf; fix by clamping the copy length to at most sizeof(buf)-1 before the memcpy and use that clamped length for the terminating NUL. In other words, compute a safe length (e.g., min(i, sizeof buf - 1)), use that length for the memcpy from comp_line to buf, and write the NUL at that safe index (use the same clamped variable instead of i). Ensure variables referenced: buf, comp_line, i, BUFSIZ/sizeof(buf), and the null-termination are updated accordingly.
🤖 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 `@cli/complete.c`:
- Around line 95-96: The code copies comp_line into buf using memcpy(buf,
comp_line, i) and then sets buf[i] = '\0' but never bounds i against sizeof(buf)
(buf is BUFSIZ), so COMP_LINE can overflow buf; fix by clamping the copy length
to at most sizeof(buf)-1 before the memcpy and use that clamped length for the
terminating NUL. In other words, compute a safe length (e.g., min(i, sizeof buf
- 1)), use that length for the memcpy from comp_line to buf, and write the NUL
at that safe index (use the same clamped variable instead of i). Ensure
variables referenced: buf, comp_line, i, BUFSIZ/sizeof(buf), and the
null-termination are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75bfc21d-7205-490e-ba47-09728bc35451
📒 Files selected for processing (2)
cli/complete.cmodules/l2/cli/vxlan.c
✅ Files skipped from review due to trivial changes (1)
- modules/l2/cli/vxlan.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
2 participants
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.
Fix use-after-free in bash completion mode where
bash_complete_node()was taking ownership of the caller'scmdlistnode, causing a double free whenmain()cleans up. Clone the node before passing it toEC_NODE_SEQ.Also fix a truncation issue in bash completion where
gr_strcpy()was cutting one character past the requested length, and fix the VXLAN local VTEP address display to handle IPv6 underlay.CLI bash completion fixes
Use-after-free in completion node handling:
Buffer truncation in completion:
Control-flow on error paths:
VXLAN display fix
IPv6-capable local VTEP address formatting: