Fix file download bug in released version #16
Conversation
WalkthroughThe changes introduce a bidirectional file transfer protocol enhancement, adding absolute path handling for shared directory items. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NodeService
participant Host
Client->>NodeService: RequestFiles (list of file paths)
loop For each file
NodeService->>Host: Open stream to Host
NodeService->>Host: Send file path length + path bytes
Host->>NodeService: Look up absolute path via path_mappings
alt Path found
Host->>NodeService: Stream file contents
NodeService->>Client: Receive file (success)
else Path not found
Host->>NodeService: Log error, close stream
NodeService->>Client: Mark as failed
end
end
NodeService->>Client: Emit DownloadCompleted/DownloadFailed events
Poem
✨ Finishing Touches
🪧 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
🔭 Outside diff range comments (1)
src/main.rs (1)
295-308: 🛠️ Refactor suggestionReading every file for preview on every 50 ms tick is expensive
handle_host_modere-opens and reads up to 4 KB of every shared file each iteration, even when nothing changed. For a directory with many large files this will thrash the disk and allocate repeatedly.
Cache the preview string insideDirectoryItem(together with a hash / modified time) and rebuild only when the file actually changes.
🧹 Nitpick comments (2)
src/app.rs (2)
34-46: Addition ofabsolute_pathlooks correct but will now participate inEq/PartialEqderives
Because the field is part of the struct, twoDirectoryItems that differ only byabsolute_pathwill be considered different. On the host side you continually rebuild the vector every 50 ms, which meansabsolute_path’sPathBufinstances will differ in allocation identity and sometimes in canonicalisation (e.g. after symlink-resolution).
This can trigger unnecessary UI refreshes and network traffic when you compare vectors (!=) inhandle_download_mode.
If the absolute path is only an implementation detail, consider:#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct DirectoryItem { @@ - #[serde(skip)] - pub absolute_path: Option<PathBuf>, + #[serde(skip)] + #[compare_ignore] // <- custom derive or manual PartialEq implementation + pub absolute_path: Option<PathBuf>, }or supply a manual
PartialEq/Eqimpl that ignores the field.
191-201:absolute_pathis initialised toNonefor local filesystem listing
This is fine for the UI/client side, but any host-side code that later relies on a non-Nonevalue must remember to overwrite it. At the moment onlyhandle_host_modedoes so; if other callers re-usecreate_directory_itemthey may forget to set it and silently break downloads.
Recommend adding a helperDirectoryItem::with_absolute_path(item, path)or a dedicated constructor for the host code to avoid accidental omissions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app.rs(2 hunks)src/main.rs(2 hunks)src/service/node.rs(6 hunks)
🔇 Additional comments (2)
src/main.rs (1)
268-285: Lowest-common-ancestor algorithm can misbehave for disjoint selections
If the user shares two items on different drives (/mnt/a vs C:\ on Windows)virtual_rootwill end up as the first path’s root because the loop stops at the first ancestor that matches, not the true LCA across all paths.
Consider using a crate such aspathdiffor implement a proper LCA that iterates over all candidates before deciding.src/service/node.rs (1)
239-259: Path-mapping table is a clean solution
Nicely separates network-visible paths from real filesystem locations and prevents path-traversal.
| // Open a new stream for each file | ||
| match stream_control | ||
| .open_stream(peer_id, JUNKANOO_FILE_PROTOCOL) | ||
| .await | ||
| { | ||
| Ok(mut stream) => { | ||
| // Send the file request to the host | ||
| let path_bytes = file_name.as_bytes(); | ||
| let path_len = u32::try_from(path_bytes.len()).unwrap(); | ||
|
|
||
| // Send path length | ||
| if let Err(e) = stream.write_all(&path_len.to_be_bytes()).await { | ||
| tracing::error!("Failed to send path length for file '{}': {}", file_name, e); | ||
| failed_transfers.push(file_name); | ||
| continue; | ||
| } | ||
|
|
||
| // Send path | ||
| if let Err(e) = stream.write_all(path_bytes).await { | ||
| tracing::error!("Failed to send path for file '{}': {}", file_name, e); | ||
| failed_transfers.push(file_name); | ||
| continue; | ||
| } | ||
|
|
||
| // Now receive the file | ||
| let mut receiver = FileReceiver::new(); | ||
| match receiver.receive_file(&mut stream).await { | ||
| Ok(received_file_name) => { | ||
| tracing::info!("Successfully received file '{}'", received_file_name); | ||
| successful_transfers.push(file_name); | ||
| } | ||
| Err(e) => { | ||
| tracing::error!( | ||
| "Failed to open stream for file '{}': {}", | ||
| "Failed to receive file '{}': {}", | ||
| file_name, | ||
| e | ||
| ); | ||
| failed_transfers.push(file_name); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if !successful_transfers.is_empty() { | ||
| event_sender | ||
| .send(Event::DownloadCompleted(successful_transfers)) | ||
| .await | ||
| .expect("Event receiver not to be dropped."); | ||
| } | ||
|
|
||
| if failed_transfers.is_empty() { | ||
| let _ = sender.send(Ok(Vec::new())); | ||
| } else { | ||
| let failed_files = failed_transfers.join(", "); | ||
| event_sender | ||
| .send(Event::DownloadFailed(failed_transfers)) | ||
| .await | ||
| .expect("Event receiver not to be dropped."); | ||
|
|
||
| let _ = sender.send(Err(Box::new(std::io::Error::other(format!( | ||
| "Failed to transfer files: {failed_files}" | ||
| ))) | ||
| as Box<dyn Error + Send>)); | ||
| Err(e) => { | ||
| tracing::error!( | ||
| "Failed to open stream for file '{}': {}", | ||
| file_name, | ||
| e | ||
| ); | ||
| failed_transfers.push(file_name); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
No length guard when sending path length
u32::try_from(path_bytes.len()).unwrap() will panic if the path exceeds u32::MAX, and you’re sending whatever length the client chooses to the host (which must also validate).
Add the same MAX_PATH_LEN guard before conversion and return a user-visible error instead of panicking.
🤖 Prompt for AI Agents
In src/service/node.rs between lines 526 and 573, the code converts the file
path length to u32 without checking if it exceeds u32::MAX, which can cause a
panic. Add a check before the conversion to ensure the path length does not
exceed MAX_PATH_LEN, and if it does, return a user-visible error instead of
unwrapping and panicking. This prevents crashes by validating the path length
before sending it.
| let mut len_bytes = [0u8; 4]; | ||
| if let Err(e) = stream.read_exact(&mut len_bytes).await { | ||
| tracing::error!("Failed to read file path length: {}", e); | ||
| drop(permit); | ||
| return; | ||
| } | ||
| Err(e) => { | ||
| tracing::error!("Failed to receive file from peer {}: {}", peer, e); | ||
| let path_len = u32::from_be_bytes(len_bytes) as usize; | ||
|
|
||
| let mut path_bytes = vec![0u8; path_len]; | ||
| if let Err(e) = stream.read_exact(&mut path_bytes).await { | ||
| tracing::error!("Failed to read file path: {}", e); | ||
| drop(permit); | ||
| return; | ||
| } | ||
|
|
||
| let requested_path = match String::from_utf8(path_bytes) { | ||
| Ok(path) => path, |
There was a problem hiding this comment.
Untrusted peer controls allocation size – potential DoS
path_len is read from the wire and used to allocate Vec<u8> without bounds.
A malicious peer could send 0xFFFF_FFFF, forcing a 4 GB allocation and crashing the host.
-let path_len = u32::from_be_bytes(len_bytes) as usize;
+const MAX_PATH_LEN: usize = 4096; // sane upper bound
+let path_len = u32::from_be_bytes(len_bytes) as usize;
+if path_len == 0 || path_len > MAX_PATH_LEN {
+ tracing::warn!("Rejecting request with suspicious path length {}", path_len);
+ drop(permit);
+ return;
+}Similar validation should be applied when sending the request (request_files).
🤖 Prompt for AI Agents
In src/service/node.rs around lines 288 to 304, the code reads path_len from the
network and uses it directly to allocate a vector, which can lead to a large
allocation and potential DoS. Fix this by adding a validation step after reading
path_len to ensure it is within a reasonable maximum size before allocating the
vector. Reject or handle requests with path_len exceeding this limit gracefully.
Also, apply similar size validation when sending requests in the request_files
function to prevent oversized allocations.
The file download issue in release mode was due to inverted file transfer logic and incorrect path resolution.
The fix involved:
src/service/node.rswas reversed: the client now sends a file path request, then receives the file data. The host receives the request, resolves the path, and sends the file.absolute_path: Option<PathBuf>field was added toDirectoryIteminsrc/app.rs, marked with%23[serde(skip)]to prevent network serialization.src/main.rs,handle_host_modewas updated to populate thisabsolute_pathwhen sharing items.src/service/node.rs, apath_mappings: HashMap<PeerId, HashMap<String, PathBuf>>was added toEventLoop.InsertDirectoryItemscommand handler now populates thispath_mappingswith absolute paths.These changes ensure the host correctly identifies and serves files using absolute paths, resolving the discrepancy between debug and release environments.
Summary by CodeRabbit