Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trace: handle key being freed while suspended. #8198

Conversation

rustyrussell
Copy link
Contributor

This happens with autoclean, which does a datastore request then frees the parent command without waiting for a response (see clean_finished).

This leaks a trace, and causes a crash if the pointer is later reused.

My solution is to create a trace variant which declares the trace key to be a tal ptr and then we can clean up in the destructor if this happens. This fixes the issue for me.

@rustyrussell rustyrussell added this to the v25.02.1 milestone Apr 2, 2025
@rustyrussell rustyrussell requested a review from cdecker April 2, 2025 20:37
@endothermicdev
Copy link
Collaborator

Looks great, just needs a changelog entry. I guess we should check for outstanding traces in the blackbox test teardown as well in case we encounter any more trace leaks.

This happens with autoclean, which does a datastore request then frees
the parent command without waiting for a response (see clean_finished).

This leaks a trace, and causes a crash if the pointer is later reused.

My solution is to create a trace variant which declares the trace key
to be a tal ptr and then we can clean up in the destructor if this happens.
This fixes the issue for me.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: autoclean: fixed occasional crash when tracepoints compiled in.
@rustyrussell rustyrussell force-pushed the fix-autoclean-trace-crash branch from 5c128be to 52887a2 Compare April 2, 2025 23:21
Copy link
Collaborator

@endothermicdev endothermicdev left a comment

Choose a reason for hiding this comment

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

ACK 52887a2

@endothermicdev endothermicdev merged commit 2e6ad3f into ElementsProject:master Apr 3, 2025
40 checks passed
This was referenced Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants