search

Azure / azure-storage-cpp

Microsoft Azure Storage Client Library for C++
http://azure.github.io/azure-storage-cpp
Apache License 2.0
131 stars 147 forks source link

Crash due to update of properties not being synchronized #373

Open dcristoloveanu opened 3 years ago

dcristoloveanu commented 3 years ago

Hi!

While using the Azure Storage SDK for C++ in our microservice in the Azure Messaging team, we noticed that the the update of properties done as a result of parsing the response for download_single_range_to_stream_async is not thread safe.

This results in crashes if multiple calls to download_single_range_to_stream_async are made for the same blob. image Callstack is below.

I guess the updating and accessing properties should be behind a RW lock in order for this to work. A fix in the SDK would be awesome of course. Until then is there a workaround that we could have while still being able to have multiple parallel calls to download_single_range_to_stream_async on the same blob?

I can also provide a user mode dump of the crash. Let me know if you need more details on how to repro the problem.

Thanks, /Dan

ucrtbased.dll!free_dbg_nolock(void * const block, const int block_use) Line 904 C++
ucrtbased.dll!_free_dbg(void * block, int block_use) Line 1030  C++
azure_storage_emulator_int_exe.exe!operator delete(void * block) Line 38    C++
azure_storage_emulator_int_exe.exe!operator delete(void * block, unsigned __int64 __formal) Line 32 C++
azure_storage_emulator_int_exe.exe!std::_Deallocate<16,0>(void * _Ptr, unsigned __int64 _Bytes) Line 221    C++
    azure_storage_emulator_int_exe.exe!std::allocator<std::_List_unchecked_iterator<std::_List_val<std::_List_simple_types<std::pair<std::wstring const ,std::wstring>>>>>::deallocate(std::_List_unchecked_iterator<std::_List_val<std::_List_simple_types<std::pair<std::wstring const ,std::wstring>>>> * const _Ptr, const unsigned __int64 _Count) Line 811    C++
    azure_storage_emulator_int_exe.exe!std::vector<std::_List_unchecked_iterator<std::_List_val<std::_List_simple_types<std::pair<std::wstring const ,std::wstring>>>>,std::allocator<std::_List_unchecked_iterator<std::_List_val<std::_List_simple_types<std::pair<std::wstring const ,std::wstring>>>>>>::_Tidy() Line 1918  C++
    azure_storage_emulator_int_exe.exe!std::vector<std::_List_unchecked_iterator<std::_List_val<std::_List_simple_types<std::pair<std::wstring const ,std::wstring>>>>,std::allocator<std::_List_unchecked_iterator<std::_List_val<std::_List_simple_types<std::pair<std::wstring const ,std::wstring>>>>>>::operator=(std::vector<std::_List_unchecked_iterator<std::_List_val<std::_List_simple_types<std::pair<std::wstring const ,std::wstring>>>>,std::allocator<std::_List_unchecked_iterator<std::_List_val<std::_List_simple_types<std::pair<std::wstring const ,std::wstring>>>>>> && _Right) Line 883 C++
    azure_storage_emulator_int_exe.exe!std::_Hash<std::_Umap_traits<std::wstring,std::wstring,std::_Uhash_compare<std::wstring,std::hash<std::wstring>,std::equal_to<std::wstring>>,std::allocator<std::pair<std::wstring const ,std::wstring>>,0>>::operator=(std::_Hash<std::_Umap_traits<std::wstring,std::wstring,std::_Uhash_compare<std::wstring,std::hash<std::wstring>,std::equal_to<std::wstring>>,std::allocator<std::pair<std::wstring const ,std::wstring>>,0>> && _Right) Line 286 C++
    azure_storage_emulator_int_exe.exe!std::unordered_map<std::wstring,std::wstring,std::hash<std::wstring>,std::equal_to<std::wstring>,std::allocator<std::pair<std::wstring const ,std::wstring>>>::operator=(std::unordered_map<std::wstring,std::wstring,std::hash<std::wstring>,std::equal_to<std::wstring>,std::allocator<std::pair<std::wstring const ,std::wstring>>> && _Right) Line 283   C++
