griefith/test2
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Fix: Non-thread-safe access to image in Compositor

Browse Source 
Image operation's get_im_buf() function was not thread-safe:
- It had TOCTOU issue around calculating multi-layer indices and
  requesting to load the image buffer.
- It accessed render result, render layer and pass pointers without
  any thread guards.

This change moves all the logic needed to access the image buffer
into a single function with proper guards around the access. The
result is user-counted, so it is usable in a thread even if another
thread modifies the image.

The is still potential TOCTOU in the compositor since the image is
acquired twice: once from init_execution(), and once from the
determine_canvas(). It could cause issues if image resolution is
changed between these calls. It is still to be looked into.

Ref #118337, #121761
This commit is contained in:
Sergey Sharybin
2024-05-17 11:06:25 +02:00
committed by Sergey Sharybin
parent fb6b759513
commit 58e0ca99a9
7 changed files with 102 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
source/blender/blenkernel/BKE_image.h
View File

@@ -150,6 +150,25 @@ bool BKE_image_has_ibuf(struct Image *ima, struct ImageUser *iuser);
* References the result, #BKE_image_release_ibuf should be used to de-reference.
*/
struct ImBuf *BKE_image_acquire_ibuf(struct Image *ima, struct ImageUser *iuser, void **r_lock);
/**
* Return image buffer for given image, user, pass, and view.
* Is thread-safe, so another thread can be changing image while this function is executed.
*
* If the image is single-layer then the pass name is completely ignored.
*
* If the image is multi-layer then this function does all needed internal configurations to read
* the pass. There is no need to acquire a temporary ImBuf prior to this call (which is what some
* legacy code had to do to ensure proper type and RenderResult).
*
* References the result, #BKE_image_release_ibuf should be used to de-reference.
*/
ImBuf *BKE_image_acquire_multilayer_view_ibuf(const RenderData &render_data,
Image &image,
const ImageUser &image_user,
const char *pass_name,
const char *view_name);
void BKE_image_release_ibuf(struct Image *ima, struct ImBuf *ibuf, void *lock);
struct ImagePool *BKE_image_pool_new(void);

71
source/blender/blenkernel/intern/image.cc
View File

