diff --git a/components/net/fetch/methods.rs b/components/net/fetch/methods.rs index 1584597da87..7d641090649 100644 --- a/components/net/fetch/methods.rs +++ b/components/net/fetch/methods.rs @@ -18,7 +18,7 @@ use hyper::StatusCode; use ipc_channel::ipc::IpcReceiver; use mime::{self, Mime}; use net_traits::blob_url_store::{parse_blob_url, BlobURLStoreError}; -use net_traits::filemanager_thread::RelativePos; +use net_traits::filemanager_thread::{FileTokenCheck, RelativePos}; use net_traits::request::{ is_cors_safelisted_method, is_cors_safelisted_request_header, Origin, ResponseTainting, Window, }; @@ -56,6 +56,7 @@ pub struct FetchContext { pub user_agent: Cow<'static, str>, pub devtools_chan: Option>, pub filemanager: FileManager, + pub file_token: FileTokenCheck, pub cancellation_listener: Arc>, pub timing: ServoArc>, } @@ -756,12 +757,12 @@ fn scheme_fetch( let (done_sender, done_receiver) = unbounded(); *done_chan = Some((done_sender.clone(), done_receiver)); *response.body.lock().unwrap() = ResponseBody::Receiving(vec![]); - let check_url_validity = true; + if let Err(err) = context.filemanager.fetch_file( &done_sender, context.cancellation_listener.clone(), id, - check_url_validity, + &context.file_token, origin, &mut response, range, diff --git a/components/net/filemanager_thread.rs b/components/net/filemanager_thread.rs index 2e6ce832394..14321311e59 100644 --- a/components/net/filemanager_thread.rs +++ b/components/net/filemanager_thread.rs @@ -11,14 +11,16 @@ use http::header::{self, HeaderValue}; use ipc_channel::ipc::{self, IpcSender}; use mime::{self, Mime}; use net_traits::blob_url_store::{BlobBuf, BlobURLStoreError}; -use net_traits::filemanager_thread::{FileManagerResult, FileManagerThreadMsg, FileOrigin}; +use net_traits::filemanager_thread::{ + FileManagerResult, FileManagerThreadMsg, FileOrigin, FileTokenCheck, +}; use net_traits::filemanager_thread::{ FileManagerThreadError, ReadFileProgress, RelativePos, SelectedFile, }; use net_traits::http_percent_encode; use net_traits::response::{Response, ResponseBody}; use servo_arc::Arc as ServoArc; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fs::File; use std::io::{BufRead, BufReader, Read, Seek, SeekFrom}; use std::mem; @@ -46,6 +48,9 @@ struct FileStoreEntry { /// by the user with createObjectURL. Validity can be revoked as well. /// (The UUID is the one that maps to this entry in `FileManagerStore`) is_valid_url: AtomicBool, + /// UUIDs of fetch instances that acquired an interest in this file, + /// when the url was still valid. + outstanding_tokens: HashSet, } #[derive(Clone)] @@ -91,7 +96,6 @@ impl FileManager { &self, sender: IpcSender>, id: Uuid, - check_url_validity: bool, origin: FileOrigin, ) { let store = self.store.clone(); @@ -99,7 +103,7 @@ impl FileManager { .upgrade() .and_then(|pool| { pool.spawn(move || { - if let Err(e) = store.try_read_file(&sender, id, check_url_validity, origin) { + if let Err(e) = store.try_read_file(&sender, id, origin) { let _ = sender.send(Err(FileManagerThreadError::BlobURLStoreError(e))); } }); @@ -110,6 +114,14 @@ impl FileManager { }); } + pub fn get_token_for_file(&self, file_id: &Uuid) -> FileTokenCheck { + self.store.get_token_for_file(file_id) + } + + pub fn invalidate_token(&self, token: &FileTokenCheck, file_id: &Uuid) { + self.store.invalidate_token(token, file_id); + } + // Read a file for the Fetch implementation. // It gets the required headers synchronously and reads the actual content // in a separate thread. @@ -118,7 +130,7 @@ impl FileManager { done_sender: &Sender, cancellation_listener: Arc>, id: Uuid, - check_url_validity: bool, + file_token: &FileTokenCheck, origin: FileOrigin, response: &mut Response, range: RangeRequestBounds, @@ -127,9 +139,9 @@ impl FileManager { done_sender, cancellation_listener, &id, + file_token, &origin, range, - check_url_validity, response, ) } @@ -175,8 +187,8 @@ impl FileManager { ); }); }, - FileManagerThreadMsg::ReadFile(sender, id, check_url_validity, origin) => { - self.read_file(sender, id, check_url_validity, origin); + FileManagerThreadMsg::ReadFile(sender, id, origin) => { + self.read_file(sender, id, origin); }, FileManagerThreadMsg::PromoteMemory(id, blob_buf, set_valid, origin) => { self.promote_memory(id, blob_buf, set_valid, origin); @@ -273,12 +285,12 @@ impl FileManager { done_sender: &Sender, cancellation_listener: Arc>, id: &Uuid, + file_token: &FileTokenCheck, origin_in: &FileOrigin, range: RangeRequestBounds, - check_url_validity: bool, response: &mut Response, ) -> Result<(), BlobURLStoreError> { - let file_impl = self.store.get_impl(id, origin_in, check_url_validity)?; + let file_impl = self.store.get_impl(id, file_token, origin_in)?; match file_impl { FileImpl::Memory(buf) => { let range = match range.get_final(Some(buf.size)) { @@ -362,11 +374,11 @@ impl FileManager { done_sender, cancellation_listener, &parent_id, + file_token, origin_in, RangeRequestBounds::Final( RelativePos::full_range().slice_inner(&inner_rel_pos), ), - false, response, ); }, @@ -392,19 +404,23 @@ impl FileManagerStore { pub fn get_impl( &self, id: &Uuid, + file_token: &FileTokenCheck, origin_in: &FileOrigin, - check_url_validity: bool, ) -> Result { match self.entries.read().unwrap().get(id) { Some(ref entry) => { if *origin_in != *entry.origin { Err(BlobURLStoreError::InvalidOrigin) } else { - let is_valid = entry.is_valid_url.load(Ordering::Acquire); - if check_url_validity && !is_valid { - Err(BlobURLStoreError::InvalidFileID) - } else { - Ok(entry.file_impl.clone()) + match file_token { + FileTokenCheck::NotRequired => Ok(entry.file_impl.clone()), + FileTokenCheck::Required(token) => { + if entry.outstanding_tokens.contains(token) { + return Ok(entry.file_impl.clone()); + } + Err(BlobURLStoreError::InvalidFileID) + }, + FileTokenCheck::ShouldFail => Err(BlobURLStoreError::InvalidFileID), } } }, @@ -412,6 +428,58 @@ impl FileManagerStore { } } + pub fn invalidate_token(&self, token: &FileTokenCheck, file_id: &Uuid) { + if let FileTokenCheck::Required(token) = token { + let mut entries = self.entries.write().unwrap(); + if let Some(entry) = entries.get_mut(file_id) { + entry.outstanding_tokens.remove(token); + + // Check if there are references left. + let zero_refs = entry.refs.load(Ordering::Acquire) == 0; + + // Check if no other fetch has acquired a token for this file. + let no_outstanding_tokens = entry.outstanding_tokens.len() == 0; + + // Check if there is still a blob URL outstanding. + let valid = entry.is_valid_url.load(Ordering::Acquire); + + // Can we remove this file? + let do_remove = zero_refs && no_outstanding_tokens && !valid; + + if do_remove { + entries.remove(&file_id); + } + } + } + } + + pub fn get_token_for_file(&self, file_id: &Uuid) -> FileTokenCheck { + let mut entries = self.entries.write().unwrap(); + let parent_id = match entries.get(file_id) { + Some(entry) => { + if let FileImpl::Sliced(ref parent_id, _) = entry.file_impl { + Some(parent_id.clone()) + } else { + None + } + }, + None => return FileTokenCheck::ShouldFail, + }; + let file_id = match parent_id.as_ref() { + Some(id) => id, + None => file_id, + }; + if let Some(entry) = entries.get_mut(file_id) { + if !entry.is_valid_url.load(Ordering::Acquire) { + return FileTokenCheck::ShouldFail; + } + let token = Uuid::new_v4(); + entry.outstanding_tokens.insert(token.clone()); + return FileTokenCheck::Required(token); + } + FileTokenCheck::ShouldFail + } + fn insert(&self, id: Uuid, entry: FileStoreEntry) { self.entries.write().unwrap().insert(id, entry); } @@ -453,6 +521,7 @@ impl FileManagerStore { // Valid here since AddSlicedURLEntry implies URL creation // from a BlobImpl::Sliced is_valid_url: AtomicBool::new(true), + outstanding_tokens: Default::default(), }, ); @@ -604,6 +673,7 @@ impl FileManagerStore { refs: AtomicUsize::new(1), // Invalid here since create_entry is called by file selection is_valid_url: AtomicBool::new(false), + outstanding_tokens: Default::default(), }, ); @@ -626,11 +696,11 @@ impl FileManagerStore { &self, sender: &IpcSender>, id: &Uuid, + file_token: &FileTokenCheck, origin_in: &FileOrigin, rel_pos: RelativePos, - check_url_validity: bool, ) -> Result<(), BlobURLStoreError> { - let file_impl = self.get_impl(id, origin_in, check_url_validity)?; + let file_impl = self.get_impl(id, file_token, origin_in)?; match file_impl { FileImpl::Memory(buf) => { let range = rel_pos.to_abs_range(buf.size as usize); @@ -686,9 +756,9 @@ impl FileManagerStore { self.get_blob_buf( sender, &parent_id, + file_token, origin_in, rel_pos.slice_inner(&inner_rel_pos), - false, ) }, } @@ -699,15 +769,14 @@ impl FileManagerStore { &self, sender: &IpcSender>, id: Uuid, - check_url_validity: bool, origin_in: FileOrigin, ) -> Result<(), BlobURLStoreError> { self.get_blob_buf( sender, &id, + &FileTokenCheck::NotRequired, &origin_in, RelativePos::full_range(), - check_url_validity, ) } @@ -724,10 +793,17 @@ impl FileManagerStore { // last reference, and if it has a reference to parent id // dec_ref on parent later if necessary let is_valid = entry.is_valid_url.load(Ordering::Acquire); + + // Check if no fetch has acquired a token for this file. + let no_outstanding_tokens = entry.outstanding_tokens.len() == 0; + + // Can we remove this file? + let do_remove = !is_valid && no_outstanding_tokens; + if let FileImpl::Sliced(ref parent_id, _) = entry.file_impl { - (!is_valid, Some(parent_id.clone())) + (do_remove, Some(parent_id.clone())) } else { - (!is_valid, None) + (do_remove, None) } } } else { @@ -762,6 +838,7 @@ impl FileManagerStore { file_impl: FileImpl::Memory(blob_buf), refs: AtomicUsize::new(1), is_valid_url: AtomicBool::new(set_valid), + outstanding_tokens: Default::default(), }, ); }, @@ -786,10 +863,16 @@ impl FileManagerStore { // and store entry id holders let zero_refs = entry.refs.load(Ordering::Acquire) == 0; + // Check if no fetch has acquired a token for this file. + let no_outstanding_tokens = entry.outstanding_tokens.len() == 0; + + // Can we remove this file? + let do_remove = zero_refs && no_outstanding_tokens; + if let FileImpl::Sliced(ref parent_id, _) = entry.file_impl { - (zero_refs, Some(parent_id.clone()), Ok(())) + (do_remove, Some(parent_id.clone()), Ok(())) } else { - (zero_refs, None, Ok(())) + (do_remove, None, Ok(())) } } else { (false, None, Ok(())) diff --git a/components/net/resource_thread.rs b/components/net/resource_thread.rs index 0bf17e7570f..79f84b65971 100644 --- a/components/net/resource_thread.rs +++ b/components/net/resource_thread.rs @@ -22,6 +22,8 @@ use embedder_traits::EmbedderProxy; use hyper_serde::Serde; use ipc_channel::ipc::{self, IpcReceiver, IpcReceiverSet, IpcSender}; use malloc_size_of::{MallocSizeOf, MallocSizeOfOps}; +use net_traits::blob_url_store::parse_blob_url; +use net_traits::filemanager_thread::FileTokenCheck; use net_traits::request::{Destination, RequestBuilder}; use net_traits::response::{Response, ResponseInit}; use net_traits::storage_thread::StorageThreadMsg; @@ -621,8 +623,31 @@ impl CoreResourceManager { _ => ResourceTimingType::Resource, }; + let mut request = request_builder.build(); + let url = request.current_url(); + + // In the case of a valid blob URL, acquiring a token granting access to a file, + // regardless if the URL is revoked after token acquisition. + // + // TODO: to make more tests pass, acquire this token earlier, + // probably in a separate message flow. + // + // In such a setup, the token would not be acquired here, + // but could instead be contained in the actual CoreResourceMsg::Fetch message. + // + // See https://github.com/servo/servo/issues/25226 + let (file_token, blob_url_file_id) = match url.scheme() { + "blob" => { + if let Ok((id, _)) = parse_blob_url(&url) { + (self.filemanager.get_token_for_file(&id), Some(id)) + } else { + (FileTokenCheck::ShouldFail, None) + } + }, + _ => (FileTokenCheck::NotRequired, None), + }; + self.thread_pool.spawn(move || { - let mut request = request_builder.build(); // XXXManishearth: Check origin against pipeline id (also ensure that the mode is allowed) // todo load context / mimesniff in fetch // todo referrer policy? @@ -632,6 +657,7 @@ impl CoreResourceManager { user_agent: ua, devtools_chan: dc, filemanager: filemanager, + file_token, cancellation_listener: Arc::new(Mutex::new(CancellationListener::new(cancel_chan))), timing: ServoArc::new(Mutex::new(ResourceFetchTiming::new(request.timing_type()))), }; @@ -651,6 +677,13 @@ impl CoreResourceManager { }, None => fetch(&mut request, &mut sender, &context), }; + + // Remove token after fetch. + if let Some(id) = blob_url_file_id.as_ref() { + context + .filemanager + .invalidate_token(&context.file_token, id); + } }); } diff --git a/components/net/tests/fetch.rs b/components/net/tests/fetch.rs index b936c70f3a5..4d338ce6391 100644 --- a/components/net/tests/fetch.rs +++ b/components/net/tests/fetch.rs @@ -29,6 +29,7 @@ use net::filemanager_thread::FileManager; use net::hsts::HstsEntry; use net::resource_thread::CoreResourceThreadPool; use net::test::HttpState; +use net_traits::filemanager_thread::FileTokenCheck; use net_traits::request::{ Destination, Origin, RedirectMode, Referrer, Request, RequestBuilder, RequestMode, }; @@ -683,6 +684,7 @@ fn test_fetch_with_hsts() { user_agent: DEFAULT_USER_AGENT.into(), devtools_chan: None, filemanager: FileManager::new(create_embedder_proxy(), Weak::new()), + file_token: FileTokenCheck::NotRequired, cancellation_listener: Arc::new(Mutex::new(CancellationListener::new(None))), timing: ServoArc::new(Mutex::new(ResourceFetchTiming::new( ResourceTimingType::Navigation, @@ -735,6 +737,7 @@ fn test_load_adds_host_to_hsts_list_when_url_is_https() { user_agent: DEFAULT_USER_AGENT.into(), devtools_chan: None, filemanager: FileManager::new(create_embedder_proxy(), Weak::new()), + file_token: FileTokenCheck::NotRequired, cancellation_listener: Arc::new(Mutex::new(CancellationListener::new(None))), timing: ServoArc::new(Mutex::new(ResourceFetchTiming::new( ResourceTimingType::Navigation, diff --git a/components/net/tests/filemanager_thread.rs b/components/net/tests/filemanager_thread.rs index 9438fcc88ee..3cda5a2f2d1 100644 --- a/components/net/tests/filemanager_thread.rs +++ b/components/net/tests/filemanager_thread.rs @@ -59,7 +59,6 @@ fn test_filemanager() { filemanager.handle(FileManagerThreadMsg::ReadFile( tx2, selected.id.clone(), - false, origin.clone(), )); @@ -113,7 +112,6 @@ fn test_filemanager() { filemanager.handle(FileManagerThreadMsg::ReadFile( tx2, selected.id.clone(), - false, origin.clone(), )); diff --git a/components/net/tests/main.rs b/components/net/tests/main.rs index 727403026de..5ad2c55f728 100644 --- a/components/net/tests/main.rs +++ b/components/net/tests/main.rs @@ -35,6 +35,7 @@ use net::fetch::methods::{self, CancellationListener, FetchContext}; use net::filemanager_thread::FileManager; use net::resource_thread::CoreResourceThreadPool; use net::test::HttpState; +use net_traits::filemanager_thread::FileTokenCheck; use net_traits::request::Request; use net_traits::response::Response; use net_traits::{FetchTaskTarget, ResourceFetchTiming, ResourceTimingType}; @@ -98,6 +99,7 @@ fn new_fetch_context( user_agent: DEFAULT_USER_AGENT.into(), devtools_chan: dc, filemanager: FileManager::new(sender, pool_handle.unwrap_or_else(|| Weak::new())), + file_token: FileTokenCheck::NotRequired, cancellation_listener: Arc::new(Mutex::new(CancellationListener::new(None))), timing: ServoArc::new(Mutex::new(ResourceFetchTiming::new( ResourceTimingType::Navigation, diff --git a/components/net_traits/filemanager_thread.rs b/components/net_traits/filemanager_thread.rs index 4da1da66bd6..e89a282fa39 100644 --- a/components/net_traits/filemanager_thread.rs +++ b/components/net_traits/filemanager_thread.rs @@ -16,6 +16,20 @@ use uuid::Uuid; /// File manager store entry's origin pub type FileOrigin = String; +/// A token modulating access to a file for a blob URL. +pub enum FileTokenCheck { + /// Checking against a token not required, + /// used for accessing a file + /// that isn't linked to from a blob URL. + NotRequired, + /// Checking against token required. + Required(Uuid), + /// Request should always fail, + /// used for cases when a check is required, + /// but no token could be acquired. + ShouldFail, +} + /// Relative slice positions of a sequence, /// whose semantic should be consistent with (start, end) parameters in /// @@ -125,7 +139,6 @@ pub enum FileManagerThreadMsg { ReadFile( IpcSender>, Uuid, - bool, FileOrigin, ), diff --git a/components/script/dom/globalscope.rs b/components/script/dom/globalscope.rs index 2ee7190cfed..d86104c1ee6 100644 --- a/components/script/dom/globalscope.rs +++ b/components/script/dom/globalscope.rs @@ -1675,8 +1675,7 @@ impl GlobalScope { let resource_threads = self.resource_threads(); let (chan, recv) = profile_ipc::channel(self.time_profiler_chan().clone()).unwrap(); let origin = get_blob_origin(&self.get_url()); - let check_url_validity = false; - let msg = FileManagerThreadMsg::ReadFile(chan, id, check_url_validity, origin); + let msg = FileManagerThreadMsg::ReadFile(chan, id, origin); let _ = resource_threads.send(CoreResourceMsg::ToFileManager(msg)); recv } diff --git a/tests/wpt/metadata/FileAPI/url/url-in-tags-revoke.window.js.ini b/tests/wpt/metadata/FileAPI/url/url-in-tags-revoke.window.js.ini index 76b398963ae..dd4ffcf4345 100644 --- a/tests/wpt/metadata/FileAPI/url/url-in-tags-revoke.window.js.ini +++ b/tests/wpt/metadata/FileAPI/url/url-in-tags-revoke.window.js.ini @@ -1,5 +1,4 @@ [url-in-tags-revoke.window.html] - expected: TIMEOUT [Fetching a blob URL immediately before revoking it works in an iframe.] expected: FAIL @@ -15,6 +14,3 @@ [Opening a blob URL in a new window by clicking an tag works immediately before revoking the URL.] expected: FAIL - [Fetching a blob URL immediately before revoking it works in