>   azure_storage_emulator_int_exe.exe!azure::storage::cloud_blob::download_single_range_to_stream_async::__l2::<lambda>(const web::http::http_response & response, const azure::storage::request_result & result, azure::storage::operation_context context) Line 600  C++
    azure_storage_emulator_int_exe.exe!std::_Invoker_functor::_Call<void <lambda>(const web::http::http_response &, const azure::storage::request_result &, azure::storage::operation_context) &,web::http::http_response const &,azure::storage::request_result const &,azure::storage::operation_context>(azure::storage::cloud_blob::download_single_range_to_stream_async::__l2::void <lambda>(const web::http::http_response &, const azure::storage::request_result &, azure::storage::operation_context) & _Obj, const web::http::http_response & <_Args_0>, const azure::storage::request_result & <_Args_1>, azure::storage::operation_context && <_Args_2>)   C++
    azure_storage_emulator_int_exe.exe!std::invoke<void <lambda>(const web::http::http_response &, const azure::storage::request_result &, azure::storage::operation_context) &,web::http::http_response const &,azure::storage::request_result const &,azure::storage::operation_context>(azure::storage::cloud_blob::download_single_range_to_stream_async::__l2::void <lambda>(const web::http::http_response &, const azure::storage::request_result &, azure::storage::operation_context) & _Obj, const web::http::http_response & <_Args_0>, const azure::storage::request_result & <_Args_1>, azure::storage::operation_context && <_Args_2>)    C++
    azure_storage_emulator_int_exe.exe!std::_Invoker_ret<void,1>::_Call<void <lambda>(const web::http::http_response &, const azure::storage::request_result &, azure::storage::operation_context) &,web::http::http_response const &,azure::storage::request_result const &,azure::storage::operation_context>(azure::storage::cloud_blob::download_single_range_to_stream_async::__l2::void <lambda>(const web::http::http_response &, const azure::storage::request_result &, azure::storage::operation_context) & <_Vals_0>, const web::http::http_response & <_Vals_1>, const azure::storage::request_result & <_Vals_2>, azure::storage::operation_context && <_Vals_3>)  C++
    azure_storage_emulator_int_exe.exe!std::_Func_impl_no_alloc<void <lambda>(const web::http::http_response &, const azure::storage::request_result &, azure::storage::operation_context),void,web::http::http_response const &,azure::storage::request_result const &,azure::storage::operation_context>::_Do_call(const web::http::http_response & <_Args_0>, const azure::storage::request_result & <_Args_1>, azure::storage::operation_context && <_Args_2>)  C++
    azure_storage_emulator_int_exe.exe!std::_Func_class<void,web::http::http_response const &,azure::storage::request_result const &,azure::storage::operation_context>::operator()(const web::http::http_response & <_Args_0>, const azure::storage::request_result & <_Args_1>, azure::storage::operation_context <_Args_2>)  C++
    azure_storage_emulator_int_exe.exe!azure::storage::core::storage_command<void>::preprocess_response(const web::http::http_response & response, const azure::storage::request_result & result, azure::storage::operation_context context) Line 380   C++
    azure_storage_emulator_int_exe.exe!azure::storage::core::executor_impl::execute_async::__l2::Concurrency::task<bool> <lambda>(void)::__l2::<lambda>(Concurrency::task<web::http::http_response> get_headers_task) Line 196  C++
    azure_storage_emulator_int_exe.exe!std::_Invoker_functor::_Call<Concurrency::task<web::http::http_response> <lambda>(Concurrency::task<web::http::http_response>) &,Concurrency::task<web::http::http_response>>(azure::storage::core::executor_impl::execute_async::__l2::Concurrency::task<bool> <lambda>(void)::__l2::Concurrency::task<web::http::http_response> <lambda>(Concurrency::task<web::http::http_response>) & _Obj, Concurrency::task<web::http::http_response> && <_Args_0>)    C++
    azure_storage_emulator_int_exe.exe!std::invoke<Concurrency::task<web::http::http_response> <lambda>(Concurrency::task<web::http::http_response>) &,Concurrency::task<web::http::http_response>>(azure::storage::core::executor_impl::execute_async::__l2::Concurrency::task<bool> <lambda>(void)::__l2::Concurrency::task<web::http::http_response> <lambda>(Concurrency::task<web::http::http_response>) & _Obj, Concurrency::task<web::http::http_response> && <_Args_0>) C++
    azure_storage_emulator_int_exe.exe!std::_Invoker_ret<Concurrency::task<web::http::http_response>,0>::_Call<Concurrency::task<web::http::http_response> <lambda>(Concurrency::task<web::http::http_response>) &,Concurrency::task<web::http::http_response>>(azure::storage::core::executor_impl::execute_async::__l2::Concurrency::task<bool> <lambda>(void)::__l2::Concurrency::task<web::http::http_response> <lambda>(Concurrency::task<web::http::http_response>) & <_Vals_0>, Concurrency::task<web::http::http_response> && <_Vals_1>)    C++
    azure_storage_emulator_int_exe.exe!std::_Func_impl_no_alloc<Concurrency::task<web::http::http_response> <lambda>(Concurrency::task<web::http::http_response>),Concurrency::task<web::http::http_response>,Concurrency::task<web::http::http_response>>::_Do_call(Concurrency::task<web::http::http_response> && <_Args_0>)  C++
    azure_storage_emulator_int_exe.exe!std::_Func_class<Concurrency::task<web::http::http_response>,Concurrency::task<web::http::http_response>>::operator()(Concurrency::task<web::http::http_response> <_Args_0>) C++
    azure_storage_emulator_int_exe.exe!Concurrency::task<web::http::http_response>::_ContinuationTaskHandle<web::http::http_response,web::http::http_response,std::function<Concurrency::task<web::http::http_response> __cdecl(Concurrency::task<web::http::http_response>)>,std::integral_constant<bool,1>,Concurrency::details::_TypeSelectorAsyncTask>::_LogWorkItemAndInvokeUserLambda<std::function<Concurrency::task<web::http::http_response> __cdecl(Concurrency::task<web::http::http_response>)> const &,Concurrency::task<web::http::http_response>>(const std::function<Concurrency::task<web::http::http_response> __cdecl(Concurrency::task<web::http::http_response>)> & _func, Concurrency::task<web::http::http_response> && _value) Line 3627    C++
    azure_storage_emulator_int_exe.exe!Concurrency::task<web::http::http_response>::_ContinuationTaskHandle<web::http::http_response,web::http::http_response,std::function<Concurrency::task<web::http::http_response> __cdecl(Concurrency::task<web::http::http_response>)>,std::integral_constant<bool,1>,Concurrency::details::_TypeSelectorAsyncTask>::_Continue(std::integral_constant<bool,1> __formal, Concurrency::details::_TypeSelectorAsyncOperationOrTask __formal) Line 3760  C++
    azure_storage_emulator_int_exe.exe!Concurrency::task<web::http::http_response>::_ContinuationTaskHandle<web::http::http_response,web::http::http_response,std::function<Concurrency::task<web::http::http_response> __cdecl(Concurrency::task<web::http::http_response>)>,std::integral_constant<bool,1>,Concurrency::details::_TypeSelectorAsyncTask>::_Perform() Line 3633    C++
    azure_storage_emulator_int_exe.exe!Concurrency::details::_PPLTaskHandle<web::http::http_response,Concurrency::task<web::http::http_response>::_ContinuationTaskHandle<web::http::http_response,web::http::http_response,std::function<Concurrency::task<web::http::http_response> __cdecl(Concurrency::task<web::http::http_response>)>,std::integral_constant<bool,1>,Concurrency::details::_TypeSelectorAsyncTask>,Concurrency::details::_ContinuationTaskHandleBase>::invoke() Line 1465 C++
    azure_storage_emulator_int_exe.exe!Concurrency::details::_TaskProcHandle::_RunChoreBridge(void * _Parameter) Line 158   C++
    azure_storage_emulator_int_exe.exe!Concurrency::details::_DefaultPPLTaskScheduler::_PPLTaskChore::_Callback(void * _Args) Line 51   C++
    msvcp140d.dll!Concurrency::details::`anonymous namespace'::_Task_scheduler_callback(_TP_CALLBACK_INSTANCE * _Pci, void * _Args, _TP_WORK * __formal) Line 142   C++
    ntdll.dll!TppWorkpExecuteCallback() Unknown
    ntdll.dll!TppWorkerThread() Unknown
    kernel32.dll!BaseThreadInitThunk() Unknown
    ntdll.dll!RtlUserThreadStart() Unknown
    [Async Call]    
    azure_storage_emulator_int_exe.exe!azure::storage::core::executor_impl::execute_async::__l2::<lambda>() Line 163    C++
    azure_storage_emulator_int_exe.exe!Concurrency::details::_do_while<Concurrency::task<bool> <lambda>(void),bool>(azure::storage::core::executor_impl::execute_async::__l2::Concurrency::task<bool> <lambda>(void) func) Line 36  C++
    azure_storage_emulator_int_exe.exe!azure::storage::core::executor_impl::execute_async(std::shared_ptr<azure::storage::core::storage_command_base> command, const azure::storage::request_options & options, azure::storage::operation_context context) Line 33  C++
    azure_storage_emulator_int_exe.exe!azure::storage::core::executor<void>::execute_async(std::shared_ptr<azure::storage::core::storage_command<void>> command, const azure::storage::request_options & options, azure::storage::operation_context context) Line 597   C++
    azure_storage_emulator_int_exe.exe!azure::storage::cloud_blob::download_single_range_to_stream_async(Concurrency::streams::basic_ostream<unsigned char> target, unsigned __int64 offset, unsigned __int64 length, const azure::storage::access_condition & condition, const azure::storage::blob_request_options & options, azure::storage::operation_context context, bool update_properties, const Concurrency::cancellation_token & cancellation_token, std::shared_ptr<azure::storage::core::timer_handler> timer_handler) Line 646 C++
    azure_storage_emulator_int_exe.exe!azure::storage::cloud_blob::download_range_to_stream_async(Concurrency::streams::basic_ostream<unsigned char> target, unsigned __int64 offset, unsigned __int64 length, const azure::storage::access_condition & condition, const azure::storage::blob_request_options & options, azure::storage::operation_context context, const Concurrency::cancellation_token & cancellation_token) Line 894    C++
    azure_storage_emulator_int_exe.exe!azure::storage::cloud_blob::download_range_to_stream_async(Concurrency::streams::basic_ostream<unsigned char> target, unsigned __int64 offset, unsigned __int64 length, const azure::storage::access_condition & condition, const azure::storage::blob_request_options & options, azure::storage::operation_context context) Line 5315   C++
    azure_storage_emulator_int_exe.exe!azure::storage::cloud_blob::download_range_to_stream_async(Concurrency::streams::basic_ostream<unsigned char> target, __int64 offset, __int64 length) Line 5300  C++
    azure_storage_emulator_int_exe.exe!azure_storage_cloud_block_blob_download_range_to_stream_async::__l14::<lambda>() Line 677    C++
    azure_storage_emulator_int_exe.exe!try_call_no_throw<int <lambda>(void)>(azure_storage_cloud_block_blob_download_range_to_stream_async::__l14::int <lambda>(void) f) Line 28    C++
Jinming-Hu commented 3 years ago

Hi @dcristoloveanu we understand the issue you reported. I think this is a defect or unwise design that blob_client is not thread-safe. As a mitigation, I suggest you make a copy of blob_client for each async operation.

dcristoloveanu commented 3 years ago

Hi @Jinming-Hu, yes, this is what we do today as a workaround. Do you see an easy fix for this issue so that we can get a clean solution where we can reuse the same blob_client?

It looks to me that the solution would be to add a RW lock over the properties class.

Thanks, /Dan

Jinming-Hu commented 3 years ago

Nah, I don't think this would be good. In my opinion, to modify the client itself in an async operation is bad-design. Adding mutex is just going further in the wrong path. I'd rather leave it undefined behavior instead.

dcristoloveanu commented 3 years ago

Hi @Jinming-Hu

But, but, but, I think undefined behavior without being documented is evil for the developers :-). Why? Luckily we found this in a test and not in production, but it took us some couple of days to identify which is the test out of the test suite that is crashing since call stacks were not always pointing in the right direction as this is a memory corruption.

Now we know about it and we won't pursue using the multiple async calls on the cloud_blob_client in parallel, but I think a line in the function class description XML doc could save a lot of headaches!

If you agree, let me know if you want me to submit a quick PR updating the XML doc for that class.

Thanks, /Dan LE: I understand the argument about this not being designed to support this and you probably do not want to invest in significant design changes on this SDK as the vNext is already available.

Jinming-Hu commented 3 years ago

Yeah, I agree that adding some docs here will be very helpful to users and save them from a lot of trouble.

DavidBerg-MSFT commented 3 years ago

If it's too hard to make the code thread safe, can it be make to throw an error if used in a non-thread safe manner? (Like WPF does if you try to modify a UI component from a thread other than the one it was created on.)

Jinming-Hu commented 3 years ago

If it's too hard to make the code thread safe, can it be make to throw an error if used in a non-thread safe manner? (Like WPF does if you try to modify a UI component from a thread other than the one it was created on.)

It's not possible. The user may maintain a mutex himself and lock the mutex when accessing the client from more than one threads. Storage SDK doesn't even know the existence of that mutex, thus cannot know if the client is being accessed in a thread-safe way or non-thread-safe way.