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

Fix some more UB #31

Merged
merged 4 commits into from
Oct 21, 2022
Merged

Fix some more UB #31

merged 4 commits into from
Oct 21, 2022

Conversation

EllipticEllipsis
Copy link
Contributor

Add casts to two of the ll wrappers to prevent UB (and ASAN shouting about UB), in particular

  • multiplying signed integers that won't fit in a u64.
  • shifting signed integers by enough that they don't fit in a u64 (this does also remove the UB for shifting negative integers, I'm not sure what IDO would do there or if it's predictable).

In theory this may not always emulate the correct behaviour, but it builds both OoT and MM OK so in the rare cases that it matters it's probably fine.

Also formatted the makefile so it doesn't use tabs for things that are not recipes, this was certainly confusing my VSCode so it probably can cause other problems as well.


ifeq ($(VERSION),7.1)
IDO_VERSION := IDO71
IDO_VERSION := IDO71
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use 4 spaces instead of 2?
Using 2 spaces confuses my vscode and makes it think it should treat tabs as they were 2 spaces instead of 4 which I don't like. This is exactly the reason why I wanted to use tabs instead of spaces here in the first place

libc_impl.c Outdated Show resolved Hide resolved
libc_impl.c Outdated Show resolved Hide resolved
@EllipticEllipsis
Copy link
Contributor Author

Addresses #29

@hensldm hensldm merged commit 7bc58d8 into master Oct 21, 2022
@hensldm hensldm mentioned this pull request Oct 21, 2022
2 tasks
@hensldm hensldm deleted the asan branch October 22, 2022 02:27
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.

3 participants