From 131310b920bda545a5cb28789e96d1f83524212f Mon Sep 17 00:00:00 2001 From: dnlmlr <34707428+dnlmlr@users.noreply.github.com> Date: Wed, 3 Aug 2022 20:01:03 +0200 Subject: [PATCH] Fix panic in `ApResolver::resolve` (#1038) - Fixed resolve function panicking when resolving endpoint type with no AP in the list - Fixed fallback APs not being applied when only some of the AP types were missing - Switch container type from `Vec` to `VecDeque` for the `AccessPoints` - Remove the note about fallback AP being used even if the port is not matching the configured `ap_port` --- core/src/apresolve.rs | 97 +++++++++++++++++++++++++++---------------- src/main.rs | 2 +- 2 files changed, 62 insertions(+), 37 deletions(-) diff --git a/core/src/apresolve.rs b/core/src/apresolve.rs index 72b089dd..c02ec1c8 100644 --- a/core/src/apresolve.rs +++ b/core/src/apresolve.rs @@ -1,3 +1,5 @@ +use std::collections::VecDeque; + use hyper::{Body, Method, Request}; use serde::Deserialize; @@ -7,23 +9,23 @@ pub type SocketAddress = (String, u16); #[derive(Default)] pub struct AccessPoints { - accesspoint: Vec, - dealer: Vec, - spclient: Vec, + accesspoint: VecDeque, + dealer: VecDeque, + spclient: VecDeque, } -#[derive(Deserialize)] +#[derive(Deserialize, Default)] pub struct ApResolveData { accesspoint: Vec, dealer: Vec, spclient: Vec, } -// These addresses probably do some geo-location based traffic management or at least DNS-based -// load balancing. They are known to fail when the normal resolvers are up, so that's why they -// should only be used as fallback. -impl Default for ApResolveData { - fn default() -> Self { +impl ApResolveData { + // These addresses probably do some geo-location based traffic management or at least DNS-based + // load balancing. They are known to fail when the normal resolvers are up, so that's why they + // should only be used as fallback. + fn fallback() -> Self { Self { accesspoint: vec![String::from("ap.spotify.com:443")], dealer: vec![String::from("dealer.spotify.com:443")], @@ -32,6 +34,12 @@ impl Default for ApResolveData { } } +impl AccessPoints { + fn is_any_empty(&self) -> bool { + self.accesspoint.is_empty() || self.dealer.is_empty() || self.spclient.is_empty() + } +} + component! { ApResolver : ApResolverInner { data: AccessPoints = AccessPoints::default(), @@ -49,27 +57,34 @@ impl ApResolver { } } - fn process_data(&self, data: Vec) -> Vec { + fn process_ap_strings(&self, data: Vec) -> VecDeque { + let filter_port = self.port_config(); data.into_iter() .filter_map(|ap| { let mut split = ap.rsplitn(2, ':'); let port = split.next()?; - let host = split.next()?.to_owned(); let port: u16 = port.parse().ok()?; - if let Some(p) = self.port_config() { - if p != port { - return None; - } + let host = split.next()?.to_owned(); + match filter_port { + Some(filter_port) if filter_port != port => None, + _ => Some((host, port)), } - Some((host, port)) }) .collect() } + fn parse_resolve_to_access_points(&self, resolve: ApResolveData) -> AccessPoints { + AccessPoints { + accesspoint: self.process_ap_strings(resolve.accesspoint), + dealer: self.process_ap_strings(resolve.dealer), + spclient: self.process_ap_strings(resolve.spclient), + } + } + pub async fn try_apresolve(&self) -> Result { let req = Request::builder() .method(Method::GET) - .uri("http://apresolve.spotify.com/?type=accesspoint&type=dealer&type=spclient") + .uri("https://apresolve.spotify.com/?type=accesspoint&type=dealer&type=spclient") .body(Body::empty())?; let body = self.session().http_client().request_body(req).await?; @@ -82,30 +97,33 @@ impl ApResolver { let result = self.try_apresolve().await; self.lock(|inner| { - let data = match result { - Ok(data) => data, - Err(e) => { - warn!("Failed to resolve access points, using fallbacks: {}", e); - ApResolveData::default() - } + let (data, error) = match result { + Ok(data) => (data, None), + Err(e) => (ApResolveData::default(), Some(e)), }; - inner.data.accesspoint = self.process_data(data.accesspoint); - inner.data.dealer = self.process_data(data.dealer); - inner.data.spclient = self.process_data(data.spclient); + inner.data = self.parse_resolve_to_access_points(data); + + if inner.data.is_any_empty() { + warn!("Failed to resolve all access points, using fallbacks"); + if let Some(error) = error { + warn!("Resolve access points error: {}", error); + } + + let fallback = self.parse_resolve_to_access_points(ApResolveData::fallback()); + inner.data.accesspoint.extend(fallback.accesspoint); + inner.data.dealer.extend(fallback.dealer); + inner.data.spclient.extend(fallback.spclient); + } }) } - fn is_empty(&self) -> bool { - self.lock(|inner| { - inner.data.accesspoint.is_empty() - || inner.data.dealer.is_empty() - || inner.data.spclient.is_empty() - }) + fn is_any_empty(&self) -> bool { + self.lock(|inner| inner.data.is_any_empty()) } pub async fn resolve(&self, endpoint: &str) -> Result { - if self.is_empty() { + if self.is_any_empty() { self.apresolve().await; } @@ -114,9 +132,9 @@ impl ApResolver { // take the first position instead of the last with `pop`, because Spotify returns // access points with ports 4070, 443 and 80 in order of preference from highest // to lowest. - "accesspoint" => inner.data.accesspoint.remove(0), - "dealer" => inner.data.dealer.remove(0), - "spclient" => inner.data.spclient.remove(0), + "accesspoint" => inner.data.accesspoint.pop_front(), + "dealer" => inner.data.dealer.pop_front(), + "spclient" => inner.data.spclient.pop_front(), _ => { return Err(Error::unimplemented(format!( "No implementation to resolve access point {}", @@ -125,6 +143,13 @@ impl ApResolver { } }; + let access_point = access_point.ok_or_else(|| { + Error::unavailable(format!( + "No access point available for endpoint {}", + endpoint + )) + })?; + Ok(access_point) }) } diff --git a/src/main.rs b/src/main.rs index 0aaa613b..e1af4c10 100644 --- a/src/main.rs +++ b/src/main.rs @@ -561,7 +561,7 @@ fn get_setup() -> Setup { .optopt( AP_PORT_SHORT, AP_PORT, - "Connect to an AP with a specified port 1 - 65535. If no AP with that port is present a fallback AP will be used. Available ports are usually 80, 443 and 4070.", + "Connect to an AP with a specified port 1 - 65535. Available ports are usually 80, 443 and 4070.", "PORT", );