Guard reqwest::proxy libtests against concurrent ENV modification
As ENV is process global, modifying it within a thread (as is normal for all test targets in a rust libtest) results in a concurrency data-race. This patch fences the two known cases of needing to modify this by locking all ENV modifications, and collection of data dependent on said modifications, into a narrow path isolated by a Mutex lock, with no test assert!()'s while the Mutex is held ( to avoid a Mutex Posioning ). However, the code doesn't treat lock failure as a special circumstance, and if the lock fails, then the pre-existing risk of conccurent ENV modification returns, and these 2 tests can still randomly fail, but _in that situation_. And as mutexes can _only_ be poisoned by the 2 threads holding this mutex, this regression can now only trip into concurrency issues when either of these 2 tests are already failing from _non test_ assertions, so this patch still improves the status quo substantially. Closes: https://github.com/seanmonstar/reqwest/issues/829
This commit is contained in:
		
				
					committed by
					
						 Sean McArthur
						Sean McArthur
					
				
			
			
				
	
			
			
			
						parent
						
							b42d2e0e0c
						
					
				
				
					commit
					6a41459862
				
			
							
								
								
									
										60
									
								
								src/proxy.rs
									
									
									
									
									
								
							
							
						
						
									
										60
									
								
								src/proxy.rs
									
									
									
									
									
								
							| @@ -691,6 +691,8 @@ fn get_from_registry() -> SystemProxyMap { | |||||||
| #[cfg(test)] | #[cfg(test)] | ||||||
| mod tests { | mod tests { | ||||||
|     use super::*; |     use super::*; | ||||||
|  |     use std::sync::Mutex; | ||||||
|  |     use lazy_static::lazy_static; | ||||||
|  |  | ||||||
|     impl Dst for Url { |     impl Dst for Url { | ||||||
|         fn scheme(&self) -> &str { |         fn scheme(&self) -> &str { | ||||||
| @@ -799,47 +801,75 @@ mod tests { | |||||||
|         } |         } | ||||||
|     } |     } | ||||||
|  |  | ||||||
|  |     // Smallest possible content for a mutex | ||||||
|  |     struct MutexInner; | ||||||
|  |  | ||||||
|  |     lazy_static! { | ||||||
|  |         static ref ENVLOCK: Mutex<MutexInner> = { | ||||||
|  |             Mutex::new(MutexInner) | ||||||
|  |         }; | ||||||
|  |     } | ||||||
|  |  | ||||||
|     #[test] |     #[test] | ||||||
|     fn test_get_sys_proxies_parsing() { |     fn test_get_sys_proxies_parsing() { | ||||||
|  |         // Stop other threads from modifying process-global ENV while we are. | ||||||
|  |         let _lock = ENVLOCK.lock(); | ||||||
|         // save system setting first. |         // save system setting first. | ||||||
|         let _g1 = env_guard("HTTP_PROXY"); |         let _g1 = env_guard("HTTP_PROXY"); | ||||||
|         let _g2 = env_guard("http_proxy"); |         let _g2 = env_guard("http_proxy"); | ||||||
|  |  | ||||||
|         // empty |         // Mock ENV, get the results, before doing assertions | ||||||
|         assert_eq!(get_sys_proxies().contains_key("http"), false); |         // to avoid assert! -> panic! -> Mutex Poisoned. | ||||||
|  |         let baseline_proxies = get_sys_proxies(); | ||||||
|         // the system proxy setting url is invalid. |         // the system proxy setting url is invalid. | ||||||
|         env::set_var("http_proxy", "123465"); |         env::set_var("http_proxy", "123465"); | ||||||
|         assert_eq!(get_sys_proxies().contains_key("http"), false); |         let invalid_proxies = get_sys_proxies(); | ||||||
|  |  | ||||||
|         // set valid proxy |         // set valid proxy | ||||||
|         env::set_var("http_proxy", "http://127.0.0.1/"); |         env::set_var("http_proxy", "http://127.0.0.1/"); | ||||||
|         let proxies = get_sys_proxies(); |         let valid_proxies = get_sys_proxies(); | ||||||
|  |  | ||||||
|         let p = &proxies["http"]; |  | ||||||
|         assert_eq!(p.scheme(), "http"); |  | ||||||
|         assert_eq!(p.host(), "127.0.0.1"); |  | ||||||
|  |  | ||||||
|         // reset user setting when guards drop |         // reset user setting when guards drop | ||||||
|  |         drop(_g1); | ||||||
|  |         drop(_g2); | ||||||
|  |         // Let other threads run now | ||||||
|  |         drop(_lock); | ||||||
|  |  | ||||||
|  |         assert_eq!(baseline_proxies.contains_key("http"), false); | ||||||
|  |         assert_eq!(invalid_proxies.contains_key("http"), false); | ||||||
|  |  | ||||||
|  |         let p = &valid_proxies["http"]; | ||||||
|  |         assert_eq!(p.scheme(), "http"); | ||||||
|  |         assert_eq!(p.host(), "127.0.0.1"); | ||||||
|     } |     } | ||||||
|  |  | ||||||
|     #[test] |     #[test] | ||||||
|     fn test_get_sys_proxies_in_cgi() { |     fn test_get_sys_proxies_in_cgi() { | ||||||
|  |         // Stop other threads from modifying process-global ENV while we are. | ||||||
|  |         let _lock = ENVLOCK.lock(); | ||||||
|         // save system setting first. |         // save system setting first. | ||||||
|         let _g1 = env_guard("REQUEST_METHOD"); |         let _g1 = env_guard("REQUEST_METHOD"); | ||||||
|         let _g2 = env_guard("HTTP_PROXY"); |         let _g2 = env_guard("HTTP_PROXY"); | ||||||
|  |  | ||||||
|  |         // Mock ENV, get the results, before doing assertions | ||||||
|  |         // to avoid assert! -> panic! -> Mutex Poisoned. | ||||||
|         env::set_var("HTTP_PROXY", "http://evil/"); |         env::set_var("HTTP_PROXY", "http://evil/"); | ||||||
|  |  | ||||||
|         // not in CGI yet |         let baseline_proxies = get_sys_proxies(); | ||||||
|         assert_eq!(get_sys_proxies()["http"].host(), "evil"); |  | ||||||
|  |  | ||||||
|         // set like we're in CGI |         // set like we're in CGI | ||||||
|         env::set_var("REQUEST_METHOD", "GET"); |         env::set_var("REQUEST_METHOD", "GET"); | ||||||
|         assert!(!get_sys_proxies().contains_key("http")); |  | ||||||
|  |         let cgi_proxies = get_sys_proxies(); | ||||||
|  |  | ||||||
|         // reset user setting when guards drop |         // reset user setting when guards drop | ||||||
|  |         drop(_g1); | ||||||
|  |         drop(_g2); | ||||||
|  |         // Let other threads run now | ||||||
|  |         drop(_lock); | ||||||
|  |  | ||||||
|  |         // not in CGI yet | ||||||
|  |         assert_eq!(baseline_proxies["http"].host(), "evil"); | ||||||
|  |         // In CGI | ||||||
|  |         assert!(!cgi_proxies.contains_key("http")); | ||||||
|     } |     } | ||||||
|  |  | ||||||
|     /// Guard an environment variable, resetting it to the original value |     /// Guard an environment variable, resetting it to the original value | ||||||
|   | |||||||
		Reference in New Issue
	
	Block a user