tree/node: add and use node_finish in destroy functions#9060
tree/node: add and use node_finish in destroy functions#9060llyyr wants to merge 1 commit intoswaywm:masterfrom
Conversation
| // _transaction_commit_dirty call reading freed memory | ||
| int dirty_idx = list_find(server.dirty_nodes, node); | ||
| if (dirty_idx >= 0) { | ||
| list_del(server.dirty_nodes, dirty_idx); |
There was a problem hiding this comment.
I wonder if this is the right spot to remove the node from the dirty list. Maybe it would make more sense to introduce a node_finish() function which fires the destroy event and removes the node from the dirty list?
There was a problem hiding this comment.
We already have *_begin_destroy functions which fire the destroy event, so I should just remove the node from dirty list in output_destroy then.
But I'm not really sure if this is still the core issue, or if the request_modeset in begin_destroy is misplaced. See: #8091 (review)
There was a problem hiding this comment.
The output is removed from the scene and from the output list in begin_destroy(), so the modeset logic will never touch it AFAIU. I don't think moving the request_modeset() call elsewhere will change anything.
llyyr
force-pushed
the
fix-use-after-free-take2
branch
from
f2c615e to
ed1c916
Compare
llyyr
changed the title
| int dirty_idx = list_find(server.dirty_nodes, &output->node); | ||
| if (dirty_idx >= 0) { | ||
| list_del(server.dirty_nodes, dirty_idx); | ||
| } |
There was a problem hiding this comment.
Hm, but this only handles the issue for outputs, and not for other kinds of nodes. That's why I've suggested to introduce a small function to finish nodes instead.
There was a problem hiding this comment.
I've done this but now I'm not sure if we should be setting nodes as dirty when destroying them in the first place.
There was a problem hiding this comment.
Hm, I'm a bit confused because there are asserts right above these new calls asserting that the node isn't part of any transaction… So we're adding the destroying node to a transaction after that assert but before the node is destroyed?
There was a problem hiding this comment.
Hmm I thought I understood what was going on here but indeed I'm not sure I can explain why this commit fixes the problem... I'll try to better understand what is going on here.
Going by the backtrace though it does seem like a race related to the request_modeset timer because apply_output_configs is called from timer_modeset_handle after a previous transaction already freed the output.
llyyr
force-pushed
the
fix-use-after-free-take2
branch
from
ed1c916 to
8307f0a
Compare
We set node as dirty in {output,workspace,container}_begin_destroy
functions but this leaves destroyed pointers in server.dirty_nodes while
destroying. This can cause crashes when waking up the screen after being
turned off for a while, because we request modeset on a 10ms timer when
starting to destroy outputs. This timer could fire while we're in the
middle of destroying the output and make us try to modeset an already
freed output.
See backtrace in swaywm#8969
llyyr
force-pushed
the
fix-use-after-free-take2
branch
from
8307f0a to
9ba2f52
Compare
llyyr
changed the title
2 participants
Otherwise we might try to modset on an output that is already removed. This happens because of the timer from request_modeset call in begin_destroy firing at inconvenient times
Correct fix to #8969