@@ -4755,6 +4755,77 @@ ImBuf *BKE_image_acquire_ibuf(Image *ima, ImageUser *iuser, void **r_lock)
return ibuf;
}
static int get_multilayer_view_index(const Image &image,
const ImageUser &image_user,
const char *view_name)
{
if (BLI_listbase_count_at_most(&image.rr->views, 2) <= 1) {
return 0;
}
const int view_image = image_user.view;
const bool is_allview = (view_image == 0); /* if view selected == All (0) */
if (is_allview) {
/* Heuristic to match image name with scene names check if the view name exists in the image.
*/
const int view = BLI_findstringindex(&image.rr->views, view_name, offsetof(RenderView, name));
if (view == -1) {
return 0;
}
return view;
}
return view_image - 1;
}
ImBuf *BKE_image_acquire_multilayer_view_ibuf(const RenderData &render_data,
Image &image,
const ImageUser &image_user,
const char *pass_name,
const char *view_name)
{
BLI_mutex_lock(static_cast<ThreadMutex *>(image.runtime.cache_mutex));
/* Local changes to the original ImageUser. */
ImageUser local_user = image_user;
/* Force load the image once, possibly with a different user from what it will need to be in the
* end. This ensures proper image type, and initializes multi-layer state when needed. */
ImBuf *tmp_ibuf = image_acquire_ibuf(&image, &local_user, nullptr);
IMB_freeImBuf(tmp_ibuf);
if (BKE_image_is_multilayer(&image)) {
BLI_assert(pass_name);
if (!image.rr) {
BLI_mutex_unlock(static_cast<ThreadMutex *>(image.runtime.cache_mutex));
return nullptr;
}
const RenderLayer *render_layer = static_cast<const RenderLayer *>(
BLI_findlink(&image.rr->layers, local_user.layer));
local_user.view = get_multilayer_view_index(image, local_user, view_name);
local_user.pass = BLI_findstringindex(
&render_layer->passes, pass_name, offsetof(RenderPass, name));
if (!BKE_image_multilayer_index(image.rr, &local_user)) {
BLI_mutex_unlock(static_cast<ThreadMutex *>(image.runtime.cache_mutex));
return nullptr;
}
}
else {
local_user.multi_index = BKE_scene_multiview_view_id_get(&render_data, view_name);
}
ImBuf *ibuf = image_acquire_ibuf(&image, &local_user, nullptr);
BLI_mutex_unlock(static_cast<ThreadMutex *>(image.runtime.cache_mutex));
return ibuf;
}
void BKE_image_release_ibuf(Image *ima, ImBuf *ibuf, void *lock)
{
if (lock != nullptr) {

1
source/blender/compositor/nodes/COM_CryptomatteNode.cc
View File

@@ -173,6 +173,7 @@ void CryptomatteNode::input_operations_from_image_source(
op->set_image(image);
op->set_image_user(*iuser);
op->set_framenumber(context.get_framenumber());
op->set_render_data(context.get_render_data());
op->set_view_name(context.get_view_name());
op->set_pass_name(render_pass->name);
r_input_operations.append(op);

1
source/blender/compositor/nodes/COM_ImageNode.cc
View File

@@ -45,6 +45,7 @@ NodeOperation *ImageNode::do_multilayer_check(NodeConverter &converter,
operation->set_image(image);
operation->set_image_user(*user);
operation->set_framenumber(framenumber);
operation->set_render_data(context.get_render_data());
operation->set_view_name(context.get_view_name());
operation->set_pass_name(pass_name);

9
source/blender/compositor/operations/COM_ImageOperation.cc
View File

@@ -34,16 +34,11 @@ ImageAlphaOperation::ImageAlphaOperation() : BaseImageOperation()
ImBuf *BaseImageOperation::get_im_buf()
{
if (image_ == nullptr) {
if (rd_ == nullptr || image_ == nullptr) {
return nullptr;
}
/* local changes to the original ImageUser */
if (BKE_image_is_multilayer(image_) == false) {
image_user_.multi_index = BKE_scene_multiview_view_id_get(rd_, view_name_);
}
ImBuf *ibuf = BKE_image_acquire_ibuf(image_, &image_user_, nullptr);
ImBuf *ibuf = BKE_image_acquire_multilayer_view_ibuf(*rd_, *image_, image_user_, "", view_name_);
if (ibuf == nullptr || (ibuf->byte_buffer.data == nullptr && ibuf->float_buffer.data == nullptr))
{
BKE_image_release_ibuf(image_, ibuf, nullptr);

43
source/blender/compositor/operations/COM_MultilayerImageOperation.cc
View File

@@ -10,47 +10,20 @@
namespace blender::compositor {
int MultilayerBaseOperation::get_view_index() const
{
if (BLI_listbase_count_at_most(&image_->rr->views, 2) <= 1) {
return 0;
}
const int view_image = image_user_.view;
const bool is_allview = (view_image == 0); /* if view selected == All (0) */
if (is_allview) {
/* Heuristic to match image name with scene names check if the view name exists in the image.
*/
const int view = BLI_findstringindex(
&image_->rr->views, view_name_, offsetof(RenderView, name));
if (view == -1) {
return 0;
}
return view;
}
return view_image - 1;
}
ImBuf *MultilayerBaseOperation::get_im_buf()
{
if (image_ == nullptr) {
if (rd_ == nullptr || image_ == nullptr) {
return nullptr;
}
const RenderLayer *render_layer = static_cast<const RenderLayer *>(
BLI_findlink(&image_->rr->layers, image_user_.layer));
image_user_.view = get_view_index();
image_user_.pass = BLI_findstringindex(
&render_layer->passes, pass_name_.c_str(), offsetof(RenderPass, name));
if (BKE_image_multilayer_index(image_->rr, &image_user_)) {
return BaseImageOperation::get_im_buf();
ImBuf *ibuf = BKE_image_acquire_multilayer_view_ibuf(
*rd_, *image_, image_user_, pass_name_.c_str(), view_name_);
if (ibuf == nullptr || (ibuf->byte_buffer.data == nullptr && ibuf->float_buffer.data == nullptr))
{
BKE_image_release_ibuf(image_, ibuf, nullptr);
return nullptr;
}
return nullptr;
return ibuf;
}
void MultilayerBaseOperation::update_memory_buffer_partial(MemoryBuffer *output,

3
source/blender/compositor/operations/COM_MultilayerImageOperation.h
View File

@@ -14,9 +14,6 @@ class MultilayerBaseOperation : public BaseImageOperation {
protected:
std::string pass_name_;
/* Returns the image view to use for the current active view. */
int get_view_index() const;
ImBuf *get_im_buf() override;
public: