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

align commands in the command buffer on 4 bytes #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

remjey
Copy link

@remjey remjey commented Dec 1, 2020

This fixes a crash when microui is used on my 32-bit ARM tablet.
The crash happens when assigning a structure variable directly to a field of an unaligned command union in the command stack.

More specifically, my situation was that I was trying to display a window with a label inside. the window alone would show just fine, but as soon as I added the label inside it, the program crashed.

I managed to narrow down the crash to line 500 of microui.c which happened while adding the label:

cmd->text.pos = pos;

cmd->text.pos was not aligned because the space occupied by the previous text command (for the window title) was not a multiple of 4 because of the length of the text.

With this patch, all commands will occupy a space that has a size which is a multiple of 4, which prevents alignment problems.

this fixes a crash when microui is used on my 32-bit ARM tablet.
the crash happens when assigning structure content to
a field of an unaligned command union.
@Jan200101
Copy link

Jan200101 commented Dec 7, 2021

the command needs to be aligned to the systems alignment (which happens to be 4 in the case of ARM)

size_t alignment = alignof(max_align_t) - 1;
size = (size + alignment) & ~alignment;

is a more portable solution but depends on C11

Since this is an ANSI C library I don't think that is going to get merged in.
an alignment of 8 would probably be better for portability sake, but also more wasteful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants