unix frotz issue with extended @save and @restore

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:

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.

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:

/*
 * 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).

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.
decode-patch.txt (4.22 KB)

Hang on, the spec is clear about this:

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.

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?

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.
achdemo.z5 (103 KB)

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.

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.

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

Sure thing, they’re in github.com/curiousdannii/ifvms. … ster/tests

I don’t know how I managed to miss this one. I’ve just now applied the patch to Unix Frotz at gitlab.com/DavidGriffith/frotz

Zarf said Frotz was already correct.

And then vlaviano said he’d update his code accordingly. In any case, garbage filenames were being produced for AUX filenames.
Could I get some people to look at gitlab.com/DavidGriffith/frotz/ … ffd37a4e39 ?

I fixed this again, back to the way it was. Oops.

Yeah, that patch was erroneous and based on my misunderstanding how filenames should be passed to the extended @save and @restore opcodes.

I meant the phrase “my code” to refer to the achievements library that was using extended @save and @restore. It seems as though it might have been interpreted to mean either the attached patch, which was unnecessary, or the “auxiliary files” test program, which would need to be fixed according to zarf’s correction before being useful as a test case. Sorry for not being more clear and creating confusion here.

I interpreted “my code” as referring to both your patch and the brief test program.