Skip to content

correctly fix invalid read from pvoc array#16

Open
Jan200101 wants to merge 1 commit intodevshane:masterfrom
Jan200101:PR/pvoc-fix
Open

correctly fix invalid read from pvoc array#16
Jan200101 wants to merge 1 commit intodevshane:masterfrom
Jan200101:PR/pvoc-fix

Conversation

@Jan200101
Copy link
Collaborator

@Jan200101 Jan200101 commented May 30, 2021

follow up to #14 after I had been urged to look into this more

the real problem was caused by prplnt, the integer used to define the size of the pvoc array, being off by 3 causing an invalid read
I recreated the environment that caused the crash from #14 and couldn't replicate anymore with this fix

the value of prplnt could have been intentional considering the place where its used increments by 3 every iteration

I came to the wrong conclusion before because I was debugging the rpm package which compiled with -O2

@justwheel
Copy link
Collaborator

Hi @devshane, me and @Jan200101 opened #14 five years ago to make some changes to package zork on Fedora Linux (see RPM source repository). This PR seems to be the "cleaner" fix to #14, and it would be nice to see if we can get this merged before submitting new patches from downstream to help ensure zork continues to build and compile on newer versions of the C programming language. In true hacker spirit! 😎

Would you be able to help review this and get it merged? I'd like to see this get merged as a late follow-up to the work we did in #14. I am aware that @jamesjer has done some great work in a downstream PR to get this working on C23, and I am hoping we can share the improvements with the wider world via your repository, to help keep this important relic of hacker and computing history alive for generations to come.

What do you say @devshane, are you able to take a look at this PR before we from the Fedora Linux community seek to upstream some other patches? There were discussions about forking into a new repo, but you accepted our patches before, so I wanted to make a best-effort to see if we can share our improvements back here with the whole world too.

@devshane
Copy link
Owner

Hi @justwheel @Jan200101. First, thanks very much to you and everybody else who have submitted changes despite my unresponsiveness. It's shameful; I truly apologize.

I'm the accidental steward of what turned out to be an important repo. This started out on a whim years ago, me wondering whether I could hack the game into Slack. I'm not even sure where I found the code. Somehow it became a canonical source.

The truth is that I'm not qualified to judge the changes that are being submitted (I don't C), so my tendency has been to preserve the code rather than keep it alive. With all the genuine interest, though, I'm happy to invite new maintainers or even transfer the repo somewhere more appropriate. I can't really be anything but in the way...

Is that interesting to either of you? Or anybody else?

@Jan200101
Copy link
Collaborator Author

I'm not sure about transferring the repo somewhere but I'd be happy to help out with code-reviews/maintaining/anything.

@Jan200101
Copy link
Collaborator Author

Just to clarify, @devshane would you be OK with me reviewing and pulling in PRs on my own or do you want me to review them and give you the final decision to merge?
I agree that changes should be as unintrusive to the original source code as possible so I'm not going to merge a complete rewrite of subsystems that don't also preserve the original in some form, e.g. a compile option.


buzlnt = 20;
prplnt = 48;
prplnt = 45;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Checked through everything again to confirm that this is the correct fix:

PRPLNT is also defined as 48 in the original Fortran code while PVOC is an array with only 45 entries resulting in out of bounds access.

The issue only turned fatal once optimizations were turned on and the C compiler turned the Undefined Behavior into a reason to do horrible stuff.

@devshane
Copy link
Owner

Just to clarify, @devshane would you be OK with me reviewing and pulling in PRs on my own or do you want me to review them and give you the final decision to merge? I agree that changes should be as unintrusive to the original source code as possible so I'm not going to merge a complete rewrite of subsystems that don't also preserve the original in some form, e.g. a compile option.

Thanks again for agreeing to take on this role!

Yes, 100% OK with you accepting or rejecting PRs and I appreciate the effort to keep the code as close to "pure" as possible. The legacy is important.

The only open PR that I have reservations about about is #10 because I'm not sure it's in the spirit of the original game, but I'm happy to leave those decisions with you and the wider community.

@justwheel
Copy link
Collaborator

Hi @devshane, I'm happy to help out with repository access here as the Fedora Linux packager and use some of my community management skills to help out how I can.

As a general principle, I agree that aiming to preserve the original game code as much as possible is a key design decision for this project, and focusing on changes that keep the code compiling for newest versions of C and compilers.

I can keep working with @Jan200101 here too, since he also has the C language experience I am lacking!

Thanks for being open to working with us on this. I remember when I first packaged this for Fedora, and it feels cool to keep this amazing relic of geek history alive for the next generation. ❤️

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

Comments