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

misc: fix tons of undefined behavior #720

Closed
wants to merge 0 commits into from
Closed

Conversation

pkova
Copy link
Collaborator

@pkova pkova commented Sep 26, 2024

These were discovered during the work on #719.

zig cc is just clang but with some more reasonable defaults. One of them is -fsanitize=undefined by default in debug builds. This results in a SIGTRAP if libubsan is not linked to the executable every time the program encounters undefined behavior at runtime. Linking the undefined behavior sanitizer gives more information about what's going on.

It turns out that just turning on a previously booted ship has thousands of instances of undefined behavior before even reaching the dojo. These are mainly of two different kinds: left shifting without the proper casts and unaligned member access of the nock bytecode programs.

This PR fixes all the instances of undefined behavior that are immediately obvious when turning on a fakezod. The fixes consist of adding the proper casts to the left shift operations and padding the members of the u3n_prog to be 8 byte aligned. The corresponding loom migration is also provided.

Note that I decided to make burnframe in nock.c a packed struct to get around the alignment issues it has. Something smarter could be considered in the future.

@pkova pkova requested a review from a team as a code owner September 26, 2024 16:12
@ripperi ripperi force-pushed the ripa/zig branch 11 times, most recently from 865be63 to 1ab0e3d Compare October 2, 2024 14:39
@pkova pkova closed this Oct 2, 2024
@ripperi ripperi deleted the pkova/undefined branch October 2, 2024 16:56
@pkova pkova mentioned this pull request Oct 8, 2024
pkova added a commit that referenced this pull request Oct 8, 2024
Looks like I made some kind of rebase snafu on #720. Today I learned
that if you force push something to a PR that results in 0 commits to
the base branch github will just close the PR and disallow any further
pushes to the branch!

Ok let's try this one then.
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.

1 participant