diff --git a/soulseek-rs-lib/src/client/mod.rs b/soulseek-rs-lib/src/client/mod.rs index 2d47b4b..1abf26e 100644 --- a/soulseek-rs-lib/src/client/mod.rs +++ b/soulseek-rs-lib/src/client/mod.rs @@ -17,7 +17,7 @@ use crate::{ peer::{ConnectionType, DownloadPeer, Peer, PeerMessage, listen::Listen}, shares::Shares, types::{Download, Search, SearchResult}, - utils::{lock::RwLockExt, md5}, + utils::lock::RwLockExt, }; use std::{ collections::{HashMap, HashSet}, @@ -90,6 +90,17 @@ fn next_download_token() -> u32 { NEXT_DOWNLOAD_TOKEN.fetch_add(1, Ordering::Relaxed) % 0x8000_0000 } +/// Source of search tokens. +/// +/// A counter, not a hash of the query: a peer's answer is routed to the first +/// search holding its token, so two queries sharing one would pour results +/// into each other. +static NEXT_SEARCH_TOKEN: AtomicU32 = AtomicU32::new(1); + +fn next_search_token() -> u32 { + NEXT_SEARCH_TOKEN.fetch_add(1, Ordering::Relaxed) +} + /// A file we have agreed to serve to a peer, awaiting their TransferResponse. struct UploadJob { downloader: String, diff --git a/soulseek-rs-lib/src/client/search.rs b/soulseek-rs-lib/src/client/search.rs index ebcac80..215deb5 100644 --- a/soulseek-rs-lib/src/client/search.rs +++ b/soulseek-rs-lib/src/client/search.rs @@ -1,7 +1,7 @@ use super::{ Arc, AtomicBool, Client, DEFAULT_WISHLIST_INTERVAL, Duration, HashMap, Instant, Ordering, Result, RwLockExt, Search, SearchResult, ServerMessage, - SoulseekRs, info, md5, sleep, + SoulseekRs, info, next_search_token, sleep, }; impl Client { @@ -62,8 +62,7 @@ impl Client { let Some(handle) = &self.server_handle else { return Err(SoulseekRs::NotConnected); }; - let hash = md5::md5(query); - let token = u32::from_str_radix(&hash[0..5], 16)?; + let token = next_search_token(); self.context.write_safe()?.searches.insert( query.to_string(), diff --git a/soulseek-rs-lib/tests/e2e.rs b/soulseek-rs-lib/tests/e2e.rs index f945ca4..939500c 100644 --- a/soulseek-rs-lib/tests/e2e.rs +++ b/soulseek-rs-lib/tests/e2e.rs @@ -3092,3 +3092,38 @@ fn a_search_reply_carries_at_most_the_file_cap() { let _ = std::fs::remove_dir_all(share_dir); } + +// Tokens were the first five hex digits of the query's MD5, so two queries +// could share one and a peer's answer to either went to whichever search the +// map handed back first. These two queries collide under that scheme. +#[test] +fn two_live_searches_never_share_a_token() { + let server = server_or_skip!(); + + let share_dir = unique_download_dir(); + std::fs::write(share_dir.join("tokenprobe aeabna.bin"), b"x").unwrap(); + let (_sharer, searcher) = sharer_and_searcher( + &server, + &share_dir, + "e2e_token_sharer", + "e2e_token_searcher", + ); + + let matching = "tokenprobe aeabna"; + let colliding = "tokenprobe soyvgt"; + let _ = searcher.search(colliding, Duration::from_millis(100)); + let _ = searcher.search(matching, Duration::from_secs(3)); + + let live = searcher.get_all_searches(); + assert_ne!( + live[matching].token, live[colliding].token, + "two live searches must not share a token" + ); + + assert!( + reply_from(&searcher, matching, "e2e_token_sharer").is_some(), + "the sharer's answer should land in the search it answers" + ); + + let _ = std::fs::remove_dir_all(share_dir); +}