Page MenuHome

Fix T93310: Crash due to broken image paths.
ClosedPublic

Authored by Jacques Lucke (JacquesLucke) on Nov 25 2021, 1:25 PM.

Details

Summary

The crash was caused either by running out of memory or because malloc returned null, because the requested buffer size was too large. The large buffer size was requested because uninitialized memory was used as canvas rectangle.
It's hard to say if I found all missing initializations, or if some of the initializations aren't actually needed. I only have statistical evidence for the correctness of the patch: in blender-v3.0-release it crashed about every third time I tried in a release build, with this patch I didn't have a crash yet.

Some determine_canvas functions don't seem to initialize the output r_area but read from r_area. E.g. NodeOperationOutput::determine_canvas. The proper fix is probably to make sure that r_area is set everywhere, but initializing the variable at the declaration felt more safe for 3.0.

Reproducing the bug in a debug build was a bit more tricky, probably because more often memory is initialized to zero already.

Diff Detail

Repository
rB Blender
Branch
compositor-canvas-crash (branched from master)
Build Status
Buildable 18967
Build 18967: arc lint + arc unit

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.Nov 25 2021, 1:25 PM
Jacques Lucke (JacquesLucke) created this revision.
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)

Yes as you said I think NodeOperationOutput::determine_canvas and NodeOperation::determine_canvas should ensure r_area is set to 0 by default before using it.

1diff --git a/source/blender/compositor/intern/COM_NodeOperation.cc b/source/blender/compositor/intern/COM_NodeOperation.cc
2index 8a7ae1f4fcb..ab27da5aee8 100644
3--- a/source/blender/compositor/intern/COM_NodeOperation.cc
4+++ b/source/blender/compositor/intern/COM_NodeOperation.cc
5@@ -128,6 +128,8 @@ void NodeOperation::add_output_socket(DataType datatype)
6
7 void NodeOperation::determine_canvas(const rcti &preferred_area, rcti &r_area)
8 {
9+ r_area = COM_AREA_NONE;
10+
11 unsigned int used_canvas_index = 0;
12 if (canvas_input_index_ == RESOLUTION_INPUT_ANY) {
13 for (NodeOperationInput &input : inputs_) {
14@@ -484,6 +486,8 @@ NodeOperationOutput::NodeOperationOutput(NodeOperation *op, DataType datatype)
15
16 void NodeOperationOutput::determine_canvas(const rcti &preferred_area, rcti &r_area)
17 {
18+ r_area = COM_AREA_NONE;
19+
20 NodeOperation &operation = get_operation();
21 if (operation.get_flags().is_canvas_set) {
22 r_area = operation.get_canvas();

I feel more safe adding those lines to your patch, it should cover most operations if not all. I don't think it could cause any issue, checked it.
Still if you think is safer for 3.0 not to do it, please do not.

This revision is now accepted and ready to land.Nov 26 2021, 12:48 AM