Skip to content

Conversation

@Eeems
Copy link
Collaborator

@Eeems Eeems commented Oct 21, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Improved input validation in protocol communication with early failure detection for invalid sizes to avoid unnecessary work.
    • Enhanced robustness with defensive null-checking during memory cleanup to prevent misuse of freed pointers.
  • Refactor

    • Internal memory allocation adjusted for safer, more exception-resilient handling without changing external behavior.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2025

📝 Walkthrough

Walkthrough

Added defensive checks and safer allocation: strv_free now guards against null elements before freeing; blight_send_message uses std::make_shared for ack_t; BlightProtocol::recv validates size ≤ 0 early and returns an error without allocating or reading.

Changes

Cohort / File(s) Summary
libblight_protocol improvements
shared/libblight_protocol/libblight_protocol.cpp
Added null-checks before calling free on elements in strv_free; replaced manual new ack_t allocation with std::make_shared<ack_t> in blight_send_message.
Input validation
shared/libblight_protocol/socket.cpp
Added an early validation in BlightProtocol::recv to treat size <= 0 as an error: logs a warning, sets errno = EINVAL, and returns an empty optional before allocation or I/O.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant Blight as BlightProtocol::recv
    Note over Blight: New early validation added
    Caller->>Blight: recv(size, ...)
    alt size <= 0
        Blight-->>Caller: log warning, errno=EINVAL, return empty
    else size > 0
        Blight->>Blight: allocate buffer
        Blight->>Socket: read loop (may retry/partial)
        alt read success and full size
            Blight-->>Caller: return buffer
        else read failure/partial
            Blight-->>Caller: set errno, return empty
        end
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I nudge a check, I guard a free,

I swap a new for shared esprit,
I sniff the size, and if it's wrong,
I hop away — safe, swift, and strong. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "Add more guards and use make_shared" directly and accurately describes the main changes in the changeset. The "guards" refer to the defensive checks added: a null-check before freeing elements in strv_free and an early validation check (size <= 0) in BlightProtocol::recv. The "make_shared" portion refers to replacing manual allocation with std::make_shared in blight_send_message. The title is concise, clear, and specific enough that a teammate scanning history would understand the primary changes without additional context.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fixes

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e39bfa5 and b1a851d.

📒 Files selected for processing (2)
  • shared/libblight_protocol/libblight_protocol.cpp (2 hunks)
  • shared/libblight_protocol/socket.cpp (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • shared/libblight_protocol/libblight_protocol.cpp
  • shared/libblight_protocol/socket.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Build and package
  • GitHub Check: Analyze (java-kotlin)
  • GitHub Check: Build web

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Eeems Eeems merged commit 2bf38d7 into master Oct 21, 2025
13 of 15 checks passed
@Eeems Eeems deleted the fixes branch October 21, 2025 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants