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

[solodon4]

[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]

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]

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.

[jonsimantov]

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.

[solodon4]

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.

[solodon4]

I can confirm the issue is resolved in 12.0.0