fix(lib): give each search its own token
A search token was the first five hex digits of the query's MD5, so two queries could share one, and a peer's answer to either landed in whichever search the map handed back first. Tokens now come from a counter, which no two live searches can share. The e2e test uses two queries that collide under the old scheme.
This commit is contained in:
@@ -17,7 +17,7 @@ use crate::{
|
|||||||
peer::{ConnectionType, DownloadPeer, Peer, PeerMessage, listen::Listen},
|
peer::{ConnectionType, DownloadPeer, Peer, PeerMessage, listen::Listen},
|
||||||
shares::Shares,
|
shares::Shares,
|
||||||
types::{Download, Search, SearchResult},
|
types::{Download, Search, SearchResult},
|
||||||
utils::{lock::RwLockExt, md5},
|
utils::lock::RwLockExt,
|
||||||
};
|
};
|
||||||
use std::{
|
use std::{
|
||||||
collections::{HashMap, HashSet},
|
collections::{HashMap, HashSet},
|
||||||
@@ -90,6 +90,17 @@ fn next_download_token() -> u32 {
|
|||||||
NEXT_DOWNLOAD_TOKEN.fetch_add(1, Ordering::Relaxed) % 0x8000_0000
|
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.
|
/// A file we have agreed to serve to a peer, awaiting their TransferResponse.
|
||||||
struct UploadJob {
|
struct UploadJob {
|
||||||
downloader: String,
|
downloader: String,
|
||||||
|
|||||||
@@ -1,7 +1,7 @@
|
|||||||
use super::{
|
use super::{
|
||||||
Arc, AtomicBool, Client, DEFAULT_WISHLIST_INTERVAL, Duration, HashMap,
|
Arc, AtomicBool, Client, DEFAULT_WISHLIST_INTERVAL, Duration, HashMap,
|
||||||
Instant, Ordering, Result, RwLockExt, Search, SearchResult, ServerMessage,
|
Instant, Ordering, Result, RwLockExt, Search, SearchResult, ServerMessage,
|
||||||
SoulseekRs, info, md5, sleep,
|
SoulseekRs, info, next_search_token, sleep,
|
||||||
};
|
};
|
||||||
|
|
||||||
impl Client {
|
impl Client {
|
||||||
@@ -62,8 +62,7 @@ impl Client {
|
|||||||
let Some(handle) = &self.server_handle else {
|
let Some(handle) = &self.server_handle else {
|
||||||
return Err(SoulseekRs::NotConnected);
|
return Err(SoulseekRs::NotConnected);
|
||||||
};
|
};
|
||||||
let hash = md5::md5(query);
|
let token = next_search_token();
|
||||||
let token = u32::from_str_radix(&hash[0..5], 16)?;
|
|
||||||
|
|
||||||
self.context.write_safe()?.searches.insert(
|
self.context.write_safe()?.searches.insert(
|
||||||
query.to_string(),
|
query.to_string(),
|
||||||
|
|||||||
@@ -3092,3 +3092,38 @@ fn a_search_reply_carries_at_most_the_file_cap() {
|
|||||||
|
|
||||||
let _ = std::fs::remove_dir_all(share_dir);
|
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);
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user