search

rajko-horvat / OpenCiv1

Open source rewrite of the original Civilization 1 Game designed by Sid Meier and Bruce Shelley in year 1991
MIT License
225 stars 11 forks source link

Out-of-bounds guard on nations access #74

Closed jakobmulvad closed 2 weeks ago

jakobmulvad commented 2 weeks ago

Description of Changes

Out-of-bounds guard before accessing nations array

Rationale behind Changes

Game crash when barbarians capture city without animations. This is because barbarians nationalityID is -1 which is used to index into the nations array. My first fix was to change this id from a short to a ushort, but after investigating an original .SVE file i can see that the barbarians nationality ID is in fact -1 in the original game, so I think we should keep it like that.

If this is an acceptable fix, there are 27 other places in the code where the nations array is accessed blindly using the nationality ID as index which should probably also be fixed.

Suggested Testing Steps

Disable animations and have barbarians capture a city

(sorry about the whitespaces in the diff, my IDE is set to removed trailing whitespaces)

rajko-horvat commented 2 weeks ago

Hi, Thank you for testing OpenCiv1. The Barbarians should have NationalityID of 0 (Atilla, Barbarian). Look at the Nations array in CivState.cs (or CivStaticData.cs in unstable branch).

That means that the NationalityID of -1 should be invalid, and the fix that you propose will not work properly. If you are willing, I would kindly suggest that you find where -1 is assigned initially to a Barbarians NationalityID so that we can properly fix the issue.

There are many errors like this in the original code. The original EXE mostly works because there were no such checks in C++ compiler that Sid Meier used, so instead of throwing errors, invalid array accesses are allowed and original game mostly works without a problem. I say mostly because corruption problems were occasional issue when playing original Civilization.

What I'm currently doing under 'Unstable' branch: 1) Migrating from image based map to a Map object (necessary in order to fix the GoTo function, almost done), 2) Rewriting GoTo code (the original algorithm is not working properly and has many array access issues, in progress), 3) Translating disassembly to a new native C# code.

Currently, we should focus on translating disassembly to a new native C# code before making any significant changes to disassembly because much of the code is intertwined together.

jakobmulvad commented 2 weeks ago

Hi, thanks for looking at my PR.

Ok it's just that i investigated a .SVE file generated by the original game and in that the barbarian nationality ID is saved as 0xFFFF (-1 in signed short) which is why i figured this was the "correct" barbarian nationality id. But what you are saying is that this is a bug in the original game and the nationality id should have been saved as 0?

In that case the proper fix would be to patch this to 0 as you load the game i guess?

But i get the point around not making changes to the assembly but instead focus on translating the assembly to human readable c# code first. So i guess i just close this PR?

rajko-horvat commented 2 weeks ago

Yes :) Will close it for You.

I guess I will have to do the old game save import/export functionality soon. As the number of issues increases the old game save format is too limited to encompass all of the changes and needed data :(

Depending on the issue that I'm fixing, or the part of the code that I'm upgrading, I'm currently translating parts of the disassembly that are linked together. But eventually we will have to translate all of the disassembly...

rajko-horvat commented 2 weeks ago

I have narrowed -1 NationalityID for the Barbarians to the StartGameMenu.cs -> F5_0000_0000()

Will fix it as soon as I implement GoTo function properly.