greptile: Reduce ruleset to memory-allocation, logging-api, and string-safety #240
greptile: Reduce ruleset to memory-allocation, logging-api, and string-safety #240suphawitwanl merged 3 commits intomasterfrom
Conversation
Greptile SummaryThis PR introduces a Greptile configuration for FRR that enforces three coding rules (memory-allocation, logging-api, string-safety) scoped to C/C++ source files. The context loading (
|
| Filename | Overview |
|---|---|
| .greptile/config.json | Introduces three enforcement rules (memory-allocation, logging-api, string-safety); the memory-allocation rule omits strdup/strndup from its banned-function list, leaving a real MTYPE-bypass vector unguarded, and the string-safety rule is missing vsprintf from its prohibition. |
| .greptile/files.json | Loads three FRR developer docs as context (workflow.rst, memtypes.rst, logging.rst); content and descriptions are accurate and complete. |
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
.greptile/config.json:26-29
**`strdup`/`strndup` not banned, leaving a MTYPE-bypass vector open**
The memory-allocation rule explicitly bans `malloc`, `calloc`, `realloc`, and `free` but silently omits `strdup()` and `strndup()`. Both perform a raw heap allocation with no MTYPE tracking — they are functionally equivalent to `malloc` + `strcpy` from the memory-accounting perspective. The codebase already contains multiple `.c` files using bare `strdup()` (confirmed via search), which is exactly the pattern this ruleset is supposed to catch and flag. The correct FRR replacement (`XSTRDUP(MTYPE_*, ...)`) is already named in the rule text, so the missing piece is simply declaring the banned side:
```
"NEVER allow standard malloc(), calloc(), realloc(), strdup(), strndup(), or free(). ..."
```
Without this addition, `strdup()` calls will pass automated review silently.
### Issue 2 of 3
.greptile/config.json:37-42
**`vsprintf` missing from string-safety ban list**
The string-safety rule bans `sprintf()` but says nothing about `vsprintf()`. The two functions are identically unsafe — neither performs bounds checking — and any `sprintf` ban is trivially circumvented by wrapping the call with a `va_list` argument. The FRR codebase already uses `vsnprintf` (the safe form) in multiple files, confirming that va_list-based string formatting is common enough to warrant explicit coverage. Add `vsprintf()` to the forbidden list alongside `sprintf()` and pair it with `vsnprintf()` as the enforcement target.
### Issue 3 of 3
.greptile/config.json:37-42
**`snprintfrr` omitted as preferred alternative to `snprintf`**
The string-safety rule enforces `snprintf` as the replacement for `sprintf`, but FRR's own logging infrastructure (`logging.rst`) defines `snprintfrr`/`vsnprintfrr` as the preferred formatting functions when printing FRR-specific types using format extensions (`%pFX`, `%pI4`, `%pSU`, etc.). A rule that only names `snprintf` as the target gives no guidance on when to prefer `snprintfrr`, which means reviewers may accept code using plain `snprintf` in contexts that require FRR format extensions, silently producing `%p`-style literal output instead of the intended address. Consider noting that `snprintfrr` (and its variants) should be preferred over `snprintf` when FRR-specific format specifiers are involved.
Reviews (2): Last reviewed commit: "greptile: Fix invalid commentTypes value..." | Re-trigger Greptile
| "rule": "NEVER allow standard malloc(), calloc(), realloc(), or free(). Enforce FRR's XCALLOC, XMALLOC, XREALLOC, XSTRDUP, and XFREE macros (requires MTYPE). Every allocation must use the MTYPE tracking system declared via DECLARE_MTYPE/DEFINE_MTYPE. Prefer DEFINE_MTYPE_STATIC for module-private types.", | ||
| "scope": ["**/*.c", "**/*.h", "**/*.cpp"], | ||
| "severity": "critical" |
There was a problem hiding this comment.
critical severity applied to a "Prefer" recommendation
The memory-allocation rule mixes a hard, mandatory ban with a soft preference under the same critical severity. Automated enforcement will treat any violation of the entire rule text as a P0 blocker — meaning a contributor who writes DEFINE_MTYPE(...) instead of DEFINE_MTYPE_STATIC(...) for a module-private type will receive a critical/must-fix flag, even though the guideline explicitly says "Prefer" (not "Require"). Either split this into two rules with different severities, or promote the DEFINE_MTYPE_STATIC preference to a separate medium-severity rule so reviewers can distinguish genuine memory-safety violations from style guidance.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .greptile/config.json
Line: 27-29
Comment:
**`critical` severity applied to a "Prefer" recommendation**
The `memory-allocation` rule mixes a hard, mandatory ban with a soft preference under the same `critical` severity. Automated enforcement will treat _any_ violation of the entire rule text as a P0 blocker — meaning a contributor who writes `DEFINE_MTYPE(...)` instead of `DEFINE_MTYPE_STATIC(...)` for a module-private type will receive a critical/must-fix flag, even though the guideline explicitly says "Prefer" (not "Require"). Either split this into two rules with different severities, or promote the `DEFINE_MTYPE_STATIC` preference to a separate `medium`-severity rule so reviewers can distinguish genuine memory-safety violations from style guidance.
How can I resolve this? If you propose a fix, please make it concise.| "id": "logging-api", | ||
| "rule": "Reject printf() or stdio logging. Enforce FRR's zlog_* API (zlog_debug, zlog_err, zlog_warn, zlog_info, zlog_notice) and structured error macros flog_err(), flog_warn(), flog_err_sys() (which require an EC_* error-code argument). All debug statements MUST be guarded with CLI-controllable debug flags — unguarded debug prints are unacceptable at scale.", | ||
| "scope": ["**/*.c", "**/*.h", "**/*.cpp"], | ||
| "severity": "high" |
There was a problem hiding this comment.
Ambiguous "stdio logging" phrase leaves common bypass patterns uncovered
"Reject printf() or stdio logging" is loose enough that a model could accept fprintf(stderr, ...), fputs(msg, stderr), or fwrite to stderr — all of which bypass the zlog_* system entirely. FRR's logging.rst notes that startup stderr output is only valid before zlog_startup_end() is called; after that, direct writes to stderr are wrong. Enumerating the forbidden functions explicitly (e.g. fprintf, fputs, fwrite, perror) removes ambiguity and produces deterministic enforcement.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .greptile/config.json
Line: 32-35
Comment:
**Ambiguous "stdio logging" phrase leaves common bypass patterns uncovered**
"Reject printf() or stdio logging" is loose enough that a model could accept `fprintf(stderr, ...)`, `fputs(msg, stderr)`, or `fwrite` to stderr — all of which bypass the `zlog_*` system entirely. FRR's `logging.rst` notes that startup stderr output is only valid before `zlog_startup_end()` is called; after that, direct writes to stderr are wrong. Enumerating the forbidden functions explicitly (e.g. `fprintf`, `fputs`, `fwrite`, `perror`) removes ambiguity and produces deterministic enforcement.
How can I resolve this? If you propose a fix, please make it concise.|
@greptileai anything i could improve more |
| "id": "memory-allocation", | ||
| "rule": "NEVER allow standard malloc(), calloc(), realloc(), or free(). Enforce FRR's XCALLOC, XMALLOC, XREALLOC, XSTRDUP, and XFREE macros (requires MTYPE). Every allocation must use the MTYPE tracking system declared via DECLARE_MTYPE/DEFINE_MTYPE. Prefer DEFINE_MTYPE_STATIC for module-private types.", | ||
| "scope": ["**/*.c", "**/*.h", "**/*.cpp"], | ||
| "severity": "critical" |
There was a problem hiding this comment.
strdup/strndup not banned, leaving a MTYPE-bypass vector open
The memory-allocation rule explicitly bans malloc, calloc, realloc, and free but silently omits strdup() and strndup(). Both perform a raw heap allocation with no MTYPE tracking — they are functionally equivalent to malloc + strcpy from the memory-accounting perspective. The codebase already contains multiple .c files using bare strdup() (confirmed via search), which is exactly the pattern this ruleset is supposed to catch and flag. The correct FRR replacement (XSTRDUP(MTYPE_*, ...)) is already named in the rule text, so the missing piece is simply declaring the banned side:
"NEVER allow standard malloc(), calloc(), realloc(), strdup(), strndup(), or free(). ..."
Without this addition, strdup() calls will pass automated review silently.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .greptile/config.json
Line: 26-29
Comment:
**`strdup`/`strndup` not banned, leaving a MTYPE-bypass vector open**
The memory-allocation rule explicitly bans `malloc`, `calloc`, `realloc`, and `free` but silently omits `strdup()` and `strndup()`. Both perform a raw heap allocation with no MTYPE tracking — they are functionally equivalent to `malloc` + `strcpy` from the memory-accounting perspective. The codebase already contains multiple `.c` files using bare `strdup()` (confirmed via search), which is exactly the pattern this ruleset is supposed to catch and flag. The correct FRR replacement (`XSTRDUP(MTYPE_*, ...)`) is already named in the rule text, so the missing piece is simply declaring the banned side:
```
"NEVER allow standard malloc(), calloc(), realloc(), strdup(), strndup(), or free(). ..."
```
Without this addition, `strdup()` calls will pass automated review silently.
How can I resolve this? If you propose a fix, please make it concise.|
@greptileai what about flow or sequence diagram that you not provided in some pr? how can i fix it |
|
You can trigger a diagram from me at any time by asking directly in a PR comment — just say something like "generate a diagram", "generate a flowchart", or "show me a sequence diagram". I'll generate one based on the PR context. Here's a flowchart for this PR showing how the three rules are enforced: %%{init: {'theme': 'neutral'}}%%
flowchart TD
A[C/C++ Source File\n*.c / *.h / *.cpp] --> B{Greptile Rule Engine\nStrictness: 2}
B --> C[memory-allocation rule\nSeverity: critical P0]
B --> D[string-safety rule\nSeverity: high P1]
B --> E[logging-api rule\nSeverity: medium P2]
C --> C1{Uses raw malloc/\ncalloc/realloc/free?}
C1 -- Yes --> C2[FAIL: Must use\nXMALLOC/XCALLOC/\nXREALLOC/XFREE]
C1 -- No --> C3{MTYPE declared via\nDECLARE_MTYPE/\nDEFINE_MTYPE?}
C3 -- No --> C4[FAIL: Missing\nMTYPE tracking]
C3 -- Yes --> C5[PASS]
D --> D1{Uses strcpy/strcat\n/sprintf?}
D1 -- Yes --> D2[FAIL: Must use\nstrlcpy/strlcat/\nsnprintf]
D1 -- No --> D3{Buffer size arg\nuses sizeof?}
D3 -- No --> D4[FAIL: Must use\nsizeof for buffer size]
D3 -- Yes --> D5[PASS]
E --> E1{Uses printf\nor stdio?}
E1 -- Yes --> E2[FAIL: Must use\nzlog_*/flog_* API]
E1 -- No --> E3{Debug statements\nguarded by CLI\ndebug flags?}
E3 -- No --> E4[FAIL: Missing\ndebug flag guard]
E3 -- Yes --> E5[PASS]
C5 --> F{All Rules Pass?}
D5 --> F
E5 --> F
C2 --> G[Status Check FAILS\nBlock Merge]
C4 --> G
D2 --> G
D4 --> G
E2 --> G
E4 --> G
F -- Yes --> H[Status Check PASSES\nAllow Merge]
The flow shows each rule's check sequence — a single failure from any of the three blocks the merge via the status check. |
Settings:
Rule structure:
Each rule has an id, description, scope, and severity.
Rules cover:
Doc context loaded by Greptile (.greptile/files.json):