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

Plugins (Blursk): Fix double free when Blursk is loaded multiple times. #382

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

kaixiong
Copy link
Member

Blursk crashes in __blursk_cleanup when the plugin is loaded and unloaded multiple times. This can occur when lv-tool cycles through actors and completes a full rotation.

The bug is due to the sharing and re-use of the variable config across instances.

config contains multiple strings allocated during plugin instance initialization in config_default():

if(conf->color_style)
    visual_mem_free(conf->color_style);
conf->color_style = visual_strdup(config_default_color_style);
if(conf->signal_color)
    visual_mem_free(conf->signal_color);
conf->signal_color = visual_strdup(config_default_signal_color);

These strings are then freed in __blursk_cleanup() when the plugin instance is cleaned up.

visual_mem_free(config.color_style);
visual_mem_free(config.signal_color);

After freeing the strings in the first instance, these pointer values remain unchanged and the next run of config_default() crashes the program.

The solution is to unshare the config struct but I ran into a header inclusion cycle with my attempt. Breaking the cycle requires a fair amount of refactoring, so I decided to instead zero out the config struct to complete the clean-up.

We can come back to this after a more thorough review of the plugin system.

@kaixiong kaixiong requested a review from hartwork January 21, 2025 17:05
@kaixiong kaixiong self-assigned this Jan 21, 2025
@kaixiong kaixiong merged commit a8c959a into master Jan 22, 2025
6 checks passed
@kaixiong kaixiong deleted the fix-blursk-double-free branch January 22, 2025 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants