Skip to content

Refactor GetWeaponProperty & GetOriginalWeaponProperty to new parser #4085

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

Merged
merged 2 commits into from
Mar 11, 2025

Conversation

FileEX
Copy link
Contributor

@FileEX FileEX commented Mar 9, 2025

No description provided.

@FileEX
Copy link
Contributor Author

FileEX commented Mar 9, 2025

After the second commit, the function does not work correctly. The second call to StringToEnum overwrites the weaponType variable, setting its value to 0. This is exactly the same case as in the mentioned PR

@Merlin
Copy link
Contributor

Merlin commented Mar 9, 2025

This happens because you changed the eWeaponSkill enum to be 1 byte wide. However, StringToEnum casts outResult to eDummy, which is 4 bytes. In CEnumInfo::FindValue, setting outResult will overwrite 4 bytes, unintentionally modifying other overlapping variables. At least, that's what I think is happening. I didn't debug anything.

@FileEX
Copy link
Contributor Author

FileEX commented Mar 9, 2025

This happens because you changed the eWeaponSkill enum to be 1 byte wide. However, StringToEnum casts outResult to eDummy, which is 4 bytes. In CEnumInfo::FindValue, setting outResult will overwrite 4 bytes, unintentionally modifying other overlapping variables. At least, that's what I think is happening. I didn't debug anything.

You're probably right. EnumToString/StringToEnum qualifies for refactoring because it causes the above issues by casting to eDummy with a size of 4 bytes if the passed enum has a smaller size

@FileEX FileEX closed this Mar 9, 2025
@FileEX FileEX reopened this Mar 10, 2025
@FileEX FileEX changed the title Fix crash #4078 Refactor GetWeaponProperty & GetOriginalWeaponProperty to new parser Mar 10, 2025
@Dutchman101
Copy link
Member

Dutchman101 commented Mar 11, 2025

Related to #4089 (comment), it's no longer bugged after 3ab068b and the refactor does solve some issues.

@Dutchman101 Dutchman101 merged commit e9e4bd8 into multitheftauto:master Mar 11, 2025
12 checks passed
MTABot pushed a commit that referenced this pull request Mar 11, 2025
e9e4bd8 Refactor GetWeaponProperty & GetOriginalWeaponProperty to new parser (#4085)
@FileEX FileEX deleted the bugfix/fix_crash_4078 branch March 11, 2025 18:28
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