fix: fix infinite loop and parser panic on partial TCP data

[ProjectRbt]

Infinite Loop and Parser Panic on TCP Partial Data (Service Unavailable) #1

• Remove redundant buffer preprocessing to fix infinite connection loop
• Add length check in bulk string parser to prevent out-of-bounds panic

Summary by CodeRabbit

• Bug Fixes

  • Improved protocol parsing robustness with enhanced null value handling, bounds checking, and better validation of malformed data.

• Refactor

  • Optimized buffering and control flow logic for more efficient data processing.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR refines RESP bulk string parsing to correctly handle null bulk strings and incomplete data, then simplifies the connection handler's async control flow by removing premature buffer pre-processing. Both subscribed and non-subscribed clients now consistently read from the socket before processing buffered data.

Changes

RESP Parser and Connection Flow

Layer / File(s)	Summary
RESP bulk string parser with null and bounds handling src/resp/parser.rs	parse_bulk_string now handles RESP null bulk strings (-1), rejects other negative lengths, validates buffer bounds before reading the payload, and decodes UTF-8 with corrected total_parsed calculation for the trailing CRLF.
Connection handler async control flow simplification src/main.rs	Removes early buffer pre-processing from both subscribed and non-subscribed code paths; subscribed clients enter tokio::select! immediately, and non-subscribed clients always read the socket first before processing buffered data.

🎯 2 (Simple) | ⏱️ ~12 minutes

🐰 A parser grows wise, bounds and nulls both blessed,
The socket now speaks first, the buffer rests,
No rushing ahead through the RESP protocol flame,
Just read, then parse true—simplicity's the game! 🎯

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main changes: fixing an infinite loop in connection handling and a parser panic on partial TCP data, which aligns with the actual modifications to src/main.rs and src/resp/parser.rs.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

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 `@src/resp/parser.rs`:
- Around line 61-71: Before returning the BulkString, validate that the two
trailer bytes are CRLF: after computing end_of_bulk_str and total_parsed, check
buffer[end_of_bulk_str] == b'\r' and buffer[end_of_bulk_str + 1] == b'\n' (using
bytes_consumed, bulk_str_len to locate them); if they are not CRLF return an
error (e.g., Incomplete or Invalid bulk string terminator) instead of accepting
the payload and converting data to String for Value::BulkString.

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3e36ef46-2b47-43d7-9009-a6c3e05b445d

📥 Commits

Reviewing files that changed from the base of the PR and between ba57376 and 241a8b5.

📒 Files selected for processing (2)

• src/main.rs
• src/resp/parser.rs

💤 Files with no reviewable changes (1)

• src/main.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate trailing CRLF before accepting bulk payload.

Line 64 only checks length, but not that the two trailer bytes are \r\n. A malformed frame can be parsed as valid and consume unrelated bytes, which can corrupt subsequent message boundaries.

Suggested fix

     let bulk_str_len = bulk_str_len as usize;
     let end_of_bulk_str = bytes_consumed + bulk_str_len;
     let total_parsed = end_of_bulk_str + 2;

     if buffer.len() < total_parsed {
         return Err(anyhow::anyhow!("Incomplete bulk string data"));
     }
+    if buffer[end_of_bulk_str] != b'\r' || buffer[end_of_bulk_str + 1] != b'\n' {
+        return Err(anyhow::anyhow!("Invalid bulk string terminator"));
+    }

     let data = &buffer[bytes_consumed..end_of_bulk_str];
     let s = String::from_utf8(data.to_vec())?;

     Ok((Value::BulkString(s), total_parsed))

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	let end_of_bulk_str = bytes_consumed + bulk_str_len;
	let total_parsed = end_of_bulk_str + 2;
	Ok((
	Value::BulkString(String::from_utf8(buffer[bytes_consumed..end_of_bulk_str].to_vec())?),
	total_parsed,
	))
	if buffer.len() < total_parsed {
	return Err(anyhow::anyhow!("Incomplete bulk string data"));
	}
	let data = &buffer[bytes_consumed..end_of_bulk_str];
	let s = String::from_utf8(data.to_vec())?;
	Ok((Value::BulkString(s), total_parsed))
	let end_of_bulk_str = bytes_consumed + bulk_str_len;
	let total_parsed = end_of_bulk_str + 2;
	if buffer.len() < total_parsed {
	return Err(anyhow::anyhow!("Incomplete bulk string data"));
	}
	if buffer[end_of_bulk_str] != b'\r' || buffer[end_of_bulk_str + 1] != b'\n' {
	return Err(anyhow::anyhow!("Invalid bulk string terminator"));
	}
	let data = &buffer[bytes_consumed..end_of_bulk_str];
	let s = String::from_utf8(data.to_vec())?;
	Ok((Value::BulkString(s), total_parsed))

🤖 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 `@src/resp/parser.rs` around lines 61 - 71, Before returning the BulkString,
validate that the two trailer bytes are CRLF: after computing end_of_bulk_str
and total_parsed, check buffer[end_of_bulk_str] == b'\r' and
buffer[end_of_bulk_str + 1] == b'\n' (using bytes_consumed, bulk_str_len to
locate them); if they are not CRLF return an error (e.g., Incomplete or Invalid
bulk string terminator) instead of accepting the payload and converting data to
String for Value::BulkString.