More debug information from libzkp #1692
Conversation
WalkthroughThe changes remove the debug proof dumping logic from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant libzkp_c
participant libzkp
User->>libzkp_c: verify_proof(proof)
libzkp_c->>libzkp: verify_proof(proof)
libzkp-->>libzkp_c: result
libzkp_c->>libzkp_c: Log verification attempt
alt Verification fails and enable_dump()
libzkp_c->>libzkp_c: Dump proof input to /tmp
libzkp_c->>User: Print error message
end
libzkp_c-->>User: Return result
sequenceDiagram
participant User
participant libzkp_c
User->>libzkp_c: gen_universal_task(task_json)
libzkp_c->>libzkp_c: Log task generation attempt
alt Generation fails and enable_dump()
libzkp_c->>libzkp_c: Dump task JSON to /tmp
libzkp_c->>User: Print error message
end
libzkp_c-->>User: Return result
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/libzkp/src/lib.rs(0 hunks)crates/libzkp_c/src/lib.rs(4 hunks)
💤 Files with no reviewable changes (1)
- crates/libzkp/src/lib.rs
⏰ 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). (1)
- GitHub Check: tests
🔇 Additional comments (5)
crates/libzkp_c/src/lib.rs (5)
12-20: LGTM: Clean environment variable checking with proper caching.The implementation correctly uses
OnceLockto cache the environment variable check and handles multiple environment variables appropriately.
65-65: Store the original proof pointer for debugging purposes.This line correctly preserves the original C string pointer for later use in the debug dumping logic.
67-67: Add tracing for verification attempts.Good addition of logging to track verification attempts with relevant context.
183-183: Add tracing for task generation attempts.Good addition of logging to track task generation attempts with relevant context.
83-88: Memory safety and JSON handling are correctThe C string passed into
verify_proofis created fromserde_json::to_string(), wrapped withCString::new(...).into_raw(), and thus is null-terminated. CallingCStr::from_ptr(...).to_bytes()reads the full JSON payload (JSON text cannot contain embedded NULs), so nothing is truncated. The same applies toc_char_to_vec. No changes required.
| Ok(result) => { | ||
| if !result && enable_dump() { | ||
| use std::time::{SystemTime, UNIX_EPOCH}; | ||
| // Dump req.input to a temporary file | ||
| let timestamp = SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .unwrap_or_default() | ||
| .as_secs(); | ||
| let filename = format!("/tmp/proof_{}.json", timestamp); | ||
| let cstr = unsafe { std::ffi::CStr::from_ptr(proof_str) }; | ||
| if let Err(e) = std::fs::write(&filename, cstr.to_bytes()) { | ||
| eprintln!("Failed to write proof to file {}: {}", filename, e); | ||
| } else { | ||
| println!("Dumped failed proof to {}", filename); | ||
| } | ||
| } | ||
| result as c_char | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address potential race conditions and security concerns in debug dumping.
The debug dumping logic has several potential issues:
- Race condition: Using only timestamp for filename could cause collisions if multiple threads/processes call this simultaneously
- Security: Writing to
/tmpmight not be secure in all environments - Portability: Hard-coded
/tmppath won't work on Windows
Consider this improved implementation:
- let filename = format!("/tmp/proof_{}.json", timestamp);
+ let filename = format!("/tmp/proof_{}_{}.json", std::process::id(), timestamp);Or better yet, use a more robust approach:
- let filename = format!("/tmp/proof_{}.json", timestamp);
+ let temp_dir = std::env::temp_dir();
+ let filename = temp_dir.join(format!("proof_{}_{}.json", std::process::id(), timestamp));📝 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.
| Ok(result) => { | |
| if !result && enable_dump() { | |
| use std::time::{SystemTime, UNIX_EPOCH}; | |
| // Dump req.input to a temporary file | |
| let timestamp = SystemTime::now() | |
| .duration_since(UNIX_EPOCH) | |
| .unwrap_or_default() | |
| .as_secs(); | |
| let filename = format!("/tmp/proof_{}.json", timestamp); | |
| let cstr = unsafe { std::ffi::CStr::from_ptr(proof_str) }; | |
| if let Err(e) = std::fs::write(&filename, cstr.to_bytes()) { | |
| eprintln!("Failed to write proof to file {}: {}", filename, e); | |
| } else { | |
| println!("Dumped failed proof to {}", filename); | |
| } | |
| } | |
| result as c_char | |
| } | |
| } | |
| Ok(result) => { | |
| if !result && enable_dump() { | |
| use std::time::{SystemTime, UNIX_EPOCH}; | |
| // Dump req.input to a temporary file | |
| let timestamp = SystemTime::now() | |
| .duration_since(UNIX_EPOCH) | |
| .unwrap_or_default() | |
| .as_secs(); | |
| - let filename = format!("/tmp/proof_{}.json", timestamp); | |
| + let temp_dir = std::env::temp_dir(); | |
| + let filename = temp_dir.join(format!("proof_{}_{}.json", std::process::id(), timestamp)); | |
| let cstr = unsafe { std::ffi::CStr::from_ptr(proof_str) }; | |
| if let Err(e) = std::fs::write(&filename, cstr.to_bytes()) { | |
| eprintln!("Failed to write proof to file {}: {}", filename.display(), e); | |
| } else { | |
| println!("Dumped failed proof to {}", filename.display()); | |
| } | |
| } | |
| result as c_char | |
| } |
🤖 Prompt for AI Agents
In crates/libzkp_c/src/lib.rs around lines 74 to 92, the debug dumping uses a
timestamp-based filename in /tmp, which risks race conditions, security issues,
and lacks portability. Fix this by generating a unique filename using a UUID or
a secure random value instead of just a timestamp, use a platform-independent
temporary directory obtained via standard library functions, and ensure file
permissions are set securely when writing the dump file.
| if enable_dump() { | ||
| use std::time::{SystemTime, UNIX_EPOCH}; | ||
| // Dump req.input to a temporary file | ||
| let timestamp = SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .unwrap_or_default() | ||
| .as_secs(); | ||
| let filename = format!("/tmp/task_{}_{}.json", fork_name_str, timestamp); | ||
| if let Err(e) = std::fs::write(&filename, task_json.as_bytes()) { | ||
| eprintln!("Failed to write task to file {}: {}", filename, e); | ||
| } else { | ||
| println!("Dumped failed task to {}", filename); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply same improvements as verification dump logic.
The task dumping logic has the same race condition and security concerns as the proof dumping logic.
Apply similar improvements:
- let filename = format!("/tmp/task_{}_{}.json", fork_name_str, timestamp);
+ let temp_dir = std::env::temp_dir();
+ let filename = temp_dir.join(format!("task_{}_{}_{}.json", fork_name_str, std::process::id(), timestamp));This addresses:
- Race conditions by including process ID
- Portability by using
std::env::temp_dir() - Better organization by including fork name in filename
📝 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.
| if enable_dump() { | |
| use std::time::{SystemTime, UNIX_EPOCH}; | |
| // Dump req.input to a temporary file | |
| let timestamp = SystemTime::now() | |
| .duration_since(UNIX_EPOCH) | |
| .unwrap_or_default() | |
| .as_secs(); | |
| let filename = format!("/tmp/task_{}_{}.json", fork_name_str, timestamp); | |
| if let Err(e) = std::fs::write(&filename, task_json.as_bytes()) { | |
| eprintln!("Failed to write task to file {}: {}", filename, e); | |
| } else { | |
| println!("Dumped failed task to {}", filename); | |
| } | |
| } | |
| if enable_dump() { | |
| use std::time::{SystemTime, UNIX_EPOCH}; | |
| // Dump req.input to a temporary file | |
| let timestamp = SystemTime::now() | |
| .duration_since(UNIX_EPOCH) | |
| .unwrap_or_default() | |
| .as_secs(); | |
| let temp_dir = std::env::temp_dir(); | |
| let filename = temp_dir.join(format!( | |
| "task_{}_{}_{}.json", | |
| fork_name_str, | |
| std::process::id(), | |
| timestamp | |
| )); | |
| if let Err(e) = std::fs::write(&filename, task_json.as_bytes()) { | |
| eprintln!("Failed to write task to file {}: {}", filename, e); | |
| } else { | |
| println!("Dumped failed task to {}", filename); | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/libzkp_c/src/lib.rs around lines 202 to 216, the task dumping logic
has race conditions and security issues similar to the proof dumping logic. Fix
this by generating the dump filename using std::env::temp_dir() for portability,
include the process ID to avoid race conditions, and ensure the fork name is
part of the filename for better organization. Update the filename construction
accordingly and handle errors as before.
|
Has been deprecated since we have #1694 |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes