intfiction.org

The Interactive Fiction Community Forum
It is currently Fri Feb 22, 2019 10:35 am

All times are UTC - 6 hours [ DST ]




Post new topic Reply to topic  [ 12 posts ]  Go to page 1, 2  Next
Author Message
PostPosted: Tue Mar 01, 2016 10:48 am 
Offline

Joined: Sat Dec 26, 2015 7:34 pm
Posts: 517
Hey everyone,

In the course of implementing persistent achievements for the zmachine using the extended @save and @restore opcodes, I've discovered an issue with unix frotz: when an auxiliary file is @saved or @restored, frotz ignores the filename suggested by the program and instead suggests an empty or corrupted string as the default filename; in some cases frotz crashes with a SIGABRT at this time.

I've finally had the time to investigate the issue, and I'd like to lay out what I've found.

First, a brief test program that demonstrates the error:
Code:
Constant Story "AUXILIARY FILES";
Constant Headline "^An Interactive Investigation^";

#ifndef TARGET_ZCODE;
Message fatalerror "This program uses z-machine specific opcodes.";
#endif;

Include "Parser";
Include "VerbLib";

Object Storage_Chamber "Storage Chamber"
  with description "Type ~mysave~ to save data and ~myload~ to load data.",
  has light;

[ Initialise;
  location = Storage_Chamber;
];

Include "Grammar";

Constant SaveDataLen = 32;
Array savedata buffer SaveDataLen;

[ MySaveSub len prompt result;
  @output_stream 3 savedata;
  print "This is some data.^";
  @output_stream -3;
  len = savedata-->0 + WORDSIZE;
  @save savedata len "mysavefile" prompt -> result;
  print "MySave: result = ", result, "^";
 ];

Verb 'mysave'
    * -> MySave;

[ MyLoadSub len prompt result;
  len = SaveDataLen + WORDSIZE;
  @restore savedata len "mysavefile" prompt -> result;
  print "MyLoad: result = ", result, "^";
];

Verb 'myload'
    * -> MyLoad;

Next, a transcript of the test program running. In the transcript, we attempt to save to a new auxiliary file and then read from it.
Quote:
AUXILIARY FILES
An Interactive Investigation
Release 1 / Serial number 160220 / Inform v6.33 Library 6/12-beta1 S

Storage Chamber
Type "mysave" to save data and "myload" to load data.

>mysave
Enter a file name.
Default is "a.AUX": mysave.aux
MySave: result = 1

>myload
Enter a file name.
Default is "??h
jna?)%?": mysave.auxAbort trap
Vince-Lavianos-iMac:misc vince$

After reading through the frotz source code, wielding gdb, and checking out the z-machine spec, I believe that I've found the root cause of the issue.

In src/common/fastmem.c, the @save and @restore opcodes are implemented by z_save() and z_restore() respectively. Both of these functions call get_default_filename() to determine what default filename to suggest to the user when prompting during a save or restore, passing in the address of a caller-provided filename if it exists. get_default_filename() looks like this:
Code:
/*
 * get_default_name
 *
 * Read a default file name from the memory of the Z-machine and
 * copy it to a string.
 *
 */
static void get_default_name (char *default_name, zword addr)
{
    if (addr != 0) {

        zbyte len;
        int i;

        LOW_BYTE (addr, len);
        addr++;

        for (i = 0; i < len; i++) {

            zbyte c;

            LOW_BYTE (addr, c);
            addr++;

            if (c >= 'A' && c <= 'Z')
                c += 'a' - 'A';

            default_name[i] = c;

        }

        default_name[i] = 0;

        if (strchr (default_name, '.') == NULL)
            strcpy (default_name + i, ".AUX");

    } else strcpy (default_name, f_setup.aux_name);

}/* get_default_name */

If an address has been provided (the case that we're interested in), it reads a byte from zmp[addr] and treats it as a length (len). Then it reads len characters, one per byte, lowercasing them. It null terminates the string and, if no file extension is present, it adds .AUX.

So the initial thing that baffled me here was that addr is supposed to be the address of a string containing a filename, and I thought that string literals on the zmachine were encoded as zstrings (3 chars per 16-bit word), not as string arrays with a length byte followed by a sequence of one-byte characters. Pursuing this idea, I figured out how the suggested filename in my program would be encoded as a zstring and took a look at location addr in the zmachine memory and did *not* find the zstring that I had computed and was expecting.

I was at a loss until I reread the sections of the zmachine spec on strings and memory addressing and came up with the hypothesis that addr was a packed address rather than a byte address. I took a look at 4*addr (since I was testing on a z5 file), and there I discovered the zstring corresponding with the filename that the test program had passed to @save/@restore.

So, to summarize, get_default_name is doing 2 things wrong:
1) treating 'addr' as a byte address rather than a packed address, and
2) decoding the string found at 'addr' as if it were a string array rather than a zstring.

This behavior leads to the corrupted filenames and crash that we saw. If the "length" byte that the function erroneously reads has a value greater than MAX_FILE_NAME (80), it will copy data beyond the bounds of the stack-allocated default_name array that z_save() or z_restore() passed in. Regardless, it will be copying len garbage bytes into the array to act as the filename.

I implemented a rough draft of a fix to confirm these conclusions. The diffs are attached at the bottom of this post.

The main idea behind it is to make decode_text() in text.c (and its associated string type enum) public and use it from get_default_name().

I did a couple of things with decode_text that I probably wouldn't do in a "real" fix.

First, I added a FILENAME string type to the enum that means "this is a high string *and* you should decode it to memory (as with the VOCABULARY type) rather than printing it out." I think that this is broken and that string type and whether to decode to memory or the screen are coupled in a way that they shouldn't be, but it was expedient.

Second, I added a global char *, defined in fastmem.c and visible in text.c, that decode_text() uses for FILENAME strings in a similar way as it uses the static 'decoded' buffer in text.c for VOCABULARY strings. I think that changing decode_text() to accept a buffer as an optional argument would be better, but I wanted to minimize changes for this prototype fix.

Here's a transcript of the test program running with the patch applied (again, saving a file and then reading it).
Quote:
AUXILIARY FILES
An Interactive Investigation
Release 1 / Serial number 160220 / Inform v6.33 Library 6/12-beta1 S

Storage Chamber
Type "mysave" to save data and "myload" to load data.

>mysave
Enter a file name.
Default is "mysavefile.AUX":
MySave: result = 1

>myload
Enter a file name.
Default is "mysavefile.AUX":
MyLoad: result = 21

Here, we see that frotz correctly offers "mysavefile" as a default file name and completes the @save and @restore without crashing.

Please let me know what you guys think and if I'm overlooking anything here.


Attachments:
decode-patch.txt [4.22 KiB]
Downloaded 38 times

_________________
Vince Laviano
Top
 Profile Send private message  
Reply with quote  
PostPosted: Tue Mar 01, 2016 11:51 am 
Offline

Joined: Sat Jan 23, 2010 4:56 pm
Posts: 5921
Hang on, the spec is clear about this:

Quote:
The extension also has (optional) parameters, which save a region of the save area, whose address and length are in bytes, and provides a suggested filename: name is a pointer to an array of ASCII characters giving this name (as usual preceded by a byte giving the number of characters).


From what you say, Frotz is right and your code is wrong. You should supply an array argument to @save and @restore, not the I6 literal "mysavefile".


Top
 Profile Send private message  
Reply with quote  
PostPosted: Tue Mar 01, 2016 6:25 pm 
Offline

Joined: Sat Dec 26, 2015 7:34 pm
Posts: 517
zarf wrote:
Hang on, the spec is clear about this:

Quote:
The extension also has (optional) parameters, which save a region of the save area, whose address and length are in bytes, and provides a suggested filename: name is a pointer to an array of ASCII characters giving this name (as usual preceded by a byte giving the number of characters).


