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 (alphablend): Fix blending and associated crashes due to buffer overruns #383

Merged
merged 4 commits into from
Jan 25, 2025

Conversation

kaixiong
Copy link
Member

The alphablend morph uses the blending routines visual_alpha_blend_nn() wrongly. It passes the pixel buffer size in bytes instead of pixel count for the size parameter. Since buffer size is always equal or greater than pixel count (bytes per pixel >= 1 and row padding >= 0), this causes buffer overruns.

@kaixiong kaixiong requested a review from hartwork January 21, 2025 17:35
@kaixiong kaixiong self-assigned this Jan 21, 2025
Copy link
Member

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

@kaixiong two small things:

uint8_t *dest_row_ptr = visual_video_get_pixels (dest);

uint8_t alpha = progress * 255;
BlendFunc blend_func = get_blend_func (depth);
Copy link
Member

Choose a reason for hiding this comment

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

get_blend_func can return NULL. If we keep allowing it to, we likely need a check for NULL somewhere around here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The default case is unreachable and largely included to silence compiler warnings about enum values unaccounted for (e.g. VISUAL_VIDEO_DEPTH_NONE and VISUAL_DEPTH_NONE_ALL).

We don't have unreachable() in C99 or C11 so I returned a NULL instead. Would you prefer abort()?

Copy link
Member

@hartwork hartwork Jan 22, 2025

Choose a reason for hiding this comment

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

@kaixiong I would favor some flavor of abort or assert — something with a message —, yes, maybe using libvisual's log_and_exit? (Maybe we need one more check for that for lv_checks.h.)

Copy link
Member Author

Choose a reason for hiding this comment

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

@hartwork I've added a critical log message followed by abort().

I spent some time thinking about the general approach of a portable unreachable() function that defaults to log-and-abort in debug builds... but I've ultimately concluded that for now, we should keep the change small for this PR 😄

@kaixiong kaixiong force-pushed the fix-alphablend-blending branch from f587e7b to bc7775b Compare January 25, 2025 19:23
@kaixiong kaixiong force-pushed the fix-alphablend-blending branch from bc7775b to aba8a84 Compare January 25, 2025 19:37
@kaixiong kaixiong requested a review from hartwork January 25, 2025 19:50
@kaixiong kaixiong merged commit 4f8c4ac into master Jan 25, 2025
6 checks passed
@kaixiong kaixiong deleted the fix-alphablend-blending branch January 25, 2025 23:26
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