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 UB in movie_lib.cc #117

Merged
merged 5 commits into from
Mar 5, 2024
Merged

Fix some UB in movie_lib.cc #117

merged 5 commits into from
Mar 5, 2024

Conversation

glebm
Copy link
Contributor

@glebm glebm commented Oct 29, 2023

Fixes warnings shown by UBSAN during the title movie playback. Mostly misaligned loads and a couple of bad shifts.

This fixes the game crashing on startup on rg350m.

Fixes warnings shown by UBSAN during the title movie playback.
Mostly misaligned loads and a couple of bad shifts.
@glebm
Copy link
Contributor Author

glebm commented Jan 20, 2024

@alexbatalov Can you please take a look at this PR?

@alexbatalov
Copy link
Owner

Thanks for pinging. Will look today.

@alexbatalov alexbatalov merged commit ad8a275 into alexbatalov:main Mar 5, 2024
9 checks passed
@alexbatalov
Copy link
Owner

I've refactored your code a bit to match surroundings, hope you don't mind. movie_lib needs complete review, so it should not have too much dependencies. Thanks.

@glebm glebm deleted the fix-ub branch March 6, 2024 12:43
@glebm
Copy link
Contributor Author

glebm commented Mar 6, 2024

Thanks, looks good overall, but I'm wondering what the purpose of these is:

if (1) {

@KulikAlex
Copy link

@glebm @alexbatalov Why doesn't getOffset work?
#136

@KulikAlex
Copy link

@alexbatalov is there any information about the _nfPkDecomp? how did you decompile?

@glebm
Copy link
Contributor Author

glebm commented Mar 9, 2024

Why doesn't getOffset work?

My original getOffset looked like this:

    constexpr auto getOffset = [](uint16_t v) {
        return static_cast<int8_t>(v & 0xFF) + dword_51F018[v >> 8];
    };

The refactored one is like this:

uint8_t getOffset(uint16_t v)
{
    return static_cast<int8_t>(v & 0xFF) + dword_51F018[v >> 8];
}

The difference is the return type, that's probably why. It should probably be int.

Zenkibou added a commit to Zenkibou/fallout1-ce that referenced this pull request Mar 23, 2024
hippydave added a commit to hippydave/fallout1-ce that referenced this pull request May 9, 2024
@giuliano-macedo
Copy link

Hey, I'm pretty sure this PR caused #194

return (b[3] << 24) | (b[2] << 16) | (b[1] << 8) | b[0];
}

uint8_t getOffset(uint16_t v)
Copy link
Contributor Author

@glebm glebm Jun 9, 2024

Choose a reason for hiding this comment

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

@alexbatalov As I mentioned in #117 (comment), the return type here (changed from auto to uint8_t when merging) is incorrect, it should be int or int32_t. This caused #136.

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.

4 participants