From 493232fa8cabba0e7fd645dd5c6c19456e93461d Mon Sep 17 00:00:00 2001 From: Kimmo Hoikka Date: Tue, 25 Aug 2015 15:31:46 +0100 Subject: [PATCH] Fixed a potential memory corruption due to update and render potentially using the same container simultaneously Fixed by not using double buffering with index but having two separate containers ==5511== ---------------------------------------------------------------- ==5511== Possible data race during read of size 4 at 0x14ECD29C by thread #4 ==5511== Locks held: none ==5511== at 0x55ADFB1: Dali::Internal::SceneGraph::UpdateManager::SaveBinary(Dali::IntrusivePtr) (update-manager.cpp:632) ==5511== by 0x5599DF4: Dali::Internal::ProgramController::StoreBinary(Dali::IntrusivePtr) (program-controller.cpp:151) ==5511== by 0x5598CFD: Dali::Internal::Program::Load() (program.cpp:559) ==5511== by 0x5598DB4: Dali::Internal::Program::Use() (program.cpp:135) ==5511== by 0x559799C: Dali::Internal::SceneGraph::Renderer::Render(Dali::Internal::Context&, Dali::Internal::SceneGraph::TextureCache&, unsigned int, Dali::Internal::SceneGraph::Shader&, Dali::Matrix const&, Dali::Matrix const&, Dali::Matrix const&, float, bool) (scene-graph-renderer.cpp:213) ==5511== by 0x55891B2: Dali::Internal::Render::ProcessRenderInstruction(Dali::Internal::SceneGraph::RenderInstruction const&, Dali::Internal::Context&, Dali::Internal::SceneGraph::TextureCache&, Dali::Internal::SceneGraph::Shader&, unsigned int, float) (render-algorithms.cpp:146) ==5511== by 0x558BDE5: Dali::Internal::SceneGraph::RenderManager::DoRender(Dali::Internal::SceneGraph::RenderInstruction&, Dali::Internal::SceneGraph::Shader&, float) (render-manager.cpp:558) ==5511== by 0x558C423: Dali::Internal::SceneGraph::RenderManager::Render(Dali::Integration::RenderStatus&) (render-manager.cpp:430) ==5511== by 0x551D25C: Dali::Internal::Core::Render(Dali::Integration::RenderStatus&) (core-impl.cpp:278) ==5511== by 0x4E72B04: Dali::Internal::Adaptor::RenderThread::Run() (render-thread.cpp:199) ==5511== by 0x4E72B58: Dali::Internal::Adaptor::RenderThread::InternalThreadEntryFunc(void*) (render-thread.h:197) ==5511== by 0x4C30E26: ??? (in /usr/lib/valgrind/vgpreload_helgrind-amd64-linux.so) ==5511== ==5511== This conflicts with a previous write of size 4 by thread #3 ==5511== Locks held: none ==5511== at 0x559F1EF: Dali::Internal::SceneGraph::SceneGraphBuffers::Swap() (scene-graph-buffers.cpp:45) ==5511== by 0x55ADADA: Dali::Internal::SceneGraph::UpdateManager::Update(float, unsigned int, unsigned int) (update-manager.cpp:1121) ==5511== by 0x551D229: Dali::Internal::Core::Update(float, unsigned int, unsigned int, Dali::Integration::UpdateStatus&) (core-impl.cpp:261) ==5511== by 0x4E74438: Dali::Internal::Adaptor::UpdateThread::Run() (update-thread.cpp:119) ==5511== by 0x4E744A8: Dali::Internal::Adaptor::UpdateThread::InternalThreadEntryFunc(void*) (update-thread.h:107) ==5511== by 0x4C30E26: ??? (in /usr/lib/valgrind/vgpreload_helgrind-amd64-linux.so) ==5511== by 0x908A181: start_thread (pthread_create.c:312) ==5511== by 0x5E8EFBC: clone (clone.S:111) ==5511== ==5511== Address 0x14ECD29C is 12 bytes inside a block of size 24 alloc'd ==5511== at 0x4C2C460: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_helgrind-amd64-linux.so) ==5511== by 0x551D745: Dali::Internal::Core::Core(Dali::Integration::RenderController&, Dali::Integration::PlatformAbstraction&, Dali::Integration::GlAbstraction&, Dali::Integration::GlSyncAbstraction&, Dali::Integration::GestureManager&, Dali::ResourcePolicy::DataRetention) (core-impl.cpp:149) ==5511== by 0x55BB824: Dali::Integration::Core::New(Dali::Integration::RenderController&, Dali::Integration::PlatformAbstraction&, Dali::Integration::GlAbstraction&, Dali::Integration::GlSyncAbstraction&, Dali::Integration::GestureManager&, Dali::ResourcePolicy::DataRetention) (core.cpp:37) ==5511== by 0x4E9E9E8: Dali::Internal::Adaptor::Adaptor::Initialize(Dali::Configuration::ContextLoss) (adaptor-impl.cpp:139) ==5511== by 0x4E9FC3C: Dali::Internal::Adaptor::Adaptor::New(Dali::Any, Dali::RenderSurface*, Dali::Configuration::ContextLoss, Dali::Internal::Adaptor::EnvironmentOptions*) (adaptor-impl.cpp:79) ==5511== by 0x4E9FD05: Dali::Internal::Adaptor::Adaptor::New(Dali::Window, Dali::Configuration::ContextLoss, Dali::Internal::Adaptor::EnvironmentOptions*) (adaptor-impl.cpp:89) ==5511== by 0x4EA22BB: Dali::Internal::Adaptor::Application::CreateAdaptor() (application-impl.cpp:134) ==5511== by 0x4EA2407: Dali::Internal::Adaptor::Application::OnInit() (application-impl.cpp:175) ==5511== by 0x4EB8B88: Dali::Internal::Adaptor::Framework::AppStatusHandler(int, void*) (framework-tizen.cpp:380) ==5511== by 0x7D6BC9D: app_appcore_create (app_main.c:149) ==5511== by 0x7F6E819: appcore_efl_main (appcore-efl.c:16) ==5511== by 0x7D6BDEC: app_efl_main (app_main.c:116) ==5511== ---------------------------------------------------------------- Change-Id: I1bdcfd8c00d9f37b0dd1f3f580f1bd69d4e0a20b --- dali/internal/update/manager/update-manager.cpp | 36 +++++++++++++++++-------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/dali/internal/update/manager/update-manager.cpp b/dali/internal/update/manager/update-manager.cpp index 0288a02..516fa93 100644 --- a/dali/internal/update/manager/update-manager.cpp +++ b/dali/internal/update/manager/update-manager.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2014 Samsung Electronics Co., Ltd. + * Copyright (c) 2015 Samsung Electronics Co., Ltd. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -21,6 +21,7 @@ // INTERNAL INCLUDES #include #include +#include #include #include @@ -131,8 +132,7 @@ typedef OwnerContainer< Shader* > ShaderContainer; typedef ShaderContainer::Iterator ShaderIter; typedef ShaderContainer::ConstIterator ShaderConstIter; -typedef std::vector ShaderDataBatchedQueue; -typedef ShaderDataBatchedQueue::iterator ShaderDataBatchedQueueIterator; +typedef std::vector ShaderDataBinaryQueue; typedef OwnerContainer GestureContainer; typedef GestureContainer::Iterator GestureIter; @@ -276,12 +276,14 @@ struct UpdateManager::Impl ObjectOwnerContainer geometries; ///< A container of geometries ObjectOwnerContainer materials; ///< A container of materials ObjectOwnerContainer samplers; ///< A container of samplers - ObjectOwnerContainer propertyBuffers; ///< A container of property buffers + ObjectOwnerContainer propertyBuffers; ///< A container of property buffers ShaderContainer shaders; ///< A container of owned shaders MessageQueue messageQueue; ///< The messages queued from the event-thread - ShaderDataBatchedQueue compiledShaders[2]; ///< Shaders compiled on Render thread are inserted here for update thread to pass on to event thread. + ShaderDataBinaryQueue renderCompiledShaders; ///< Shaders compiled on Render thread are inserted here for update thread to pass on to event thread. + ShaderDataBinaryQueue updateCompiledShaders; ///< Shaders to be sent from Update to Event + Mutex compiledShaderMutex; ///< lock to ensure no corruption on the renderCompiledShaders float keepRenderingSeconds; ///< Set via Dali::Stage::KeepRendering bool animationFinishedDuringUpdate; ///< Flag whether any animations finished during the Update() @@ -629,7 +631,11 @@ void UpdateManager::SaveBinary( Internal::ShaderDataPtr shaderData ) { DALI_ASSERT_DEBUG( shaderData && "No NULL shader data pointers please." ); DALI_ASSERT_DEBUG( shaderData->GetBufferSize() > 0 && "Shader binary empty so nothing to save." ); - mImpl->compiledShaders[mSceneGraphBuffers.GetUpdateBufferIndex() > 0 ? 0 : 1].push_back( shaderData ); + { + // lock as update might be sending previously compiled shaders to event thread + Mutex::ScopedLock lock( mImpl->compiledShaderMutex ); + mImpl->renderCompiledShaders.push_back( shaderData ); + } } RenderTaskList* UpdateManager::GetRenderTaskList( bool systemLevel ) @@ -919,16 +925,24 @@ void UpdateManager::ForwardCompiledShadersToEventThread() DALI_ASSERT_DEBUG( (mImpl->shaderSaver != 0) && "shaderSaver should be wired-up during startup." ); if( mImpl->shaderSaver ) { - ShaderDataBatchedQueue& compiledShaders = mImpl->compiledShaders[mSceneGraphBuffers.GetUpdateBufferIndex()]; - if( compiledShaders.size() > 0 ) + // lock and swap the queues + { + // render might be attempting to send us more binaries at the same time + Mutex::ScopedLock lock( mImpl->compiledShaderMutex ); + mImpl->renderCompiledShaders.swap( mImpl->updateCompiledShaders ); + } + + if( mImpl->updateCompiledShaders.size() > 0 ) { ShaderSaver& factory = *mImpl->shaderSaver; - ShaderDataBatchedQueueIterator i = compiledShaders.begin(); - ShaderDataBatchedQueueIterator end = compiledShaders.end(); + ShaderDataBinaryQueue::iterator i = mImpl->updateCompiledShaders.begin(); + ShaderDataBinaryQueue::iterator end = mImpl->updateCompiledShaders.end(); for( ; i != end; ++i ) { mImpl->notificationManager.QueueMessage( ShaderCompiledMessage( factory, *i ) ); } + // we don't need them in update anymore + mImpl->updateCompiledShaders.clear(); } } } @@ -1009,7 +1023,7 @@ unsigned int UpdateManager::Update( float elapsedSeconds, // 6) Post Process Ids of resources updated by renderer mImpl->resourceManager.PostProcessResources( bufferIndex ); - // 6.1) Forward a batch of compiled shader programs to event thread for saving + // 6.1) Forward compiled shader programs to event thread for saving ForwardCompiledShadersToEventThread(); // Although the scene-graph may not require an update, we still need to synchronize double-buffered -- 2.7.4