vtysh, lib: fix 'show ip[v6] prefix-list ... json' JSON display by moving it to vtysh to fix formatting issue#151
Conversation
…ng it to vtysh
Json output is not valid for 'show ip[v6] prefix-list ... json' commands,
as it goes through all the running daemons and for each one it calls
'vty_show_prefix_list' creating a new json object. To aggreagate the output
and create a valid json that can be parsed, the commands were moved to vtysh.
Before:
{
"ZEBRA":{
"DEFAULT":{
"addressFamily":"IPv4",
"entries":[
{
"sequenceNumber":10,
"type":"permit",
"prefix":"0.0.0.0/0"
}
]
}
}
}
{
"OSPF":{
"DEFAULT":{
"addressFamily":"IPv4",
"entries":[
{
"sequenceNumber":10,
"type":"permit",
"prefix":"0.0.0.0/0"
}
]
}
}
}
{
"BGP":{
"DEFAULT":{
"addressFamily":"IPv4",
"entries":[
{
"sequenceNumber":10,
"type":"permit",
"prefix":"0.0.0.0/0"
}
]
}
}
}
After:
{"zebra":{
"DEFAULT":{
"addressFamily":"IPv4",
"entries":[
{
"sequenceNumber":10,
"type":"permit",
"prefix":"0.0.0.0/0"
}
]
}
}
,"ospfd":{
"DEFAULT":{
"addressFamily":"IPv4",
"entries":[
{
"sequenceNumber":10,
"type":"permit",
"prefix":"0.0.0.0/0"
}
]
}
}
,"bgpd":{
"DEFAULT":{
"addressFamily":"IPv4",
"entries":[
{
"sequenceNumber":10,
"type":"permit",
"prefix":"0.0.0.0/0"
}
]
}
}
}
Signed-off-by: Piotr Suchy <piotrsuchy@proton.me>
Greptile SummaryThis PR fixes malformed JSON output for Key changes:
The core JSON aggregation logic is sound, but the vtysh-side installation introduces a user-visible regression by breaking access for non-privileged users. Confidence Score: 1/5
Last reviewed commit: c89fd58 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }; | ||
|
|
||
| extern enum vtysh_write_integrated vtysh_write_integrated; | ||
| extern enum display_type display_type; |
There was a problem hiding this comment.
Spurious extern variable declaration
extern enum display_type display_type; declares a global variable named display_type of type enum display_type, but no such variable is ever defined anywhere in the codebase. This is a copy-paste error modelled after the vtysh_write_integrated pattern — but display_type is meant to be a type name, not a variable. While this won't cause a compile error because the symbol is never referenced, it is incorrect and will cause a linker error the moment any translation unit actually uses it.
| extern enum display_type display_type; | |
| extern enum vtysh_write_integrated vtysh_write_integrated; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: vtysh/vtysh.h
Line: 107
Comment:
**Spurious extern variable declaration**
`extern enum display_type display_type;` declares a global *variable* named `display_type` of type `enum display_type`, but no such variable is ever defined anywhere in the codebase. This is a copy-paste error modelled after the `vtysh_write_integrated` pattern — but `display_type` is meant to be a *type name*, not a variable. While this won't cause a compile error because the symbol is never referenced, it is incorrect and will cause a linker error the moment any translation unit actually uses it.
```suggestion
extern enum vtysh_write_integrated vtysh_write_integrated;
```
How can I resolve this? If you propose a fix, please make it concise.| if (afi == AFI_IP) | ||
| snprintf(command_line, sizeof(command_line), | ||
| "do show ip prefix-list "); | ||
| else if (afi == AFI_IP6) | ||
| snprintf(command_line, sizeof(command_line), | ||
| "do show ipv6 prefix-list "); |
There was a problem hiding this comment.
Uninitialized buffer if afi is neither AFI_IP nor AFI_IP6
command_line is a local array that is only initialised via snprintf inside the if (afi == AFI_IP) or else if (afi == AFI_IP6) branches. If afi holds any other value, the buffer is left uninitialised and all subsequent strlcat calls operate on garbage memory, invoking undefined behaviour.
Although the current callers always pass AFI_IP or AFI_IP6, the static analysis risk and future-proofing argue for an else branch with an early return, or at minimum an assert/return guard:
if (afi == AFI_IP)
snprintf(command_line, sizeof(command_line),
"do show ip prefix-list ");
else if (afi == AFI_IP6)
snprintf(command_line, sizeof(command_line),
"do show ipv6 prefix-list ");
else
return;Prompt To Fix With AI
This is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 3485-3490
Comment:
**Uninitialized buffer if afi is neither AFI_IP nor AFI_IP6**
`command_line` is a local array that is only initialised via `snprintf` inside the `if (afi == AFI_IP)` or `else if (afi == AFI_IP6)` branches. If `afi` holds any other value, the buffer is left uninitialised and all subsequent `strlcat` calls operate on garbage memory, invoking undefined behaviour.
Although the current callers always pass `AFI_IP` or `AFI_IP6`, the static analysis risk and future-proofing argue for an `else` branch with an early return, or at minimum an `assert`/`return` guard:
```c
if (afi == AFI_IP)
snprintf(command_line, sizeof(command_line),
"do show ip prefix-list ");
else if (afi == AFI_IP6)
snprintf(command_line, sizeof(command_line),
"do show ipv6 prefix-list ");
else
return;
```
How can I resolve this? If you propose a fix, please make it concise.| #define VTYSH_PREFIX_LIST_SHOW \ | ||
| VTYSH_ZEBRA | VTYSH_RIPD | VTYSH_RIPNGD | VTYSH_OSPFD | VTYSH_OSPF6D | \ | ||
| VTYSH_BGPD | VTYSH_ISISD | VTYSH_PIMD | VTYSH_EIGRPD | \ | ||
| VTYSH_FABRICD |
There was a problem hiding this comment.
VTYSH_PREFIX_LIST_SHOW duplicates VTYSH_RMAP_SHOW exactly
Both macros expand to the identical bitmask. Defining them separately creates a latent maintenance hazard: if a new daemon is added to one but not the other, the coverage silently diverges. Consider defining one in terms of the other, or at least adding a comment tying them together:
/* Same set of daemons as VTYSH_RMAP_SHOW — keep in sync */
#define VTYSH_PREFIX_LIST_SHOW VTYSH_RMAP_SHOWPrompt To Fix With AI
This is a comment left during a code review.
Path: vtysh/vtysh.h
Line: 71-74
Comment:
**`VTYSH_PREFIX_LIST_SHOW` duplicates `VTYSH_RMAP_SHOW` exactly**
Both macros expand to the identical bitmask. Defining them separately creates a latent maintenance hazard: if a new daemon is added to one but not the other, the coverage silently diverges. Consider defining one in terms of the other, or at least adding a comment tying them together:
```c
/* Same set of daemons as VTYSH_RMAP_SHOW — keep in sync */
#define VTYSH_PREFIX_LIST_SHOW VTYSH_RMAP_SHOW
```
How can I resolve this? If you propose a fix, please make it concise.
| enum display_type { | ||
| normal_display, | ||
| summary_display, | ||
| detail_display, | ||
| sequential_display, | ||
| longer_display, | ||
| first_match_display | ||
| }; |
There was a problem hiding this comment.
Duplicate enum display_type definition
enum display_type is already defined as a file-private type in lib/plist.c at line 912 with identical enumerators. By adding a second definition here in vtysh/vtysh.h, two independent definitions now exist in separate translation units that must be kept manually in sync.
If a new display type is ever added to lib/plist.c but not vtysh/vtysh.h (or vice versa), the mismatch will be silent — the code will compile cleanly but show_prefix_list_send will use the wrong enum values.
The correct fix is to move the definition into lib/plist.h (the public header for plist.c), and include lib/plist.h from vtysh/vtysh.h or vtysh/vtysh.c, removing the duplicate:
/* In lib/plist.h */
enum display_type {
normal_display,
summary_display,
detail_display,
sequential_display,
longer_display,
first_match_display
};Then remove the definition from both lib/plist.c and vtysh/vtysh.h, and add #include "lib/plist.h" where needed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: vtysh/vtysh.h
Line: 97-104
Comment:
**Duplicate `enum display_type` definition**
`enum display_type` is already defined as a file-private type in `lib/plist.c` at line 912 with identical enumerators. By adding a second definition here in `vtysh/vtysh.h`, two independent definitions now exist in separate translation units that must be kept manually in sync.
If a new display type is ever added to `lib/plist.c` but not `vtysh/vtysh.h` (or vice versa), the mismatch will be silent — the code will compile cleanly but `show_prefix_list_send` will use the wrong enum values.
The correct fix is to move the definition into `lib/plist.h` (the public header for `plist.c`), and include `lib/plist.h` from `vtysh/vtysh.h` or `vtysh/vtysh.c`, removing the duplicate:
```c
/* In lib/plist.h */
enum display_type {
normal_display,
summary_display,
detail_display,
sequential_display,
longer_display,
first_match_display
};
```
Then remove the definition from both `lib/plist.c` and `vtysh/vtysh.h`, and add `#include "lib/plist.h"` where needed.
How can I resolve this? If you propose a fix, please make it concise.| if (json && !first) | ||
| vty_out(vty, ","); | ||
| else | ||
| first = false; |
There was a problem hiding this comment.
first flag never set when json=false
The else branch executes when !(json && !first) — which includes both "first connected daemon in JSON mode" AND "any connected daemon in non-JSON mode". As a result, when json=false, first is reset to false on every iteration but is never used to suppress a comma (since the if condition is already gated on json). While harmless in non-JSON mode, the problem shows up in JSON mode for edge cases where connected daemons appear after non-connected ones:
Consider the sequence: daemon A (not connected → skipped), daemon B (connected, first=true):
json && !first→true && true→false→else: first = false- Daemon B outputs without a leading comma ✓
Then daemon C (connected, first=false):
json && !first→true && true→true→ comma printed ✓
This path is actually correct. However, the non-JSON path means first is unconditionally set to false even on non-connected daemons that were skipped via continue. Since the continue skips both the flag check and the body, first is only mutated for connected daemons, keeping the logic intact.
The concern is that this non-obvious control flow is identical to the show_route_map_send template but could be confusing. A clearer formulation would be:
| if (json && !first) | |
| vty_out(vty, ","); | |
| else | |
| first = false; | |
| if (json) { | |
| if (!first) | |
| vty_out(vty, ","); | |
| else | |
| first = false; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 3521-3524
Comment:
**`first` flag never set when `json=false`**
The `else` branch executes when `!(json && !first)` — which includes both "first connected daemon in JSON mode" AND "any connected daemon in non-JSON mode". As a result, when `json=false`, `first` is reset to `false` on every iteration but is never used to suppress a comma (since the `if` condition is already gated on `json`). While harmless in non-JSON mode, the problem shows up in JSON mode for edge cases where connected daemons appear after non-connected ones:
Consider the sequence: daemon A (not connected → skipped), daemon B (connected, `first=true`):
- `json && !first` → `true && true` → `false` → `else: first = false`
- Daemon B outputs without a leading comma ✓
Then daemon C (connected, `first=false`):
- `json && !first` → `true && true` → `true` → comma printed ✓
This path is actually correct. However, the non-JSON path means `first` is unconditionally set to `false` even on non-connected daemons that were skipped via `continue`. Since the `continue` skips both the flag check and the body, `first` is only mutated for **connected** daemons, keeping the logic intact.
The concern is that this non-obvious control flow is identical to the `show_route_map_send` template but could be confusing. A clearer formulation would be:
```suggestion
if (json) {
if (!first)
vty_out(vty, ",");
else
first = false;
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| }; | ||
|
|
||
| extern enum vtysh_write_integrated vtysh_write_integrated; | ||
| extern enum display_type display_type; |
There was a problem hiding this comment.
extern enum display_type display_type; declares a global variable that is never defined anywhere in the codebase. This is a copy-paste error modeled after line 106 (extern enum vtysh_write_integrated vtysh_write_integrated;), but unlike that variable which has a real definition in vtysh.c, display_type is a type, not a variable. This declaration should be removed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: vtysh/vtysh.h
Line: 107
Comment:
`extern enum display_type display_type;` declares a global *variable* that is never defined anywhere in the codebase. This is a copy-paste error modeled after line 106 (`extern enum vtysh_write_integrated vtysh_write_integrated;`), but unlike that variable which has a real definition in vtysh.c, `display_type` is a type, not a variable. This declaration should be removed.
How can I resolve this? If you propose a fix, please make it concise.
| install_element(ENABLE_NODE, &show_ip_prefix_list_cmd); | ||
| install_element(ENABLE_NODE, &show_ip_prefix_list_summary_cmd); | ||
| install_element(ENABLE_NODE, &show_ip_prefix_list_detail_cmd); | ||
| install_element(ENABLE_NODE, &show_ipv6_prefix_list_cmd); | ||
| install_element(ENABLE_NODE, &show_ipv6_prefix_list_summary_cmd); | ||
| install_element(ENABLE_NODE, &show_ipv6_prefix_list_detail_cmd); |
There was a problem hiding this comment.
All six show ip[v6] prefix-list commands are registered in each daemon's CLI in VIEW_NODE (see lib/plist.c:1662-1665, 1681-1684). Before this PR, vtysh would forward the command to the daemons when it wasn't registered locally, so it remained accessible from the unprivileged VIEW prompt.
After this PR the vtysh-level handlers are installed only in ENABLE_NODE (lines 5232-5237). Any user who has not entered enable mode in vtysh will now get % Unknown command when running these show commands — a clear behavioural regression.
The reference PR FRRouting#14856 (show route-map) did not introduce a regression because show route-map was already gated behind ENABLE_NODE in each daemon. That is not the case here.
To preserve existing behaviour:
install_element(VIEW_NODE, &show_ip_prefix_list_cmd);
install_element(ENABLE_NODE, &show_ip_prefix_list_cmd);
install_element(VIEW_NODE, &show_ip_prefix_list_summary_cmd);
install_element(ENABLE_NODE, &show_ip_prefix_list_summary_cmd);
install_element(VIEW_NODE, &show_ip_prefix_list_detail_cmd);
install_element(ENABLE_NODE, &show_ip_prefix_list_detail_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_cmd);
install_element(ENABLE_NODE, &show_ipv6_prefix_list_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_summary_cmd);
install_element(ENABLE_NODE, &show_ipv6_prefix_list_summary_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_detail_cmd);
install_element(ENABLE_NODE, &show_ipv6_prefix_list_detail_cmd);Prompt To Fix With AI
This is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 5232-5237
Comment:
All six `show ip[v6] prefix-list` commands are registered in each daemon's CLI in `VIEW_NODE` (see `lib/plist.c:1662-1665, 1681-1684`). Before this PR, vtysh would forward the command to the daemons when it wasn't registered locally, so it remained accessible from the unprivileged VIEW prompt.
After this PR the vtysh-level handlers are installed **only** in `ENABLE_NODE` (lines 5232-5237). Any user who has not entered `enable` mode in vtysh will now get `% Unknown command` when running these show commands — a clear behavioural regression.
The reference PR #14856 (`show route-map`) did not introduce a regression because `show route-map` was already gated behind `ENABLE_NODE` in each daemon. That is not the case here.
To preserve existing behaviour:
```c
install_element(VIEW_NODE, &show_ip_prefix_list_cmd);
install_element(ENABLE_NODE, &show_ip_prefix_list_cmd);
install_element(VIEW_NODE, &show_ip_prefix_list_summary_cmd);
install_element(ENABLE_NODE, &show_ip_prefix_list_summary_cmd);
install_element(VIEW_NODE, &show_ip_prefix_list_detail_cmd);
install_element(ENABLE_NODE, &show_ip_prefix_list_detail_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_cmd);
install_element(ENABLE_NODE, &show_ipv6_prefix_list_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_summary_cmd);
install_element(ENABLE_NODE, &show_ipv6_prefix_list_summary_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_detail_cmd);
install_element(ENABLE_NODE, &show_ipv6_prefix_list_detail_cmd);
```
How can I resolve this? If you propose a fix, please make it concise.2 participants
Json output is not valid for
show ip prefix-list ... jsoncommands, because it goes through each running daemon and for each one it callsvty_show_prefix_listcreating a new json object. To aggregate the output and create a valid json that can be parsed, the commands were moved to vtysh, similarly to this PR: FRRouting#14856This solves: FRRouting#15277
Before:
{ "ZEBRA":{ "DEFAULT":{ "addressFamily":"IPv4", "entries":[ { "sequenceNumber":10, "type":"permit", "prefix":"0.0.0.0/0" } ] } } } { "OSPF":{ "DEFAULT":{ "addressFamily":"IPv4", "entries":[ { "sequenceNumber":10, "type":"permit", "prefix":"0.0.0.0/0" } ] } } } { "BGP":{ "DEFAULT":{ "addressFamily":"IPv4", "entries":[ { "sequenceNumber":10, "type":"permit", "prefix":"0.0.0.0/0" } ] } } }JSON is not valid.
After:
{"zebra":{ "DEFAULT":{ "addressFamily":"IPv4", "entries":[ { "sequenceNumber":10, "type":"permit", "prefix":"0.0.0.0/0" } ] } } ,"ospfd":{ "DEFAULT":{ "addressFamily":"IPv4", "entries":[ { "sequenceNumber":10, "type":"permit", "prefix":"0.0.0.0/0" } ] } } ,"bgpd":{ "DEFAULT":{ "addressFamily":"IPv4", "entries":[ { "sequenceNumber":10, "type":"permit", "prefix":"0.0.0.0/0" } ] } } }JSON is valid.
Analogous behavior for:
show ip prefix-list summary jsonshow ip prefix-list detail jsonshow ipv6 prefix-list jsonshow ipv6 prefix-list summary jsonshow ipv6 prefix-list detail json