Skip to content

[DF] Fix potential use-after-delete in sample callbacks #11251

Merged
eguiraud merged 2 commits intoroot-project:masterfrom
eguiraud:fix-df-action-callbacks
Aug 25, 2022
Merged

[DF] Fix potential use-after-delete in sample callbacks #11251
eguiraud merged 2 commits intoroot-project:masterfrom
eguiraud:fix-df-action-callbacks

Conversation

@eguiraud
Copy link
Copy Markdown
Contributor

Before this commit, if an action with a sample callback (currently
only Snapshot) or a DefinePerSample went out of scope, we did not
deregister the corresponding callbacks from the RLoopManager, which
would try to run them anyway during the following event loop.
As callbacks typically perform a call on the original action or
define objects, this could cause a use-after-delete.

With this patch, we now associate each callback to the address of
the corresponding node of the computation graph and remove callbacks
when the nodes go out of scope.

This fixes #11222.

Before this commit, if an action with a sample callback (currently
only Snapshot) or a DefinePerSample went out of scope, we did not
deregister the corresponding callbacks from the RLoopManager, which
would try to run them anyway during the following event loop.
As callbacks typically perform a call on the original action or
define objects, this could cause a use-after-delete.

With this patch, we now associate each callback to the address of
the corresponding node of the computation graph and remove callbacks
when the nodes go out of scope.

This fixes root-project#11222.
@eguiraud eguiraud requested a review from vepadulano August 25, 2022 07:17
@eguiraud eguiraud self-assigned this Aug 25, 2022
@phsft-bot
Copy link
Copy Markdown

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

Copy link
Copy Markdown
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, and fixes gtest-tree-dataframe-test-dataframe-snapshot (cf #11222). The new test also passes with an AddressSanitizer build 👍

@phsft-bot
Copy link
Copy Markdown

Build failed on mac1015/cxx17.
Running on macitois21.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@eguiraud eguiraud merged commit 0b49293 into root-project:master Aug 25, 2022
@eguiraud eguiraud deleted the fix-df-action-callbacks branch August 25, 2022 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[DF] gtest-tree-dataframe-test-dataframe-snapshot fails with AddressSanitizer

4 participants