Depsgraph: Fix 'use after free' error in some cases during undo step #106661

Closed

Bastien Montagne wants to merge 1 commits from F-fix-deg-memuse-after-free into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

Bastien Montagne commented

2023-04-07 14:14:45 +02:00

Owner

The root of the issue seems to be that the depsgraph stores the name
parts of its Component and Operation ID keys as string pointers, which
may reffer to some data from the underlying ID.

During undo, to reduce updates and speedup undo steps, a lot of IDs get
'replaced in place', i.e. new data read from the undo memfile is moved
into the existing 'old' ID memory address. And the depsgraph is also
re-used from the old BMain.

Calling DEG_id_tag_update and similar on the ID can then cause
depsgraph code to access freed memory from the 'old' data.

To address this issue, this commit replaces the raw C string pointer by
an std::string for both ComponentIDKey and OperationIDKey name.
This has a performance cost on depsgraph building though, of about 6% to
7%. Depsgraph evaluation remains unchanged.

The root of the issue seems to be that the depsgraph stores the name parts of its Component and Operation ID keys as string pointers, which may reffer to some data from the underlying ID. During undo, to reduce updates and speedup undo steps, a lot of IDs get 'replaced in place', i.e. new data read from the undo memfile is moved into the existing 'old' ID memory address. And the depsgraph is also re-used from the old BMain. Calling `DEG_id_tag_update` and similar on the ID can then cause depsgraph code to access freed memory from the 'old' data. To address this issue, this commit replaces the raw C string pointer by an std::string for both `ComponentIDKey` and `OperationIDKey` name. This has a performance cost on depsgraph building though, of about 6% to 7%. Depsgraph evaluation remains unchanged.

Bastien Montagne added the

Dependency Graph

labels 2023-04-07 14:14:45 +02:00

Bastien Montagne added 1 commit 2023-04-07 14:14:57 +02:00

7ee24e4d86 Depsgraph: Fix 'use after free' error in some cases during undo step.

The root of the issue seems to be that the depsgraph stores the name
parts of its Component and Operation ID keys as string pointers, which
may reffer to some data from the underlying ID.

During undo, to reduce updates and speedup undo steps, a lot of IDs get
'replaced in place', i.e. new data read from the undo memfile is moved
into the existing 'old' ID memory address. And the depsgraph is also
re-used from the old BMain.

Calling `DEG_id_tag_update` and similar on the ID can then cause
depsgraph code to access freed memory from the 'old' data.

To address this issue, this commit replaces the raw C string pointer by
an std::string for both `ComponentIDKey` and `OperationIDKey` name.
This has a performance cost on depsgraph building though, of about 6% to
7%. Depsgraph evaluation remains unchanged.

Bastien Montagne added this to the Core project 2023-04-07 14:15:03 +02:00

Bastien Montagne requested review from Sergey Sharybin 2023-04-07 14:15:14 +02:00

Hans Goudey changed title from ~~Depsgraph: Fix 'use after free' error in some cases during undo step.~~ to Depsgraph: Fix 'use after free' error in some cases during undo step

2023-04-07 14:32:25 +02:00

Sergey Sharybin commented

2023-04-07 14:34:08 +02:00

Owner

The 7% kind of sounds a lot, but the non-dangerous access to the graph nodes probably worth it.

There is a tricky way to avoid it by utilizing the fact that tagging is only interested in the components with the default name (aka, empty) and does not happen for per-bone components. We can modify the storage of such default names (use a string with known pointer, i.e.) and modify the node comparison so that it does not look into the node->name content when the name is expected to be empty (roll-back to pointer comparison in that case). It will avoid having all names heap-allocated, but makes things more tricky from the safety perspective.

Maybe we have some cheaper replacement of the std::string, or maybe there is another trick to solve the problem in a reliable way without a performance penalty that I am missing.

This is something where I'd really like to have second pair of thoughts from @JacquesLucke and @dr.sybren.

From my side I am kind of inclined to say: if this a stopping issue and no quick alternative is found lets go with the proposed solution until a better one is found.

The 7% kind of sounds a lot, but the non-dangerous access to the graph nodes probably worth it. There is a tricky way to avoid it by utilizing the fact that tagging is only interested in the components with the default name (aka, empty) and does not happen for per-bone components. We can modify the storage of such default names (use a string with known pointer, i.e.) and modify the node comparison so that it does not look into the node->name content when the name is expected to be empty (roll-back to pointer comparison in that case). It will avoid having all names heap-allocated, but makes things more tricky from the safety perspective. Maybe we have some cheaper replacement of the `std::string`, or maybe there is another trick to solve the problem in a reliable way without a performance penalty that I am missing. This is something where I'd really like to have second pair of thoughts from @JacquesLucke and @dr.sybren. From my side I am kind of inclined to say: if this a stopping issue and no quick alternative is found lets go with the proposed solution until a better one is found.

Sybren A. Stüvel commented

2023-04-07 14:55:56 +02:00

Member

From my side I am kind of inclined to say: if this a stopping issue and no quick alternative is found lets go with the proposed solution until a better one is found.

I agree.

There could be some optimization done by giving the calling code some more 'say' in how the name is handled. ComponentNode::{find,get,has}_operation() construct a temporary OperationIDKey for which it's fine to reuse the char* for comparisons. It's only when things get stored, like in ComponentNode::add_operation(), where it's important to make a copy.

We could go further and let the OperationIDKey and OperationNode share the same name (via a std::weak_ptr<> or something), but I'm afraid that that'll cause more trouble than it's worth.

> From my side I am kind of inclined to say: if this a stopping issue and no quick alternative is found lets go with the proposed solution until a better one is found. I agree. There could be some optimization done by giving the calling code some more 'say' in how the name is handled. `ComponentNode::{find,get,has}_operation()` construct a temporary `OperationIDKey` for which it's fine to reuse the `char*` for comparisons. It's only when things get stored, like in `ComponentNode::add_operation()`, where it's important to make a copy. We could go further and let the `OperationIDKey` and `OperationNode` share the same name (via a `std::weak_ptr<>` or something), but I'm afraid that that'll cause more trouble than it's worth.

Bastien Montagne commented

2023-04-07 16:01:41 +02:00

Author

Owner

Fixed as better solution (no more performances cost) as ade103d7b8, thanks!

Fixed as better solution (no more performances cost) as ade103d7b8a76dcc1, thanks!

Bastien Montagne closed this pull request

2023-04-07 16:01:42 +02:00

Bastien Montagne removed this from the Core project 2023-07-03 12:47:00 +02:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

3 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#106661

No description provided.