Fix clang-tidy stderr logging warning in c_test

[xingbowang]

Summary:

• Replace c_test fprintf(stderr, ...) diagnostics with portable stderr helpers that avoid the clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling warning.
• Bound dynamic diagnostic string output to 1024 bytes and print RocksDB value slices with their explicit length to avoid over-reading non-NUL-terminated data on failure paths.
• Avoid fprintf_s because C11 Annex K support is optional and not portable across RocksDB-supported libc implementations.

Testing:
CI

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 0.0s.

[meta-codesync]

@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D106183622.

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 5700061

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not update PATH: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

• Codex may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /codex-review [context] — Request a code review
• /codex-query <question> — Ask about the PR or codebase

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 5700061

---

Summary

Clean, correct change that replaces fprintf(stderr, ...) with custom bounded helpers in db/c_test.c to fix clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling warnings. No correctness bugs found. The implementation is mathematically sound (buffer sizing, INT_MIN handling) and improves safety for non-NUL-terminated data in CheckEqual.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. PrintStderrUnsignedInt uses per-digit fputc instead of single fwrite -- db/c_test.c:72

• Issue: The function reverses digits into a buffer then outputs them one at a time via fputc. Since the reversed buffer is already constructed, a single fwrite(digits_reversed, 1, length, stderr) would be cleaner and avoid multiple stdio calls. While performance is irrelevant here (cold path), the per-character approach is unnecessarily complex.
• Root cause: The digits are stored in reverse order in the buffer, so they can't be fwrite'd directly. A simple reverse-then-fwrite, or building the string forward from the end of the buffer, would allow a single write call.
• Suggested fix: Build the string from the end of the buffer backward, then fwrite the tail portion:

  static void PrintStderrUnsignedInt(unsigned int value) {
    char digits[CHAR_BIT * sizeof(unsigned int)];
    size_t pos = sizeof(digits);
    do {
      digits[--pos] = (char)('0' + (value % 10));
      value /= 10;
    } while (value != 0);
    fwrite(digits + pos, 1, sizeof(digits) - pos, stderr);
  }

🟢 LOW / NIT

L1. Consider #define for kMaxStderrStringLength -- db/c_test.c:43

• Issue: In C (unlike C++), static const size_t is not a compile-time constant and cannot be used in array size declarations in C89 or as a case label. While it works fine here (only used in runtime comparisons), #define kMaxStderrStringLength 1024 would be more idiomatic C.
• Suggested fix: Use #define or keep as-is if targeting C99+ (which RocksDB does).

L2. PrintStderrBytes truncation at 1024 could hide diagnostic details -- db/c_test.c:55

• Issue: If a test value exceeds 1024 bytes, the diagnostic output will be silently truncated. This is a tradeoff for safety (preventing unbounded reads). In practice, test values in c_test.c are short strings, so this is unlikely to matter.
• Suggested fix: No action needed. If desired, a truncation indicator could be added, but this is very low priority.

L3. Minor: variable-length array concern on exotic compilers -- db/c_test.c:72

• Issue: char digits[CHAR_BIT * sizeof(unsigned int)] -- while CHAR_BIT and sizeof are compile-time constants, some extremely pedantic C89 compilers might not treat CHAR_BIT * sizeof(unsigned int) as an integer constant expression since CHAR_BIT is a macro that expands to an implementation-defined value. In practice, all compilers RocksDB supports handle this correctly.
• Suggested fix: No action needed.

Cross-Component Analysis

Context	Applies?	Safe?	Notes
Linux	Yes	Yes	Standard C library functions
Windows (MSVC)	Yes	Yes	fwrite/fputs/fputc all available
macOS	Yes	Yes	Standard C library functions
Multi-threaded	No	N/A	c_test is single-threaded
Production code	No	N/A	Test-only change, all functions static

Assumption stress-test results:

• Buffer sizing (CHAR_BIT * sizeof(unsigned int)): Mathematically proven sufficient for all platforms. Decimal digits needed = ceil(N * log10(2)) < N for all N >= 1.
• INT_MIN handling ((unsigned int)(-(value + 1)) + 1): Avoids all signed integer overflow. Each intermediate step is representable.
• 1024-byte string bound: Adequate for all realistic diagnostic strings in this test file.

Positive Observations

1. CheckEqual fix is a genuine improvement: The original code used %s to print v which could read past the buffer for non-NUL-terminated data. The new PrintStderrBytes(v, n) correctly uses the explicit length parameter. This fixes a latent safety issue.

2. INT_MIN handling is elegant: The (unsigned int)(-(value + 1)) + 1 technique avoids undefined behavior that would occur with naive -INT_MIN negation.

3. NULL pointer handling is thorough: All print functions handle NULL inputs gracefully by printing "(null)".

4. Approach is justified: The PR description correctly notes that fprintf_s (C11 Annex K) is not portable, and #pragma cannot suppress clang-analyzer checks (the codebase uses #ifndef __clang_analyzer__ for that purpose, which would be more invasive here).

5. Style is consistent: Function naming (CamelCase), constant naming (k-prefix), and overall structure match existing c_test.c patterns.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase