Repository navigation
Conversation
soreau
left a comment
soreau
left a comment
There was a problem hiding this comment.
The idea seems like a good way to fix the problem. Just a few things that need to be addressed. See other comments.
| } | ||
| // Only touch live views: the map may hold entries for views that | ||
| // were destroyed while zoomed, and dereferencing them here is a | ||
| // use-after-free. |
There was a problem hiding this comment.
This comment is gratuitous. We could do without it. It does show that we could do better though, like handling view unmap signal and removing from the transformers list. However going even further, there is probably a way to omit transformers map altogether. This was written a long time ago and the plugin API has changed over time so some of this stuff might be for hysterical reasons.
| // were destroyed while zoomed, and dereferencing them here is a | ||
| // use-after-free. | ||
| remove_transformers(); | ||
| transformers.clear(); |
There was a problem hiding this comment.
The .clear() line could be moved to remove_transformers() function.
| private: | ||
| void pop_transformer(wayfire_view view) | ||
| { | ||
| auto it = transformers.find(view); |
There was a problem hiding this comment.
We do not need to .find() the view here. I would like to see keycolor pop_transformer() function copied.
| return; | ||
| } | ||
|
|
||
| if (it->first->get_transformed_node()->get_transformer(transformer_name)) |
There was a problem hiding this comment.
it->first can be replaced with view.
| it->first->get_transformed_node()->rem_transformer(it->second); | ||
| } | ||
|
|
||
| transformers.erase(it); |
5748ec4 to
779aba3
Compare
|
Addressed in 779aba3:
Re-verified with the ASan repro from the issue (nested wayfire, zoom triggered through update_winzoom()): zoomed window closed, plugin unloaded — no reports, clean fini. |
779aba3 to
0ad3736
Compare
The transformers map keyed on a raw view pointer was only cleaned up in fini(), which dereferenced every key. A zoomed window closed before the plugin was disabled left a dangling pointer, and the later fini() dereferenced it (heap-use-after-free -> segfault, whole session lost; deterministic under AddressSanitizer). The scale->1.0 path also removed the transformer from the node tree while retaining the map entry. Follow the pattern used by keycolor: a transformer_name member, pop_transformer() with a get_transformer() guard, and remove_transformers() which iterates wf::get_core().get_all_views() (live views only) in fini(). The map is then cleared, so fini() never dereferences a map key and no entry outlives the plugin.
|
Fixed uncrustify issues |
2 participants
Fixes #351.
The transformers map keyed on a raw view pointer was only cleaned up in fini(), which dereferenced every key. A zoomed window closed before the plugin was disabled left a dangling pointer; the later fini() dereferenced it (heap-use-after-free, deterministic under AddressSanitizer). The scale->1.0 path also removed the transformer from the node tree while retaining the map entry.
Follows the pattern used by the keycolor plugin: transformer_name member, pop_transformer() with a get_transformer() guard, and remove_transformers() iterating wf::get_core().get_all_views() (live views only) from fini(), after which the map is cleared. fini() no longer dereferences a map key.
Verified with the ASan repro from the issue (nested wayfire on top of a live session, zoom triggered through the plugin's own update_winzoom()): stock build crashes with the reported use-after-free; this build unloads cleanly with no report.