From what you say, Frotz is right and your code is wrong. You should supply an array argument to @save and @restore, not the I6 literal "mysavefile".


D'oh. Ok, thanks. I'll update my code accordingly.

_________________
Vince Laviano


Top
 Profile Send private message  
Reply with quote  
PostPosted: Mon Dec 19, 2016 11:48 am 
Offline
User avatar

Joined: Wed Oct 14, 2009 4:02 am
Posts: 2600
Do you have a compiled game which uses the extended @save and @restore modes?

And does anyone know if there are any older published games that use them?


Top
 Profile Send private message  
Reply with quote  
PostPosted: Mon Dec 19, 2016 5:16 pm 
Offline

Joined: Sat Dec 26, 2015 7:34 pm
Posts: 517
Dannii wrote:
Do you have a compiled game which uses the extended @save and @restore modes?

The code in the achievements zipfile here uses them. I've attached a compiled version of achdemo.inf.

When I was last working on the project, I converted achdemo.z5 with zcode2js.py and tried using it with a local I7-generated parchment template, but the "save files" appeared to be encoded as urls and it didn't seem to distinguish between a normal save and a save to an auxiliary file. After testing with various interpreters, I concluded that glulx aux files were more or less working, but that using z5 aux files in the way that I wanted would require interpreter work.


Attachments:
achdemo.z5 [103 KiB]
Downloaded 39 times

_________________
Vince Laviano
Top
 Profile Send private message  
Reply with quote  
PostPosted: Mon Dec 19, 2016 5:40 pm 
Offline
User avatar

Joined: Wed Oct 14, 2009 4:02 am
Posts: 2600
Yeah the extended @save function has never been supported in Parchment, but I'm looking at adding support for it now.

Thanks for the demo! If you don't mind I'll include it in ZVM's test cases.


Top
 Profile Send private message  
Reply with quote  
PostPosted: Mon Dec 19, 2016 6:03 pm 
Offline

Joined: Sat Dec 26, 2015 7:34 pm
Posts: 517
Dannii wrote:
Thanks for the demo! If you don't mind I'll include it in ZVM's test cases.

I'm fine with that, although it might be better to take the test program from earlier in this thread, fix the error that zarf pointed out (pass an array containing the file name instead of a zstring containing the file name), and tweak that to create a more targeted test. The achievements demo has a lot of extra cruft in it that's not relevant to extended @save/@restore.

_________________
Vince Laviano


Top
 Profile Send private message  
Reply with quote  
PostPosted: Fri Dec 23, 2016 10:16 am 
Offline
User avatar

Joined: Fri Sep 30, 2016 7:02 pm
Posts: 406
Location: USA
Dannii wrote:
Thanks for the demo! If you don't mind I'll include it in ZVM's test cases.


Got a link to the code for these test cases? Thank you.

_________________
Thunderword not forgotten, will pick back up 2nd half of 2018.


Top
 Profile Send private message  
Reply with quote  
PostPosted: Fri Dec 23, 2016 11:02 am 
Offline
User avatar

Joined: Wed Oct 14, 2009 4:02 am
Posts: 2600
Sure thing, they're in https://github.com/curiousdannii/ifvms. ... ster/tests


Top
 Profile Send private message  
Reply with quote  
PostPosted: Fri Feb 01, 2019 4:35 pm 
Offline
User avatar

Joined: Mon Dec 12, 2011 7:03 pm
Posts: 576
Location: Washington
I don't know how I managed to miss this one. I've just now applied the patch to Unix Frotz at https://gitlab.com/DavidGriffith/frotz

_________________
David Griffith


Top
 Profile Send private message  
Reply with quote  
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 12 posts ]  Go to page 1, 2  Next

All times are UTC - 6 hours [ DST ]


Who is online

Users browsing this forum: No registered users and 5 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
cron
Powered by phpBB® Forum Software © phpBB Group