search

appleseedhq / appleseed

A modern open source rendering engine for animation and visual effects
https://appleseedhq.net/
MIT License
2.21k stars 331 forks source link

Fix race condition in localsampleaccumulationbuffer.cpp #2898

Closed joris-nijs closed 4 years ago

joris-nijs commented 4 years ago

The m_active_level atomic is modified in store_samples outside the critical section protected by m_lock. Ensure the same level is used as source and target level.

oktomus commented 4 years ago

Thanks for your contribution @jnijs ! Can you tell me more about the bug, what happened ? A crash, assert failure, something else ?

joris-nijs commented 4 years ago

The asserts in Tile::copy_from were triggered because the level and read_level tile are of different dimensions when the m_active_level atomic gets modified in between getting the current level tile from the 2 arrays. There are never crashes since the target tile will always >= the source tile.

oktomus commented 4 years ago

Hi @jnijs, there is still an error at compile time. https://travis-ci.org/github/appleseedhq/appleseed/jobs/739207799#L1879

/home/travis/build/appleseedhq/appleseed/src/appleseed/renderer/kernel/rendering/localsampleaccumulationbuffer.cpp:260:55: error: ‘class boost::atomics::atomic<unsigned int>’ has no member named ‘value’
     const std::uint32_t active_level = m_active_level.value(); // Ensure the same active_level is used for both arrays.
                                                       ^~~~~