Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Bug] StorageReferenceInternal::GetFile path argument is not copied #1570

Open
solodon4 opened this issue Apr 18, 2024 · 4 comments
Open

[Bug] StorageReferenceInternal::GetFile path argument is not copied #1570

solodon4 opened this issue Apr 18, 2024 · 4 comments
Assignees
@jonsimantov
Labels
api: storage new New issue. type: question

Comments

@solodon4
Copy link

[REQUIRED] Please fill in the following fields:

  • Pre-built SDK from the website
  • Firebase C++ SDK version: 11.9.0
  • Problematic Firebase Component: Storage
  • Other Firebase Components in use: Auth, App, Analytics, Remote Config, Dynamic Links
  • Platform you are using the C++ SDK on: Mac
  • Platform you are targeting: iOS

[REQUIRED] Please describe the issue here:

The argument path of StorageReferenceInternal::GetFile is not copied and passed verbatim as a pointer to another thread in:

download_task = [my_impl writeToFile:[NSURL URLWithString:@(path)] completion:completion];

This would not work if path was passed from a C++ temporary and the expectation that the caller must ensure the lifetime of path supersedes the download time is unrealistic. Most developers would expect path to be copied if you need to carry it to another thread.

Stack trace during EXC_BREAKPOINT (code=1, subcode=0x1bc5fa4b0) as it processes garbage after temporary:

#0	0x00000001bc5fa4b0 in static URL._unconditionallyBridgeFromObjectiveC(_:) ()
#1	0x00000001064caf5c in @objc StorageReference.write(toFile:completion:) ()
#2	0x0000000101c6622c in invocation function for block in firebase::storage::internal::StorageReferenceInternal::GetFile(char const*, firebase::storage::Listener*, firebase::storage::Controller*) at /Users/me/Projects/firebase-cpp-sdk/storage/src/ios/storage_reference_ios.mm:149
#3	0x00000001067ac08c in _dispatch_call_block_and_release ()
#4	0x00000001067ad8c4 in _dispatch_client_callout ()
#5	0x00000001067bc428 in _dispatch_main_queue_drain ()
#6	0x00000001067bc080 in _dispatch_main_queue_callback_4CF ()
#7	0x00000001c2013828 in __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ ()
#8	0x00000001c1ff74a8 in __CFRunLoopRun ()
#9	0x00000001c1ffbb58 in CFRunLoopRunSpecific ()
#10	0x00000001f81fb984 in GSEventRunModal ()
#11	0x00000001c43a6628 in -[UIApplication _run] ()
#12	0x00000001c43a62a0 in UIApplicationMain ()
#13	0x0000000100ea697c in main at /Users/me/path/to/main.m:24
#14	0x00000001decc5df0 in start ()
Enqueued from com.apple.NSXPCConnection.m-user.com.apple.ondemandd.client (Thread 5) Queue : com.apple.NSXPCConnection.m-user.com.apple.ondemandd.client (serial)
#0	0x00000001067b1dc0 in dispatch_async ()
#1	0x0000000101cb8708 in firebase::util::DispatchAsyncSafeMainQueue(void () block_pointer) ()
#2	0x0000000101c65c58 in firebase::storage::internal::StorageReferenceInternal::GetFile(char const*, firebase::storage::Listener*, firebase::storage::Controller*) at /Users/me/Projects/firebase-cpp-sdk/storage/src/ios/storage_reference_ios.mm:145
#3	0x0000000101c5af6c in firebase::storage::StorageReference::GetFile(char const*, firebase::storage::Listener*, firebase::storage::Controller*) at /Users/me/Projects/firebase-cpp-sdk/storage/src/common/storage_reference.cc:150
#4	0x0000000100b35fa0 in start_download(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&,std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) at my/path/myfile.cpp:3967

Steps to reproduce:

No. Your Firebase C++ quickstarts is useless in reproducing real-world issues as it busy-waits after each call, hiding all the multi-threading issues you guys have. This is just one of them, more to follow. Rewrite it to include multi-threading scenarios.

What's the issue repro rate? 100%

Just use the following function to reproduce the problem:

Relevant Code:

void start_download(const std::string& local_path, const std::string& remote_path)
{
    firebase::storage::StorageReference path_reference = firebase::storage::Storage::GetInstance(::firebase::App::GetInstance())->GetReference(remote_path);
    path_reference.GetFile(("file://" + local_path).c_str(), listener.get(), &listener->m_controller); // Temporary ("file://" + local_path) is destroyed after ;
}
@solodon4
Copy link
Author

Never mind, Objective-C blocks seem to handle that differently. The actual error I was getting was that local file url was not url encoded (e.g. it had spaces in the path).

I'm still getting an error upon second download of the same file, which seems to be in Firebase code not specifying file:// when deleting previous temporary:

