DavidKinder / Windows-Frotz

Z-code interpreter for Windows, based on Stefan Jokisch's Frotz interpreter core.
http://www.davidkinder.co.uk/frotz.html
GNU General Public License v2.0
59 stars 12 forks source link

Potential for illegal opcode during timeout event #13

Closed DavidGriffith closed 3 years ago

DavidGriffith commented 5 years ago

When a timeout keycode is received, make sure that it is expected, otherwise a non-existent timeout routine may be called. Then, instead of passing on an timeout event when there is no timeout set, just skip it.

This bug in the core was discovered by @auraes at https://gitlab.com/DavidGriffith/frotz/issues/136 and a fix was presented by @welash.

To tickle the bug, do the following in this game. After the replay command is finished, you'll see Fatal error: Illegal opcode and then a prompt to exit Windows Frotz.

recording on
enter
recording off
replay
Include "parser.h";
Include "verblib.h";

Object room1 "Room 1"
with description "You are in room 1. ^Come in, Room 2 is waiting for you.",
 Before [;
  GoIn:
   print "You are hesitating... ";
   MyKeyDelay(15);
   print "then you decide to enter.^";
   PlayerTo(room2);
   rtrue;
 ], 
has light;

[ MyKeyTimerInterrupt;
    rtrue;
];

[ MyKeyDelay tenths key;
    @read_char 1 tenths MyKeyTimerInterrupt -> key;
    return key;
];

Object room2 "Room 2"
with description "You are in room 2." 
has light;

[ Initialise; location = room1; ];

Include "grammar.h";

replay.z5.gz

DavidKinder commented 3 years ago

I didn't really like the patch applied to Frotz, testing whether or not to call the routine should depend on the "routine" argument, not the "timeout" argument. I've fixed this my own way in Windows Frotz with commit 62b9238ccebf59697ea89c68e77993b1322b1fcf.

DavidKinder commented 3 years ago

A tidying of the logic for default arguments for z_char() and z_read_char(): e76d37dab8143adab4b605c89db1840f379c4394

DavidKinder commented 3 years ago

As noted by William Lash in https://gitlab.com/DavidGriffith/frotz/-/issues/136, direct_call() already checks whether its argument is zero, so the check added by 62b9238ccebf59697ea89c68e77993b1322b1fcf is unnecessary. Removed by e9dbff0e8fd81a77af564ed5edfbef3599583f94.

DavidGriffith commented 3 years ago

Are you saying that e76d37dab8143adab4b605c89db1840f379c4394 obviated the need for 62b9238ccebf59697ea89c68e77993b1322b1fcf ?

DavidKinder commented 3 years ago

No, I'm saying that the changes to stream.c in 62b9238ccebf59697ea89c68e77993b1322b1fcf were unnecessary.