2024-04-18 00:33:49.256306-0700 Metal-App-mobile[6336:956788] CFURLCopyResourcePropertyForKey failed because it was passed a URL which has no scheme
2024-04-18 00:33:49.258400-0700 Metal-App-mobile[6336:956788] Could not remove previous file at /private/var/mobile/Containers/Data/Application/058E5B75-6B15-441E-8487-4BA8A38108B4/tmp/CFNetworkDownload_OZ4ZxN.tmp due to Error Domain=NSCocoaErrorDomain Code=262 "The file couldn’t be opened because the specified URL type isn’t supported." UserInfo={NSURL=}
2024-04-18 00:33:49.260673-0700 Metal-App-mobile[6336:956788] *** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '*** -[NSFileManager createDirectoryAtURL:withIntermediateDirectories:attributes:error:]: URL is nil'

@solodon4
Copy link
Author

solodon4 commented Apr 18, 2024 •
edited

Alright, I'd let you guys decide whether this is an actual bug or not as I start getting crashes again and I see garbage in path pointer every time that happens. I don't know Objective-C well enough to vouch whether its runtime makes a proper copy of C-string inside its block. Here is additionally a slightly different call stack from the recent EXC_BREAKPOINT I see, which seem to point to the fact it happens due to processing of garbage in the path string:

#0	0x00000001bc5fa4b0 in static URL._unconditionallyBridgeFromObjectiveC(_:) ()
#1	0x0000000106906f5c in @objc StorageReference.write(toFile:completion:) ()
#2	0x00000001020f122c in invocation function for block in firebase::storage::internal::StorageReferenceInternal::GetFile(char const*, firebase::storage::Listener*, firebase::storage::Controller*) at /Users/me/Projects/firebase-cpp-sdk/storage/src/ios/storage_reference_ios.mm:149
#3	0x0000000106be808c in _dispatch_call_block_and_release ()
#4	0x0000000106be98c4 in _dispatch_client_callout ()
#5	0x0000000106bf8428 in _dispatch_main_queue_drain ()
#6	0x0000000106bf8080 in _dispatch_main_queue_callback_4CF ()
#7	0x00000001c2013828 in __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ ()
#8	0x00000001c1ff74a8 in __CFRunLoopRun ()
#9	0x00000001c1ffbb58 in CFRunLoopRunSpecific ()
#10	0x00000001f81fb984 in GSEventRunModal ()
#11	0x00000001c43a6628 in -[UIApplication _run] ()
#12	0x00000001c43a62a0 in UIApplicationMain ()
#13	0x0000000101330f44 in main at /Users/me/path/to/main.m:24
#14	0x00000001decc5df0 in start ()
Enqueued from com.apple.NSXPCConnection.m-user.com.apple.ondemandd.client (Thread 27) Queue : com.apple.NSXPCConnection.m-user.com.apple.ondemandd.client (serial)
#0	0x0000000106beddc0 in dispatch_async ()
#1	0x0000000102143708 in firebase::util::DispatchAsyncSafeMainQueue(void () block_pointer) ()
#2	0x00000001020f0c58 in firebase::storage::internal::StorageReferenceInternal::GetFile(char const*, firebase::storage::Listener*, firebase::storage::Controller*) at /Users/me/Projects/firebase-cpp-sdk/storage/src/ios/storage_reference_ios.mm:145
#3	0x00000001020e5f6c in firebase::storage::StorageReference::GetFile(char const*, firebase::storage::Listener*, firebase::storage::Controller*) at /Users/me/Projects/firebase-cpp-sdk/storage/src/common/storage_reference.cc:150

I probably was just lucky a few times when crash didn't happen last night and I closed this bug. Reopening for you to decide.

P.S. All the Firebase component come from 11.9.0 downloaded from the website except for libfirebase_storage.a, which I built with debug information from commit ecc41f353a8ebc72efd7788a56337f6726b8d3ff of your main.

@solodon4 solodon4 reopened this Apr 18, 2024
@jonsimantov
Copy link
Contributor

Looks like this actually is a bug if your C++ code is running outside of the main (UI) thread:

firebase-cpp-sdk/storage/src/ios/storage_reference_ios.mm

Line 149 in e877b22

download_task = [my_impl writeToFile:[NSURL URLWithString:@(path)] completion:completion];

This code uses the path that was passed in, but if GetFile was called from a secondary thread, then this is dispatched to the main thread and the function will have returned already.

Shouldn't be too difficult a fix, thanks for raising the issue.

@jonsimantov jonsimantov self-assigned this Apr 19, 2024
@jonsimantov jonsimantov mentioned this issue Apr 19, 2024
Merged
3 tasks
@solodon4
Copy link
Author

Thank you, @jonsimantov! That actually explains why I didn't see the crash a few times leading me to close the bug. To repro the issue, I commented out other code that tries using On-Demand Resources before the Firebase Storage is used as a back-up. That caused the actual call to happen on the main thread. When I restored the original logic, the call happens from a callback on a different thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jonsimantov jonsimantov

Labels
api: storage new New issue. type: question
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@solodon4 @jonsimantov @google-oss-bot